[HOLD for payment 2024-07-24] [$250] Split - App does not throw an error when saving the empty merchant field

[izarutskaya]

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

---

Version Number: 9.0.6-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Go to + > Split expense > Scan.
4. Upload an invalid receipt and submit it.
5. Wait for the scanning to fail.
6. After the scanning fails, click on the split preview.
7. Click Merchant.
8. Click Save without entering the merchant.

Expected Result:

App will throw an error when saving the empty merchant field (production behavior).

Actual Result:

App does not throw an error when saving the empty merchant field.

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

Bug6538012_1720623797916.20240710_225917.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bf2bed8e1956b05e
• Upwork Job ID: 1811366237800983005
• Last Price Increase: 2024-07-11
• Automatic offers:
• c3024 | Reviewer | 103093042
• Krishna2323 | Contributor | 103093043

Issue Owner

Current Issue Owner: @miljakljajic

[melvin-bot]

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

[melvin-bot]

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

[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.

[izarutskaya]

@miljakljajic 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.

[izarutskaya]

We think this issue might be related to the #vip-split

[izarutskaya]

Production

bandicam.2024-07-10.18-38-04-752.mp4

[Krishna2323]

Proposal

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

Split - App does not throw an error when saving the empty merchant field

What is the root cause of that problem?

The isMerchantRequired remains undefined for split expense because we aren't checking for ReportUtils.isPolicyExpenseChat(report)

App/src/pages/iou/request/step/IOURequestStepMerchant.tsx

Line 66 in 7bed113

const isMerchantRequired = ReportUtils.isExpenseRequest(report) || transaction?.participants?.some((participant) => !!participant.isPolicyExpenseChat);

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

Use || ReportUtils.isPolicyExpenseChat(report) here. I think we can also remove ReportUtils.isExpenseRequest(report) check.

What alternative solutions did you explore? (Optional)

Use || isEditingSplitBill here.

[danieldoglas]

I don't think this is related to backend, removing the DeployBlocker tag

[Julesssss]

Demoting. No functionality is blocked, as the user can just enter a merchant

[melvin-bot]

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

[c3024]

@Krishna2323, the bug does not exist on production. Could you find the changes in the recent PRs that caused this issue?

[Krishna2323]

@c3024, I think #44721

[kabeer95]

Proposal: Fix Empty Merchant Field Error on Split Expense

Problem Statement:

The Split app does not throw an error when saving an expense with an empty merchant field, leading to incomplete and inaccurate expense reports.

Root Cause:

The isMerchantRequired variable remains undefined for split expenses because the code does not check for ReportUtils.isPolicyExpenseChat(report) in the IOURequestStepMerchant.tsx file.

Proposed Solution:

To resolve this issue, we recommend updating the isMerchantRequired variable to include the ReportUtils.isPolicyExpenseChat(report) check. Specifically, we suggest modifying the code as follows:

const isMerchantRequired = ReportUtils.isExpenseRequest(report) || transaction?.participants?.some((participant) =>!!participant.isPolicyExpenseChat);

const isMerchantRequired = ReportUtils.isPolicyExpenseChat(report) || transaction?.participants?.some((participant) =>!!participant.isPolicyExpenseChat);
By adding the ReportUtils.isPolicyExpenseChat(report) check, we ensure that the isMerchantRequired variable is properly set, and the app will throw an error when saving an expense with an empty merchant field.

Alternative Solutions Explored:

We also considered using || isEditingSplitBill as an alternative solution, but it was deemed less effective in addressing the root cause of the issue.

Priority:

We consider this issue a medium priority, as it affects the accuracy and completeness of expense reports, but does not impact the overall functionality of the app.

Please let us know if you would like to discuss this proposal further or proceed with implementation

[c3024]

Apparently transaction has participants only for drafts. So, the check participant?.isPolicyExpenseChat check is not useful in this case.

So, I think adding the ReportUtils.isPolicyExpenseChat(report) is the right way to fix this and @Krishna2323 's proposal here looks good to me.

🎀 👀 🎀 C+ Reviewed

[melvin-bot]

Current assignee @Julesssss is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[Julesssss]

Hey @c3024, can I just check what you mean by this?

Apparently transaction has participants only for drafts

To confirm, we are suggesting that the 'do participants exist' check only needs to happy during creation?

Use || ReportUtils.isPolicyExpenseChat(report) here. I think we can also remove ReportUtils.isExpenseRequest(report) check.

I agree with this.

[c3024]

Yea, after creation there is no participants field in transaction because there is no participants field added in buildOptimisticTransaction

App/src/libs/TransactionUtils.ts

Lines 106 to 125 in 0d60861

	function buildOptimisticTransaction(
	amount: number,
	currency: string,
	reportID: string,
	comment = '',
	created = '',
	source = '',
	originalTransactionID = '',
	merchant = '',
	receipt?: OnyxEntry<Receipt>,
	filename = '',
	existingTransactionID: string | null = null,
	category = '',
	tag = '',
	taxCode = '',
	taxAmount = 0,
	billable = false,
	pendingFields: Partial<{[K in TransactionPendingFieldsKey]: ValueOf<typeof CONST.RED_BRICK_ROAD_PENDING_ACTION>}> | undefined = undefined,
	reimbursable = true,
	): Transaction {

App/src/libs/TransactionUtils.ts

Lines 138 to 157 in 0d60861

	return {
	...(!isEmptyObject(pendingFields) ? {pendingFields} : {}),
	transactionID,
	amount,
	currency,
	reportID,
	comment: commentJSON,
	merchant: merchant || CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT,
	created: created || DateUtils.getDBTime(),
	pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
	receipt,
	filename,
	category,
	tag,
	taxCode,
	taxAmount,
	billable,
	reimbursable,
	};
	}

so after creation there is never a participants field in transaction and later checks for transaction?.participants fail.

[c3024]

Whereas we add participants to the transaction draft while the request is created here

App/src/libs/actions/IOU.ts

Lines 451 to 452 in 1048f9a

	function setMoneyRequestParticipants(transactionID: string, participants: Participant[] = []) {
	Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`, {participants});

So, while creating the action, the !!participant.isPolicyExpenseChat check works correctly.

[Julesssss]

Yea, after creation there is no participants field in transaction because there is no participants field added in buildOptimisticTransaction

I see. So we don't need to optimistically add participants in the case of split, but the condition doesn't currently match our expectation here?

[Julesssss]

Confirmed in Slack here.

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

📣 @Krishna2323 🎉 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 📖

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

• fix: Split - App does not throw an error when saving the empty merchant field. #45371

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

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

• @Krishna2323 requires payment automatic offer (Contributor)
• @c3024 requires payment automatic offer (Reviewer)

[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:

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

[miljakljajic]

both contributors paid, thank you!