[HOLD for payment 2024-01-11] [HOLD for payment 2024-01-09] [$500] IOU - When adding a receipt to a manual IOU, the error "Receipt missing details" appears

[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.20-0
Reproducible in staging?: Y
Reproducible in production?: N
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. Open New Expensify app
2. Log in with any account
3. Click on FAB > Request Money > Manual
4. Input an amount and click Next
5. Enter/select an option for the request
6. Click on the three-dot menu at the top right
7. Click on "Add Receipt"
8. Click on "Choose File" and Select an image
9. Complete the request

Expected Result:

No error should appear when adding a receipt to a manual IOU

Actual Result:

When adding a receipt to a manual IOU, the error "Receipt missing details" appears

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

Bug6328742_1703884402000.Recording__1008.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014720a73c52b8fa9d
• Upwork Job ID: 1740860349188747264
• Last Price Increase: 2023-12-29
• Automatic offers:
• paultsimura | Contributor | 28077743
• situchan | Contributor | 28080992

Issue Owner

Current Issue Owner: @CortneyOfstad

[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 @joelbettner (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014720a73c52b8fa9d

[melvin-bot]

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

[joelbettner]

@situchan @waterim I think this might be happening because of this PR, but I'm not entirely sure.

[situchan]

cc: @mountiny

[mountiny]

Yeah Someone can try to reproduce, then revert on main and repro again to confirm, I am fixing some issues and we we have weekend to resolve, its been quite complicated Pr so ideally we fix this instead of reverting

[unidev727]

Proposal

from: @unicorndev-727

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

When adding a receipt to a manual IOU, the error "Receipt missing details" appears

What is the root cause of that problem?

The root cause is that we don't have merchat in transaction here.

App/src/libs/TransactionUtils.ts

Line 149 in 595bf40

return isMerchantMissing(transaction) || isAmountMissing(transaction) || isCreatedMissing(transaction);

App/src/libs/TransactionUtils.ts

Lines 132 to 138 in 595bf40

	function isMerchantMissing(transaction: Transaction) {
	const isMerchantEmpty = transaction.merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT || transaction.merchant === '';
	const isModifiedMerchantEmpty = !transaction.modifiedMerchant || transaction.modifiedMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT || transaction.modifiedMerchant === '';
	return isMerchantEmpty && isModifiedMerchantEmpty;
	}

We are sending PARTIAL_TRANSACTION_MERCHANT here and here.
in here

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

We need to move areRequiredFieldsEmpty and hasMissingSmartscanFields from TransactionUtils to ReportUtils.js in order to avoid dependency cycle.
And update hasMissingSmartscanFields to hasMissingSmartscanFieldsForTransaction because we have hasMissingSmartscanFields functions in ReportUtils already.
And should update TransactionUtils.hasMissingSmartscanFields to ReportUtils.hasMissingSmartscanFieldsForTransaction.

/**
 * Check if the transaction has a non-smartscanning receipt and is missing required fields
 */
function hasMissingSmartscanFieldsForTransaction(transaction: OnyxEntry<Transaction>): boolean {
    return Boolean(transaction && TransactionUtils.hasReceipt(transaction) && !TransactionUtils.isDistanceRequest(transaction) && !TransactionUtils.isReceiptBeingScanned(transaction) && areRequiredFieldsEmpty(transaction));
}

function areRequiredFieldsEmpty(transaction: Transaction): boolean {
    const report = getReport(transaction.reportID);
    if (report) {
        const parentReport = getReport(report.parentReportID);
        if (parentReport) {
            return (isExpenseReport(parentReport) && TransactionUtils.isMerchantMissing(transaction)) || TransactionUtils.isAmountMissing(transaction) || TransactionUtils.isCreatedMissing(transaction);
        }
    }
    return TransactionUtils.isAmountMissing(transaction) || TransactionUtils.isCreatedMissing(transaction);
}

screen-capture.2.webm

What alternative solutions did you explore? (Optional)

N/A

[paultsimura]

Proposal

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

What is the root cause of that problem?

We have added extra validations for the missing merchant on transactions with receipt, but did not consider the fact that manual requests can have receipts as well without requiring the merchant.

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

We should persist the following 2 parameters into the transaction when creating the request:

App/src/CONST.ts

Lines 1177 to 1186 in 422f173

	TYPE: {
	SEND: 'send',
	SPLIT: 'split',
	REQUEST: 'request',
	},
	REQUEST_TYPE: {
	DISTANCE: 'distance',
	MANUAL: 'manual',
	SCAN: 'scan',
	},

It's already being used during the request creation, but it's not persisted:

App/src/libs/TransactionUtils.ts

Lines 42 to 49 in 3ba6f55

	function isScanRequest(transaction: Transaction): boolean {
	// This is used during the request creation flow before the transaction has been saved to the server
	if (lodashHas(transaction, 'iouRequestType')) {
	return transaction.iouRequestType === CONST.IOU.REQUEST_TYPE.SCAN;
	}
	return Boolean(transaction?.receipt?.source);
	}

This will give us more fine-grained control over the behavior of different types of transactions.

After it, we should make use of transaction.isScan to check for the missing fields instead of hasReceipt(transaction) (as manual requests can have receipts too, and they don't require the SmartScan-required fields) here:

App/src/libs/TransactionUtils.ts

Lines 415 to 420 in 3ba6f55

	/**
	* Check if the transaction has a non-smartscanning receipt and is missing required fields
	*/
	function hasMissingSmartscanFields(transaction: OnyxEntry<Transaction>): boolean {
	return Boolean(transaction && hasReceipt(transaction) && !isDistanceRequest(transaction) && !isReceiptBeingScanned(transaction) && areRequiredFieldsEmpty(transaction));
	}

What alternative solutions did you explore? (Optional)

[abzokhattab]

Proposal

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

When adding a receipt to a manual IOU, the error "Receipt missing details" appears

What is the root cause of that problem?

the condition that decides whether error on the merchant field should be shown or not in the MoneyReportView is not in sync with the confirmlation list:

App/src/components/MoneyRequestConfirmationList.js

Line 271 in b1c6883

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

App/src/components/ReportActionItem/MoneyRequestView.js

Lines 243 to 250 in f2b74e8

	description={translate('common.merchant')}
	title={transactionMerchant}
	interactive={canEdit}
	shouldShowRightIcon={canEdit}
	titleStyle={styles.flex1}
	onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.MERCHANT))}
	brickRoadIndicator={hasErrors && isEmptyMerchant ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
	error={hasErrors && isEmptyMerchant ? translate('common.error.enterMerchant') : ''}

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

We need to add isPolicyExpenseChat to TransactionUtils.hasMissingSmartscanFields to be in sync with the condition above.
so we need to change the function to:

function hasMissingSmartscanFields(transaction: OnyxEntry<Transaction>): boolean {
    const report = ReportUtils.getReport(transaction?.reportID);
    const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
    return Boolean(
        transaction && hasReceipt(transaction) && isPolicyExpenseChat && !isDistanceRequest(transaction) && !isReceiptBeingScanned(transaction) && areRequiredFieldsEmpty(transaction),
    );
}

POC

Screen.Recording.2023-12-30.at.1.51.48.AM.mov

[mountiny]

To clarify the expected behaviour here, the problem is about the fact this is P2P IOU request and in this case we do not require the merchant with the receipt (different case for workspace chats in which case this error is correct)

So I think the solution here is just to see if the parent report is expense type and only then show this violation

[Tony-MK]

Proposal

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

What is the root cause of that problem?

The hasMissingSmartscanFields is true because the transaction has an empty merchant. Hence the areRequiredFieldsEmpty function will be true.

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

Move and export the areRequiredFieldsEmpty function to IOU.js. Then, update the places where TransactionUtils.areRequiredFieldsEmpty is being used to IOU.areRequiredFieldsEmpty.

function areRequiredFieldsEmpty(transaction){
    return (TransactionUtils.isMerchantMissing(transaction) && ReportUtils.isExpenseReport(ReportUtils.getParentReport(transaction.reportID))) || TransactionUtils.isAmountMissing(transaction) || TransactionUtils.isCreatedMissing(transaction);
}

What alternative solutions did you explore? (Optional)

Option A
Just like in IOU.js, TaskUtil.ts, ReportUtils.ts, and many others, we can connect to the onyx and get only parent reports from ONYXKEYS.COLLECTION.REPORT. Then, store them in an array in TransactionUtils.js like such: let allReports: OnyxCollection<Report>.

Option B
We could also import allReports from one of the mentioned files. Then, we can use allReports to find the parent report of the transaction.

The last action for both options A and B will be checking if is an expense report with parentReport?.type === CONST.REPORT.TYPE.EXPENSE.

[unidev727]

@mountiny
I update proposal and added the result video.

[joelbettner]

@Tony-MK I like your proposal because areRequiredFieldsEmpty is used in several different places, so moving the logic into that method will prevent us from having to keep logic updated everywhere it is called.

We will just need to import ReportUtils into the TransactionUtils.ts file in order to be able to call isExpenseReport, and then we will also need to make some changes to isExpenseReport so it can take a report object or a reportID (see ReportUtils.isMoneyRequest as an example).

Edit: I just saw the updated proposal from @unicorndev-727 that mentions a dependancy loop if we import ReportUtils into TransactionUtils. @situchan / @eVoloshchak your thoughts?

[Tony-MK]

Kindly, review my updated prospal. Thanks

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

[mountiny]

I do feel like all the proposals are quite good, @paultsimura seems the simplest but is there going to be the dependency cycle in your solutions?

[paultsimura]

is there going to be the dependency cycle in your solutions?

There shouldn't be, since all the fields will be on the same transaction object.
However, I think we'll need the BE changes as well in order to persist the transaction.iouType not only optimistically when creating the request.
But as for me, this data is quite useful and worth adding.

[mountiny]

I dont think we need that, all thats needed is the fact if the expense is on group or personal policy, which in exchange can be checked by the report type.

Are you able to make a PR with a that fix and verify there are no dependency cycles?

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (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]

📣 @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]

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

• fix: Make merchant required only on Expense reports #33850

If no regressions arise, payment will be issued on 2024-01-11. 🎊

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 — @paultsimura paid $250 via Upwork; @Tony-MK paid $250 via Upwork
• Contributor+ that helped on the issue and/or PR — @situchan paid $500 via Upwork

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

• @paultsimura 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.
• [@CortneyOfstad] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Tony-MK]

