Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] IOS - The Chat report displays the wrong Latest Sender's name in LHN after logging in #26609

Closed
1 of 6 tasks
kbecciv opened this issue Sep 3, 2023 · 73 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Sep 3, 2023

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:

PRECONDITION: The user has joined a public group chat with a long chat history. For Example:

- Bob-testing: https://staging.new.expensify.com/r/5593084223054221
  1. Log in to the account that has joined a public/group chat with a long chat history.
  2. On the LHN: check the name of the Latest Sender displayed.
  3. Open the chat to view chat's details.
  4. Verify the actual sender who sent the Last Message in the chat.
  5. Compare the Sender's name from step Moving files and folders a little #4 with the name obtained in step Fix spaces #2.
  6. Observe that the latest sender's name is different.

Expected Result:

The latest sender's name should match between step #4 and #2

Actual Result:

The latest sender's name is different.

Workaround:

Unknown

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: 1.3.62.0
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
Notes/Photos/Videos: Any additional supporting documentation

Wrong-Latest-Sender.mp4

wrong-sender-name

Expensify/Expensify Issue URL:
Issue reported by: @tranvantoan-qn
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693213921747209

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bd85da703f1b3f23
  • Upwork Job ID: 1699155042104852480
  • Last Price Increase: 2023-09-26
@kbecciv kbecciv added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 3, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Sep 5, 2023
@slafortune
Copy link
Contributor

Applying the external label

@melvin-bot melvin-bot bot removed the Overdue label Sep 5, 2023
@slafortune slafortune added the External Added to denote the issue can be worked on by a contributor label Sep 5, 2023
@melvin-bot melvin-bot bot changed the title IOS - The Chat report displays the wrong Latest Sender's name in LHN after logging in [$500] IOS - The Chat report displays the wrong Latest Sender's name in LHN after logging in Sep 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01bd85da703f1b3f23

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

Current assignee @slafortune is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

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

@slafortune
Copy link
Contributor

Still waiting on proposals.

Assigning a new BZ member, I am out on sabbatical

@slafortune slafortune removed their assignment Sep 8, 2023
@slafortune slafortune added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

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

@Expensify Expensify deleted a comment from melvin-bot bot Sep 8, 2023
@melvin-bot melvin-bot bot added the Overdue label Sep 11, 2023
@isabelastisser
Copy link
Contributor

Not overdue; waiting on proposals!

@melvin-bot melvin-bot bot removed the Overdue label Sep 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2023

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

@isabelastisser
Copy link
Contributor

Still waiting for proposals!

@melvin-bot melvin-bot bot added the Overdue label Sep 15, 2023
@isabelastisser
Copy link
Contributor

Still waiting for proposals.

@isabelastisser
Copy link
Contributor

hey @barttom, any updates?

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
@barttom
Copy link
Contributor

barttom commented Oct 30, 2023

I agree with the proposal from @MrMuzyk. Looks like we waiting for reviewing proposal by @aimane-chnaif.

@aimane-chnaif
Copy link
Contributor

I tested and reproduced with #bobs-testing. It says "No activity yet" before opening report

Screen.Recording.2023-10-31.at.8.16.24.AM.mov

@aimane-chnaif
Copy link
Contributor

It's super weird that last message sender name is concierge@expensify.com before opening report

Screen.Recording.2023-10-31.at.8.29.13.AM.mov

@mkhutornyi
Copy link
Contributor

Proposal

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

Issue 1. Wrong sender name (from whisper message) shows in LHN before opening report after visit that report, logout and re-login
Issue 2. "No activity yet" shows after refresh

What is the root cause of that problem?

Issue 1. wrong sender name (from whisper message)
Whispered messages are not filtered here:

const reportActionsForDisplay = actionsArray.filter(
(reportAction, actionKey) =>
ReportActionsUtils.shouldReportActionBeVisible(reportAction, actionKey) &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
);
visibleReportActionItems[reportID] = reportActionsForDisplay[reportActionsForDisplay.length - 1];

So lastActorDisplayName is set to sender name of whisper message here:
const lastActorDisplayName = visibleReportActionItems[report.reportID]?.person?.[0]?.text;

Issue 2. "No activity yet" after refresh

} else if (lastAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && lastActorDisplayName && lastMessageTextFromReport) {
result.alternateText = `${lastActorDisplayName}: ${lastMessageText}`;
} else {
result.alternateText = lastAction && lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');
}

lastActorDisplayName and lastAction don't exist so fallback to Localize.translate(preferredLocale, 'report.noActivityYet')

Summary:
When visit the report, visibleReportActionItems are set globally here:

const visibleReportActionItems: ReportActions = {};

But after logout, this data is not cleared so remains as stale value.
After re-login, app is still using stale data and that's why Issue 1. happens.
After refresh, all stale data is cleared so whisper message no longer exists until open that report again. That's why Issue 1 changes to Issue 2.

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

Solution to Issue 1:
add !ReportActionsUtils.isWhisperAction(reportAction) && to filter condition

Solution to Issue 2:
Remove lastAction && condition here:

result.alternateText = lastAction && lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');

As a result, it shows {lastMessageText} without {sender name}: prefix if sender name doesn't exist.
So it's better than No activity yet

What alternative solutions did you explore? (Optional)

As others proposed, add new field lastActorDisplayName to report object, along with lastMessageText.
But this is new feature request and I don't think we wanna do that right now.

@aimane-chnaif
Copy link
Contributor

🎀 👀 🎀

@melvin-bot
Copy link

melvin-bot bot commented Oct 31, 2023

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

@aimane-chnaif
Copy link
Contributor

@thienlnam thanks for taking this issue. There are 2 possible solutions to fix this issue

Issue: report object doesn't contain sender's display name so no way to display if sender is not in personal details list

Solution 1: introduce new key (something like report.lastActorDisplayName) (proposal)
Solution 2: hide sender name in last message, like DM (proposal)

Which solution do you suggest?

@thienlnam
Copy link
Contributor

Ah interesting, so you might not have the personal details of the last actor accountID and so it just falls back to the most recent visible action?

If we added the key lastActorDisplayName, that would essentially be the fallback if don't have that accountID in your personal details right because otherwise it would pull the actual display name or email.

I don't really like the idea of adding another key to determine which name to show considering it's only applicable for certain users, and then there's always the question of which display name to use - from the lastReportAction or the report and keeping those in sync we'd probably have to make a decent amount of onyx optimistic updates

I'm leaning towards the second solution since that seems to be how we handle these other scenarios where we don't have personal details for a user.

Though curious if ya'll agree (@puneetlath / @Beamanator)

@puneetlath
Copy link
Contributor

Ah interesting. So we get the personalDetails in the openReport command, but since that hasn't been called yet it can be wrong. I agree with just not showing a display name in the case where we don't have it.

@aimane-chnaif
Copy link
Contributor

Ah interesting. So we get the personalDetails in the openReport command, but since that hasn't been called yet it can be wrong. I agree with just not showing a display name in the case where we don't have it.

Thanks for the feedback

Let's go with @mkhutornyi's proposal
cc: @thienlnam

@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Nov 1, 2023

@puneetlath one more thing to confirm:

when sender name is in the middle, what to show? Maybe something like [Hidden] or leave empty as is?

Screen.Recording.2023-11-01.at.8.00.24.PM.mov

EDIT:

I noticed it's not possible to customize. It's something to be fixed in backend.

Screenshot 2023-11-01 at 8 05 23 PM

@puneetlath
Copy link
Contributor

Yeah, we'll need to handle that on the back-end.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 Monthly KSv2 and removed Daily KSv2 labels Nov 3, 2023
@aimane-chnaif
Copy link
Contributor

This is ready for payment. @isabelastisser can you please update to Daily?

@isabelastisser isabelastisser added Daily KSv2 and removed Weekly KSv2 labels Nov 30, 2023
@isabelastisser
Copy link
Contributor

Payment summary:

Issue reported by: @tranvantoan-qn $50
C+ review: @aimane-chnaif $500
Accepted proposal: @mkhutornyi $500

@isabelastisser
Copy link
Contributor

Offers sent in Upwork. All set!

@tranvantoan-qn
Copy link

@isabelastisser
I've accepted the offer. But shouldn't we process the payment first before closing it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests