search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.79k forks source link

[$250] The mobile attachment picker on Android web is giving option to record audio and video #46336

Closed m-natarajan closed 6 days ago

m-natarajan commented 2 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.13-3 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @quinthar Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1721965815025499

Action Performed:

  1. Go to staging.new.expensify.com
  2. Tap Global +
  3. Tap Track > Scan > Attachment picker

Expected Result:

  1. On Android native, we don't offer "Take photo" when opening the attachment menu from the camera
  2. On Android web, let's really try to just follow the exact same flow as Android native
  3. On Android web, let's not ask to take a video or record audio

Actual Result:

Not matching with Android native and giving option to record audio and video for attachment picker

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014aa6bba3d11a7600
  • Upwork Job ID: 1818599086364884619
  • Last Price Increase: 2024-08-07
  • Automatic offers:
    • eh2077 | Reviewer | 103436535
Issue OwnerCurrent Issue Owner: @eh2077
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 2 months ago

@miljakljajic Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~014aa6bba3d11a7600

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

daledah commented 2 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Not matching with Android native and giving option to record audio and video for attachment picker.

What is the root cause of that problem?

When using AttachmentPicker for scanning here https://github.com/Expensify/App/blob/73ec7f8e3e3b34b81c5bbda91290ecb44aafc373/src/pages/iou/request/step/IOURequestStepScan/index.tsx#L590, we don't limit the file types, so by default it will accept any file types https://github.com/Expensify/App/blob/73ec7f8e3e3b34b81c5bbda91290ecb44aafc373/src/components/AttachmentPicker/index.tsx#L78

What changes do you think we should make in order to solve the problem?

Allow the users of AttachmentPicker to customize the file types that the input will accept, that means the AttachmentPicker will have a new params acceptedFileTypes, which it will pass to input. If acceptedFileTypes exists it will be used, otherwise it will fallback to the existing logic https://github.com/Expensify/App/blob/73ec7f8e3e3b34b81c5bbda91290ecb44aafc373/src/components/AttachmentPicker/index.tsx#L78

Then we can pass acceptedFileTypes to AttachmentPicker here https://github.com/Expensify/App/blob/73ec7f8e3e3b34b81c5bbda91290ecb44aafc373/src/pages/iou/request/step/IOURequestStepScan/index.tsx#L590. The list of file types supported is here https://github.com/Expensify/App/blob/73ec7f8e3e3b34b81c5bbda91290ecb44aafc373/src/CONST.ts#L114

We need to construct a string of accepted file types that is compatible with accept prop of the input. Reference: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#unique_file_type_specifiers

Or a slightly better code design is we pass this list https://github.com/Expensify/App/blob/73ec7f8e3e3b34b81c5bbda91290ecb44aafc373/src/CONST.ts#L114 as is, as acceptedFileTypes, then we have a transformer in AttachmentPicker that will convert it to accept-compatible string

What alternative solutions did you explore? (Optional)

NA

daledah commented 2 months ago

Proposal has a minor update for clarification

eh2077 commented 2 months ago

@daledah Thanks for your proposal!

Does the inconsistency only happen with mobile Chrome? Or both mobile web platforms have same issue?

As your solution suggests to pass accepted files types in App/src/components/AttachmentPicker/index.tsx, I'm not sure if it would break some feature on web platforms.

daledah commented 1 month ago

Does the inconsistency only happen with mobile Chrome? Or both mobile web platforms have same issue?

@eh2077 Both mobile platforms have the issue, Safari is also allowing users to record/select videos.

As your solution suggests to pass accepted files types in App/src/components/AttachmentPicker/index.tsx, I'm not sure if it would break some feature on web platforms.

@eh2077 It won't break any feature because we only use it when selecting files for Scan, and accept is a standard web feature for input, the OS will allow selecting according to the file formats specified by accept. From my testing everything looks fine.

eh2077 commented 1 month ago

@daledah 's proposal looks good to me. Coding details can be discussed in PR and we need to ensure mobile platforms have consistent behaviour.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

