[HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [$250] Android - Review duplicate - App crashes when tapping "Keep all" on modified chat link

[m-natarajan]

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: 9.0.1-0
Reproducible in staging?: y
Reproducible in production?: no 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
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Go to staging.new.expensify.com
2. On web, open DM with mobile user, copy the report link and add /duplicates/review to the end of the link. (Example - https://staging.new.expensify.com/r/4796612456700465/duplicates/review
   )
3. Send it to mobile user.
4. As mobile user (Android or iOS), open the DM.
5. Tap on the link.
6. Tap Keep all.

Expected Result:

App will not crash.

Actual Result:

App crashes on Android and iOS app.
On web, it shows console error.

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 - Console error
• MacOS: Desktop

Screenshots/Videos

Bug6521589_1719068371303.Screen_Recording_20240622_225622_One_UI_Home.1.mp4

https://platform.utest.com/services/links/v1/internal/51927148?.log

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e59eefc454d9599a
• Upwork Job ID: 1805181350791543526
• Last Price Increase: 2024-06-24
• Automatic offers:
• bernhardoj | Contributor | 102859572

Issue Owner

Current Issue Owner: @OfstadC

[melvin-bot]

Triggered auto assignment to @OfstadC (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.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @neil-marcellini (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[c3024]

Proposal

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

App crashes when pressing Keep all when reviewing duplicates.

What is the root cause of that problem?

transactionIDs can be '-1' here

App/src/pages/TransactionDuplicate/Review.tsx

Line 29 in 425804a

const transactionID = ReportActionsUtils.getLinkedTransactionID(reportAction, report?.reportID ?? '-1') ?? '-1';

and when they are mapped over here they are added as undefined in the mapped array here

App/src/libs/actions/Transaction.ts

Lines 294 to 295 in 425804a

	const currentTransactions = transactionIDs.map((id) => allTransactions?.[id]);
	const transactionsReportActions = currentTransactions.map((transaction) => ReportActionsUtils.getIOUActionForReportID(transaction.reportID ?? '', transaction.transactionID ?? ''));

and accessing reportID on undefined crashes the app.

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

Filter the undefined in the mapped array by modifying this

App/src/libs/actions/Transaction.ts

Line 295 in 425804a

const transactionsReportActions = currentTransactions.map((transaction) => ReportActionsUtils.getIOUActionForReportID(transaction.reportID ?? '', transaction.transactionID ?? ''));

to

const currentTransactions = transactionIDs.map((id) => allTransactions?.[id]).filter((transaction) => transaction);

What alternative solutions did you explore? (Optional)

[m-natarajan]

@OfstadC 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

[m-natarajan]

We think that this bug might be related to #wave-collect - Release 2

[bernhardoj]

Proposal

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

App crash when choose Keep all in review duplicates page of a DM chat.

What is the root cause of that problem?

When we open a DM chat and manually append the link with /duplicates/review, it will open the duplicate review page for the DM. But because it's a DM, the transactionID here defaults to -1.

App/src/pages/TransactionDuplicate/Review.tsx

Line 29 in 0176052

const transactionID = ReportActionsUtils.getLinkedTransactionID(reportAction, report?.reportID ?? '-1') ?? '-1';

App/src/pages/TransactionDuplicate/Review.tsx

Line 35 in 0176052

const transactionIDs = [transactionID, ...duplicateTransactionIDs];

duplicateTransactionIDs is an empty array because there is no duplicate. When we press keep all, it will map the transaction IDs and get the reportID here which will crash the app because there is no transaction with id of -1.

App/src/pages/TransactionDuplicate/Review.tsx

Line 40 in 0176052

Transaction.dismissDuplicateTransactionViolation(transactionIDs, currentPersonalDetails);

App/src/libs/actions/Transaction.ts

Lines 294 to 295 in 0176052

	const currentTransactions = transactionIDs.map((id) => allTransactions?.[id]);
	const transactionsReportActions = currentTransactions.map((transaction) => ReportActionsUtils.getIOUActionForReportID(transaction.reportID ?? '', transaction.transactionID ?? ''));

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

I think we can show not found page (or any other view, like an empty view saying there is no transaction found) when there is no linked transaction of a report by wrapping the page with FullPageNotFoundView.
<FullPageNotFoundView shouldShow={transactionID === '-1'}>
or
<FullPageNotFoundView shouldShow={!TransactionUtils.getTransaction(transactionID)}>

If we still want to show the page, then we can include the transactionID to the array here only if it's not -1.

App/src/pages/TransactionDuplicate/Review.tsx

Line 35 in 0176052

const transactionIDs = [transactionID, ...duplicateTransactionIDs];

[melvin-bot]

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

[melvin-bot]

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

[mountiny]

@allroundexperts can you please review the proposals for this blocker?

[allroundexperts]

Yep. I'll finalise in an hour or so.

[allroundexperts]

@c3024's proposal looks good to me. Since showing a not found page is something we can decide on while creating the PR, I would go with the first proposal with the correct RCA.

C+ reviewed 🎀 👀 🎀

[melvin-bot]

Current assignee @neil-marcellini is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[neil-marcellini]

I'm going to go with @bernhardoj's proposal instead because it's the only one that fully solves the issue. Showing a not found page is the right approach. If we went with the other proposal it wouldn't solve the issue completely, so I think it's most fair to go with the next proposal.

[neil-marcellini]

I'm going to fix this myself since it's a blocker and Bernhard isn't around right now.

[pecanoro]

I think you can remove the blocker labels, this is under a beta. It's also such an edge case that unless you know the codebase and you know you can do it, no one should trigger it 😄

[pecanoro]

Either way, thank you so much for fixing it!

[melvin-bot]

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

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.1-19 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Not found page for review of missing transaction #44296

If no regressions arise, payment will be issued on 2024-07-02. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests
• @bernhardoj requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] 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:
• [@allroundexperts] 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:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] 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.
• [@OfstadC] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: Feat/ Dupe detection review page #41093

[OfstadC]

@allroundexperts Could you provide an update on this checklist by EOD? Thank you!

[allroundexperts]

Checklist

1. Feat/ Dupe detection review page #41093
2. https://github.com/Expensify/App/pull/41093/files#r1660187883
3. N/A
4. A regression test would be helpful. Steps given in the OP look good to me!

[OfstadC]

@allroundexperts Could you please mark off each item completed on the checklist 😃 ? Thanks so much!

Payment Summary:

Contributor: @bernhardoj paid $250 via Upwork
Contributor+: @allroundexperts due $250 via NewDot

[allroundexperts]

@OfstadC I can't edit or mark as complete the checklist message (permission issues). Usually, the C+ just writes the checklist as I did here.

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Not found page for review of missing transaction #44296

If no regressions arise, payment will be issued on 2024-07-10. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests
• @bernhardoj requires payment automatic offer (Contributor)

[melvin-bot]

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:

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

[OfstadC]

This was already paid 🤔 - Asking about this here

[OfstadC]

Already paid - closing

[JmillsExpensify]

$250 approved for @allroundexperts