[HOLD for payment 2024-02-01] [$500] Bug: description instead of merchant being used for expense room titles

[dylanexpensify]

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.15-1
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: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C03U7DCU4/p1704382128969099

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Click on an Expense that has the merchant field populated
3. Add a description to the expense

Expected Result:

The description is saved and room title doesn't change

Actual Result:

The expense room title changes to use the description instead of the merchant

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f555a808f6be075d
• Upwork Job ID: 1737996800572018688
• Last Price Increase: 2024-01-05
• Automatic offers:
• dukenv0307 | Contributor | 28091153

[melvin-bot]

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

[dukenv0307]

Proposal

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

The expense room title changes to use the description instead of the merchant

What is the root cause of that problem?

In here, we're using the transaction description in the for part

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

Remove the comment, or use merchant there instead. Currently we never use merchant in the for part, but depending on requirement we can show it there (and fall back on the description if necessary)

Here, it can be something like

comment: !TransactionUtils.isMerchantMissing(transaction) ? transactionDetails?.merchant : transactionDetails?.comment ?? '',

We should have the confirmation here to determine exact implementation because it's not clear yet from the OP.

If we only want this behavior in the room title, we can pass in a flag to getReportName to control it

What alternative solutions did you explore? (Optional)

There seems to be a bug in isMerchantMissing that might affect this issue when we use it as well. The problem is if the merchant is initially set when the money request is created, then it's removed. isMerchantMissing is still true.

I believe this is affecting other places that use isMerchantMissing as well, to fix it we need to update isMerchantMissing to

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

    return isMerchantEmpty;
}

So that if the modifiedMerchant is defined, it will be the source of truth when validating merchant, because when it's defined, that means it's the current merchant state of the transaction (not the initial merchant field. This will fix all places that are using isMerchantMissing

[paultsimura]

Proposal

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

The transaction thread title does not prioritize the merchant over the description for Expense requests

What is the root cause of that problem?

We are using this function for building the Transaction Report title:

App/src/libs/ReportUtils.ts

Lines 1987 to 2015 in 9b74851

	function getTransactionReportName(reportAction: OnyxEntry<ReportAction>): string {
	if (ReportActionsUtils.isReversedTransaction(reportAction)) {
	return Localize.translateLocal('parentReportAction.reversedTransaction');
	}
	if (ReportActionsUtils.isDeletedAction(reportAction)) {
	return Localize.translateLocal('parentReportAction.deletedRequest');
	}
	const transaction = TransactionUtils.getLinkedTransaction(reportAction);
	if (!isNotEmptyObject(transaction)) {
	// Transaction data might be empty on app's first load, if so we fallback to Request
	return Localize.translateLocal('iou.request');
	}
	if (TransactionUtils.hasReceipt(transaction) && TransactionUtils.isReceiptBeingScanned(transaction)) {
	return Localize.translateLocal('iou.receiptScanning');
	}
	if (TransactionUtils.hasMissingSmartscanFields(transaction)) {
	return Localize.translateLocal('iou.receiptMissingDetails');
	}
	const transactionDetails = getTransactionDetails(transaction);
	return Localize.translateLocal(ReportActionsUtils.isSentMoneyReportAction(reportAction) ? 'iou.threadSentMoneyReportName' : 'iou.threadRequestReportName', {
	formattedAmount: CurrencyUtils.convertToDisplayString(transactionDetails?.amount ?? 0, transactionDetails?.currency, TransactionUtils.isDistanceRequest(transaction)) ?? '',
	comment: transactionDetails?.comment ?? '',
	});
	}

It uses only transactionDetails.comment here:

App/src/libs/ReportUtils.ts

Lines 2011 to 2014 in 9b74851

	return Localize.translateLocal(ReportActionsUtils.isSentMoneyReportAction(reportAction) ? 'iou.threadSentMoneyReportName' : 'iou.threadRequestReportName', {
	formattedAmount: CurrencyUtils.convertToDisplayString(transactionDetails?.amount ?? 0, transactionDetails?.currency, TransactionUtils.isDistanceRequest(transaction)) ?? '',
	comment: transactionDetails?.comment ?? '',
	});

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

If the transaction belongs to an expense report, we should prioritize using the merchant instead of comment:

    const transactionDetails = getTransactionDetails(transaction);
    const isEmptyMerchant = !transactionDetails?.merchant || transactionDetails?.merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT;
    const comment = TransactionUtils.isFromExpenseReport(transaction) && !isEmptyMerchant ? transactionDetails?.merchant : transactionDetails?.comment;

    return Localize.translateLocal(ReportActionsUtils.isSentMoneyReportAction(reportAction) ? 'iou.threadSentMoneyReportName' : 'iou.threadRequestReportName', {
        formattedAmount: CurrencyUtils.convertToDisplayString(transactionDetails?.amount ?? 0, transactionDetails?.currency, TransactionUtils.isDistanceRequest(transaction)) ?? '',
        comment: comment ?? '',
    });

For this, we also need to move the isFromExpenseReport logic from this function into a separate one for reuse:

function isFromExpenseReport(transaction: Transaction): boolean {
    const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transaction?.reportID}`] ?? null;
    return parentReport?.type === CONST.REPORT.TYPE.EXPENSE;
}

function areRequiredFieldsEmpty(transaction: Transaction): boolean {
    return (isFromExpenseReport(transaction) && isMerchantMissing(transaction)) || isAmountMissing(transaction) || isCreatedMissing(transaction);
}

App/src/libs/TransactionUtils.ts

Lines 158 to 162 in 82b76c1

	function areRequiredFieldsEmpty(transaction: Transaction): boolean {
	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);
	}

What alternative solutions did you explore? (Optional)

If we want to prioritize the merchant for every transaction, not just the ones that belong to the Expense report, we should plainly use:

    const transactionDetails = getTransactionDetails(transaction);
    const isEmptyMerchant = !transactionDetails?.merchant || transactionDetails?.merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT;
    const comment = !isEmptyMerchant ? transactionDetails?.merchant : transactionDetails?.comment;

    return Localize.translateLocal(ReportActionsUtils.isSentMoneyReportAction(reportAction) ? 'iou.threadSentMoneyReportName' : 'iou.threadRequestReportName', {
        formattedAmount: CurrencyUtils.convertToDisplayString(transactionDetails?.amount ?? 0, transactionDetails?.currency, TransactionUtils.isDistanceRequest(transaction)) ?? '',
        comment: comment ?? '',
    });

Note: we shouldn't use TransactionUtils.isMerchantMissing(transaction) for checking if the merchant is empty here, because it will return false if the transaction was created with a merchant, and then the merchant was removed. In this case, we will show "{amount} for (none)" in the title.

[dukenv0307]

Currently the for part will show description (if exists), it not exists, it doesn't show for in room title.

@dylanexpensify @shawnborton can you clarify the expectation here:

1. It should always show merchant in for, if there's no merchant, doesn't show for in room title
2. It should prioritize showing merchant in for, if there's no merchant, fall back to the transaction description, if no description, doesn't show for in room title
3. It should not show for at all

[paultsimura]

Proposal

Updated proposal #34029 (comment)

[dukenv0307]

Proposal updated to add example code changes

[melvin-bot]

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

[sobitneupane]

@dylanexpensify @shawnborton Just to clarify, are we aiming to display the merchant if it's available, and if not, show the description?

Pseudocode:

If merchant:
  $1.30 request for ${merchant}
else if description:
  $1.30 request for ${description}
else:
  $1.30 request

[shawnborton]

Yup, that's my understanding.

cc @trjExpensify @JmillsExpensify

[sobitneupane]

@dukenv0307

If we only want this behavior in the room title, we can pass in a flag to getReportName to control it

Where else do we use this pattern except room title? Can you please add more details.

[sobitneupane]

@paultsimura

If we want to prioritize the merchant for every transaction, not just the ones that belong to the Expense report,

What other transaction(other than that of Expense report) are we talking about in the alternative solution? And how will they be affected if we go with alternative solution.

[paultsimura]

What other transaction(other than that of Expense report) are we talking about in the alternative solution?

The IOU (1:1 and split) transactions of any type.
The reason why I initially thought we'd like to prioritize merchant only for the Expense transactions (the ones requested from a workspace) is because recently there were a lot of changes to make merchant required only for Expense requests, not IOU ones. Plus, the issue title says "expense room titles".

How will they be affected if we go with alternative solution

We will show the merchant if it's present. Otherwise – show the description. For any transaction, regardless of its type (1:1, split, expense (workspace)).

[dukenv0307]

Proposal updated to additionally address an existing bug with isMerchantMissing

[dukenv0307]

Where else do we use this pattern except room title? Can you please add more details.

@sobitneupane it's the getReportName so it's used in a lot of places: LHN, avatar tootltip, report settings page, ...

But I do think all those places should be consistent, I add it just in case there're places that we don't want it. But I don't find any for now.

[dylanexpensify]

Nice! @sobitneupane to review!

[trjExpensify]

Yup, that's my understanding.

cc @trjExpensify @JmillsExpensify

Yep, agreed. Merchant > Description > Blank!

[sobitneupane]

Thanks for the proposal @paultsimura and @dukenv0307

Both of the proposals are very similar. Looking at the edit history, both the proposals were incomplete when initially posted. Please try to post a complete proposal.

Approach followed in both proposals is same (from the initial proposal) and implementation is not much different.

I think it's good idea to make use of existing function. Let's go with @dukenv0307's proposal (it was initially posted proposal with correct RCA and approach). Let's solve the issue in isMerchantMissing if any.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[dylanexpensify]

Nice! @sobitneupane let's get it!

[dylanexpensify]

bump @sobitneupane

[sobitneupane]

I am reviewing the PR. The changes are looking good. It should be ready for final review today.

[dylanexpensify]

How's this going @sobitneupane

[sobitneupane]

PR merged. Waiting for it to deploy in production.

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

• Display merchant in report name of transaction report if exist #34216

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

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

• @sobitneupane requires payment through NewDot Manual Requests
• @dukenv0307 requires payment automatic offer (Contributor)

[dylanexpensify]

payment coming up!

[dylanexpensify]

Payment summary:

• Contributor: @dukenv0307 $500
• Contributor+: @sobitneupane $500

Please request!

[dylanexpensify]

@sobitneupane can you complete checklist?

Payment sent @dukenv0307

[dylanexpensify]

bump @sobitneupane

[sobitneupane]

@dylanexpensify I am not sure if reviewer checklist is valid here. The code was introduced first by this #34216 PR as per the need back then. I think it is more of a feature request than bug.

[sobitneupane]

Regression Test Proposal:

1. Request money with merchant field populated and the description field empty.
2. Verify that the expense report header contains merchant.
3. Add description in the expense report.
4. Verify that the expense report header still shows merchant not description.

Do we agree 👍 or 👎

[JmillsExpensify]

$500 approved for @sobitneupane based on this summary.