[$250] Hold is not displayed in the preview for the expense which is held

[m-natarajan]

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.11-5
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: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1721848036803009

Action Performed:

1. Submit a report with 2 expenses and with a violation
2. Sign in as a approver
3. Open the report
4. Put the expense on Hold which has violation
5. Look at the expense preview

Expected Result:

Preview should show "Hold"

Actual Result:

Violation error only appears. Hold should take higher priority

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

Recording.382.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01081754863788f760
• Upwork Job ID: 1819385736069367085
• Last Price Increase: 2024-08-16
• Automatic offers:
• shubham1206agra | Reviewer | 103593386

Issue Owner

Current Issue Owner: @dylanexpensify

[melvin-bot]

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

[dominictb]

Proposal

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

Violation error only appears. Hold should take higher priority

What is the root cause of that problem?

We always display violation message or error message in:

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 200 to 222 in a79189f

	if (violations?.[0]) {
	const violationMessage = ViolationsUtils.getViolationTranslation(violations[0], translate);
	const violationsCount = violations.filter((v) => v.type === CONST.VIOLATION_TYPES.VIOLATION).length;
	const isTooLong = violationsCount > 1 || violationMessage.length > 15;
	const hasViolationsAndFieldErrors = violationsCount > 0 && hasFieldErrors;
	return `${message} ${CONST.DOT_SEPARATOR} ${isTooLong || hasViolationsAndFieldErrors ? translate('violations.reviewRequired') : violationMessage}`;
	}
	if (hasFieldErrors) {
	const isMerchantMissing = TransactionUtils.isMerchantMissing(transaction);
	const isAmountMissing = TransactionUtils.isAmountMissing(transaction);
	if (isAmountMissing && isMerchantMissing) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('violations.reviewRequired')}`;
	} else if (isAmountMissing) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('iou.missingAmount')}`;
	} else if (isMerchantMissing) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('iou.missingMerchant')}`;
	}
	return message;
	}
	if (shouldShowHoldMessage) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('violations.hold')}`;
	}

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

Should use:

+if (violations?.[0] && !shouldShowHoldMessage) { // Can use violations?.[0] && !isOnHold
                const violationMessage = ViolationsUtils.getViolationTranslation(violations[0], translate);
                const violationsCount = violations.filter((v) => v.type === CONST.VIOLATION_TYPES.VIOLATION).length;
                const isTooLong = violationsCount > 1 || violationMessage.length > 15;
                const hasViolationsAndFieldErrors = violationsCount > 0 && hasFieldErrors;

                return `${message} ${CONST.DOT_SEPARATOR} ${isTooLong || hasViolationsAndFieldErrors ? translate('violations.reviewRequired') : violationMessage}`;
            }
          if (hasFieldErrors) {
                const isMerchantMissing = TransactionUtils.isMerchantMissing(transaction);
                const isAmountMissing = TransactionUtils.isAmountMissing(transaction);
                if (isAmountMissing && isMerchantMissing) {
                    message += ` ${CONST.DOT_SEPARATOR} ${translate('violations.reviewRequired')}`;
                } else if (isAmountMissing) {
                    message += ` ${CONST.DOT_SEPARATOR} ${translate('iou.missingAmount')}`;
                } else if (isMerchantMissing) {
                    message += ` ${CONST.DOT_SEPARATOR} ${translate('iou.missingMerchant')}`;
                }
                return message;
            }
            if (shouldShowHoldMessage) {
                message += ` ${CONST.DOT_SEPARATOR} ${translate('violations.hold')}`;
            }

We can consider applying the same to hasFieldErrors condition.

[bernhardoj]

Proposal

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

Hold error message isn't prioritized to be displayed.

What is the root cause of that problem?

This is actually the same issue as #44509. However, the selected solution doesn't prioritize hold, but prioritize violation type of violation. I also posted a proposal there to prioritize the hold, so I'm gonna copy and update it here a little bit since we also want to prioritize hold over review required when there is too many violations.

