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

[PAID] [$500] Scan - Inconsistency of flashlight/torch behavior in Scan tab #34457

Closed
4 of 6 tasks
lanitochka17 opened this issue Jan 12, 2024 · 27 comments
Closed
4 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 12, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.24-4
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: Applause - Internal Team
Slack conversation:

Action Performed:

Prerequisite: Camera use permission is accepted previously
Android App

  1. Login to android Expensify App
  2. Tap FAB button
  3. Tap Request money
  4. Tap on flashlight icon to enable (verify nothing happens)
  5. Tap on green circle to take a shoot with camera (verify photo is taken with flashlight is ON)
    Mobile Web
  6. Login to ND Expensify
  7. Tap FAB button
  8. Tap Request money
  9. Tap on flashlight icon to enable (verify flashlight is ON)
  10. Tap on flashlight icon to disable ((verify flashlight is OFF)

Expected Result:

Flashlight should function exactly same behaviour both mobile apps and mobile web
Flashlight icon should be shown in Scan tab within all devices

Actual Result:

Both Android and IOS apps works similarly; flashlight is only ON while taking a photo shoot
In Mobile Web, tapping on the flash icon turns the flashlight ON&OFF without camera taking a shoot

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6340357_1705077132365.Realme_GT2_Pro_App_Chrome.mp4
Bug6340357_1705077132330.No_flashlight_icon_on_mobile_web.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017684a08e378933c3
  • Upwork Job ID: 1745866187913547776
  • Last Price Increase: 2024-01-12
  • Automatic offers:
    • s77rt | Reviewer | 28108708
    • tienifr | Contributor | 28108709
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 12, 2024
@melvin-bot melvin-bot bot changed the title Scan - Inconsistency of flashlight/torch behavior in Scan tab [$500] Scan - Inconsistency of flashlight/torch behavior in Scan tab Jan 12, 2024
Copy link

melvin-bot bot commented Jan 12, 2024

Job added to Upwork: https://www.upwork.com/jobs/~017684a08e378933c3

Copy link

melvin-bot bot commented Jan 12, 2024

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 12, 2024
Copy link

melvin-bot bot commented Jan 12, 2024

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

@neonbhai
Copy link
Contributor

neonbhai commented Jan 12, 2024

Proposal

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

Inconsistency of flashlight/torch behavior in Scan tab

What is the root cause of that problem?

We don’t pass the torch param here

What changes do you think we should make in order

We should pass the torch prop like we do for web:

torchOn={flash}

To make sure we have torch available, we may use the onTorchAvailability prop [Ref: here] to set a custom state (to disable the torch button from press):

onTorchAvailability={setIsTorchAvailable}

Alternatively

We may consider renaming flash variable to a more descriptive name like isFlashLightOn like we have on web

@bernhardoj
Copy link
Contributor

bernhardoj commented Jan 12, 2024

Proposal

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

The torch for native only appears when taking a picture, but for mWeb, the torch shows immediately when turned on.

What is the root cause of that problem?

For native, the behavior is the flash is applied when taking the picture.

camera.current
.takePhoto({
qualityPrioritization: 'speed',
flash: flash ? 'on' : 'off',

But for web, we set up the torch like this:

useEffect(() => {
if (!trackRef.current) {
return;
}
trackRef.current.applyConstraints({
advanced: [{torch: torchOn}],
});
}, [torchOn]);

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

For native, the camera can receive a torch prop that we can set to true.

torch={flash && device.hasTorch}

@tienifr
Copy link
Contributor

tienifr commented Jan 13, 2024

Proposal

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

Both Android and IOS apps works similarly; flashlight is only ON while taking a photo shoot
In Mobile Web, tapping on the flash icon turns the flashlight ON&OFF without camera taking a shoot

What is the root cause of that problem?

In Android/iOS native, we only apply the torch when the user is taking photo here. So the flash only applies when talking photos.

Meanwhile for web, we'll turn on the flashlight as soon as the user press on it in the app. This is not correct because the flashlight should only be on when taking photos, similar to how native camera works. This is to avoid unintended inconvenience for other people and also save device battery life.

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

Similar to native Android, iOS, if the user enable flash, in web we should only apply the torch (by trackRef.current.applyConstraints({ ) right before taking the screenshot here, after that we should turn the torch off (also by trackRef.current.applyConstraints({ ). If the user doesn't enable flash/torch, then we don't need to do anything and can just take the screenshot normally.

What alternative solutions did you explore? (Optional)

NA

@s77rt
Copy link
Contributor

s77rt commented Jan 13, 2024

@neonbhai Thanks for the proposal. I think we are looking to achieve the consistency here the other way around i.e. web should behave like native.

@s77rt
Copy link
Contributor

s77rt commented Jan 13, 2024

@bernhardoj Thanks for the proposal. Same note as above ^

@neonbhai
Copy link
Contributor

neonbhai commented Jan 13, 2024

@s77rt hi, purely from a UX perspective, I will expect the flash button to turn on flash immediately (and if it doesn't do that, it seems broken). The user is very used to this happening from other camera interfaces.

Maybe we could have eyes from the design team on this?

User Story

User clicks receipt in dark environment:

  1. User needs flash as they are clicking receipt in low light mode
  2. User would want the flash to turn on so they can frame it
  3. User clicks flash icon
  4. The button just flickers
  5. User cannot frame it, as the flash only turns on for a split second

@s77rt
Copy link
Contributor

s77rt commented Jan 13, 2024

@tienifr Thanks for the proposal. I agree on the RCA. Regarding the solution, can we improve this and use the track to construct ImageCapture. This Web API have a takePhoto method that supports fillLightMode to choose if flash should be enabled.

PS: I think it would be better if we ship this feature upstream to react-webcam

@s77rt
Copy link
Contributor

s77rt commented Jan 13, 2024

@neonbhai The mobile (native) behavior was implemented first. Then support was added to web. I think it makes sense to match the web to the native. If you think the native design is incorrect please raise that to Slack (and tag me and the design team).

@tienifr
Copy link
Contributor

tienifr commented Jan 13, 2024

@tienifr Thanks for the proposal. I agree on the RCA. Regarding the solution, can we improve this and use the track to construct ImageCapture. This Web API have a takePhoto method that supports fillLightMode to choose if flash should be enabled.

@s77rt thanks for the suggestion! we can definitely explore that, I'll try it and update the proposal accordingly 👍

@tienifr
Copy link
Contributor

tienifr commented Jan 15, 2024

Thanks for the proposal. I agree on the RCA. Regarding the solution, can we improve this and use the track to construct ImageCapture

@s77rt it seems that this Web API is only an experimental feature and is not supported in many major browsers like Safari, Firefox (reference), so I think we won't be able to use it in our app, except if we check the browser then decide which approach to use in the code, which I'm not sure is worth the trouble, and the old approach still has to be used for some browsers anyway.

@melvin-bot melvin-bot bot added the Overdue label Jan 15, 2024
@s77rt
Copy link
Contributor

s77rt commented Jan 16, 2024

Raised this on Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1705368355349939 for more 👀

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 16, 2024
@s77rt
Copy link
Contributor

s77rt commented Jan 18, 2024

I think we should proceed here with @tienifr's proposal.

🎀 👀 🎀 C+ reviewed
Link to proposal

@melvin-bot melvin-bot bot removed the Overdue label Jan 18, 2024
Copy link

melvin-bot bot commented Jan 18, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 18, 2024
Copy link

melvin-bot bot commented Jan 18, 2024

📣 @s77rt 🎉 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

Copy link

melvin-bot bot commented Jan 18, 2024

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 19, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Feb 12, 2024
Copy link

melvin-bot bot commented Feb 12, 2024

This issue has not been updated in over 15 days. @strepanier03, @s77rt, @luacmartins, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Feb 12, 2024
@s77rt
Copy link
Contributor

s77rt commented Feb 12, 2024

@tienifr still working on this

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels Mar 27, 2024
@melvin-bot melvin-bot bot changed the title [$500] Scan - Inconsistency of flashlight/torch behavior in Scan tab [HOLD for payment 2024-04-03] [$500] Scan - Inconsistency of flashlight/torch behavior in Scan tab Mar 27, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 27, 2024
Copy link

melvin-bot bot commented Mar 27, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot added the Overdue label Mar 27, 2024
Copy link

melvin-bot bot commented Mar 27, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.56-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-04-03. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Mar 27, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
  • [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@s77rt] Determine if we should create a regression test for this bug.
  • [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@strepanier03] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@strepanier03
Copy link
Contributor

This is on hold for payment for another weekish so just clearing the overdue until then.

@melvin-bot melvin-bot bot removed the Overdue label Mar 27, 2024
@s77rt
Copy link
Contributor

s77rt commented Mar 27, 2024

@strepanier03 strepanier03 added Daily KSv2 and removed Weekly KSv2 labels Apr 5, 2024
@strepanier03
Copy link
Contributor

Looks like the automation didn't work here either. Handling payment stuff now.

@strepanier03
Copy link
Contributor

All paid, closing!

@strepanier03 strepanier03 changed the title [HOLD for payment 2024-04-03] [$500] Scan - Inconsistency of flashlight/torch behavior in Scan tab [PAID] [$500] Scan - Inconsistency of flashlight/torch behavior in Scan tab Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants