[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")
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:
- Create a free workspace and set it as your default
- 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
Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
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);
Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)
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);
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.
Awaiting proposal review @mollfpr
@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.
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!
Triggered auto assignment to @hayata-suenaga, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@grgia please feel free to un assign me and review proposals as you suggested the first solution
@grgia do you mind confirming this contributor assignment? https://github.com/Expensify/App/issues/34622#issuecomment-1908157936
Thanks!
I'm back from OOO today, looking now
@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!
Hey @abzokhattab - thoughts on when you'll be able to get a PR rolling?
working on it today
@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.
- Login to OldDot
- Create an expense report and add expense
- Fill the field with any info unmark the Reimbursable checkbox, and save the expense
- Open the NewDot and go to the workspace report
- 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.
cc @abzokhattab
@abzokhattab Could you complete the PR? Thanks!
Sorry for the delay, I have been very busy the last few days. The PR is now ready please have a look.
PR is in the review stages! next up is @mollfpr
@greg-schroeder Reviewed and tested well!
Reviewing label has been removed, please complete the "BugZero Checklist".
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)
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:
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
Processing
Annoying. Okay then, I'll create a job manually.
https://github.com/Expensify/App/issues/36653
@greg-schroeder I’ll do a manual request in NewDot and complete the checklist later today!
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.
(payment summary)
$500 offer sent to @abzokhattab @mollfpr can make a manual request for $500