Hey everyone,

Advice by @mountiny, this comment is a continuation of this slack discussion.

Below are the PR changes meant the fix this issue from his Proposal.

App/src/libs/TransactionUtils.ts

Lines 33 to 38 in ff95c0c

	let allReports: OnyxCollection<Report>;
	Onyx.connect({
	key: ONYXKEYS.COLLECTION.REPORT,
	waitForCollectionCallback: true,
	callback: (value) => (allReports = value),
	});

App/src/libs/TransactionUtils.ts

Lines 159 to 161 in ff95c0c

	const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transaction?.reportID}`] ?? null;
	const isFromExpenseReport = parentReport?.type === CONST.REPORT.TYPE.EXPENSE;
	return (isFromExpenseReport && isMerchantMissing(transaction)) || isAmountMissing(transaction) || isCreatedMissing(transaction);

Also, here are the suggestions for my proposal's alternative solution A presented.

What alternative solutions did you explore? (Optional)

Option A Just like in IOU.js, TaskUtil.ts, ReportUtils.ts, and many others, we can connect to the onyx and get only parent reports from ONYXKEYS.COLLECTION.REPORT. Then, store them in an array in TransactionUtils.js like such: let allReports: OnyxCollection<Report>.

The last action for both options A and B will be checking if is an expense report with parentReport?.type === CONST.REPORT.TYPE.EXPENSE.

[mountiny]

Note for @CortneyOfstad I think @paultsimura did not do this intentionally but in Slack mentioned he is happy to split the reward for fairness. In that case we can pay $250 each for @paultsimura and @Tony-MK

$500 to @situchan

[CortneyOfstad]

@Tony-MK sent you a proposal — please let me know once you accept so I can get that paid to you 👍

@paultsimura — paid you $250 per the comment above via Upwork

@situchan — paid you $500 via Upwork

[Tony-MK]

Thanks, @CortneyOfstad . I accepted the offer.

[CortneyOfstad]

Paid!

Payment Summary

Contributor that fixed the issue — @paultsimura paid $250 via Upwork; @Tony-MK paid $250 via Upwork
Contributor+ that helped on the issue and/or PR — @situchan paid $500 via Upwork

[CortneyOfstad]

@situchan any updates on the checklist above? TIA!

[situchan]

• The PR that introduced the bug has been identified. Link to the PR: Feature: Update merchant to be empty and with validation  #32486 (comment)
• 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: Feature: Update merchant to be empty and with validation  #32486
• 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: N/A
• Determine if we should create a regression test for this bug.
• 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.

This was deploy blocker. Caught during regression testing so no need another one.

[CortneyOfstad]

Perfect! Closing!