[$500] Expense - No error when saving an empty merchant in created workspace request

[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: v1.4.42-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com
2. Go to workspace chat.
3. Go to + > Request money > Manual.
4. Create a manual request with a merchant name
5. Save
6. Navigate to the request details
7. Click Merchant.
8. Empty the merchant field and save it.

Expected Result:

A merchant name is required on a money request > there should be an error "Enter a Merchant Name" if the merchant is blank. The user should not be able to save the IOU without adding a merchant

Actual Result:

User can save empty merchant when editing created request and no error shows up.
The same issue also occurs when the scan request fails. There is no error on Merchant row.

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

Bug6380628_1708001131762.bandicam_2024-02-15_15-47-56-452__1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b1421b286665858e
• Upwork Job ID: 1758111511563407360
• Last Price Increase: 2024-02-15
• Automatic offers:
• Ollyws | Reviewer | 0
• DylanDylann | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 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.

[melvin-bot]

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

[izarutskaya]

We think that this bug might be related to #wave5-free-submitters
CC @dylanexpensify

[eucool]

Proposal

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

No error when saving an empty merchant in created workspace request

What is the root cause of that problem?

Currently we do not have any set state to define errors on editing merchant, also we have a && condition over here which checks for both conditions i.e. the new merchant value is empty as well as old merchant value is CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT, is these are satisfied only then we will return without saving otherwise we save the value as empty string

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

Lines 116 to 119 in 9a598e0

	if (newMerchant === merchant || (newMerchant === '' && merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT)) {
	navigateBack();
	return;
	}

But while creating a IOU manual, we have a useState taking care of the error field:

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Line 307 in 9a598e0

const [merchantError, setMerchantError] = useState(false);

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Lines 528 to 531 in 9a598e0

	if ((isMerchantRequired && isMerchantEmpty) || (shouldDisplayFieldError && TransactionUtils.isMerchantMissing(transaction))) {
	setMerchantError(true);
	return;
	}

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

Update the above condition to check separately for is newMerchant empty, if so then don't save it into Onyx.

We also need to set error over here if the field is empty. otherwise it will only return to the main report without doing anything (i.e. it will not save values but it will simply return to main report page, but we should throw an error over here same as we do when we first create the manual report.

What alternative solutions did you explore? (Optional)

N/A

[eucool]

Also if someone points out that this can be a regression from #35641, i think we had not set the error message even before this PR, but would like to hear what the author @DylanDylann has to say about this

[jayeshmangwani]

Proposal

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

For the workspace expense there is no error when saving an empty merchant while editing merchant

What is the root cause of that problem?

We are using the wrong comparison for the isMerchantRequired constant here, we are checking if the transaction's participants has the prop isPolicyExpenseChat, but for the workspace transaction has no participants array.

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

Line 78 in f8f093b

const isMerchantRequired = _.some(transaction.participants, (participant) => Boolean(participant.isPolicyExpenseChat));

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

instead checking the participants array and checking the child for verifying if isMerchantRequired, rahter we should use existing function ReportUtils.isPolicyExpenseChat to determine if the merchant is required.

we need to do these changes for this

1. add report key to withOnyx
   

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

   Lines 168 to 174 in f8f093b

   	withOnyx({
   	splitDraftTransaction: {
   	key: ({route}) => {
   	const transactionID = lodashGet(route, 'params.transactionID', 0);
   	return `${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${transactionID}`;
   	},
   	},

    report: {
        key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
    },

2. define report prop here,
   

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

   Lines 67 to 68 in f8f093b

   	policyCategories,
   	}) {

3. and finally chage the isMerchantRequired to
   

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

   Line 78 in f8f093b

   const isMerchantRequired = _.some(transaction.participants, (participant) => Boolean(participant.isPolicyExpenseChat));

const isMerchantRequired = ReportUtils.isPolicyExpenseChat(ReportUtils.getRootParentReport(report));

note - before we were using the EditRequestMerchantPage page to display the edit merchant, thats why this issue happening after we are using the common MONEY_REQUEST_STEP_MERCHANT pages for the edit merchant

What alternative solutions did you explore? (Optional)

none

[DylanDylann]

@jayeshmangwani

note - before we were using the EditRequestMerchantPage page to display the edit merchant, thats why this issue happening after we are using the common MONEY_REQUEST_STEP_MERCHANT pages for the edit merchant

The logic is migrated totally from EditRequestMerchantPage to IOURequestStepMerchant. Thus, the logic should be the same in the EditRequestMerchantPage and IOURequestStepMerchant. Is there any difference here?

[jayeshmangwani]

@DylanDylann yes there is a difference,

in this PR, you have removed below code from src/pages/EditRequestPage.js page

    if (fieldToEdit === CONST.EDIT_REQUEST_FIELD.MERCHANT) {
        return (
            <EditRequestMerchantPage
                defaultMerchant={transactionMerchant}
                isPolicyExpenseChat={isPolicyExpenseChat}
                onSubmit={saveMerchant}
            />
        );
    }

and isPolicyExpenseChat defined like this, on the EditRequestPage page, and that was being passed to EditRequestMerchantPage, and on the EditRequestMerchantPage we were checking if isPolicyExpenseChat is true then show the error for empty merchant value

App/src/pages/EditRequestPage.js

Lines 86 to 87 in 50e07e8

	// A flag for verifying that the current report is a sub-report of a workspace chat
	const isPolicyExpenseChat = ReportUtils.isGroupPolicy(report);

and on IOURequestStepMerchant page we are using below code to determine if we have to show the error for empty merchant

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

Line 78 in d55757f

const isMerchantRequired = _.some(transaction.participants, (participant) => Boolean(participant.isPolicyExpenseChat));

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

Lines 91 to 93 in d55757f

	if (isMerchantRequired && _.isEmpty(value.moneyRequestMerchant)) {
	errors.moneyRequestMerchant = 'common.error.fieldRequired';
	}

now did you get why it was working for EditRequestMerchantPage and not for the IOURequestStepMerchant ?

[dukenv0307]

Proposal

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

User can save empty merchant when editing created request and no error shows up.
The same issue also occurs when the scan request fails. There is no error on Merchant row.

What is the root cause of that problem?

We are using isMerchantRequired field to validate the merchant field

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

Line 78 in e3331db

const isMerchantRequired = _.some(transaction.participants, (participant) => Boolean(participant.isPolicyExpenseChat));

But the logic in this field is incorrect because transaction.participants is not returned from BE, It seems this field is locally

Thus, I don't think this issue is a regression from #35641 because this logic was created before that

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

Line 78 in e3331db

const isMerchantRequired = _.some(transaction.participants, (participant) => Boolean(participant.isPolicyExpenseChat));

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

We should use another way to detect if the merchant field is required or not. I suggest we should use the same logic like here

App/src/pages/EditRequestPage.js

Line 91 in e3331db

const isPolicyExpenseChat = ReportUtils.isGroupPolicy(report);

Just a note: We should not use ReportUtils.isPolicyExpenseChat here, because this logic is incorrect and in this PR the internal engineer updated to use ReportUtils.isGroupPolicy instead of ReportUtils.isPolicyExpenseChat

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

📣 @Ollyws 🎉 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

[Christinadobrzyn]

Updated the OP - this is still happening - the issue is that you can save a request money without a merchant name.

A merchant name is required so here should be an error Enter a Merchant Name and an inability to save the IOU without adding a merchant name.

Maybe this is a regression from this? #34997

cc @DylanDylann

[Christinadobrzyn]

Okay sorry, it sounds like this might be a regression from #36574 (comment) as @jayeshmangwani already noted here #36574 (comment) (sorry for mixing up everything)

[DylanDylann]

As @dukenv0307's proposal mentioned

Thus, I don't think this issue is a regression from #35641 because this logic was created before that

I agree with him at this point. But this issue happened after that PR was merged and this issue also be resolved If we revert that PR

@hoangzinh Should we handle this issue as a regression of #35641? Ping you here because you are the reviewer of #35641

cc @Ollyws

[lakchote]

As @dukenv0307's proposal mentioned

Thus, I don't think this issue is a regression from #35641 because this logic was created before that

I agree with him at this point. But this issue happened after that PR was merged and this issue also be resolved If we revert that PR

@hoangzinh Should we handle this issue as a regression of #35641? Ping you here because you are the reviewer of #35641

cc @Ollyws

Since the issue would be gone if your PR was reverted:
Yes, you should handle this issue as regression of #35641.
cc @hoangzinh

[hoangzinh]

Agreed. It's an old logic but we should handle it @DylanDylann

[Beamanator]

What exactly is the plan here? It looks like we didn't assign a contributor, right @lakchote ? Or is @DylanDylann planning to handle it as it's a regression from their PR?

[DylanDylann]

I will raise a PR to fix soon

[DylanDylann]

I have the same idea as @dukenv0307's proposal and I am creating a PR. @hoangzinh What do you think about it?

[lakchote]

What exactly is the plan here? It looks like we didn't assign a contributor, right @lakchote ? Or is @DylanDylann planning to handle it as it's a regression from their PR?

As said previously in my message, the plan since it's a regression, is for @DylanDylann to handle it.

[dukenv0307]

@lakchote Do we have a payment here since we used my proposal to fix this blocker as mentioned here?

Similar case: https://expensify.slack.com/archives/C01GTK53T8Q/p1707782382926619?thread_ts=1707781846.347169&cid=C01GTK53T8Q (In this case, the contributor still be compensated 50% because they help to point out the RCA even though the proposal is not correct)

cc @bondydaa

[Christinadobrzyn]

Hi! For payment, can someone help me understand who helped with creating the solution and PR?

Was it @lakchote, @dukenv0307 and @DylanDylann? If yes, what amount of compensation seems fair for the work you did? Let me know and I can discuss with @Ollyws

Also, can we move this to Daily?

[hoangzinh]

Also, can we move this to Daily?

I think yes, the PR was merged as well.

[hoangzinh]

This DB is a regression from the PR of @DylanDylann and me #35641.

• @DylanDylann helped to create a PR to fix it.
• @dukenv0307 provided a solution that @DylanDylann used in his PR.

[Christinadobrzyn]

Thanks @hoangzinh - okay if I have this right, then we pay C+ and the contributor with our 50% regression penalty.

@dukenv0307 & @DylanDylann what amount do you think is fair for the work you did?

[DylanDylann]

@Christinadobrzyn My PR causes this regression, I am not eligible for payment here. Let's pay for @dukenv0307 only

[lakchote]

@Christinadobrzyn as noted above, it should be @dukenv0307 that should be paid.

As @DylanDylann produced the regression with his PR, he's not eligible for payment.

[Christinadobrzyn]

Thanks @lakchote and @DylanDylann!

@dukenv0307 what payment do you think is okay for the work you did? $250?

Can you accept this upwork job here so I can pay you?

[dukenv0307]

@dukenv0307 what payment do you think is okay for the work you did? $250?

@Christinadobrzyn Yes I'm good with this ($250), I've accepted the offer.

Thank you!

[Christinadobrzyn]

Awesome! thanks @dukenv0307! I paid you $250 in Upwork. I also ended the other contracts with @Ollyws and @DylanDylann - let me know if we need a new contract for any reason.

Otherwise, I think this can be closed!