[HOLD for payment 2024-05-02] [$500] Avatars used in report and transaction actions are not consistent

[m-natarajan]

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: 1.4.56-5
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1711385263315559

Action Performed:

1. Create an expense report in a workspace chat
2. Edit merchant name ,description or category
3. Observe the preview of the report and report details page for the transaction actions

Expected Result:

Both pages should have consistent avatar display

Actual Result:

Combo avatar is shown in the report preview (WS avatar and user) and in the transaction details it shows only user avatar

Workaround:

unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Recording.2902.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011c09e56c59c19b0d
• Upwork Job ID: 1773443474563608576
• Last Price Increase: 2024-03-28
• Automatic offers:
• alitoshmatov | Reviewer | 0
• ahmedGaber93 | Contributor | 0

[melvin-bot]

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

[Pranav2612000]

Proposal

[ Updated after @shawnborton 's suggestion ]

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

Report Preview should just show the User Avatar, similar to the one we have for Transaction Detail.
Current Report Preview Item:

Expected Report Preview Item:

Also, while doing this we need to ensure that the request on the workspace page still shows the combo icons i.e

Isn't that right @shawnborton ?

What is the root cause of that problem?

The ReportActionItemSingle component takes a shouldShowSubscriptAvatar prop which determines whether to display the combo icon or not.
This prop value is being passed as true here because the ReportUtils.isExpenseReport(report) is true.

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

Removing the ReportUtils.isExpenseReport(report) condition from the check here should start displaying just the user icon on the Report Preview page.

What alternative solutions did you explore? (Optional)

If we want more fine-grained control over which actions should display the combo icon, we can also configure that by choosing the reportAction types to allow it for here

[puneetlath]

@slafortune we should get the design team's input on which avatar we want to use. I was discussing this with @shawnborton before.

[shawnborton]

I think we should just use the person's avatar without the subscript workspace avatar attached to it. Thoughts on that? cc @Expensify/design

[dannymcclain]

That works for me! I honestly don't have a super strong preference for which to use, but totally agree that we should just use the same one for everything. Down with ditching the subscript.

[gijoe0295]

Actually I think this is expected. Those are two completely different actions. The report preview is an expense request and thus should have subscript avatar, indicating that this request belongs to that user and workspace.

On the other hand, in transaction details, the user is the owner/creator of the actions made there (modify merchant, description, delete, hold request...) so it should be himself who made the actions and thereby shows his avatar as the sender.

[shawnborton]

We're recommending that we remove the subscript from the avatar - it feels extraneous in the comments, given that in the chat view header you see the user avatar + workspace avatar. When comments are actions are happening, they are just happening from the user, so we don't need any special kind of avatar there.

[ahmedGaber93]

Proposal

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

Avatars used in report and transaction actions are not consistent.
As I understand from the design team conversation, we should remove the subscript workspace avatar from the expense Report.

What is the root cause of that problem?

Here we allow showing the subscript workspace avatar while ReportUtils.isExpenseReport(report))

App/src/pages/home/report/ReportActionsListItemRenderer.tsx

Lines 136 to 137 in d8361ee

	shouldShowSubscriptAvatar={
	(ReportUtils.isPolicyExpenseChat(report) || ReportUtils.isExpenseReport(report)) &&

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

we need to remove ReportUtils.isExpenseReport(report)) part from the condition here

What alternative solutions did you explore? (Optional)

[Pranav2612000]

We're recommending that we remove the subscript from the avatar - it feels extraneous in the comments, given that in the chat view header you see the user avatar + workspace avatar. When comments are actions are happening, they are just happening from the user, so we don't need any special kind of avatar there.

Thank you @shawnborton . That makes sense.

I've updated my proposal to support this. I also had a question about your suggestion, that I've asked in the proposal. Could you take a look whenever you have time?

[melvin-bot]

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

[melvin-bot]

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

[slafortune]

@alitoshmatov Will you have time to review the submitted proposals?

[alitoshmatov]

Yes, working on it right now

[alitoshmatov]

@Pranav2612000 Thank you for your proposal, removing ReportUtils.isExpenseReport(report) does indeed solve this issue. But I see @ahmedGaber93 was the first one to suggest this change.

[alitoshmatov]

We can go with @ahmedGaber93 's proposal which removes ReportUtils.isExpenseReport(report) condition for showing workspace subscript. Thus all transaction report previews will be displayed without workspace subscript

C+ reviewed 🎀 👀 🎀

[melvin-bot]

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

[melvin-bot]

📣 @alitoshmatov 🎉 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]

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

[ahmedGaber93]

@alitoshmatov PR #39473 is ready for review.

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

• remove subscript workspace avatar from expense report #39473

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

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

• @ahmedGaber93 requires payment automatic offer (Contributor)
• @alitoshmatov requires payment automatic offer (Reviewer)

[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:

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

[alitoshmatov]

• The PR that introduced the bug has been identified. Link to the PR: [NoQA] Memoize expensive calls in ReportActionsList renderItem #27957
• 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/27957/files#r1585897837
• 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

[slafortune]

@alitoshmatov @ahmedGaber93 Paid ✅