[HOLD for payment 2023-08-03] [$1000] Web - Temporary display of welcome message after deleting the last message

[kbecciv]

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:

1. Login to the userA’s account
2. Go to the Setting > Workspace > New Workspace
3. Navigate to the "New room" section
4. Enter a name for the room and select the created workspace
5. Set the visibility of the room to "Public"
6. Click on the "Create room" button
7. Go to the room details > Settings > Welcome message
8. Enter welcome message and save it
9. Now go to Share code > Copy URL to clipboard
10. Open another browser and log in with userB account
11. Join the room by pasting the copied room URL
12. Pin the room
13. Send a message after joining the room and delete it

Expected Result:

The welcome message should not be displayed for a while after deleting the last message

Actual Result:

The welcome message is displayed for a while after deleting the last message

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.38-3
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

screen-recording-2023-07-10-at-75435-pm_dyFVnvza.mp4

Recording.3540.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689001566882329

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ddbab3fa4cdb7228
• Upwork Job ID: 1679926471892664320
2023-07-14
• Automatic offers:
• | | 0
• tienifr | Contributor | 25679418
• | | 0

[melvin-bot]

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

[melvin-bot]

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]

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

[melvin-bot]

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

[melvin-bot]

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

[mallenexpensify]

Added this to the op @ayazhussain79, otherwise the room disappears from LHN.

12. Pin the room

If you don't pin it, the room disappears from LHN, I'm unsure how someone would find it once it's gone if they don't know the room name. I wonder if can easily fix that as part of this bug or if we should have two bug issues? Or... maybe the one I found is a feature request? I'm uncertain of expected behaviour. What do you think @allroundexperts ?

[s-alves10]

Proposal

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

When deleting the last message, welcome message is displayed temporarily.

What is the root cause of that problem?

We calculate the lastMessageText(this is displayed in LHN) here

App/src/libs/actions/Report.js

Line 902 in e2e4057

const {lastMessageText = '', lastMessageTranslationKey = ''} = ReportActionsUtils.getLastVisibleMessage(originalReportID, optimisticReportActions);

As you can see, we get the last visible message from the report actions. That's why last visible action's message will be displayed in the LHN regardless it is whisper or not(Remember the welcome message is also a visible action)

This is the root cause

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

We need to filter out whisper actions when getting lastMessageText.

Add the 3rd parameter to the ReportActionsUtils.getLastVisibleMessage and ReportActionsUtils.getLastVisibleAction functions named includeWhisper and change the logic for getting visible actions as follows.

function getLastVisibleAction(reportID, actionsToMerge = {}, includeWhisper = true) {
    ...
    const visibleActions = _.filter(actions, (action) => !isDeletedAction(action) && (includeWhisper || !isWhisperAction(action)));


    ...
}

function getLastVisibleMessage(reportID, actionsToMerge = {}, includeWhisper = true) {
    const lastVisibleAction = getLastVisibleAction(reportID, actionsToMerge, includeWhisper);

    ...
}

This works as expected.

Result

22591.mp4

What alternative solutions did you explore? (Optional)

[tienifr]

Proposal

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

The welcome message is displayed for a while after deleting the last message.

What is the root cause of that problem?

There're many issues and inconsistencies with how we're checking which lastMessageText to display, in the following 2 locations.

1. When deleting a message:

a. For this particular issue, the cause is in here where we're not excluding whisper report action when getting the last report action to show.

b. Besides that we're also not excluding the pendingRemove message, we should as explained in here. This causes the issue where in offline mode, if we flag a second-to-last report action as "Assault", then delete our "last" report action, it will show the flagged report action text as the lastMessageText in LHN.

2. When we show lastMessageText in LHN

a. In here, we're not excluding the pending deleted messages (pending deleted messages will pass the shouldReportActionBeVisible check). This causes the issue where in offline mode, if we delete a second-to-last report action, then flag the "last" report action as "Assault", it will always show 'No activity yet' as the Last message text in the LHN because the .text of a deleted message is always empty.

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

1. We need to make the lastMessageText filtering logic consistent. We already have the shouldReportActionBeVisible util to check if a report action should be visible in the report action list or not. So we can create a similar util to check if a report action should be visible as the last action or not.

function shouldReportActionBeVisibleAsLastAction(reportAction) {
    return shouldReportActionBeVisible(reportAction, reportAction.reportActionID) && !isWhisperAction(reportAction) && !isDeletedAction(reportAction);
}

(in shouldReportActionBeVisible we already have the condition to exclude pendingRemove, the report action should be eligible for display in the report action list first before we consider if we want to display it as the last action/last message text)

2. Use shouldReportActionBeVisibleAsLastAction when deleting message in here

const visibleActions = _.filter(actions, (action) => shouldReportActionBeVisibleAsLastAction(action));

3. Use shouldReportActionBeVisibleAsLastAction when showing in LHN in here

