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

[Pay 6/30] [$1000] App gets stuck in infinite loop between cash and amount page when sending money #17637

Closed
6 tasks
kavimuru opened this issue Apr 18, 2023 · 35 comments
Assignees
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Apr 18, 2023

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


Action Performed:

  1. Open the app
  2. Open any report
  3. Click on plus besides compose box and click on send money
  4. Insert any amount and click on continue
  5. Click on amount
  6. Click on back button, observe it goes to new amount description page, again click on back button and observe it again opens amount page.
  7. Keep on clicking back button and observe that it is stuck between new amount description page and amount page
  8. Close this and now click on green plus btton
  9. Insert any amount and click on continue
  10. Select any user
  11. Click on amount to open amount page
  12. Click on back button, it will take us back to new amount description page, again click on back button and observe that now it can go all the way back to very first Send money page

Expected Result:

App should not get stuck in infinite loop on back click and should allow user to go back to send money page like it does when we try to send money using green plus button

Actual Result:

App gets stuck in infinite loop on back button between new amount description page and amount page

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.1.0
Reproducible in staging?: y
Reproducible in production?: new feature
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
Notes/Photos/Videos: Any additional supporting documentation

Recording.277.mp4
infinite.stuck.between.new.amount.description.and.amount.page.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681812176434499

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010f4ffd0457045b5a
  • Upwork Job ID: 1648783666486652928
  • Last Price Increase: 2023-04-19
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 18, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Apr 18, 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

@kavimuru kavimuru changed the title App gets stuck in infinite loop between cash and amount page when sending money to individual user from report (works fine when sending money using green plus button) App gets stuck in infinite loop between cash and amount page when sending money Apr 18, 2023
@garrettmknight garrettmknight added the External Added to denote the issue can be worked on by a contributor label Apr 19, 2023
@melvin-bot melvin-bot bot changed the title App gets stuck in infinite loop between cash and amount page when sending money [$1000] App gets stuck in infinite loop between cash and amount page when sending money Apr 19, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~010f4ffd0457045b5a

@MelvinBot
Copy link

Current assignee @garrettmknight is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 19, 2023
@MelvinBot
Copy link

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

@kmorales13
Copy link

kmorales13 commented Apr 19, 2023

Proposal

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

App gets stuck in infinite loop on back button between new amount description page and amount page

What is the root cause of that problem?

The following condition returns true regardless of the direction we are going
const isEditingAmountAfterConfirm = currentStepIndex === 0 && previousStepIndex === _.indexOf(steps, Steps.MoneyRequestConfirm);

i.e MoneyRequestAmount -> MoneyRequestConfirm -> MoneyRequestAmount is the same as MoneyRequestAmount <- MoneyRequestConfirm <- MoneyRequestAmount

This doesn't happen when using the FAB menu because there's an extra MoneyRequestParticipants step in between.

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

To fix this issue we could use a history array of the steps we are navigating through instead of just the current and previous index:
const [stepHistory, setStepHistory] = useState([0])
This way we can add the stepIndex when we move to a next step (setStepHistory([...stepHistory, stepIndex])) and remove it when we go back (setStepHistory(stepHistory.slice(0, -1))).

By having this history we can determine when we are back to the first step and stop showing the backButton.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Apr 20, 2023

Proposal

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

App gets stuck in infinite loop on back button between new amount description page and amount page

What is the root cause of that problem?

There're 2 ways to go from the MoneyRequestConfirm page to the MoneyRequestAmount page:

  1. Clicking the Amount
  2. Clicking back button in MoneyRequestConfirm page
    Both these options will lead to the condition here
    const isEditingAmountAfterConfirm = currentStepIndex === 0 && previousStepIndex === _.indexOf(steps, Steps.MoneyRequestConfirm);
    becomes true.
    When it's true, clicking back from MoneyRequestAmount will always go to the MoneyRequestConfirm page so there's a loop.

So we can see that the condition here

const isEditingAmountAfterConfirm = currentStepIndex === 0 && previousStepIndex === _.indexOf(steps, Steps.MoneyRequestConfirm);
is wrong, it should not be true when clicking back from the MoneyRequestConfirm page since if we do that, we're moving back to the start of the flow rather than "edit amount after confirm".

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

