[Ready for C+ checklist and payment][$250] Split - Group chat does not show bill creator name in LHN preview after refreshing the page

lanitochka17 commented 5 months 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: 1.4.60-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to FAB > Request money > Manual
Create a split bill with two users
Once landed in group chat, note that group chat in LHN shows the bill creator name in the preview
Refresh the page

Expected Result:

The group chat will continue to show the bill creator name in LHN preview after refreshing the page

Actual Result:

The group chat does not show the bill creator name in LHN preview after refreshing the page

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/f9407c5a-6c4d-4aee-808a-ce4639cb42c2

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0126a3e7c5e2856078
Upwork Job ID: 1777107502133911552
Last Price Increase: 2024-04-28
Automatic offers:

eh2077 | Reviewer | 0
bernhardoj | Contributor | 0

melvin-bot[bot] commented 5 months ago

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

lanitochka17 commented 5 months ago

@kadiealexander 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

bernhardoj commented 5 months ago

Proposal

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

The LHN subtitle shows the sender name when creating the split bill, but then it disappears.

What is the root cause of that problem?

When we refresh/wait for a moment, we will get the correct response from the BE and the sender name disappears is already correct because we don't want to show the sender name if it's coming from ourself. https://github.com/Expensify/App/blob/2995f2c79751d5096919eb98a26e0bd7b8533658/src/libs/OptionsListUtils.ts#L529-L533

The logic to show the sender name is, by taking the report lastActorAccountID and getting the user data from the ID. https://github.com/Expensify/App/blob/2995f2c79751d5096919eb98a26e0bd7b8533658/src/libs/SidebarUtils.ts#L297

However, when we create a split bill, the lastActorAccountID is 0, so the user detail is empty. https://github.com/Expensify/App/blob/2995f2c79751d5096919eb98a26e0bd7b8533658/src/libs/ReportUtils.ts#L3781

If it's empty, we will get it from the last report action person data which contains the user display name. https://github.com/Expensify/App/blob/2995f2c79751d5096919eb98a26e0bd7b8533658/src/libs/SidebarUtils.ts#L299-L307

Because 0 is not the same as the current user ID, the current user display name is shown as the sender.

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

Set the lastActorAccountID optimistically to currentUserAccountID when creating a split bill (and revert it when fails). We did this when adding a message/task too.

We might need to do this for other money request too.

What alternative solutions did you explore? (Optional)

Don't show the display name when the lastActorAccountID is 0.

OR

If we always want to show the sender name, we need to remove the accountID check (lastActorDetails.accountID !== currentUserAccountID) here. https://github.com/Expensify/App/blob/9a6cafcabfcbaa4f81fe57d0fecbeab682b8309a/src/libs/OptionsListUtils.ts#L536-L537

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

eh2077 commented 5 months ago

Reviewing proposals

eh2077 commented 5 months ago

@bernhardoj 's proposal looks good to me. I agree with their RCA and their solution. I also agree we should fix other money request flows together.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

kadiealexander commented 5 months ago

@aldo-expensify bump on this!

aldo-expensify commented 5 months ago

I have some doubts about the "Expected results" being correct here. If we say that the frontend is right, and the split creator name shouldn't appear in the report's last message in the LHN from the perspective of the creator, this means that the last message of the report is different for each participant, and I think we may want to avoid that.

@marcaaron can I get your eyes here since I think you know about groups. 🙏

marcaaron commented 5 months ago

I agree 1000%.

With Group Chats, we did not really change much about how this works on the frontend side of things though - just replaced the Group DM with a Group Chat so I'm not sure where we went wrong or if this was just always awkward.

Could probably take this to #vip-split room and loop in whoever is working on Splits now. cc @arielgreen @dubielzyk-expensify and @youssef-lr? Can we see about rolling this in as a polish item for this project?

dubielzyk-expensify commented 5 months ago

Seems like a minor bug. I agree that it'd be nice to fix it though, but fine with this as a polish item

eh2077 commented 5 months ago

@kadiealexander Based on the discussions above, can you help to update the Expected results section of this issue?

melvin-bot[bot] commented 5 months ago

@kadiealexander @aldo-expensify @eh2077 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!

eh2077 commented 5 months ago

@kadiealexander Based on the discussions above, can you help to update the Expected results section of this issue?

@kadiealexander Friendly bump!

eh2077 commented 5 months ago

Or @bernhardoj would you like to update your proposal according to @aldo-expensify 's https://github.com/Expensify/App/issues/39647#issuecomment-2057456493 ?

bernhardoj commented 5 months ago

Updated!

eh2077 commented 5 months ago

@bernhardoj Thanks for the update!

Based on the above discussions, yes, it makes more sense to Don't show the display name when the lastActorAccountID is 0.

So, @bernhardoj 's alternative solution looks good.

@aldo-expensify All yours.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 5 months ago

Current assignee @aldo-expensify is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 5 months ago

📣 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] commented 5 months ago

@kadiealexander, @aldo-expensify, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick!

eh2077 commented 5 months ago

@aldo-expensify Should we assign @bernhardoj to move this issue forward?

aldo-expensify commented 5 months ago

It is not clear to me from the conversation which option we want, so I asked here https://expensify.slack.com/archives/C05RECHFBEW/p1713833726038629 to confirm

aldo-expensify commented 5 months ago

Sorry @eh2077 @bernhardoj for the back and forward. The conclusion from the slack conversation is:

The bill creator name should appear in the last message, and the front end should take care of that. The backend won't send the last message with the bill creator in it.

I think this was what @bernhardoj original solution was trying to achieve, correct?

This code seems related to that:

https://github.com/Expensify/App/blob/42d4ed7b72dc979df430a2a7eac4ba30a3eac790/src/libs/OptionsListUtils.ts#L711-L713

eh2077 commented 5 months ago

@aldo-expensify Thanks for driving clarity.

@bernhardoj Can you take some time to update proposal based on the expected result^ ?

aldo-expensify commented 5 months ago

I just noticed a mistake in the expected results I typed above. The backend WON'T send the bill creator as part of the last message:

bernhardoj commented 5 months ago

My proposal has both behaviors, showing or not showing the sender name if it's the current user. If I understand correctly from @aldo-expensify comment, we always want to show the sender's name, so it should be the 2nd alternative.

melvin-bot[bot] commented 5 months ago

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

eh2077 commented 5 months ago

@bernhardoj Thanks for the update. Yeah, the 2nd alternative makes sense to me.

@aldo-expensify All yours.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 5 months ago

Current assignee @aldo-expensify is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 5 months ago

📣 @eh2077 🎉 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] commented 5 months ago

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

bernhardoj commented 5 months ago

PR is ready

cc: @eh2077

eh2077 commented 4 months ago

The fix has hit production https://github.com/Expensify/App/pull/41483#issuecomment-2101804136

kadiealexander commented 4 months ago

Sorry guys, this one slipped through the cracks!

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:

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

Payouts due:

[x] Contributor: $250 @bernhardoj
[ ] Contributor+: $250 @eh2077

Upwork job is here.

eh2077 commented 4 months ago

BugZero Checklis

[x] [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I can't find the specific PR caused this issue. I think this case is not handled when we added the feature.
[x] [@eh2077] 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: N/A
[x] [@eh2077] 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: N/A
[x] [@eh2077] Determine if we should create a regression test for this bug. This bug was reported by QA team and it's an edge case, so I think we don't need a regression test for it.

Expensify / App