[HOLD for payment 2023-04-24] Show message sender name in the LHN

[mountiny]

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

---

Action Performed:

Break down in numbered steps

1. Send a message in some room
2. On other device/account which has access to the room check the LHN
3. Verify the message sent from the other account has the display name preceding the message

Expected Result:

Describe what you think should've happened

1. We should shown the display name of the person who sent the message as we do for group messages
   

Actual Result:

The name of the person is missing

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number:
Reproducible in staging?:
Reproducible in production?:
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
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1678200268817529

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01650d4b35eb3af650
• Upwork Job ID: 1633133151083597824
• Last Price Increase: 2023-03-07

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @parasharrajat (Internal)

[BeeMargarida]

Hi, I'm Ana from Callstack and I would like to work on this issue

[MelvinBot]

📣 @BeeMargarida! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[parasharrajat]

@BeeMargarida Please do post a proposal on how will you solve this issue before jumping to the PR. This helps us understand your solution better. Thanks.

[BeeMargarida]

Investigating this, since running locally it's working as intended but in staging the problem can be replicated

[BeeMargarida]

Proposal

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

In room chats there's a possibility that the preview of the last message appears without the user's name. This behavior only happens when that user details are not currently saved as the details the user has access to (in Onyx).

What is the root cause of that problem?

If the last message's user details are not available in personalDetails, no name will be shown in the LHN, only the message content.
There's more context in this slack thread

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

The line of code that gets the value of the last user detail is the one below, and after that the last message text is built.

const lastActorDetails = personalDetails[report.lastActorEmail] || null;

let lastMessageText = hasMultipleParticipants && lastActorDetails && (lastActorDetails.login !== currentUserLogin.email)
    ? `${lastActorDetails.displayName}: `
    : '';

In addition to this check, we could also utilize the information in the lastReportAction to fetch the last user details if there's no info in the personalDetails. This information is available by accessing lastReportActions[report.reportID].person[0].text (is it possible to have more than one person in this array?).

A possible solution would be

let lastActorDetails = personalDetails[report.lastActorEmail] || null;
if (!lastActorDetails && lastReportActions[report.reportID]) {
   const lastActorDisplayName = lodashGet(lastReportActions[report.reportID], 'person[0].text');
   lastActorDetails = lastActorDisplayName ? {displayName: lastActorDisplayName, login: lastReportActions[report.reportID].actorEmail} : null;
}

let lastMessageText = hasMultipleParticipants && lastActorDetails && (lastActorDetails.login !== currentUserLogin.email)
    ? `${lastActorDetails.displayName}: `
    : '';

What alternative solutions did you explore? (Optional)

--

[parasharrajat]

Hmm, analyzing...

[parasharrajat]

Ok, I can see that we have a similar code in

App/src/libs/OptionsListUtils.js

Line 333 in b08bf48

const lastActorDetails = personalDetailMap[report.lastActorEmail] || null;

.

Did we check if the issue exists on other pages as well? Mostly Search Page.

Doing this solution will not solve the issue everywhere.

SidebarUtils is responsible for giving LHN options. (this is optimized for LHN)
OptionListUtils gives options for all other pages.

[parasharrajat]

For reproduction,

Send a message to some room

I think only members that have not interacted with me earlier will see this issue. BCZ for others, Onyx should already have personal details for me.

Anyways, which room should it be?

[BeeMargarida]

SidebarUtils is responsible for giving LHN options. (this is optimized for LHN)
OptionListUtils gives options for all other pages.

There's differences between the code in OptionsListUtils and SidebarUtils, specifically in the fallback logic if there's no information in personalDetails about the last actor. In this piece of code below responsible for building the personalDetailMap, it fallbacks to the login for the display name, for example showing the email. I can check that this happen if I try to invite an account i've not interacted previously, it only shows the email and not the display name chosen.

Should we apply this logic in the SidebarUtils?

App/src/libs/OptionsListUtils.js

Lines 107 to 113 in b08bf48

	if (!personalDetail) {
	personalDetail = {
	login,
	displayName: Str.removeSMSDomain(login),
	avatar: ReportUtils.getDefaultAvatar(login),
	};
	}