We need to change the way we're calculating the isEditingAmountAfterConfirm:

  1. Introduce a new state for the isEditingAmountAfterConfirm instead of depending on the previous currentStepIndex and previousStepIndex.
  2. In
    const navigateToStep = useCallback((stepIndex) => {
    , we need to check if we're going from the MoneyRequestConfirm page to the MoneyRequestAmount page. If yes then set isEditingAmountAfterConfirm to true.
  3. Add an useEffect that listens to the currentStepIndex, if we go out of the MoneyRequestAmount page (which has index 0), we'll set isEditingAmountAfterConfirm back to false.

What alternative solutions did you explore? (Optional)

There're other ways we can do the step 2 above for example by introducing a new param to navigateToStep and pass true in here

onPress={() => this.props.navigateToStep(0)}
where we're going from the MoneyRequestConfirm page to the MoneyRequestAmount page.

The 2nd part of my proposal on another issue here, if accepted, will also fix this issue.

@garrettmknight
Copy link
Contributor

@aimane-chnaif any of these proposals looking good?

@melvin-bot melvin-bot bot removed the Overdue label Apr 21, 2023
@garrettmknight garrettmknight removed the Bug Something is broken. Auto assigns a BugZero manager. label Apr 21, 2023
@garrettmknight garrettmknight removed their assignment Apr 21, 2023
@garrettmknight garrettmknight added the Bug Something is broken. Auto assigns a BugZero manager. label Apr 21, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@garrettmknight
Copy link
Contributor

Reassigning since I'm OOO next week.

@s77rt
Copy link
Contributor

s77rt commented Apr 22, 2023

This issue seems to have the same root cause as #17710 (wrong animation part) so we may handle it there. Do you want to hold / close this one?
cc @mallenexpensify @aimane-chnaif @dangrous

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. Help Wanted Apply this label when an issue is open to proposals by contributors labels May 12, 2023
@mallenexpensify
Copy link
Contributor

Bumped to weekly and removed Help Wanted. Also removed Bug because we're waiting on a refactor and I don't think folks assigned here aren't assigned to #16526 (comment) to push forward. (ie. I don't want to be that guy/BZ who's pinging that issue all the time with "any update?")

@melvin-bot melvin-bot bot added the Overdue label May 22, 2023
@dangrous
Copy link
Contributor

Still holding

@melvin-bot melvin-bot bot removed the Overdue label May 22, 2023
@melvin-bot melvin-bot bot added the Overdue label May 30, 2023
@dangrous
Copy link
Contributor

still holding

@melvin-bot melvin-bot bot removed the Overdue label May 30, 2023
@dangrous
Copy link
Contributor

dangrous commented Jun 6, 2023

still on hold

@dangrous
Copy link
Contributor

continues to be on hold

@dangrous
Copy link
Contributor

still waiting

@dangrous
Copy link
Contributor

PR fixing #17662 is on staging! We can probably move ahead with retesting and (if necessary) updating proposals

@aimane-chnaif
Copy link
Contributor

This is not reproducible anymore and can be closed.

@dhanashree-sawant
Copy link

Hi @aimane-chnaif, @dangrous, the following bug was raised before 17662 so will it be eligible for reporting bonus?

@dangrous dangrous changed the title [HOLD #17662][$1000] App gets stuck in infinite loop between cash and amount page when sending money [$1000] App gets stuck in infinite loop between cash and amount page when sending money Jun 26, 2023
@dangrous
Copy link
Contributor

Yep! @dhanashree-sawant you should be paid out for the report (cc @mallenexpensify) and then I think we can close this out!

@mallenexpensify
Copy link
Contributor

@dhanashree-sawant can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~012afce5349d800769

@mallenexpensify mallenexpensify added Daily KSv2 Weekly KSv2 and removed Weekly KSv2 Daily KSv2 labels Jun 26, 2023
@mallenexpensify mallenexpensify changed the title [$1000] App gets stuck in infinite loop between cash and amount page when sending money [Pay 6/30] [$1000] App gets stuck in infinite loop between cash and amount page when sending money Jun 26, 2023
@dhanashree-sawant
Copy link

Thanks @mallenexpensify, I have accepted the job.

@mallenexpensify
Copy link
Contributor

@dhanashree-sawant paid! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants