[HOLD for payment 2024-02-26] [$500] [ECARD TRANSACTIONS] Header in expense view shows $%amount% request instead of $%amount% for %merchant%

[kevinksullivan]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue reported by: @kevinksullivan
Slack conversation: https://expensify.slack.com/archives/C05DWUDHVK7/p1705358211614989

Action Performed:

1. Create a free workspace and set it as your default
2. Incur a charge on an Expensify Card
3. Tap into the report

Expected Result:

The report title should use the format $%amount% for %merchant%

Actual Result:

The report title shows $%amount% request

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0122f6862da1ad40a2
• Upwork Job ID: 1748127879346397184
• Last Price Increase: 2024-02-01
• Automatic offers:
• fedirjh | Reviewer | 28144607
• neonbhai | Contributor | 28144608

[melvin-bot]

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

[grgia]

Changes Needed

if the transaction is non-reimbursable (transaction.reimbursable = false) then the report header should be %amount% for %merchant% (transaction.amount and transaction.merchant)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0122f6862da1ad40a2

[melvin-bot]

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

[abzokhattab]

Proposal

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

if the transaction is non-reimbursable then the report header should be %amount% for %merchant% (transaction.amount and transaction.merchant)

What is the root cause of that problem?

here we have the translation for the iou header:

App/src/languages/en.ts

Line 629 in c884245

threadRequestReportName: ({formattedAmount, comment}: ThreadRequestReportNameParams) => `${formattedAmount} request${comment ? ` for ${comment}` : ''}`,

this translation is being called here:

App/src/libs/ReportUtils.ts

Lines 2098 to 2101 in 2c9b08f

	formattedAmount: CurrencyUtils.convertToDisplayString(transactionDetails?.amount ?? 0, transactionDetails?.currency, TransactionUtils.isDistanceRequest(transaction)) ?? '',
	comment: transactionDetails?.comment ?? '',
	});
	}

now we need to add the merchant

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

we need to change the comment to merchant and with keeping the transaction.reimbursable condition into consideration:

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

we can remove the comment in the else statement but i kept it for backeward compibabilty .. however we can rename the comment argument in the translation function to a more generic term since this attribute now can be either comment or merchant

[Tony-MK]

Proposal

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

[ECARD TRANSACTIONS] Header in expense view shows %amount% request instead of `%amount%`` for %merchant%

What is the root cause of that problem?

We are not translating money request the Expensify card when creating the name of the transaction request.
This happens in the ReportUtils.getTransactionReportName function.

App/src/libs/ReportUtils.ts

Lines 2095 to 2100 in 2c9b08f

	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 ?? '',
	});

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

const shouldShowMerchant = 
 !transaction?.reimbursable &&
 transactionDetails?.merchant &&
 !TransactionUtils.isDistanceRequest(transaction) && 
 !TransactionUtils.isExpensifyCardTransaction(transaction);

We use the boolean value of shouldShowMerchant to determine whether the transaction was paid by an Expensify card.

TransactionUtils.isExpensifyCardTransaction(transaction) will be used to check if the money request was paid by an Expensify Card.

We can remove the !TransactionUtils.isDistanceRequest condition if the merchant of a distance request is required.

Furthermore, we should check if the transaction is a distance request to ensure the money request header title is not too long.

These checks should be needed to properly display the header title without affecting reports paid by other payment methods such as Paid Elsewhere.

Therefore, should create a condition to set the comment property using shouldShowMerchant.

comment: shouldShowMerchant?  transactionDetails?.merchant : transactionDetails?.comment ?? '',

[Tony-MK]

Proposal

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

[ECARD TRANSACTIONS] Header in expense view shows %amount% request instead of %amount% for %merchant%

What is the root cause of that problem?

We are not translating money request the Expensify card when creating the name of the transaction request.
This happens in the ReportUtils.getTransactionReportName function.

App/src/libs/ReportUtils.ts

Lines 2095 to 2100 in 2c9b08f

	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 ?? '',
	});

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

const shouldShowMerchant = 
 !transaction?.reimbursable &&
 transactionDetails?.merchant &&
 !TransactionUtils.isDistanceRequest(transaction) && 
 !TransactionUtils.isExpensifyCardTransaction(transaction);

We use the boolean value of shouldShowMerchant to determine whether the transaction was paid by an Expensify card.

TransactionUtils.isExpensifyCardTransaction(transaction) will be used to check if the money request was paid by an Expensify Card.

We can remove the !TransactionUtils.isDistanceRequest condition if the merchant of a distance request is required.

Furthermore, we should check if the transaction is a distance request to ensure the money request header title is not too long.

These checks should be needed to properly display the header title without affecting reports paid by other payment methods such as Paid Elsewhere.

Therefore, should create a condition to set the comment property using shouldShowMerchant.

comment: shouldShowMerchant?  transactionDetails?.merchant : transactionDetails?.comment ?? '',

[neonbhai]

Proposal

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

Header in expense view shows $%amount% request instead of $%amount% for %merchant%

What is the root cause of that problem?

We have not formatted the language function correctly, and not passed in the merchant:

App/src/languages/en.ts

Line 629 in c884245

threadRequestReportName: ({formattedAmount, comment}: ThreadRequestReportNameParams) => `${formattedAmount} request${comment ? ` for ${comment}` : ''}`,

What changes do you think we should make in order to solve the problem?
We can either:

We can either:

• Pass merchant as comment if it is available, format threadRequestReportName like this:

  threadRequestReportName: ({formattedAmount, comment}: ThreadRequestReportNameParams) => 
  `${formattedAmount} ${comment ? ` for ${comment}` : 'request'}`,

  This makes sure the language is correct for all four cases possible
  • Request without comment and merchant: $1.50 request
  • Request with comment and merchant: $1.50 for merchant
  • Request with just comment: $1.50 for custom-comment
  • Request with just merchant: $1.50 for merchant

Alternatively

We can contruct the phrase by passing merchant as third param to threadRequestReportName

[peterdbarkerUK]

@fedirjh few proposals for your review here when you get the chance!

[fedirjh]

@abzokhattab your solution does not match the expected result; could you please verify that it displays the correct result?

@Tony-MK Why do we have to exclude request that was paid by an Expensify Card ?

@neonbhai It is intentional that you didn’t include the checks for non-reimbursable state mentioned in this comment #34627 (comment) ?

[neonbhai]

@fedirjh hi, we now pass merchant for reimbursable transactions also. Latest main code was updated in this PR to always use merchant if available:

App/src/libs/ReportUtils.ts

Line 2240 in a1fe671

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

So a check for the reimbursable state is now not needed.

[fedirjh]

In this case, let's wait until #34216 is deployed to staging and then we can check if we need any further changes.

cc @peterdbarkerUK I think #34216 has covered this issue. The header title will be displayed as :

1. $%amount% request for %merchant% -> if it has a merchant
2. $%amount% request for %comment% -> if it merchant is missing and it has a comment
3. $%amount% request -> if it doesn’t have a merchant nor a comment

Should we still require any further changes?

[melvin-bot]

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

[neonbhai]

I think the bug here is that the Header should be:

1. $%amount% for %merchant% -> if it has a merchant
2. $%amount% for %comment% -> if it merchant is missing and it has a comment

i.e, as the title says, we shouldn't use the request word

cc: @fedirjh

[fedirjh]

cc @grgia @peterdbarkerUK friendly bump for #34627 (comment) , should we proceed with @neonbhai suggestion in #34627 (comment) ?

[melvin-bot]

@peterdbarkerUK @fedirjh @grgia 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]

📣 @neonbhai 🎉 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 📖

[grgia]

Thank you @muttmuure @fedirjh! Assigned @neonbhai

[kevinksullivan]

just catching up, but @neonbhai is on the right track there! Here is what is specifically proposed in the card transactions doc:

When there is a merchant or description associated with an expense of any kind, the workspace request title will be ${amount} for {merchant or description}. If neither exist on the expense (i.e. a manual request where neither field was completed), the workspace request title will be ${amount} request

So the order would be:

$%amount% for %merchant% -> if it has a merchant
$%amount% for %comment% -> if it has no merchant and has a comment
%$amount% request -> if it has no merchant or description

[melvin-bot]

@fedirjh @grgia @muttmuure @neonbhai this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[neonbhai]

Raising PR shortly!

[grgia]

thanks @neonbhai !

[melvin-bot]

@fedirjh, @grgia, @muttmuure, @neonbhai Whoops! This issue is 2 days overdue. Let's get this updated quick!

[neonbhai]

Finalizing PR shortly

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

• Fix: Expense View Header  #36109

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

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

• @fedirjh requires payment automatic offer (Reviewer)
• @neonbhai 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:

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

[fedirjh]

BugZero Checklist:

• Link to the PR: N/A It is more of a feature request than a bug.
• Link to comment: N/A
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: Yes

---

Regression Test Proposal

1. Open chat with any user
2. Click on FAB -> Request Money
3. Enter an amount
4. Leave Merchant and Description empty, then create Request
5. Go to the Request View
6. Verify Header as the title "{amount} request"
7. Add a description to the request
8. Verify the header title changes to "{amount} for {description}"
9. Add a merchant to the title
10. Verify the header title changes to "{amount} for {merchant}"

• Do we agree 👍 or 👎

[muttmuure]

Paid