[$250] Group chat - LHN preview shows "made admin" incorrectly when changing Group Chat member role

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.71-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: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB > Start chat
3. Create a group chat
4. Send a message to the group chat
5. Click on chat header > Members
6. Promote a member to admin
7. Note that group chat preview in LHN is the message sent in Step 4
8. Send a message to another chat
9. Go to Account settings > Troubleshoot
10. Select Clear cache and restart
11. After the app is restarted, note the group chat preview in LHN

Expected Result:

The whisper message in LHN should not be there for the incorrect user.

Actual Result:

The group chat preview in LHN shows system message for making member admin after the app is restarted (incorrect) while there is no system message in the group chat (correct)

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

Bug6474443_1715138110463.20240508_110803.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d089c025af3fea83
• Upwork Job ID: 1790556079306735616
• Last Price Increase: 2024-05-22

Issue Owner

Current Issue Owner: @jayeshmangwani

[melvin-bot]

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

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

[melvin-bot]

@anmurali Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

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

[melvin-bot]

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

[anmurali]

This is odd cause the message shows up only in LHN but not in the room!

[MrMuzyk]

I am Michał from Callstack - expert contributor group. I’d like to work on this job.

[MrMuzyk]

I've managed to reproduce that. Can you specify what expected result should be here?

Should we display message about making a new admin in chat and LHN is previewing the message correctly or should we preview last message send in chat and ignore info about making a new admin - so LHN is previewing message incorrectly?

[jayeshmangwani]

IMO, the Expected result should be that the system message should display a message in chat that made ${user} an admin, but what is your opinion about the expected behavior here @anmurali ?

[MrMuzyk]

In that case I believe the issue is on the BE side. This message about making someone an admin is a whisper targeted only at selected account IDs and not a message sent to the whole chat room.

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

Lines 200 to 211 in e8ae3c5

	const sortedVisibleReportActions = useMemo(
	() =>
	sortedReportActions.filter(
	(reportAction) =>
	(isOffline ||
	ReportActionsUtils.isDeletedParentAction(reportAction) ||
	reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ||
	reportAction.errors) &&
	ReportActionsUtils.shouldReportActionBeVisible(reportAction, reportAction.reportActionID),
	),
	[sortedReportActions, isOffline],
	);

shouldReportActionBeVisible filters out any message that is a whisper to an account that has different ID than the currently logged in user.

App/src/libs/ReportActionsUtils.ts

Lines 539 to 542 in e8ae3c5

	if (isWhisperActionTargetedToOthers(reportAction)) {
	return false;
	}

In order for the message to display properly it should not be a whisper

[jayeshmangwani]

Hi @marcaaron I am tagging you here because this issue is related to Group creation flow. Recently, you have worked on this PR, and in that PR we have added a comment about system messages, so the issue we are getting here is:

If we make a user to admin and then refresh the page, we are getting the text made {user} an admin for the LHN alternate text, but not getting the system message inside the chat,

So we have two questions:

1. The system message made {user} an admin  should be shown in the chat ? If yes, it looks like a BE issue; if Not...
2. Should we hide the made {user} an admin on LHN alternate text and should show the last text from the report?

Note: Removed User and Invited User system message shown in both chat and the LHN.

[marcaaron]

The system message made {user} an admin should be shown in the chat

Incorrect, this should only appear for the user who's role was changed.

Should we hide the made {user} an admin on LHN alternate text and should show the last text from the report?

I would say that it should not be showing up at all. It's a whisper message intended for one person so it should not appear. Can you verify where this data comes from?

[marcaaron]

I updated the title and description as there were some things incorrect about it.

[MrMuzyk]

Proposal

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

Group chat - LHN preview shows "made admin" incorrectly when changing Group Chat member role

What is the root cause of that problem?

Component displaying the text on the LHN uses this util method - getLastMessageTextForReport - and line below is causing the issue

App/src/libs/OptionsListUtils.ts

Line 635 in e8ae3c5

return lastMessageTextFromReport || (report?.lastMessageText ?? '');

report?.lastMessageText returns text for the last message sent in this report but this isn't the last message that should be displayed to the end user.

I'm rather sure that this is correct behaviour from the BE side (would be great to double check with someone who is certain of it)

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

We should update this line so it returns text from last visible report action instead.

lastReportAction is such action that is created in the beginning of this util method so we can reuse that here

return lastMessageTextFromReport || (lastReportAction?.message?.[0]?.text ?? '');

What alternative solutions did you explore? (Optional)

If it would be case that BE returns incorrect lastMessageText field inside the report then it would have to be fixed on the BE side. Like I've stated in root cause I'm not 100% sure - technically it was lastMessageText but this message is not meant to be displayed to end user

[melvin-bot]

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

[melvin-bot]

@anmurali @jayeshmangwani 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!

[rlinoz]

You mean this logic https://github.com/Expensify/Auth/pull/10734/files#diff-71e5dfb89f61488924fbd1778e402bfe798b419a31c368f1ca7dd3d2aa0a5387R6965?

Yeah, I think I agree, before my change it was only the ACTION_ACTIONABLEMENTIONWHISPER, but I think you are right and we should do it for every whisper, not sure what side effects that can have though.

[marcaaron]

Hopefully, a good side effect where the LHN has the correct message in it 😄

[melvin-bot]

@anmurali, @jayeshmangwani Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@anmurali, @jayeshmangwani Still overdue 6 days?! Let's take care of this!

[jayeshmangwani]

Not overdue, BE issue so will be handled internally

[melvin-bot]

@anmurali, @jayeshmangwani Whoops! This issue is 2 days overdue. Let's get this updated quick!

[jayeshmangwani]

@rlinoz Has an internal issue been created to handle this backend bug, or should we assign someone to handle it?

[rlinoz]

No internal issue has been created, I believe we can add the hot pick to this one though, since it already has the internal label.

[melvin-bot]

@anmurali, @jayeshmangwani Whoops! This issue is 2 days overdue. Let's get this updated quick!

[jayeshmangwani]

Hot Pick label added, though waiting to be picked by an internal engineer.

[melvin-bot]

@anmurali, @jayeshmangwani Whoops! This issue is 2 days overdue. Let's get this updated quick!

[jayeshmangwani]

Waiting for an internal engineer to grab this

[melvin-bot]

@anmurali, @jayeshmangwani Whoops! This issue is 2 days overdue. Let's get this updated quick!

[anmurali]

All things split is paused. So would make sense to make this a Monthly.

[melvin-bot]

@anmurali, @jayeshmangwani, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.