[HOLD for payment 2024-03-13] [$500] Split - "Enter a merchant name" error message appears while scanning is in process

[lanitochka17]

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: 1.4.29-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4211017
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:

Pre-requisite: user must have created a workspace and invited a member as employee

1. As the employee, go to the Workspace chat
2. Tap on the "+" button and select "Split bill"
3. On "Scan" option, upload any image and create the Split
4. On the Workspace chat, tap on the split preview while it is still scanning
5. Scroll down to "Merchant" field

Expected Result:

Error message for missing merchant should not appear while the scanning is in process

Actual Result:

Error message for missing merchant appears while scanning is in process

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

Bug6350890_1705948714600.Xfyp7681_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e28b0f9e8a41bb6e
• Upwork Job ID: 1749515793504890880
• Last Price Increase: 2024-02-14
• Automatic offers:
• situchan | Contributor | 28146340
• bernhardoj | Contributor | 0

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-split-p2p-chat-groups

[dukenv0307]

Proposal

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

Error message for missing merchant appears while scanning is in process

What is the root cause of that problem?

We always display the error if merchant is missed without checking the scan is scanning or not

App/src/components/MoneyRequestConfirmationList.js

Line 715 in 616d75f

error={shouldDisplayMerchantError || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''}

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

We should pass isScaning from SplitbillDetailPage into MoneyRequestConfirmationList and use it to not display any error if the request is scaning

App/src/components/MoneyRequestConfirmationList.js

Line 715 in 616d75f

error={shouldDisplayMerchantError || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''}

Or we can wait for MoneyRequestConfirmationList to be removed and then handle it in MoneyTemporaryForRefactorRequestConfirmationList

What alternative solutions did you explore? (Optional)

NA

[jeremy-croff]

I'm actually unable to reproduce.
Is it specific to some receipts?

[bernhardoj]

Proposal

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

The merchant field shows an error on a split bill details page while the receipt is still scanning.

What is the root cause of that problem?

Currently, we show the error if shouldDisplayMerchantError or shouldDisplayFieldError is true.

App/src/components/MoneyRequestConfirmationList.js

Line 715 in 9c552ab

error={shouldDisplayMerchantError || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''}

shouldDisplayFieldError will be false when the receipt is still scanning.

App/src/components/MoneyRequestConfirmationList.js

