Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$2000] Split bill - Split bill is displayed twice if swipe back from split bill chat #22313

Closed lanitochka17 closed 1 year ago

lanitochka17 commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open https://staging.new.expensify.com/
  2. Tap on "+" FAB
  3. Tap "Split bill"
  4. Enter amount
  5. Tap "Next"
  6. Select users
  7. Tap "Next"
  8. Tap "Split"
  9. Swipe to the right
  10. Swipe to the right again

Expected Result:

User lands in LHN either at step 9 or 10

Actual Result:

"Split bill enter mount" page is displayed twice when user swipes to the right at steps 9 and 10 from split bill chat

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.37.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

https://github.com/Expensify/App/assets/78819774/db145200-7e46-4e93-9485-454b8a421b7f

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019fd945448c329b5e
  • Upwork Job ID: 1679918056642871296
  • Last Price Increase: 2023-09-05
cubuspl42 commented 1 year ago

Advertised on Slack

ShogunFire commented 1 year ago

I think we need more informations on why the dismissModal act this way:

Navigation.dismissModal(); can accept a targetReportID. If it's empty or equal to getTopmostReportId, it will simply pop all the RHP modals. In this case, going back won't open back the RHP. https://github.com/Expensify/App/blob/391c41267ccb6dc14530ea552bf08772f99ce0ec/src/libs/Navigation/Navigation.js#L154-L162 But this is not the case when targetReportID exists and is not equal to getTopmostReportId. It will replace the RHP with the target report screen, and going back will open back the RHP. This issue is happening https://github.com/Expensify/App/issues/21356 and https://github.com/Expensify/App/issues/22313 which I believe you came from the 2nd issue.

I feel like dismissModal should pop the RHP screens in every cases. Do you know about this @adamgrzybowski ?

Also if I am not wrong we are replacing the current page with a report that could already be in the stack

pradeepmdk commented 1 year ago

Proposal

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

Split bill - Split bill is displayed twice if swipe back from split bill chat

What is the root cause of that problem?

when the split spil was created we reset the form already. so when swipe again come back to MoneyRequestParticipantsPage page shouldReset will be true only. at the time we are forming the fallback route to again split/new page.

https://github.com/Expensify/App/blob/b95c1dd22474fb48929e78a102c1121c1e6e4360/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L75-L80 here when shouldReset resetting the form we are creating the fallback route. why means when navigateBack(true) it will pass to here https://github.com/Expensify/App/blob/b95c1dd22474fb48929e78a102c1121c1e6e4360/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L59 here we are passing fallbackroute to goback function in this shouldEnforceFallback true means it will create the fallbackroute again its goes to "split/new/" we already one have ""split/new/"" so that its rendered twice

https://github.com/Expensify/App/blob/b95c1dd22474fb48929e78a102c1121c1e6e4360/src/libs/Navigation/Navigation.js#L130

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

we should update shouldReset false only we need to create fallback route

 navigateBack(!shouldReset);

when this false it just trigger goback https://github.com/Expensify/App/blob/b95c1dd22474fb48929e78a102c1121c1e6e4360/src/libs/Navigation/Navigation.js#L135

ShogunFire commented 1 year ago

Proposal

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

When we swipe back from the split bill chat we see split bill screen displayed twice

What is the root cause of that problem?

When we split the bill we do dismissModal here https://github.com/bernhardoj/Expensify/blob/6b1a6679042c4fa9bb628360e028f93a5f27b2f2/src/libs/actions/IOU.js#L973

But when we pass a chat report that is not the top most report it doesn't pop the screens from the RHP and just do a replace here: https://github.com/Expensify/App/blob/b95c1dd22474fb48929e78a102c1121c1e6e4360/src/libs/Navigation/Navigation.js#L166-L173

So when we go back from the chat report it shows the RHP, this is the real issue

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

In dismissModal we should pop the stack in every cases, after that, if the report we want to go to is not the top most report, we should navigate to that report.

What alternative solutions did you explore? (Optional)

napster125 commented 1 year ago

I can't reproduce this issue, is there any specific step?

bernhardoj commented 1 year ago

Same, could this be fixed by https://github.com/Expensify/App/pull/24165?

cubuspl42 commented 1 year ago

Ouch. It seems that I made a fuss, assuming that this bug is reproducible. I just checked that this indeed does not occur anymore.

@napster125 @bernhardoj Thank you for checking!

@flaviadefaria I believe that we can just close this in such case

pradeepmdk commented 1 year ago

@cubuspl42 this is weird I am still able to reproduce in iOS/Safari. I am in main with latest only.

https://github.com/Expensify/App/assets/16595846/a77541fc-3080-4c5f-8f1f-dba061c5f8ad

pradeepmdk commented 1 year ago

@cubuspl42 on staging I am not able to reproduce this one. There may be something wrong in my local.

melvin-bot[bot] commented 1 year ago

@cubuspl42, @flaviadefaria Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 year ago

@cubuspl42, @flaviadefaria Eep! 4 days overdue now. Issues have feelings too...

cubuspl42 commented 1 year ago

This issue should be closed, as finally it turned out it's not reproducible

flaviadefaria commented 1 year ago

Thanks @cubuspl42 and everyone else.