pocket-casts-android icon indicating copy to clipboard operation
pocket-casts-android copied to clipboard

Task/upload files crash fix

Open CalebKL opened this issue 1 year ago • 2 comments

Description

This PR fixes file uploading crash. I have tried to reproduce this crash but I was unsuccessful after numerous attempts. Although after going through the logs, the crash was a permission error specifically for WRITE_EXTERNAL_STORAGE and INTERNET permissions. The WRITE_EXTERNAL_STORAGE was causing the FileNotFoundException and INTERNET permissions caused a connection error leading to a StorageFileLoadException. The PR adds checks to confirm that the permissions have been enabled and request permissions if they are not enabled.

Fixes #2285

Testing Instructions

Screenshots or Screencast

  1. Sign in
  2. Go to profile
  3. Click files
  4. Add a new file

Checklist

  • [ ] If this is a user-facing change, I have added an entry in CHANGELOG.md
  • [X] Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • [ ] I have considered whether it makes sense to add tests for my changes
  • [ ] All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • [ ] Any jetpack compose components I added or changed are covered by compose previews
  • [ ] I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

I have tested any UI changes...

  • [ ] with different themes
  • [ ] with a landscape orientation
  • [ ] with the device set to have a large display and font size
  • [ ] for accessibility with TalkBack

CalebKL avatar Jul 11 '24 11:07 CalebKL

I'm sorry but the reasoning doesn't explain the #2285 issue. The stack trace shows that the crash happens for an input stream which means reading a file and not writing to one.

WRITE_EXTERNAL_STORAGE is needed for writing to an external shared storage. And since SDK 33 this permission is deprecated and instead file interactions should be handled via createWriteRequest(). This is something we should probably look into as a separate issue.

INTERNET permission doesn't make sense because it is a normal permission granted at the installation time.

MiSikora avatar Jul 11 '24 13:07 MiSikora

I'm sorry but the reasoning doesn't explain the #2285 issue. The stack trace shows that the crash happens for an input stream which means reading a file and not writing to one.

WRITE_EXTERNAL_STORAGE is needed for writing to an external shared storage. And since SDK 33 this permission is deprecated and instead file interactions should be handled via createWriteRequest(). This is something we should probably look into as a separate issue.

INTERNET permission doesn't make sense because it is a normal permission granted at the installation time.

Thanks for the feedback, so we should check permissions for READ_EXTERNAL_STORAGE I will go ahead and remove the INTERNET permissions check. The reasoning to that was since we also had a connection error, we should have a check for the permission, but since its a normal permission we don't need that.

CalebKL avatar Jul 11 '24 19:07 CalebKL

@MiSikora any update on this?

CalebKL avatar Aug 19 '24 05:08 CalebKL

The solution isn't correct. You're invoking the permission request after the file request has been submitted. Also, this permission is deprecated in SDK 33+ and doesn't work there. And most importantly this permission shouldn't be needed since we're not accessing other apps' files when using android.intent.action.GET_CONTENT Intent.

If you want to fix this issue I'm going to have to ask you to provide a reproducible scenario that is fixed with your changes.

MiSikora avatar Aug 19 '24 07:08 MiSikora

I'm closing this PR as there is no reproducible scenario that we could test this with and the proposed solution has issues that I mentioned above.

MiSikora avatar Sep 20 '24 06:09 MiSikora