fix "Downloaded photo is not appearing in the photo album" in android #12033

aimane-chnaif · 2022-10-20T05:41:56Z

Details

To export file to external storage, we need to follow https://github.com/RonRadtke/react-native-blob-util#copytomediastore.
After export, remove temp file from internal storage.
All files are downloaded in /Download/Expensify folder.
All media (images, photos) can be seen in native Gallery app.

Fixed Issues

$ #11101
PROPOSAL: #11101 (comment)

Tests

Login with any account in android app
Send or receive any attachment in any chat
Open attachment detail
Download
Open native Files app and locate /Download/Expensify
Verify that downloaded file appears in that folder.
For media (images or videos) only:
Open native Gallery app
Verify that downloaded media appears in list

Verify that no errors appear in the JS console

Offline tests

Download not working in offline mode

QA Steps

Login with any account in android app
Send or receive any attachment in any chat
Open attachment detail
Download
Open native Files app and locate /Download/Expensify
Verify that downloaded file appears in that folder.
For media (images or videos) only:
Open native Gallery app
Verify that downloaded media appears in list

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mchrome.mp4

Mobile Web - Safari

msafari.mp4

Desktop

desktop.mov

iOS

ios.mp4

Android

android.mp4

src/libs/fileDownload/index.android.js

aimane-chnaif · 2022-10-20T15:14:24Z

I re-requested review from @parasharrajat but it seems github bug that removed request for @thienlnam

parasharrajat · 2022-10-20T15:28:56Z

src/libs/fileDownload/index.android.js

            FileUtils.showSuccessAlert();
+            RNFetchBlob.fs.unlink(attachmentPath);


Suggested change

FileUtils.showSuccessAlert();

RNFetchBlob.fs.unlink(attachmentPath);

RNFetchBlob.fs.unlink(attachmentPath);

FileUtils.showSuccessAlert();

let's swap these lines.

parasharrajat

Although it is working fine, there is an issue.

BUG: Android: Clicking on the download notification throws error media not found.

Steps:

Send any attachment (image) message.
Click to open the attachment.
Click ⬇️ download icon.
Open notification tray.
Click on the download complete notification.

aimane-chnaif · 2022-10-26T03:10:39Z

BUG: Android: Clicking on the download notification throws error media not found.

@parasharrajat I am not able to reproduce. Can you share your video?

demo.mp4

aimane-chnaif · 2022-10-26T04:21:36Z

BTW, I suggest not to use android download manager.
I tried to download file on android whatsapp and it's not using download manager so our app can be the same.

parasharrajat · 2022-10-26T16:18:06Z

BTW, I suggest not to use android download manager.

Why?

I tried to download file on android whatsapp and it's not using download manager so our app can be the same.

Not a good reasoning.

I think it is useful and there is a motive behind using it. But if you think so, feel free to open a slack discussion and explain the problem and reason. Don't forget to share the link here. You can tag me.

Interesting, I can reproduce. Try with an image.

screen-2022-10-26_21.44.29.mp4

aimane-chnaif · 2022-10-26T16:20:58Z

I can reproduce. Try with an image.

@parasharrajat Can you test on physical android device? There might be a problem with emulator storage

parasharrajat · 2022-10-26T16:21:46Z

Sure.

parasharrajat · 2022-10-26T16:32:20Z

There is a similar problem on the device. The Logical reason is that we are deleting the file after moving to mediasource which was downloaded via the manager.

I think it is useful and there is a motive behind using it. But if you think so, feel free to open a slack discussion and explain the problem and reason. Don't forget to share the link here. You can tag me.

Or we have to find a fix.

parasharrajat · 2022-10-29T18:27:21Z

Any update?

Julesssss · 2022-11-14T18:07:49Z

There is a similar problem on the device. The Logical reason is that we are deleting the file after moving to mediasource which was downloaded via the manager.

Pretty certain we'll need to find a fix for this. It should be as simple as hiding the DownloadManager notification. Hopefully the 3rd party library exposes this to us and it should be simple to fix 🤞 -- else I guess we'll need a fork or feature request.

We also need the android.permission.DOWNLOAD_WITHOUT_NOTIFICATION permission, but maybe this will be merged from the 3rd party lib's manifest file 🤷

parasharrajat · 2022-11-15T21:30:06Z

Let me bump the thread. We need to plan out the changes here. Simply removing the notification is not a good plan.

Either we remove the notification and create a new issue to find a fix
Or we just find a fix here on this issue?

Note: Removing notifications means we will have to do that for iOS as well to make it consistent.

parasharrajat · 2022-11-15T21:35:53Z

@Julesssss Could you please create a new issue we want to implement the notification?

For now, it seems many agree to disable the notification for moving this PR. @aimane-chnaif let's do that for now. Also, same for iOS.

