-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[$500] Android - It is impossible to scan the receipt with the camera, an error is displayed #34750
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01dcae17afc7313b12 |
Triggered auto assignment to @alexpensify ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Error displayed while trying to scan a receipt on Android What is the root cause of that problem?Error is displayed while trying to capture image with camera before requesting for permissions. Even though a message requesting CameraPermission is shown above the shutter button, tap on the shutter button itself is not prevented.
This causes camera.takePicture to get invoked even before camera ref is set, resulting in the error App/src/pages/iou/request/step/IOURequestStepScan/index.native.js Lines 256 to 268 in 31b19ea
What changes do you think we should make in order to solve the problem?Though is it quite evident from the message that camera permission is required, the shutter button is not disabled. <PressableWithFeedback
role={CONST.ACCESSIBILITY_ROLE.BUTTON}
accessibilityLabel={translate('receipt.shutter')}
style={[styles.alignItemsCenter]}
onPress={cameraPermissionStatus !== RESULTS.GRANTED? askForPermissions: capturePhoto}
>
<ImageSVG
contentFit="contain"
src={Shutter}
width={CONST.RECEIPT.SHUTTER_SIZE}
height={CONST.RECEIPT.SHUTTER_SIZE}
/>
</PressableWithFeedback> Result
What alternative solutions did you explore? (Optional)Other option is to disable the shutter button. To give visual feedback to user that it is disabled, we'll need a disabled shutter button SVG image (probably a grey version) to denote that the button is not active until permission is granted. |
ProposalPlease re-state the problem that we are trying to solve in this issueCamera error displayed when tapping the shutter button of the camera on scan request. What is the root cause of that problem?When While the permission is not granted, if the user will press the shutter button, the What changes do you think we should make in order to solve the problem?In order to make things as convenient as possible for the user, without extra click / press I suggest to have a useEffect(() => {
if (cameraPermissionStatus === RESULTS.GRANTED) {
return;
}
askForPermissions();
}, []); This way every button has 1 function and the permission prompt will show up as long as the permission is not granted everytime the user opens the scan request screen - this will clear things up as there won't be any confusion as to why the error shows when the user is pressing the shutter button right after they denied / dismissed the camera permission prompt. VideosAndroid: NativeScreen.Recording.2024-01-19.at.03.57.55.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.Camera error displayed when tapping the shutter button of the camera on scan request. What is the root cause of that problem?We intentionally show error message alert when camera ref is not there. App/src/pages/iou/request/step/IOURequestStepScan/index.native.js Lines 179 to 186 in 31b19ea
What changes do you think we should make in order to solve the problem?When we show the error alert, we can check for the permission status, if it is We will add this inside if (!camera.current && (cameraPermissionStatus === RESULTS.DENIED || cameraPermissionStatus === RESULTS.BLOCKED)) {
askForPermissions();
return;
} We can't apply directly ResultOptionalWe can add a ref like App/src/components/MapView/MapView.tsx Lines 35 to 62 in 56fb814
We can modify the error check condition to use similar check like we did in mWeb/Web. App/src/pages/iou/request/step/IOURequestStepScan/index.js Lines 166 to 168 in e6b0761
We also need to modify the check above because if the cameraRef.current in this( Condition to add in native if (!camera.current || !camera.current.takePhoto) {
showCameraAlert();
return;
} |
Proposal UpdatedAdded more options in optional section that can be implemented to improve UX, that might look similar to @ikevin127 proposal but its completely different because I use ref and use similar code that is already implemented in distance page. Added this for clarity. |
@thesahindia - Can you please review the proposals here? Thanks! |
@thesahindia - any update here? |
@thesahindia - keep us posted if you can review this one or let me know if you can't, so we can find a replacement. Thanks! |
@ikevin127's proposal looks good to me! 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @aldo-expensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Sorry for the delay. Not in a good shape right now! |
As a summary, I understand that we have the following proposals:
I wanted to confirm that @ikevin127 's proposal behaviour is really what we want and not @aswin-s 's proposed behaviour, so I'll ask in slack |
I was pointed out that in web mobile we ask immediately for permissions, so I think we should just be consistent with that. |
📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@aldo-expensify, the selected proposal is incorrect, it will ask for permission every time we visit scan tab. I mentioned the correct way to implement it in optional section. |
@ikevin127 can you modify your PR to include the changes from @Krishna2323 's proposal? 🙏 |
I think you guys might've missed this line from earlier, the PR is gone (deleted, incl. branch) as I thought @Krishna2323 is going to handle this one himself. I took on other issues in the meantime and have a few other opened PR's - therefore no capacity to take on this one really, sorry guys! |
No problem, I will handle this, will create a PR in 2-3 days. |
Awesome, thanks! |
Probably raising a PR today, there is another bug in android created after applying the required solution which I'm trying to solve. |
Thanks for the update! |
PR is in the works |
@aldo-expensify @thesahindia, need your suggestions on the PR |
Weekly Update: PR is merged and making progress forward! |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
Looks like the automation failed to set the 7-day hold for payment here since the PR #35640 (comment) was deployed to production 5 days ago (02/15/2024, 07:24 EET)🧐 Hold for payment date should be: cc @alexpensify Also the possible regression reported above was not caused by this PR, according to #36551 (comment). |
@ikevin127 - thanks for flagging, I agree and have a reminder to continue the payment process in two days. |
Here is the payment summary:
Extra Notes regarding payment: #34750 (comment) that's the discussion where we decided to include feedback from multiple proposals and split up the payment. |
@Krishna2323 and @aswin-s - please accept the job in Upworks and I can complete the process. Thanks! |
Accepted, thank you! 🙌 |
Accepted. |
@alexpensify Accepted the offer |
This has been addressed, we are back on track now. |
The contributors have been paid via Upwork. I'm still working with @aldo-expensify to confirm the payment for the C+ since there is one GH stating this PR created regression and then another GH states it did not. For now, those paid via Upwork have been paid there. |
I updated the summary here: #34750 (comment) After reviewing more, there is no clear evidence that regression was caused by this GH. @thesahindia please input a request in chat for the payment. Closing for now. Thanks to everyone here for figuring out a good plan. |
$500 approved for @thesahindia |
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.27-1
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:
Expected Result:
You can take a photo and continue the process
Actual Result:
Camera error displayed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6346784_1705598526136.video_2024-01-18_19-21-35.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: