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

[HOLD for payment 2024-02-14] [$500] Request Money - Previous currency is shown selected if edit Amount&Currency in Request Money #33608

Closed
3 of 6 tasks
kbecciv opened this issue Dec 26, 2023 · 54 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Dec 26, 2023

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.17-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:

  1. Login to ND Expensify
  2. Tap FAB button
  3. Tap Request money
  4. Select Manual tab
  5. Select a currency and enter any amount
  6. Tap Next
  7. Select an attandee
  8. When in final review page, click on Amount field to edit
  9. Click on Currency and change to another one
  10. Again click on Currency and verify if new selected currency is shown

Expected Result:

Last selected currency should be shown while editing Request Money

Actual Result:

Previous currency is shown selected if user changes it while editing Request Money

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

image

Bug6326315_1703614106567.2023-12-26_20-14-43.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0137f820b6bde4b24a
  • Upwork Job ID: 1739718074911305728
  • Last Price Increase: 2023-12-26
  • Automatic offers:
    • hoangzinh | Reviewer | 28076604
    • shubham1206agra | Contributor | 28076605
@kbecciv kbecciv 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 Dec 26, 2023
Copy link

melvin-bot bot commented Dec 26, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0137f820b6bde4b24a

@melvin-bot melvin-bot bot changed the title Request Money - Previous currency is shown selected if edit Amount&Currency in Request Money [$500] Request Money - Previous currency is shown selected if edit Amount&Currency in Request Money Dec 26, 2023
Copy link

melvin-bot bot commented Dec 26, 2023

Triggered auto assignment to @bfitzexpensify (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 Dec 26, 2023
Copy link

melvin-bot bot commented Dec 26, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Copy link

melvin-bot bot commented Dec 26, 2023

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 26, 2023

Proposal

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

Request Money - Previous currency is shown selected if edit Amount&Currency in Request Money

What is the root cause of that problem?

We are not updating draft transaction when the pageindex is confirm here

if (pageIndex !== 'confirm') {
IOU.setMoneyRequestCurrency_temporaryForRefactor(transactionID, option.currencyCode);
}

from this pr to avoid the saving of the currency on transaction when the user just navigates back after changing the currency without pressing save.

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

Use the same mechnism we used in IOURequestStepAmount and give the current currency in the route by naviagting with currency param here

const navigateToCurrencySelectionPage = () => {
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CURRENCY.getRoute(iouType, transactionID, reportID, backTo ? 'confirm' : '', Navigation.getActiveRouteWithoutParams()));
};

to

const navigateToCurrencySelectionPage = () => {
        Navigation.navigate(
            `${ROUTES.MONEY_REQUEST_STEP_CURRENCY.getRoute(iouType, transactionID, reportID, backTo ? 'confirm' : '', Navigation.getActiveRouteWithoutParams())}&currency=${currency}`,
        );
    };

Inside IOURequestStepCurrency

    const currency = pageIndex === 'confirm' ? currencyFromRoute : transactionCurrency;

What alternative solutions did you explore? (Optional)

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Dec 26, 2023

Proposal

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

Request Money - Previous currency is shown selected if edit Amount&Currency in Request Money

What is the root cause of that problem?

This PR #32826 prohibits the change in currency in transaction draft when on confirm page.

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

Add new property originalCurrency, and use it to restore the currency on pressing on back button only. And let IOU.setMoneyRequestCurrency_temporaryForRefactor(transactionID, option.currencyCode); run unconditionally

Explaination

This works by creating a field inside transaction draft onyx structure named original currency

/**
 * @param {String} transactionID
 * @param {String} originalCurrency
 */
function setMoneyRequestOriginalCurrency_temporaryForRefactor(transactionID, originalCurrency) {
    Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`, {originalCurrency});
}

And use this method on mount method of IOURequestStepAmount component, since this is the only place from where we can change currency.

And then restore the currency if save method was never hit on this step during unmount method

What alternative solutions did you explore? (Optional)

@namhihi237
Copy link
Contributor

Proposal

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

Currency is not updated when edit

What is the root cause of that problem?

Currently, we get currency from the draft transaction, When we select from the list we don't save anywhere when slect row until we save on the IOURequestStepAmount screen.

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

We should pass the query current currency from IOURequestStepAmount when open IOURequestStepCurrency to show the currency selected before. We will priority show currency from url first and then the draft

What alternative solutions did you explore? (Optional)

N/A

@dukenv0307
Copy link
Contributor

Proposal

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

Previous currency is shown selected if user changes it while editing Request Money

What is the root cause of that problem?

The selected currency is displayed as the currency which we stored in transaction. When we select a currency and the pageIndex is confirm we don't store this in transaction. So when we go to this page again, the selected currency is still the previous currency

if (pageIndex !== 'confirm') {

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

We don't store this to transaction to avoid the case the user just goes back to confirm page without saving. We can remove this condition to always store to selected currency in transaction

Keyboard.dismiss();
IOU.setMoneyRequestCurrency_temporaryForRefactor(transactionID, option.currencyCode);
navigateBack(option.currencyCode);

if (pageIndex !== 'confirm') {

And then on IOURequestStepAmount page, we can create a useEffect to restore the currency if this page is unmounted without saving

  1. Create a ref to know the amount is saved or not
const isSavedAmountAndCurrency = useRef(false);
  1. Update this to true in navigateToNextPage
isSavedAmountAndCurrency.current = true;

const navigateToNextPage = ({amount}) => {

  1. Create a useEffect to restore the currency
useEffect(() => {
    return () => {
        if (isSavedAmountAndCurrency.current) {
            return;
        }
        IOU.setMoneyRequestCurrency_temporaryForRefactor(transactionID, originalCurrency);
    }
}, [])

What alternative solutions did you explore? (Optional)

NA

@bfitzexpensify
Copy link
Contributor

Few proposals ready for review @hoangzinh

@hoangzinh
Copy link
Contributor

hoangzinh commented Dec 28, 2023

ah yes, gonna review today.

@aim-salam
Copy link

aim-salam commented Dec 28, 2023

Proposal

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

Previous currency is shown selected if edit Amount&Currency in Request Money

What is the root cause of that problem?

Updated
currency in transactionPropTypes not update changes

transaction: transactionPropTypes,

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

Instead of read currency just from transaction: {currency}, read from both transaction: {currency} and selectedCurrency (from previous page) directly.

Updated
Q : can you share more details about how would you save the previousPageCurrency
A : Nothing extra action needed. It is same as other pages handled, for example IOURequestStepAmount.js , currency read from transactionPropTypes and previous page's currency (selectedCurrency)

function IOURequestStepAmount({
report,
route: {
params: {iouType, reportID, transactionID, backTo, currency: selectedCurrency},
},
transaction,
transaction: {currency: originalCurrency},
}) {

Ok, to be synchronize I change my term from previousPageCurrency to selectedCurrency

function IOURequestStepCurrency({
currencyList,
route: {
params: {backTo, iouType, pageIndex, reportID, transactionID},
},
transaction: {currency},
}) {

function IOURequestStepCurrency({
    currencyList,
    route: {
        params: {backTo, iouType, pageIndex, reportID, transactionID,currency: selectedCurrency}, //<=== added here
    },
  transaction: {currency: originalCurrency}, //<== changed here
}) {

    const currency = selectedCurrency || originalCurrency; // <=== added here

p/s :Got small changes from other files too.
IOURequestStepAmount.js and ROUTES.ts that Im not include here.

Updated: it worked on web too.

Screen.Recording.2023-12-29.at.3.53.15.PM.mov

What alternative solutions did you explore? (Optional)

@hoangzinh
Copy link
Contributor

Thanks for your proposals, everyone.

  • @FitseTLT, @namhihi237 your solutions are almost same approach (get currency from URL). But @FitseTLT, because you didn't follow contributing guide here (post an Updated comment), it makes me hard to know who is the first one propose the solution, therefore I have to review this version of your proposal
Screenshot 2023-12-29 at 14 08 30
  • @shubham1206agra @dukenv0307 can you share more details about how would you save the originalCurrency? (when and where). Let me know when you update your proposal. Thanks

  • @aim-salam your RCA is not clear enough, we should avoid "somehow" as much as possible in RCA. Otherwise, we might not fix the issue correctly. Moreover, can you share more details about how would you save the previousPageCurrency? And will it work in Web platform? Let me know when you update your proposal. Thanks

@dukenv0307
Copy link
Contributor

dukenv0307 commented Dec 29, 2023

@hoangzinh

I tested this useEffect without any dependency and see that when the cleanup function is called, originalCurrency is the first currency when this page is mounted. originalCurrency is the current variable we use on this page. All details are already added in my proposal.

useEffect(() => {
    return () => {
        if (isSavedAmountAndCurrency.current) {
            return;
        }
        IOU.setMoneyRequestCurrency_temporaryForRefactor(transactionID, originalCurrency);
    }
}, [])

@shubham1206agra
Copy link
Contributor

@hoangzinh Proposal updated #33608 (comment)

@FitseTLT
Copy link
Contributor

@hoangzinh I 'm really sorry for making your job difficult but the reason i didn't notify when updating was because there was no similar proposal on my last update as you can see from the snapshout my last update came 6 min before @namhihi237 commented 👍
image
image

@aim-salam
Copy link

aim-salam commented Dec 29, 2023

@hoangzinh proposal updated #33608 (comment)

@hoangzinh
Copy link
Contributor

I tested this useEffect without any dependency and see that when the cleanup function is called, originalCurrency is the first currency when this page is mounted. originalCurrency is the current variable we use on this page. All details are already added in my proposal.

@dukenv0307 If I understand your proposal correctly

  • You suggest remove this condition and save currency to transaction draft in Onyx
  • Because the originalCurrency here is subscribed from Onyx, so because of the above step, it's overwritten with new currency value above.

Am I correct?

@hoangzinh
Copy link
Contributor

@aim-salam Thanks for the update. It looks like your proposal is same approach as @FitseTLT and @namhihi237. Moreover, the RC is not clear to me, why does "Updated currency in transactionPropTypes not update changes" cause this issue?

@dukenv0307
Copy link
Contributor

@hoangzinh Yes, correct. The main ideal of my proposal is the same way we do when we edit the distance.

@hoangzinh
Copy link
Contributor

Thanks for proposals, everyone. I think all of proposals would fix this issue. But in my opinion, I like the idea of @shubham1206agra and @dukenv0307, using originalCurrency to backup and restore currency if it's not saved yet

  • It gets rid of dealing with both selectedCurrency and originalCurrency (data source is from both URL and Onxy)
  • It also divides and conquers the edge case better (handle click back button without saving in one place - unmount component), giving the code of happy case is clean and clear.

But because @shubham1206agra is first, so I suggest we go with his proposal.

Link to proposal: #33608 (comment)

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Dec 29, 2023

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

@dukenv0307
Copy link
Contributor

Add new property originalCurrency, and use it to restore the currency on pressing on back button only.

Here is the ideal from proposal of @shubham1206agra. It will only restore the currency when click on back button. So this will not work if we open confirm page by deeplink when we're on amount page.

And my proposal already had the detail enough firstly after this proposal update the explaination.

@hoangzinh
Copy link
Contributor

Melvin, we're still working on the PR

@melvin-bot melvin-bot bot added the Overdue label Jan 12, 2024
Copy link

melvin-bot bot commented Jan 15, 2024

@hoangzinh, @techievivek, @bfitzexpensify, @shubham1206agra Huh... This is 4 days overdue. Who can take care of this?

@shubham1206agra
Copy link
Contributor

Still deciding the fix to the small Onyx bug in the issue.

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

melvin-bot bot commented Jan 16, 2024

@hoangzinh @techievivek @bfitzexpensify @shubham1206agra this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

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

melvin-bot bot commented Feb 6, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 7, 2024
@melvin-bot melvin-bot bot changed the title [$500] Request Money - Previous currency is shown selected if edit Amount&Currency in Request Money [HOLD for payment 2024-02-14] [$500] Request Money - Previous currency is shown selected if edit Amount&Currency in Request Money Feb 7, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 7, 2024
Copy link

melvin-bot bot commented Feb 7, 2024

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

Copy link

melvin-bot bot commented Feb 7, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.37-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-02-14. 🎊

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

Copy link

melvin-bot bot commented Feb 7, 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:

  • [@hoangzinh / @shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:
  • [@hoangzinh / @shubham1206agra] 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:
  • [@hoangzinh / @shubham1206agra] 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:
  • [@hoangzinh / @shubham1206agra] Determine if we should create a regression test for this bug.
  • [@hoangzinh / @shubham1206agra] 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.
  • [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 13, 2024
@bfitzexpensify
Copy link
Contributor

Payments complete, @hoangzinh please complete the BZ checklist when you get a moment - thanks!

@hoangzinh
Copy link
Contributor

BugZero Checklist:

  • The PR that introduced the bug has been identified. Link to the PR: [CP Staging] fix: store selected currency in URL query temporarily #32851
  • 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: https://github.com/Expensify/App/pull/32851/files#r1491081021
  • 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: N/A
  • Determine if we should create a regression test for this bug: Yes
  • 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.

Regression Test Proposal

  1. Login to ND Expensify
  2. Tap FAB button
  3. Tap Request money
  4. Select Manual tab
  5. Enter any amount
  6. Tap Next
  7. Select any participant
  8. When on the confirmation page, tap Amount field to edit
  9. Tap on Currency and change to another one
  10. Tap again on Currency
  11. Verify that the new selected currency in step 9 is shown selected

Do we agree 👍 or 👎

@bfitzexpensify
Copy link
Contributor

Thanks @hoangzinh. Agree with those regression steps - added via https://github.com/Expensify/Expensify/issues/370147.

Let's close this one out.

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 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants