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

[$250] Duplicate expense - Review duplicates confirmation displays again when going back after resolving the duplicates #58497

Open
6 of 8 tasks
vincdargento opened this issue Mar 14, 2025 · 13 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@vincdargento
Copy link

vincdargento commented Mar 14, 2025

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


Issue was found while executing QA for PR #57197

Version Number: 9.1.13-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both
If this was caught during regression testing, add the test name, ID and link from TestRail: #57197
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause Internal Team
Device used: iOS 18.3.1/ iphone 16 pro
App Component: Money Requests

Action Performed:

  1. Open app
  2. Go to a workspace which having submitted expense
  3. Create 2 money requests with the same amount
  4. Open one of them
  5. Tap on "Review duplicates"
  6. Tap on "Keep this one" and select one merchant
  7. Tap on "Confirm" of Review duplicates page
  8. Tap back button on the top screen
  9. Tap on "Confirm" of Review duplicates page again
  10. Tap back button on the top screen

Expected Result:

Tap back button from resolved duplicate page, app should return to workspace report

Actual Result:

Tap back button from resolved duplicate page, Review duplicates confirmation displays again. Tap back on it, Keep all button is displayed

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021901506012348084549
  • Upwork Job ID: 1901506012348084549
  • Last Price Increase: 2025-03-17
@vincdargento vincdargento added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Mar 14, 2025
Copy link

melvin-bot bot commented Mar 14, 2025

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@FitseTLT
Copy link
Contributor

FitseTLT commented Mar 14, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-03-14 16:02:44 UTC.

Proposal

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

Duplicate expense - Review duplicates confirmation displays again when going back after resolving the duplicates

What is the root cause of that problem?

This is because we navigate without poping the state from navigation stack in

if ('merchant' in comparisonResult.change) {
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_REVIEW_MERCHANT_PAGE.getRoute(route.params?.threadReportID, backTo));
} else if ('category' in comparisonResult.change) {
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_REVIEW_CATEGORY_PAGE.getRoute(route.params?.threadReportID, backTo));
} else if ('tag' in comparisonResult.change) {
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_REVIEW_TAG_PAGE.getRoute(route.params?.threadReportID, backTo));
} else if ('description' in comparisonResult.change) {
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_REVIEW_DESCRIPTION_PAGE.getRoute(route.params?.threadReportID, backTo));
} else if ('taxCode' in comparisonResult.change) {
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_REVIEW_TAX_CODE_PAGE.getRoute(route.params?.threadReportID, backTo));
} else if ('billable' in comparisonResult.change) {
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_REVIEW_BILLABLE_PAGE.getRoute(route.params?.threadReportID, backTo));
} else if ('reimbursable' in comparisonResult.change) {
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_REVIEW_REIMBURSABLE_PAGE.getRoute(route.params?.threadReportID, backTo));
} else {
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_CONFIRMATION_PAGE.getRoute(route.params?.threadReportID, backTo));

and
}
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(reportAction?.childReportID));
}, [reportAction?.childReportID, transactionsMergeParams]);