Lines 276 to 281 in 9c552ab

	const shouldDisplayFieldError = useMemo(() => {
	if (!props.isEditingSplitBill) {
	return false;
	}
	return (props.hasSmartScanFailed && TransactionUtils.hasMissingSmartscanFields(transaction)) || (didConfirmSplit && TransactionUtils.areRequiredFieldsEmpty(transaction));

But shouldDisplayMerchantError is always true because isScanRequest is always false.

App/src/components/MoneyRequestConfirmationList.js

Line 285 in 9c552ab

const shouldDisplayMerchantError = props.isPolicyExpenseChat && !props.isScanRequest && isMerchantEmpty;

The shouldDisplayMerchantError was added in #32486. The condition was added in both MoneyRequestConfirmationList and MoneyTemporaryForRefactorRequestConfirmationList (with a different name). I'm pretty sure they just copied it from MoneyTemporaryForRefactorRequestConfirmationList to MoneyRequetConfirmationList without testing the existence of isScanRequest (it's not available on both pages).

The isScanRequest is added in #29473 where it was added for the money request create page only, and not for the split bill details page intentionally.

The reason for the condition is to not show a merchant field if we are going to create a new scan request (just like other field, such as amount), but this condition is not applicable for the split bill details page (which still uses MoneyRequestConfirmationList). On MoneyRequestConfirmationList, we use this condition to decide whether to show an error or not which is not correct.

If we look at MoneyRequestView, we can see the condition to decide whether to show the merchant error or not.

App/src/components/ReportActionItem/MoneyRequestView.js

Line 225 in 9c552ab

hasErrors = canEdit && TransactionUtils.hasMissingSmartscanFields(transaction);

App/src/components/ReportActionItem/MoneyRequestView.js

Line 317 in 9c552ab

error={hasErrors && isPolicyExpenseChat && isEmptyMerchant ? translate('common.error.enterMerchant') : ''}

And that's the one that we should follow.

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

We can update shouldDisplayMerchantError to:

const shouldDisplayMerchantError = props.isPolicyExpenseChat && shouldDisplayFieldError && isMerchantEmpty;

Then we can remove the shouldDisplayFieldError here

App/src/components/MoneyRequestConfirmationList.js

Line 715 in 9c552ab

error={shouldDisplayMerchantError || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''}

If we later migrate to MoneyTemporaryForRefactorRequestConfirmationList, then we need to pass isEditingSplitBill as false from IOURequestStepConfirmation

[neonbhai]

Proposal

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

Split - "Enter a merchant name" error message appears while scanning is in process

What is the root cause of that problem?

This happens as this condition evaluates to true:

App/src/components/MoneyRequestConfirmationList.js

Line 715 in b68abb4

error={shouldDisplayMerchantError || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''}

shouldDisplayFieldErrors would evaluate to be true if there is any missing field in the request.

The problem is we OR the result of shouldDisplayMerchantError, which does not use the variable correctly.

We use shouldDisplayMerchantError to check if the report requires merchant and if an error should be shown

We use this to only show merchant errors, in places where it is a required field.

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


The || condition here should be an && condition:



App/src/components/MoneyRequestConfirmationList.js

Line 715 in b68abb4

error={shouldDisplayMerchantError || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''}

error={shouldDisplayMerchantError && shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction) ? translate('common.error.enterMerchant') : ''}

We should also add shouldDisplayMerchantError check for brickRoadIndicator prop condition here:

App/src/components/MoneyRequestConfirmationList.js

Line 714 in b68abb4

brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}

brickRoadIndicator={shouldDisplayMerchantError && shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}

Result:

Before

Screen.Recording.2024-01-23.at.1.51.04.AM.mov

After

Screen.Recording.2024-01-23.at.1.46.30.AM.mov

Alternatively

For a finer change, since merchant is not a required field for Split Request, while showing error, we should check for !isSplitBill condition:

error={shouldDisplayMerchantError || (shouldDisplayFieldError && !isSplitBill 
&& TransactionUtils.isMerchantMissing(transaction)) ? translate('common.error.enterMerchant') : ''}

[neonbhai]

@lanitochka17 hi, looks like we don't have a C+ assigned here 🤔

[situchan]

Even BZ member not assigned here 🤔

I can take this as C+

[melvin-bot]

6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[melvin-bot]

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

[melvin-bot]

@anmurali this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick!

[anmurali]

I will rotate the external label and see if that kicks off the Upwork process. I will assign you @situchan

[melvin-bot]

📣 @situchan 🎉 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 1.4.47-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix merchant error is shown in split bill details page while scanning #37393

If no regressions arise, payment will be issued on 2024-03-13. 🎊

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

• @bernhardoj requires payment automatic offer (Contributor)
• @situchan 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:

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

[anmurali]

@bernhardoj and @situchan are paid.

@situchan can you complete the BZ checklist?

[melvin-bot]

@anmurali, @srikarparsi, @bernhardoj, @situchan Eep! 4 days overdue now. Issues have feelings too...

[anmurali]

@situchan - this is waiting on you to complete the checklist. Can you please handle?

[situchan]

yes, I was looking for culprit PR but not found. The issue existed for a while.

We already have regression test here:
https://expensify.testrail.io/index.php?/tests/view/4211017

[srikarparsi]

Cool, in that case do you want to check off the boxes @situchan or is there something pending before payment?

[srikarparsi]

@situchan, do you think you can complete the checklist whenever you get a chance?

[melvin-bot]

@anmurali, @srikarparsi, @bernhardoj, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[anmurali]

@situchan - bump on the checklist.

[melvin-bot]

@anmurali, @srikarparsi, @bernhardoj, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@anmurali, @srikarparsi, @bernhardoj, @situchan 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[anmurali]

Since the regression test exists here, and everyone's been paid, I will close this issue