[$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 [email protected] 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[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[bot]]

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

[melvin-bot[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: https://github.com/Expensify/App/blob/c884245e7204abad57e01752ec54e4011c1e43e9/src/languages/en.ts#L629

this translation is being called here: https://github.com/Expensify/App/blob/2c9b08fb9196922e3ceb8284668c08c5a939dfea/src/libs/ReportUtils.ts#L2098-L2101

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. https://github.com/Expensify/App/blob/2c9b08fb9196922e3ceb8284668c08c5a939dfea/src/libs/ReportUtils.ts#L2095-L2100

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. https://github.com/Expensify/App/blob/2c9b08fb9196922e3ceb8284668c08c5a939dfea/src/libs/ReportUtils.ts#L2095-L2100

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: https://github.com/Expensify/App/blob/c884245e7204abad57e01752ec54e4011c1e43e9/src/languages/en.ts#L629

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 https://github.com/Expensify/App/issues/34627#issuecomment-1899388204 ?

[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: https://github.com/Expensify/App/blob/a1fe671e94f69a694e2847a6b74685270149e991/src/libs/ReportUtils.ts#L2240

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

[fedirjh]

In this case, let's wait until https://github.com/Expensify/App/pull/34216 is deployed to staging and then we can check if we need any further changes.

cc @peterdbarkerUK I think https://github.com/Expensify/App/pull/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[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 https://github.com/Expensify/App/issues/34627#issuecomment-1908040431 , should we proceed with @neonbhai suggestion in https://github.com/Expensify/App/issues/34627#issuecomment-1911317195 ?

[melvin-bot[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[bot]]

@peterdbarkerUK, @fedirjh, @grgia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[dylanexpensify]

@muttmuure @grgia mind replying here when ya can? 🙇‍♂️

[melvin-bot[bot]]

@fedirjh, @grgia, @muttmuure Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[muttmuure]

Looking!

[muttmuure]

I agree with this:

https://github.com/Expensify/App/issues/34627#issuecomment-1911317195

We should be falling back to the comment.

[muttmuure]

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

[muttmuure]

@fedirjh if you could verify that @neonbhai is correct here or if another of the solutions are correct we can proceed.

[fedirjh]

@muttmuure Thanks for the feedback! Yes @neonbhai Proposal looks good to me. Let's proceed with it.

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

Current assignee @grgia is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[melvin-bot[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