const navigateToNextScreen = () => {
switch (nextScreen) {
case 'merchant':
Navigation.navigate(ROUTES.TRANSACTION_DUPLICATE_REVIEW_MERCHANT_PAGE.getRoute(threadReportID, backTo));

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

We can goBack to the report instead of navigate when confirming the merge duplicates here

Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(reportAction?.childReportID));

or dismissModal as we do for resolveDuplicates
Navigation.dismissModal(reportAction?.childReportID);

or
We need to pop the last nav states before navigating in all these cases for instance by using Navigation.goBack() before the navigate in all places above

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N / A - navigation bug or if needed we can make a test similar to GoBackTests to assert that the duplicate screens are removed from the screen stack after finishing the merge duplicate flow

What alternative solutions did you explore? (Optional)

@truph01
Copy link
Contributor

truph01 commented Mar 15, 2025

I think it is dupe of #58460

@kadiealexander kadiealexander added the External Added to denote the issue can be worked on by a contributor label Mar 17, 2025
@melvin-bot melvin-bot bot changed the title Duplicate expense - Review duplicates confirmation displays again when going back after resolving the duplicates [$250] Duplicate expense - Review duplicates confirmation displays again when going back after resolving the duplicates Mar 17, 2025
Copy link

melvin-bot bot commented Mar 17, 2025

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

@melvin-bot melvin-bot bot added Overdue Help Wanted Apply this label when an issue is open to proposals by contributors labels Mar 17, 2025
Copy link

melvin-bot bot commented Mar 17, 2025

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

@melvin-bot melvin-bot bot removed the Overdue label Mar 17, 2025
@kadiealexander
Copy link
Contributor

@jjcoffee do you think it's a dupe of #58460? It seems different but maybe has the same fix?

@truph01
Copy link
Contributor

truph01 commented Mar 17, 2025

@jjcoffee If this issue is a duplicate of #58460, could you also help review my proposal in chronological order - Since that issue is created first? Thanks!

@jjcoffee
Copy link
Contributor

@truph01 Can you repost your proposal here? I'll take into account the time it was posted on the other issue.

@truph01
Copy link
Contributor

truph01 commented Mar 17, 2025

Re-post my proposal:

Proposal

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

  • After keeping the second expense on the review duplicates process, and using device´s back button, the user lands on a "It´s not here" page, and tapping on the app´s back button on that page, leads to infinite loading.

What is the root cause of that problem?

Here’s the breakdown of the screen stack after that action:

Root Stack  
├── ReportsSplitNavigator  
   ├── Home  
   └── Report [reportID: 8848574196123131]  
├── RightModalNavigator  
   └── TransactionDuplicate  
       ├── Transaction_Duplicate_Review [threadReportID: 8848574196123131]  
       └── Transaction_Duplicate_Confirmation [threadReportID: 8848574196123131]  
└── ReportsSplitNavigator (duplicate)  
    ├── Home  
    └── Report [reportID: 8848574196123131]  
  • So when clicking on back device button, incorrect screen is shown, in this case, it is Transaction_Duplicate_Confirmation.

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

  • We can dismiss the modal then navigate to the target report:

Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(reportAction?.childReportID));

        Navigation.dismissModalWithReport({reportID: reportAction?.childReportID})

or

        Navigation.dismissModal();
        Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(reportAction?.childReportID));

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  • Since this issue is related to navigation and device back button behavior, it primarily involves user interaction and system-level events that are difficult to fully replicate in automated tests.

What alternative solutions did you explore? (Optional)

@jjcoffee
Copy link
Contributor

Confirmed that this is a dupe, but since this was exported first it makes sense to handle it here to keep things moving quickly.

That makes @truph01's proposal first, and I appreciate their clear breakdown of the problem. Happy to go with them!

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Mar 17, 2025

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

@FitseTLT
Copy link
Contributor

@jjcoffee The selected proposal doesn't have the appropriate behavior. It adds duplicate screens of the expense request here is the output

2025-03-17.15-02-38.mp4

Please re-review my proposal

cc @aldo-expensify

@truph01
Copy link
Contributor

truph01 commented Mar 17, 2025

@jjcoffee In this video, we start with two separate transactions before merging. After the merge, the user is first navigated to the transaction thread (following my proposal using the dismiss method). When going back from there, the user is redirected to the expense report. However, since the transactions are now combined into one, the expense report appears identical to the transaction thread in term of one-transaction-thread. It isn't that the duplicate screens are shown as mentioned in the comment above.

The solution to address this issue is that, in here, we will calculate the number of transaction after merging, if it is only one transaction, the target report is iouReport.reportID, otherwise, reportAction?.childReportID. But I think we can consider skipping this issue because:

  1. It also occurs in staging, so it’s not a regression caused by my solution.
  2. The single transaction data is only retrieved after the merge API call succeeds. If we navigate to iouReport.reportID before the BE responds, it will open the IOU report without the transaction data. This might confuse users since they expect to be redirected to the transaction thread, not the IOU report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

6 participants