(action) => ReportActionUtils.shouldReportActionBeVisibleAsLastAction(action)

This will make sure all cases for lastMessageText 1.a, 1.b, 2.a work properly

What alternative solutions did you explore? (Optional)

If we want to isolate the above shouldReportActionBeVisibleAsLastAction logic in getLastVisibleAction to only the delete message case, we can control it with a flag. However I've checked all usage of getLastVisibleAction in the app and it seems all those cases will benefit from the same logic change, since all of them are using getLastVisibleAction for the purpose of checking what last message text to show.

[allroundexperts]

is a feature request? I'm uncertain of expected behaviour. What do you think @allroundexperts ?

I think as long as you are the member of the room, it should display in LHN. If its not displaying, then it looks like a different bug to me.

[mallenexpensify]

Thanks @allroundexperts , and.... if 'room not showing in LHN' is a different/separate issue, can you think of any reason we'd want to address that first, before this bug report? I don't think it's necessary but would like bonus 👀 in case I'm missing something

[allroundexperts]

can you think of any reason we'd want to address that first, before this bug report

I think that would result in this issue being more clearly visible and more easily testable as well. It's a good call IMO.

[mallenexpensify]

Proposed the new feature https://expensify.slack.com/archives/C01GTK53T8Q/p1689714154006189
👀 please @allroundexperts to ensure I didn't miss anything. Removed Help Wanted for now.

[allroundexperts]

Proposed the new feature https://expensify.slack.com/archives/C01GTK53T8Q/p1689714154006189

Is this the correct link?

[mallenexpensify]

We're not going to address this issue of the room disappearing in LHN because, once some/anyone posts in the room, it'll show in LHN, so it's safe to assume that people will post in room and, if not, then the room isn't really useful.

So... we're clear to move forward with this specific issue. @allroundexperts , will you please review the proposals above and provide feedback? Thx

[allroundexperts]

On it today @mallenexpensify!

[allroundexperts]

Thanks for your proposal @s-alves10. While you've identified the correct root cause and have proposed a working solution as well, I think @tienifr's proposal is better in the sense that it incorporates all the places where this might be an issue.

Let's go with @tienifr's proposal.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[stitesExpensify]

@tienifr proposal looks good to me!

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

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

[allroundexperts]

Checklist

1. There isn't a specific PR that caused this. Whispers were not there initially when the getLastVisibleMessage and getLastVisibleAction functions were implemented. As such, when we later added whispers, we never updated the above functions.
2. N/A
3. https://expensify.slack.com/archives/C049HHMV9SM/p1690764003029809
4. I think that a regression test would be helpful here.

Regression Test Steps

1. Login to the userA’s account and create a new public room.
2. In room settings, set a welcome message.
3. Login to the userB's account and open the previously created room.
4. Pin that room and send any message.
5. Go offline and delete that message.
6. Verify that the LHN shows the correct last message.

Do we 👍 or 👎

[melvin-bot]

@mallenexpensify, @allroundexperts, @stitesExpensify, @tienifr Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@mallenexpensify, @allroundexperts, @stitesExpensify, @tienifr Still overdue 6 days?! Let's take care of this!

[stitesExpensify]

LGTM @allroundexperts. Can you fill out the checklist and then we can get this paid out?

[allroundexperts]

@stitesExpensify The checklist is complete.

[stitesExpensify]

Ah i see, I specifically meant can you check the boxes here #22591 (comment)

@mallenexpensify can we pay this and close it?

[allroundexperts]

Ah i see, I specifically meant can you check the boxes here #22591 (comment)

I don't have access to do that 😄

[stitesExpensify]

You can't check boxes!? Then why do we tag you on each of them haha

Either way, let's get this taken care of if we can @mallenexpensify

[melvin-bot]

@mallenexpensify, @allroundexperts, @stitesExpensify, @tienifr Still overdue 6 days?! Let's take care of this!

[mallenexpensify]

Issue reporter: @ayazhussain79 paid $250 via Upwork
Contributor: @tienifr paid $1500 via Upwork
Contributor+: @allroundexperts paid via Newdot

[ayazhussain79]

@mallenexpensify offer accepted, Thank you

[allroundexperts]

Requested money in ND!

[JmillsExpensify]

Reviewed the details for @allroundexperts. $1,500 approved for payment in NewDot based on the BZ summary above.

[melvin-bot]

@mallenexpensify, @allroundexperts, @stitesExpensify, @tienifr Eep! 4 days overdue now. Issues have feelings too...

[stitesExpensify]

Is this ready to be closed?

[allroundexperts]

Yep!

[ayazhussain79]

Payment pending on Upwork

[mallenexpensify]

Updated above, also pasting here
Issue reporter: @ayazhussain79 paid $250 via Upwork
Contributor: @tienifr paid $1500 via Upwork
Contributor+: @allroundexperts paid via Newdot

Testrail update GH - https://github.com/Expensify/Expensify/issues/312295