App icon indicating copy to clipboard operation
App copied to clipboard

[ECARD TRANSACTIONS] Report name on an expense report with non-reimbursable transactions is showing the workspace name instead of the requestor name (see "Free Workspace - ECard testing spent")

Open kevinksullivan opened this issue 2 years ago • 20 comments

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/p1705356562602929

Action Performed:

  1. Create a free workspace and set it as your default
  2. Incur a charge on an Expensify Card

Expected Result:

The report title should be %firstName% spent

Actual Result:

The report title is using the workspace name instead of firstName of the requestor

image

kevinksullivan avatar Jan 17 '24 01:01 kevinksullivan

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

melvin-bot[bot] avatar Jan 17 '24 01:01 melvin-bot[bot]

Solution Notes

https://github.com/Expensify/App/blob/497ed9273d99afd1cdb061acb78014e15ffdefe8/src/components/ReportActionItem/ReportPreview.js#L214-L228

        const managerName = isPolicyExpenseChat ? ReportUtils.getPolicyName(props.chatReport) : ReportUtils.getDisplayNameForParticipant(managerID, true);

on this line, we never want to use the workspace name if the report has non-reimbrsable transactions. it should always be %FirstName% spent

so maybe we can try

        const managerName = (isPolicyExpenseChat && !hasNonReimbursableTransactions) ? ReportUtils.getPolicyName(props.chatReport) : ReportUtils.getDisplayNameForParticipant(managerID, true);

grgia avatar Jan 19 '24 00:01 grgia

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] avatar Jan 19 '24 00:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 19 '24 00:01 melvin-bot[bot]

Proposal

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

Report name on an expense report with non-reimbursable transactions is showing the workspace name instead of the requestor name

What is the root cause of that problem?

we dont have a check for non-reimbursable transactions here https://github.com/Expensify/App/blob/497ed9273d99afd1cdb061acb78014e15ffdefe8/src/components/ReportActionItem/ReportPreview.js#L225-L226

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

if the report has NonReimbursable Transactions we need to display the first name:

        const managerName = (isPolicyExpenseChat && !hasNonReimbursableTransactions) ? ReportUtils.getPolicyName(props.chatReport) : ReportUtils.getDisplayNameForParticipant(managerID, true);

OR we can also validate that the transaction is a card transaction by using

const isCardTransaction = TransactionUtils.isCardTransaction(transaction);

then the final condition would be:

        const managerName = (isPolicyExpenseChat && !hasNonReimbursableTransactions &&!isCardTransaction ) ? ReportUtils.getPolicyName(props.chatReport) : ReportUtils.getDisplayNameForParticipant(managerID, true);

abzokhattab avatar Jan 19 '24 00:01 abzokhattab

Proposal

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

[ECARD TRANSACTIONS] Report name on an expense report with non-reimbursable transactions is showing the workspace name instead of the requestor name (see "Free Workspace - ECard testing spent")

What is the root cause of that problem?

We have not implemented a condition in the getPreviewMessage function to handle titles for IOUs paid by an Expensify card.

https://github.com/Expensify/App/blob/e6b07616db3a818574d60acf1d9ebaf1cb26bf02/src/components/ReportActionItem/ReportPreview.js#L234

Furthermore, we will only display the only firstName of the participant if and only if the report isPolicyExpenseChat.

Hence, we end up displaying the policy name.

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

In addition to !hasNonReimbursableTransactions, we should create a condition such as usedExpensifyCard to further check if every or some of the transactions were paid using an Expensify card.

This will ensure that IOUs paid by other methods do not get affected.

Hence, the changes to the code should look close to below.

const usedExpensifyCard = !hasNonReimbursableTransactions &&
TransactionUtils.getAllReportTransactions(report.iouReportID).every(
 (transaction) => TransactionUtils.isExpensifyCardTransaction(transaction)
);

const managerName = (isPolicyExpenseChat && !usedExpensifyCard) ? ReportUtils.getPolicyName(props.chatReport) : ReportUtils.getDisplayNameForParticipant(managerID, true);

Finally, I suggest we change the MoneyReportHeader's title and subtitle and the LHNOptionRow's title and subtitle to be consistent with the report preview.

Therefore, we will also have to change functions such as ReportUtils.getReportName and ReportUtils.getChatRoomSubtitle.

This will ensure consistency with other components mentioned earlier, which require the same title as the report preview.

Tony-MK avatar Jan 19 '24 04:01 Tony-MK

Awaiting proposal review @mollfpr