eh2077 commented 1 month ago

Not overdue, we're waiting for @danieldoglas 's review

melvin-bot[bot] commented 1 month ago

📣 @eh2077 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

📣 @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

daledah commented 1 month ago

@eh2077 This PR is ready for review.

miljakljajic commented 1 month ago

Deployed to prod two days ago. Will update the payment schedule header.

eh2077 commented 1 month ago

@daledah Can you take a look at this comment https://github.com/Expensify/App/pull/47028#issuecomment-2294890330 please?

miljakljajic commented 1 month ago

@daledah please apply to this job too and we'll get you paid

eh2077 commented 1 month ago

@daledah Can you take a look at this comment https://github.com/Expensify/App/pull/47028#issuecomment-2294890330 please?

@daledah Friendly bump

daledah commented 1 month ago

@eh2077 I'm working on this PR

miljakljajic commented 1 month ago

Seems like this one isn't ready to be paid yet

daledah commented 1 month ago

@daledah please apply to this job too and we'll get you paid

@miljakljajic I don't have any Upwork Connect so I could not apply. Could you send the offer directly to my profile here https://www.upwork.com/freelancers/~0138d999529f34d33f? Thx

miljakljajic commented 3 weeks ago

Offer for @daledah is here: https://www.upwork.com/nx/wm/offer/103795902

eh2077 commented 3 weeks ago

Hi @miljakljajic, sorry this issue is not ready yet for payment. Because we haven't fully get the expected behaviors. We have active discussions on the closed PR https://github.com/Expensify/App/pull/47028

miljakljajic commented 3 weeks ago

Don't worry, I have just extended the offer but I will not pay til payment is due. Thank you @eh2077

eh2077 commented 2 weeks ago

I'll post a summary of our discussions from the closed PR https://github.com/Expensify/App/pull/47028 soon

miljakljajic commented 1 week ago

Did we decide to close without moving forward, or will payment still be due?

eh2077 commented 1 week ago

Did we decide to close without moving forward, or will payment still be due?

@miljakljajic Thanks for checking this. Sorry for the late update, I think it's better to hold it for a while to hear feedback from internal engineering team.

eh2077 commented 1 week ago

Here's the summary of the status on this issue

  1. We have a PR https://github.com/Expensify/App/pull/47028 merged but only managed to remove the record audio option

  2. On mobile Chrome, we still have the Camera Camcorder option - the record video option

    The screenshot of what's look like after the PR merged

@daledah and I spent sometime to find a solution to remove the Camera Camcorder option but we ended up a conclusion that, on mobile Chrome, we can't control options to show on the attachment picker modal. Instead, it's the default behaviour of mobile Browser. There's a demonstration video https://github.com/Expensify/App/pull/47028#issuecomment-2335154502 by @daledah

See also

  1. Discussions from the merged PR https://github.com/Expensify/App/pull/47028#issuecomment-2317312123
  2. https://stackoverflow.com/questions/48503345/how-to-disable-camera-option-for-file-upload-from-mobile-browser
  3. https://stackoverflow.com/questions/21523544/html-file-input-control-with-capture-and-accept-attributes-works-wrong

That said, I tend to leave the record video option, Camera Camcorder, as it is.

@danieldoglas What do you think?

cc @quinthar @daledah

danieldoglas commented 1 week ago

I see. So, that's more of a limitation on Android, not on our side. If that's not something we can control, I think it's fine to keep it as is. @miljakljajic are you OK with that too?

miljakljajic commented 1 week ago

I agree - it doesn't look like a bug. Closing. Thank you for investigating!

eh2077 commented 1 week ago

@miljakljajic This should be eligible for payment right? We have worked on a PR https://github.com/Expensify/App/pull/47028 here.

danieldoglas commented 1 week ago

Yep, this is eligible for payment - we indeed solved the situation with the audio recording.

miljakljajic commented 6 days ago

Sorry, paid!