When showing the violation, we only take the first violation.

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 196 to 207 in b0f810d

	if (shouldShowRBR && transaction) {
	const violations = TransactionUtils.getTransactionViolations(transaction.transactionID, transactionViolations)?.sort((a) =>
	a.type === CONST.VIOLATION_TYPES.VIOLATION ? -1 : 0,
	);
	if (violations?.[0]) {
	const violationMessage = ViolationsUtils.getViolationTranslation(violations[0], translate);
	const violationsCount = violations.filter((v) => v.type === CONST.VIOLATION_TYPES.VIOLATION).length;
	const isTooLong = violationsCount > 1 || violationMessage.length > 15;
	const hasViolationsAndFieldErrors = violationsCount > 0 && hasFieldErrors;
	return `${message} ${CONST.DOT_SEPARATOR} ${isTooLong || hasViolationsAndFieldErrors ? translate('violations.reviewRequired') : violationMessage}`;
	}

We previously prioritized hold in this PR, but was reverted because it caused double Hold to show up. It's because we append the violation message (that contains hold) with another hold text. But it's not guaranteed that the hold violation is always at the first item of the violations.

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

If we want the hold to be prioritized over other violations, including Review required, then we can move this condition higher in the block.

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 220 to 222 in b0f810d

	if (shouldShowHoldMessage) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('violations.hold')}`;
	}

if (shouldShowHoldMessage) return hold message
if (violations[0]) ...
...

Last, we can "revert" #45325 because we fix it differently here.

[melvin-bot]

@dylanexpensify Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01081754863788f760

[melvin-bot]

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

[melvin-bot]

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

[shubham1206agra]

Last, we can "revert" #45325 because we fix it differently here.

@bernhardoj Can you give a proper solution here?
Thanks

[bernhardoj]

Last, we can "revert" #45325 because we fix it differently here.

@bernhardoj Can you give a proper solution here?

Sorry, I don't get what do you mean by proper solution of the revert. Can you clarify it?

[shubham1206agra]

Sorry, I don't get what do you mean by proper solution of the revert. Can you clarify it?

@bernhardoj After reverting PR, what will be the solution to fix this issue?

[bernhardoj]

This one

The current condition looks like this:

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 194 to 216 in 0a33186

	if (violations?.[0]) {
	const violationMessage = ViolationsUtils.getViolationTranslation(violations[0], translate);
	const violationsCount = violations.filter((v) => v.type === CONST.VIOLATION_TYPES.VIOLATION).length;
	const isTooLong = violationsCount > 1 || violationMessage.length > 15;
	const hasViolationsAndFieldErrors = violationsCount > 0 && hasFieldErrors;
	return `${message} ${CONST.DOT_SEPARATOR} ${isTooLong || hasViolationsAndFieldErrors ? translate('violations.reviewRequired') : violationMessage}`;
	}
	if (hasFieldErrors) {
	const isMerchantMissing = TransactionUtils.isMerchantMissing(transaction);
	const isAmountMissing = TransactionUtils.isAmountMissing(transaction);
	if (isAmountMissing && isMerchantMissing) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('violations.reviewRequired')}`;
	} else if (isAmountMissing) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('iou.missingAmount')}`;
	} else if (isMerchantMissing) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('iou.missingMerchant')}`;
	}
	return message;
	}
	if (shouldShowHoldMessage) {
	message += ` ${CONST.DOT_SEPARATOR} ${translate('violations.hold')}`;
	}

The solution will update it to:

if (shouldShowHoldMessage) return `${message} ${CONST.DOT_SEPARATOR} ${translate('violations.hold')}`;
if (violations[0]) ...
if (hasFieldErrors) ...

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[shubham1206agra]

Lets go with @bernhardoj's proposal

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[bernhardoj]

PR is ready

cc: @shubham1206agra

[dylanexpensify]

Woot!

[dylanexpensify]

merged!

[shubham1206agra]

@dylanexpensify Can you add labels for payment?

[dylanexpensify]

pending prod deploy

[shubham1206agra]

@dylanexpensify This was deployed to prod 2 weeks ago

[roryabraham]

Sorry for the confusion @dylanexpensify. I can confirm this was deployed to prod on 2024-08-28

[dylanexpensify]

Payment summary:

Contributor: @bernhardoj $250
Contributor+: @shubham1206agra $250

Please apply/request!

[dylanexpensify]

@shubham1206agra please accept offer! 🙇‍♂️

[dylanexpensify]

@bernhardoj please apply to job! 🙇‍♂️

[shubham1206agra]

@dylanexpensify Offer accepted

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj

[shubham1206agra]

@dylanexpensify Bump here

[dylanexpensify]

done!