greg-schroeder avatar Jan 22 '24 16:01 greg-schroeder

@abzokhattab @Tony-MK How do you guys do step 2?

I stuck on setup the expensify cards. I keep getting this, even thought I already add secondary login.

Screenshot 2024-01-23 at 21 31 28

mollfpr avatar Jan 23 '24 14:01 mollfpr

I can still not reproduce the issue with the above step because I don't have the Expensify cards set up yet.

on this line, we never want to use the workspace name if the report has non-reimbrsable transactions. it should always be %FirstName% spent

I think we only need to check hasNonReimbursableTransactions for the payer name. I don't have any other case in mind. So let's go with @abzokhattab proposal.

🎀 👀 🎀 C+ reviewed!

mollfpr avatar Jan 24 '24 13:01 mollfpr

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

melvin-bot[bot] avatar Jan 24 '24 13:01 melvin-bot[bot]

@grgia please feel free to un assign me and review proposals as you suggested the first solution

hayata-suenaga avatar Jan 24 '24 22:01 hayata-suenaga

@grgia do you mind confirming this contributor assignment? https://github.com/Expensify/App/issues/34622#issuecomment-1908157936

Thanks!

greg-schroeder avatar Jan 26 '24 18:01 greg-schroeder

I'm back from OOO today, looking now

grgia avatar Jan 26 '24 22:01 grgia

@mollfpr @greg-schroeder @grgia @abzokhattab 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] avatar Jan 31 '24 15:01 melvin-bot[bot]

Hey @abzokhattab - thoughts on when you'll be able to get a PR rolling?

greg-schroeder avatar Jan 31 '24 17:01 greg-schroeder

working on it today

abzokhattab avatar Jan 31 '24 17:01 abzokhattab

@greg-schroeder @grgia I haven't got my account set up with an Expensify card. So here's the step I use to reproduce the issue without the card. Let me know if we can use this.

  1. Login to OldDot
  2. Create an expense report and add expense
  3. Fill the field with any info unmark the Reimbursable checkbox, and save the expense
  4. Open the NewDot and go to the workspace report
  5. Take a note on the latest money preview title
Video step

Also, we have a similar issue on the header of the money request report and thread. Where I think we also need to fix it together.

Screenshot 2024-02-01 at 15 56 30

Screenshot 2024-02-01 at 15 56 38

cc @abzokhattab

mollfpr avatar Feb 01 '24 09:02 mollfpr

@abzokhattab Could you complete the PR? Thanks!

mollfpr avatar Feb 05 '24 04:02 mollfpr

Sorry for the delay, I have been very busy the last few days. The PR is now ready please have a look.

abzokhattab avatar Feb 05 '24 08:02 abzokhattab

PR is in the review stages! next up is @mollfpr

greg-schroeder avatar Feb 06 '24 01:02 greg-schroeder

@greg-schroeder Reviewed and tested well!

mollfpr avatar Feb 06 '24 04:02 mollfpr

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

melvin-bot[bot] avatar Feb 08 '24 18:02 melvin-bot[bot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.38-6 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/35529

If no regressions arise, payment will be issued on 2024-02-15. :confetti_ball:

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

  • @mollfpr requires payment (Needs manual offer from BZ)
  • @abzokhattab requires payment (Needs manual offer from BZ)

melvin-bot[bot] avatar Feb 08 '24 18:02 melvin-bot[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:

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

melvin-bot[bot] avatar Feb 08 '24 18:02 melvin-bot[bot]

Payment Summary

Upwork Job

  • ROLE: @mollfpr paid $(AMOUNT) via Upwork (LINK)
  • ROLE: @abzokhattab paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@greg-schroeder)

  • [ ] 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//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] avatar Feb 15 '24 15:02 melvin-bot[bot]

Processing

greg-schroeder avatar Feb 16 '24 02:02 greg-schroeder

Annoying. Okay then, I'll create a job manually.

https://github.com/Expensify/App/issues/36653

greg-schroeder avatar Feb 16 '24 02:02 greg-schroeder

@greg-schroeder I’ll do a manual request in NewDot and complete the checklist later today!

mollfpr avatar Feb 16 '24 02:02 mollfpr

Ah, nice, I didn't realize you were eligible for manual requests @mollfpr! I'll update our documentation on that - you can ignore the offer I sent you then.

greg-schroeder avatar Feb 16 '24 02:02 greg-schroeder

(payment summary)

$500 offer sent to @abzokhattab @mollfpr can make a manual request for $500

greg-schroeder avatar Feb 16 '24 02:02 greg-schroeder