I have not been able to reproduce the behaviour mentioned in the issue this in chats. Every time I invite some account I've not interacted with to a chat with other accounts in the same situation, the other accounts already show the display name of this new account. I can see in the logs that they receive an update for personal details when the user is invited.
Can this possibly be a situation where this update was not made previously on invite?

[parasharrajat]

Hmm, Thanks for the clarification. I think we will have to wait to confirm that. Generally, I will go with existing behavior and implement it everywhere but having email for this place does not look good to me. We might want to rethink that behavior.

There is a case where only email is available for that user so we have to use that only as displayName. You can observe that for new chats. The chat title is the email of another user.

But, I agree with the slack discussion that it seems better to fetch the displayName from the last report action when available. Because the user's displayName is not their email thus fallback to email is not the best option here.

I have not been able to reproduce the behaviour mentioned in the issue this in chats.

That's what I imagined. #15711 (comment)

[BeeMargarida]

Hmm, Thanks for the clarification. I think we will have to wait to confirm that.

Please let me know the resolution to this confirmation.

That's what I imagined. #15711 (comment)

I've been adding accounts to a workspace and then writing a message in the #announce channel from the new account. However, all other accounts that have not interacted with the new account previously see the display name correctly, so I'm not sure this is 100% the reproduction edge case.

[mountiny]

I think when you add them to the policy, you are the admin of the policy so you will get a DM created with all the new members you add. So you might have to try to log out of the admin account and log in to one of those members who dont have DM with the other workspace members and then observe if the name is in the LHN of the announce room

[BeeMargarida]

I think when you add them to the policy, you are the admin of the policy so you will get a DM created with all the new members you add. So you might have to try to log out of the admin account and log in to one of those members who dont have DM with the other workspace members and then observe if the name is in the LHN of the announce room

You're right, I needed to create a new workspace and add two members that did not have DM between them. With this I can replicate the behaviour. However, if I then add another one to the group after (that the others haven't DMed with), the problematic behaviour stops and the display names appear for all of them.

[mountiny]

It might be that the personal details got loaded in the mean time. Maybe we could then try do a fallback kind of behaviour where in case we cannot get the name from the personalDetails we can look at the reportAction actor name.

Does that make sense?

Is there any other concerns left?

Also feel free to create a new thread in open-source channel to maybe speed up the discussion :)

[parasharrajat]

I think we can go ahead with proposal @BeeMargarida.

It is safe to say that OptionListUtils will get the correct displayName as it is only creating options from the reports that were created earlier so the personal details of participants must have been synced.

Let me quickly check a few things.

[mountiny]

Let me know once you confirm

[mountiny]

PR being reviewed

[MelvinBot]

@sonialiap, @parasharrajat, @BeeMargarida, @mountiny Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mountiny]

Merged

[MelvinBot]

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

[MelvinBot]

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

• [NoQA] fix: do not use last actor details for LHN if last action is for closing chat #16606

If no regressions arise, payment will be issued on 2023-04-24. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

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:

• [@parasharrajat / @mountiny] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat / @mountiny] 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:
• [@parasharrajat / @mountiny] 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:
• [@sonialiap] Determine if we should create a regression test for this bug.
• [@parasharrajat] 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.
• [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/283047

[mountiny]

@parasharrajat are there any regression tests to add here?

This was just a follow up to previous PR so no need for the bugs convo

[parasharrajat]

Regression test steps

1. Create a workspace.
2. Invite two members to the workspace who never interacted with each other.
3. Logout and login to one of these accounts
4. Send a message to the #announce channel.
5. Log out and then log in with the other invited account.
6. Check that the sender's name appears in the preview of the last message of the chat.

Note: It might not work for the reports where messages are not loaded yet.

Do you agree 👍 or 👎 ?

[mountiny]

I think that works, thanks @parasharrajat

[mountiny]

@sonialiap I think this is all good ot be paid out

[parasharrajat]

Bump @sonialiap #15711 (comment)

[mallenexpensify]

@parasharrajat can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01672c65c1c0c60da3

[parasharrajat]

@mallenexpensify Done.

[parasharrajat]

Bump @mallenexpensify

[mallenexpensify]

@parasharrajat paid $1000
regression test steps GH https://github.com/Expensify/Expensify/issues/283047