Julesssss · 2022-11-16T10:21:46Z

To move forward we would need an alternative solution for the invalid image URI, else we agree with this workaround and hold this while a custom notification is implemented.

parasharrajat · 2022-11-16T11:18:52Z

I don't have access to the slack discussion. Could you please post it here? I am good with holding this.

Julesssss · 2022-11-16T11:50:19Z

@parasharrajat oops sorry, I pasted the wrong link. Updated.

aimane-chnaif · 2022-11-16T11:52:43Z

For now, it seems many agree to disable the notification for moving this PR. @aimane-chnaif let's do that for now. Also, same for iOS.

Ok, so go ahead with this for now?

src/libs/fileDownload/index.android.js

Julesssss · 2023-03-20T16:58:29Z

Here's screencasts for Android 8 and 13

Android 13
https://user-images.githubusercontent.com/10736861/226413086-7e49cc54-1c5d-4da0-a995-4eca9257085b.mp4

Android 8 (doesn't show in photos or files)
https://user-images.githubusercontent.com/10736861/226413126-64275473-2248-4c09-9d85-2cdf265d335b.mp4

aimane-chnaif · 2023-03-20T16:59:56Z

The MediaCollection solution only appears to work for newer Android OS versions. On my older device (Android 8.0) the image isn't saved to public storage.

Oh really? I will try to check on lower versions.

parasharrajat · 2023-04-04T08:10:01Z

@aimane-chnaif Any update?

aimane-chnaif · 2023-04-23T18:14:15Z

Latest update:

Device	OS	Files app	Gallery app	Google Photos
Samsung Galaxy S10	12	✅	✅	✅
Samsung Galaxy Tablet A7	12	✅	✅	✅
Nexus 5 (emulator)	8	❌ (WhatsApp: ✅)		❌ (WhatsApp: ✅)
Google Pixel XL (emulator)	13	✅		✅

Still investigating why it doesn't save on old OS devices, though upstream says it's fixed - Copy file to Download directory in android < 10

I see the notification on both Andorid 8 and Android 13

Btw, download manager notification never shows on any devices.

Julesssss · 2023-04-26T10:42:37Z

From the Google developer site:

Downloaded files, which are stored in the Download/ directory. On devices that run Android 10 (API level 29) and higher, these files are stored in the MediaStore.Downloads table. This table isn't available on Android 9 (API level 28) and lower.

So I'd be interested to see how the upstream lib maintainers achieved this.

parasharrajat · 2023-04-26T10:47:37Z

We are still using 0.16.0. I think upgrading might solve this @aimane-chnaif

aimane-chnaif · 2023-04-26T10:55:53Z

We are still using 0.16.0. I think upgrading might solve this @aimane-chnaif

yes I checked version also. They fixed <10 issue on exactly 0.16.0. But still I will retest on latest version today

parasharrajat · 2023-04-27T02:12:28Z

@aimane-chnaif Were you able to test it?

…o fix-11101

aimane-chnaif · 2023-04-27T09:10:57Z

@parasharrajat I confirmed working on latest version - 0.17.3. Tested on Android 8 emulator. Just pushed.
@Julesssss can you please test again on Android 8 real device?

android8.mov

Out of date

parasharrajat · 2023-04-27T10:15:07Z

@aimane-chnaif can you please update POD lock file?

parasharrajat · 2023-04-27T10:36:58Z

I am trying on an emulator now with Oreo OS.

parasharrajat

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

cc: @Julesssss

🎀 👀 🎀 C+ reviewed

parasharrajat · 2023-04-27T11:11:44Z

Tested the following features as well and everything looks good.

Add Attachment
Download attachments of all types.
Upload avatar cropping and save.

Tes devices - Android 8, 13 and IOS 16.2.

I noticed that folder name is different on Android 8 then the Android 13. It is Expensify on Android 13 but DownloadExpensify on android 8. No biggy for me.

aimane-chnaif · 2023-04-27T11:13:16Z

I tried to find solution of replacing DownloadExpensify with Expensify on lower versions but no documenation or no GHs found for that

Julesssss

👍

Julesssss · 2023-04-27T13:01:23Z

I'm ignoring the checklist failure, it matches the provided link and looks good to me

OSBotify · 2023-04-27T14:21:37Z

🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.7-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-04-28T07:51:34Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.7-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

fix "Downloaded photo is not appearing in the photo album" in android

2ddefde

aimane-chnaif requested a review from a team as a code owner October 20, 2022 05:41

melvin-bot bot requested review from parasharrajat and thienlnam and removed request for a team October 20, 2022 05:42

parasharrajat suggested changes Oct 20, 2022

View reviewed changes

src/libs/fileDownload/index.android.js Outdated Show resolved Hide resolved

aimane-chnaif added 2 commits October 21, 2022 01:03

chain promise

2ae0092

handle case of null attachment

91813f8

aimane-chnaif requested review from parasharrajat and removed request for thienlnam October 20, 2022 15:13

parasharrajat reviewed Oct 20, 2022

View reviewed changes

swap lines between unlink and success alert

35cc9b3

parasharrajat suggested changes Oct 25, 2022

View reviewed changes

Julesssss assigned aimane-chnaif Nov 14, 2022

Julesssss mentioned this pull request Nov 14, 2022

[HOLD for payment 2023-05-05] [$500] Android - Downloaded photo is not appearing in the photo album #11101

Closed

Julesssss reviewed Mar 20, 2023

View reviewed changes

src/libs/fileDownload/index.android.js Show resolved Hide resolved

aimane-chnaif added 2 commits April 27, 2023 19:07

Merge branch 'main' of https://github.com/aimane-chnaif/Expensify int…

f8daa3f

…o fix-11101

update react-native-blob-util version to 0.17.3

5d9cceb

aimane-chnaif dismissed parasharrajat’s stale review via 5d9cceb April 27, 2023 09:10

Julesssss added the Ready To Build label Apr 27, 2023

update Podfile.lock

2b0c0ab

aimane-chnaif requested review from Julesssss and parasharrajat April 27, 2023 10:29

parasharrajat approved these changes Apr 27, 2023

View reviewed changes

Julesssss approved these changes Apr 27, 2023

View reviewed changes

Julesssss merged commit c3fa98d into Expensify:main Apr 27, 2023

OSBotify mentioned this pull request Apr 27, 2023

Deploy Checklist: New Expensify 2023-04-27 #18063

Closed

19 tasks

aimane-chnaif mentioned this pull request Aug 22, 2023

[HOLD for payment 2023-09-06] [$1000] Android - Error attempting to download an attachment #25491

Closed

6 tasks

		FileUtils.showSuccessAlert();
		RNFetchBlob.fs.unlink(attachmentPath);

fix "Downloaded photo is not appearing in the photo album" in android #12033

fix "Downloaded photo is not appearing in the photo album" in android #12033

Conversation

aimane-chnaif commented Oct 20, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

aimane-chnaif commented Oct 20, 2022

parasharrajat Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

aimane-chnaif commented Oct 26, 2022

aimane-chnaif commented Oct 26, 2022

parasharrajat commented Oct 26, 2022

aimane-chnaif commented Oct 26, 2022

parasharrajat commented Oct 26, 2022

parasharrajat commented Oct 26, 2022

parasharrajat commented Oct 29, 2022

Julesssss commented Nov 14, 2022 • edited Loading

parasharrajat commented Nov 15, 2022

parasharrajat commented Nov 15, 2022

Julesssss commented Nov 16, 2022 • edited Loading

parasharrajat commented Nov 16, 2022

Julesssss commented Nov 16, 2022

aimane-chnaif commented Nov 16, 2022

Julesssss commented Mar 20, 2023

aimane-chnaif commented Mar 20, 2023

parasharrajat commented Apr 4, 2023

aimane-chnaif commented Apr 23, 2023 • edited Loading

Julesssss commented Apr 26, 2023 • edited Loading

parasharrajat commented Apr 26, 2023

aimane-chnaif commented Apr 26, 2023

parasharrajat commented Apr 27, 2023

aimane-chnaif commented Apr 27, 2023

parasharrajat commented Apr 27, 2023 • edited Loading

parasharrajat commented Apr 27, 2023 • edited Loading

parasharrajat left a comment • edited by Julesssss Loading

Choose a reason for hiding this comment

Reviewer Checklist

parasharrajat commented Apr 27, 2023 • edited Loading

aimane-chnaif commented Apr 27, 2023

Julesssss left a comment

Choose a reason for hiding this comment

Julesssss commented Apr 27, 2023 • edited Loading

OSBotify commented Apr 27, 2023

OSBotify commented Apr 28, 2023

aimane-chnaif commented Oct 20, 2022 •

edited

Loading

parasharrajat Oct 20, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

Julesssss commented Nov 14, 2022 •

edited

Loading

Julesssss commented Nov 16, 2022 •

edited

Loading

aimane-chnaif commented Apr 23, 2023 •

edited

Loading

Julesssss commented Apr 26, 2023 •

edited

Loading

parasharrajat commented Apr 27, 2023 •

edited

Loading

parasharrajat commented Apr 27, 2023 •

edited

Loading

parasharrajat left a comment •

edited by Julesssss

Loading

parasharrajat commented Apr 27, 2023 •

edited

Loading

Julesssss commented Apr 27, 2023 •

edited

Loading