-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[HOLD for payment 2024-06-28] [HOLD for payment 2024-06-24] [$250] Android - Scan - Pay elsewhere greyed on submitting expense with currency other than local #41445
Comments
Triggered auto assignment to @bfitzexpensify ( |
@bfitzexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
Job added to Upwork: https://www.upwork.com/jobs/~0198922fe1d95a1791 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.The pay elsewhere button is shown as greyed out or disabled when the user selects any other currency instead of the local currency when trying to pay a user. What is the root cause of that problem?
In the above code, there is a condition written in the function "hasUpdatedTotal" which checks if the local currency matches the selected currency if they don't match then the SettlementButton is shown as disabled. Line 6091 in 90b67a5
What changes do you think we should make to solve the problem?As the condition prevents the user from trying to pay before the currency is converted, the condition cannot be removed, instead of removing the condition, we can show the settlement button in a loading state when the currency is being converted and disable the settlement button if the user is offline. |
📣 @shreykul! 📣
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
@shreykul, Could you please provide more details on the root cause analysis, including links to the relevant code, and the specific code changes proposed in your solution? This will assist us in better understanding and evaluating your proposal. Also, please utilize the exact proposal template. Kindly update your original proposal and tag me again when it's ready for review. |
Hi @rayane-djouah , Please check if the proposal is okay now...? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Bump @shreykul |
@rayane-djouah Sorry, I didn't get the time to read the mentioned issue fully. I'll make sure to update you tomorrow about this issue clearly. |
@rayane-djouah, I've gone through #35062, and it appears that when submitting expenses with currencies other than the local one, the "pay elsewhere" option is being greyed out. This occurs because the currency is first converted to the local currency before enabling the option. The button remains disabled until the converted currency is fetched through the backend. A similar issue was encountered before, and the solution was to inform the user to wait while the currency is being converted, rather than simply showing the button as disabled. The currency condition cannot be removed because it affects payment calculations when the user is offline, as mentioned in #35062. To address this, we can implement a toast notification to inform the user why the button is disabled. |
Yeah, It seems that it was decided to implement Pattern C for this case. Therefore, I don't think this is a valid bug. @Expensify/design - Do you think we need to add a message (like the "You appear offline" message pattern) to inform the user that the currency is being converted? |
After reading the thread and issue I feel like there's a few solutions to this. Ultimately we want the user to wait until the conversion is done and by putting it as disabled it looks like an action is not possible, which isn't the case, it's more that the user needs to wait until they can press the button. Here's the three solutions I've come up with (though I'm sure there's more):
Keen to hear what @Expensify/design thinks here. I'm okay with all though |
Great ideas. I think we're using the third option for the login button, it will be good if we make it consistent. |
Oh are we? Awesome find @rayane-djouah! If so, then that's my vote, but let's hear from @shawnborton and @dannymcclain first. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.84-3 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-06-24. 🎊 For reference, here are some details about the assignees on this issue:
|
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:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.85-7 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-06-28. 🎊 For reference, here are some details about the assignees on this issue:
|
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:
|
Payouts due:
@eVoloshchak do we need a regression test? |
Payment in a few days |
Payment Summary
BugZero Checklist (@bfitzexpensify)
|
This is ready for NewDot payment based on this payment summary - #41445 (comment) @eVoloshchak let me know about a regression test! |
DM'd @eVoloshchak to see about a regression test |
going to move this to weekly since the only remaining step is creating a regression test, if needed. |
Regression Test Proposal
Do we agree 👍 or 👎 |
Thanks @eVoloshchak! Regression test is here - https://github.com/Expensify/Expensify/issues/409726 NewDot payment summary here - #41445 (comment) Closing as complete! |
$250 approved for @eVoloshchak based on summary above. |
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: 1.4.69
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4534392
Issue reported by: Applause - Internal Team
Action Performed:
Pre-condition: Workspace set in local currency and submission frequency set to manual
Expected Result:
In workspace, expense created with currency other than local currency when submitted, greyed out pay elsewhere button must not be displayed
Actual Result:
In workspace, expense created with currency other than local currency when submitted, greyed out pay elsewhere button displayed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Record_2024-05-01-22-36-10_4f9154176b47c00da84e32064abf1c48.mp4
Bug6468291_1714591468528.ws.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @eVoloshchak / @ChristinadobrzynIssue Owner
Current Issue Owner: @bfitzexpensify / @ChristinadobrzynThe text was updated successfully, but these errors were encountered: