[WAITING ON CHECKLIST] [$250] Expense Details - The header of the expense details displays "[amount] for [Description]"

lanitochka17 commented 1 month ago

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

Version Number: 9.0.42-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5023024 Email or phone of affected tester (no customers): biruknew45+1278@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Navigate to https://staging.new.expensify.com/
Create a workspace
Go to the workspace chat
Click the plus button > Submit Expense > enter amount, description, and merchant, and complete the submission
Open the expense details

Expected Result:

The conversation title in the LHN should display "[amount] for [Merchant]"

Actual Result:

The conversation title in the LHN displays "[amount] for [Description]"

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/68534efe-26e9-4652-a19d-ae6e42395f3c

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021841274388644598293
Upwork Job ID: 1841274388644598293
Last Price Increase: 2024-10-02

Issue Owner

Current Issue Owner: @

melvin-bot[bot] commented 1 month ago

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

lanitochka17 commented 1 month ago

@jliexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

nkdengineer commented 1 month ago

Proposal

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

What is the root cause of that problem?

We pass the comment as description here then LHN displays amount for description

https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L3471

https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L3482

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

Here we should get the merchant first then fallback to description if the merchant is empty

let comment = !isEmptyObject(linkedTransaction) ? TransactionUtils.getMerchant(linkedTransaction) || TransactionUtils.getDescription(linkedTransaction) : undefined;

https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L3471

OPTIONAL: we can also do the same here

What alternative solutions did you explore? (Optional)

bernhardoj commented 1 month ago

Proposal

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

The last message when we submit an expense with a merchant and description will show the description instead of the merchant.

What is the root cause of that problem?

We always use the description here. https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L3471 https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L3482

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

We need to follow the same logic from getTransactionReportName which checks whether the merchant is empty or not before using it because (none) merchant is also empty. https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L3322

let comment = !isEmptyObject(linkedTransaction) ? (!TransactionUtils.isMerchantMissing(linkedTransaction) ? TransactionUtils.getMerchant(linkedTransaction) : TransactionUtils.getDescription(linkedTransaction)) : undefined;

We can apply the same fix here https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L3373 https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L3395 https://github.com/Expensify/App/blob/d46f21888154dc77a799e06d22e576eaece80368/src/libs/ReportUtils.ts#L7193

NOTE: I think we can create a getMerchantOrDescription so the logic is reusable.

jliexpensify commented 1 month ago

I can repro but not sure if we made this change intentionally, so asking here: https://expensify.slack.com/archives/C036QM0SLJK/p1727826242838209

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

jliexpensify commented 1 month ago

Confirmed that this seems to be a bug:

merchant should take priority over description like the preview component

jliexpensify commented 1 month ago

@alitoshmatov I'm OOO from 3rd to 14th, but I don't think anything is needed from me during this period. Feel free to reassign to another B0 if urgent payment is needed, otherwise I'll catch up on this issue when I get back!

alitoshmatov commented 1 month ago

@nkdengineer @bernhardoj Thank you both for proposals. Your RCA is correct and you provide with almost the same solution.

It is hard choice to make since both proposals are very similar solution, even though @bernhardoj was the second one to submit he did have small additional changes.

I think we should go with @bernhardoj's proposal since it has additional changes that we might have missed it

C+ reviewed 🎀 👀 🎀

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

❌ There was an error making the offer to @alitoshmatov for the Reviewer role. The BZ member will need to manually hire the contributor.

bernhardoj commented 1 month ago

PR is ready

cc: @alitoshmatov

melvin-bot[bot] commented 1 month ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/50372

If no regressions arise, payment will be issued on 2024-10-22. :confetti_ball:

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

@bernhardoj requires payment through NewDot Manual Requests
@alitoshmatov requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 1 month ago

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:

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

jliexpensify commented 3 weeks ago

Payment Summary

C: @bernhardoj $250 (owed via New.Dot payment)
C+: @alitoshmatov $250 (owed vis Upworks)

bernhardoj commented 3 weeks ago

Requested in ND.

melvin-bot[bot] commented 3 weeks ago

Payment Summary

Upwork Job

Reviewer: @bernhardoj owed $250 via NewDot
ROLE: @alitoshmatov paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@jliexpensify)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1841274388644598293/hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

melvin-bot[bot] commented 3 weeks ago

Current assignee @alitoshmatov is eligible for the External assigner, not assigning anyone new.

jliexpensify commented 3 weeks ago

Sorry, upworks is being really weird and won't let me access the job so I closed it and am trying to see if another Job is created.

jliexpensify commented 3 weeks ago

@alitoshmatov waiting on checklist, and invited you to a new job here:

https://www.upwork.com/jobs/~021848861002130198557

jliexpensify commented 3 weeks ago

Paid @alitoshmatov via Upworks, just waiting on checklist

alitoshmatov commented 3 weeks ago

The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/37282
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: https://github.com/Expensify/App/pull/37282/files#r1812400386
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: No need
Determine if we should create a regression test for this bug. No need

JmillsExpensify commented 3 weeks ago

$250 approved for @bernhardoj based on this summary.

Expensify / App