Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CP Staging] Fix document upload on Android #7277

Merged

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Jan 17, 2022

@mountiny

Details

Add file:// scheme prefix to attachment files (when it's missing)
Related to: #4908 (comment)

RN Document picker does not add a URI scheme for the file uri on Android. This seems to work ok for showing the document in previews, but instantly fails File upload when file:// is missing

Checked on iOS and there we do have a file:// prefix
So the logic that adds the prefix is conditional
Maybe we should try to address this on RN document picker's repo - this PR would remove the deploy blocker in the meantime

Fixed Issues

$ #7267

Tests

  • Verify that no errors appear in the JS console
  • Add a Document attachment in Android (verify it gets uploaded)
  • Add a Document attachment in iOS (verify it gets uploaded)
  • Add a Document attachment on other platforms (verify it gets uploaded)

QA Steps

Same as above

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

New.Expensify.-.Google.Chrome.2022-01-17.12-54-35.mp4

Mobile Web

Screen.Recording.2022-01-17.at.13.07.03.mov

Desktop

Screen.Recording.2022-01-17.at.13.00.04.mov

iOS

Screen.Recording.2022-01-17.at.12.32.24.mov

Android

Android.Emulator.-.Pixel_2_API_29_5554.2022-01-17.12-40-31.mp4

@kidroca kidroca requested a review from a team as a code owner January 17, 2022 10:43
@MelvinBot MelvinBot requested review from mountiny and removed request for a team January 17, 2022 10:43
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thank you for fixing this deploy blocker. I agree this would be better to fix in the RN Attachment picker.

@mountiny mountiny changed the title Fix document upload on Android [CP Staging] Fix document upload on Android Jan 17, 2022
@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@mountiny mountiny merged commit e63788d into Expensify:main Jan 17, 2022
OSBotify pushed a commit that referenced this pull request Jan 17, 2022
…-fix

[CP Staging] Fix document upload on Android

(cherry picked from commit e63788d)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @mountiny in version: 1.1.30-1 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@kidroca
Copy link
Contributor Author

kidroca commented Jan 17, 2022

Great! Thank you for fixing this deploy blocker. I agree this would be better to fix in the RN Attachment picker.

I've opened an issue here: react-native-documents/document-picker#526
I guess it would be OK if we can open a PR about it

@mountiny
Copy link
Contributor

Thank you for following up with the issue. It would be great if the fix could be implemented in the package itself. Would you be able to also create the PR? Do you need me to create a separate issue in our repo so you could be paid for it or can you just add time spent on that to this issue?

@kidroca
Copy link
Contributor Author

kidroca commented Jan 17, 2022

Thank you for following up with the issue. It would be great if the fix could be implemented in the package itself. Would you be able to also create the PR? Do you need me to create a separate issue in our repo so you could be paid for it or can you just add time spent on that to this issue?

You're welcome

I've identified the cause react-native-documents/document-picker#526 (comment) and I can submit a PR
I can time track it and log it under #7267
Can you add me there (but remove the deploy blocker label)?

@kidroca kidroca deleted the kidroca/android-document-picker-fix branch January 17, 2022 18:43
@mountiny
Copy link
Contributor

Done @kidroca! Thank you!

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.30-3 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants