[HOLD for payment 2023-12-06] [$500] IOU-App crashes on tapping split bill created in #announce as split bill participant

[izarutskaya]

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.3.97-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. Launch app
2. Tap profile icon---Workspaces--Create a Workspace
3. Tap members
4. Invite members to Workspace
5. Tap on 3dots--#announce
6. Tap plus icon -- Split money
7. Enter an amount and tap next
8. Tap split amount
9. Now login new dot as a member of Workspace (ie.split bill participant)
10. Navigate to announce room'
11. Tap on split bill skeleton

Expected Result:

Tapping on split bill skeleton in #announce room from Workspace member (ie.split bill participant) account, app must not crash.

Actual Result:

Tapping on split bill skeleton in #announce room from Workspace member (ie.split bill participant) account, app crashes.

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

Bug6270142_1699544635649.crask.mp4

Bug6270142_1699544234017!crash.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012ba842ed3953850f
• Upwork Job ID: 1722645379128688640
• Last Price Increase: 2023-11-09
• Automatic offers:
• bernhardoj | Contributor | 27647078

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

Proposal

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

Pressing the split bill IOU preview skeleton crash the app.

What is the root cause of that problem?

When the split bill preview shows a skeleton, it means that the transaction data is not available on the Onyx yet. When we press it, it will navigate us to the split bill details page.

In split bill details page, the transaction data is required.

App/src/pages/iou/SplitBillDetailsPage.js

Line 40 in 732aad9

transaction: transactionPropTypes.isRequired,

But because it's not available, we get undefined and there are some parts of the code that is trying to access the transaction data and crash the app:

App/src/pages/iou/SplitBillDetailsPage.js

Line 103 in 732aad9

} = isEditingSplitBill && props.draftTransaction ? ReportUtils.getTransactionDetails(props.draftTransaction) : ReportUtils.getTransactionDetails(props.transaction);

App/src/pages/iou/SplitBillDetailsPage.js

Lines 136 to 137 in 732aad9

	receiptPath={props.transaction.receipt && props.transaction.receipt.source}
	receiptFilename={props.transaction.filename}

App/src/pages/iou/SplitBillDetailsPage.js

Line 143 in 732aad9

transactionID={props.transaction.transactionID}

App/src/libs/TransactionUtils.ts

Lines 89 to 108 in 732aad9

	function isMerchantMissing(transaction: Transaction) {
	const isMerchantEmpty =
	transaction.merchant === CONST.TRANSACTION.UNKNOWN_MERCHANT || transaction.merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT || transaction.merchant === '';
	const isModifiedMerchantEmpty =
	!transaction.modifiedMerchant ||
	transaction.modifiedMerchant === CONST.TRANSACTION.UNKNOWN_MERCHANT ||
	transaction.modifiedMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT ||
	transaction.modifiedMerchant === '';
	return isMerchantEmpty && isModifiedMerchantEmpty;
	}
	function isAmountMissing(transaction: Transaction) {
	return transaction.amount === 0 && (!transaction.modifiedAmount || transaction.modifiedAmount === 0);
	}
	function isCreatedMissing(transaction: Transaction) {
	return transaction.created === '' && (!transaction.created || transaction.modifiedCreated === '');
	}

App/src/libs/ReportUtils.js

Line 1602 in 8bd4a7b

const report = getReport(transaction.reportID);

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

Use a null-safety to access the transaction data on every usage mentioned above, example:

transaction?.amount

What alternative solutions did you explore? (Optional)

1. As the transaction data is supposed to be required on the split bill details page, we could disable the press when the transaction data is unavailable for the split bill.

2. (Simplest) Make the transaction props optional in SplitBillDetailsPage and set the default value as an empty object.

if you can't reproduce it, try re-login. I think another issue here is that the transaction data is never loaded, but I think we should still fix the crash as this can happen when the data is loading

[kuttyhub]

Proposal

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

App crashes on tapping split bill created

What is the root cause of that problem?

The root cause of this bug arises while the transaction object passed as undefined for the required props in SplitBillDetailsPage.

why it becomes undefined is because the Onyx store doesn't have the transaction data and returns undefined. and you can see details in the below video.

New.Expensify.-.Google.Chrome.2023-11-10.01-58-07.mp4

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

1. while triggering the navigation we can have check whether the Transaction has data or not.
2. Fix the missing transaction records in the Onyx store - ( New to the Expensify ecosystem, any help or suggestion would be great).

What alternative solutions did you explore? (Optional)

prevent the clicking action itself with an appropriate hint when on the loading state or the transition becomes empty.

[DylanDylann]

Proposal

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

• IOU-App crashes on tapping split bill created in #announce as split bill participant

What is the root cause of that problem?

• When the transaction data is not available, clicking on split bill action will open the SplitBillDetailsPage. In this page, we call the function getTransactionDetails with the first param (transaction) is empty, that lead to the bug "Uncaught TypeError: Cannot read properties of undefined (reading 'reportID')".
• And the error that transaction data is empty occurs inconsistently, so maybe it is BE isssue.

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

• We need to handle the case: The transaction is empty.
• Firstly, we need to prevent user from clicking on the split bill preview that will navigate the user to SplitBillDetailsPage. Do it by updating:
  

  App/src/components/ReportActionItem/MoneyRequestPreview.js

  Line 359 in 99dd643

  <PressableWithFeedback

  
  to:

        <PressableWithFeedback
+            disabled={isEmpty(props.transaction) && props.isBillSplit}

• Secondly, we need to prevent user from opening the SplitBillDetailsPage by deeplink. Do it by dismissing the RHN or displaying PageNotFoundView if the transaction is empty by adding the below logic to to SplitBillDetailsPage:

+   if (isEmpty(props.transaction)) {
+       return <NotFoundPage />;
+   }

What alternative solutions did you explore? (Optional)

• NA

[mananjadhav]

Thanks for the proposals folks. I can see all of them have correct RCA. Essentially we need to fix property access on transaction. Hence, I am going ahead with @bernhardoj's proposal.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[melvin-bot]

📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($500)

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

I'll open the PR tomorrow.

[bernhardoj]

PR is ready

cc: @mananjadhav

[Christinadobrzyn]

I'm going ooo until Monday Nov 27th so going to assign another BZ teammate to monitor this until I'm back. Thanks!

Status- reviewing PR

[melvin-bot]

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

[melvin-bot]

@mananjadhav, @grgia, @bfitzexpensify, @bernhardoj, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Christinadobrzyn]

I'm back from ooo so I'll take this back. Thanks Ben!

Looks like we're reviewing this PR so I think this can be moved to weekly

[melvin-bot]

⚠️ 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.

[Christinadobrzyn]

Looks like this blocker is not a blocker but behaving as expected. It just might need design to review it.

@mananjadhav @grgia @bernhardoj can you just confirm there's no regression with our PR as it relates to this GH and this GH?

[bernhardoj]

@Christinadobrzyn yes, there are no regressions. Both blocker issues had been closed.

[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 1.4.4-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix crash when opening a split bill preview on an #announce room #31291

If no regressions arise, payment will be issued on 2023-12-06. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @mananjadhav does not require payment (Eligible for Manual Requests)
• @bernhardoj requires payment 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:

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

[Christinadobrzyn]

prepping for payment

Payouts due:

Issue Reporter: NA
Contributor: $500 @bernhardoj (Paid in Upwork)
Contributor+: $500 @mananjadhav (in NewExpensify)

Eligible for 50% #urgency bonus? NA

Upwork job is here.

@mananjadhav do we need a regression test for this?

[mananjadhav]

Yes because this resulted in the app crash I think it makes sense to add the regression test here. We can add the QA steps from the PR body for the regression test.

I couldn't link any exact PR and it looks like this exists since a long time.

[Christinadobrzyn]

Awesome! Made this regression test - https://github.com/Expensify/Expensify/issues/344480

There's no regression so paying this out based on this payment structure. Gonna close this out!

[JmillsExpensify]

$500 payment approved for @mananjadhav based on this comment.