[HOLD for payment 2024-04-15] [$500] Room - Members added and deleted offline are not crossed out

[lanitochka17]

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.56-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #35355

Action Performed:

1. Login to New Expensify
2. Navigate to any room
3. Disable the internet connection
4. Click on room header > Members
5. Invite new members
6. Delete the new members

Expected Result:

Deleted members should be grayed out and crossed out/strikethrough

Actual Result:

Members added and deleted offline are grayed out but strikethrough is not applied

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

Bug6423422_1711127542111.2024-03-22_17-50-01.mp4

Bug6423422_1711127542089.Record_2024-03-22-17-54-58.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011972614e24d84517
• Upwork Job ID: 1772560274353958912
• Last Price Increase: 2024-03-26
• Automatic offers:
• mollfpr | Reviewer | 0
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @laurenreidexpensify

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[lanitochka17]

We think that this bug might be related to #vip-vsp

[lanitochka17]

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

[francoisl]

This looks related to #35355
cc @tienifr @alitoshmatov.

Can't tell if it's on purpose or not, but regardless it doesn't sound like a blocker. I'll demote for now.

[bernhardoj]

Proposal

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

The deleted member while offline is grayed out but not strikethrough-ed.

What is the root cause of that problem?

We have a property called pendingChatMembers. When we add or remove a member, we will push a new pendingChatMembers to the existing one.

App/src/libs/ReportUtils.ts

Lines 2846 to 2849 in 191d932

	function getPendingChatMembers(accountIDs: number[], previousPendingChatMembers: PendingChatMember[], pendingAction: PendingAction): PendingChatMember[] {
	const pendingChatMembers = accountIDs.map((accountID) => ({accountID: accountID.toString(), pendingAction}));
	return [...previousPendingChatMembers, ...pendingChatMembers];
	}

For example, if we add user A and then delete it, the pendingChatMembers will be:
[{accountID: A_ID, pendingAction: 'add'}, {accountID: A_ID, pendingAction: 'delete'}]

Then, we get the first pending member and get the pending action value,

App/src/pages/RoomMembersPage.tsx

Line 201 in 191d932

const pendingChatMember = report?.pendingChatMembers?.find((member) => member.accountID === accountID.toString());

App/src/pages/RoomMembersPage.tsx

Line 218 in 191d932

pendingAction: pendingChatMember?.pendingAction,

in this case, the pending action add is the first one to be found, that's why we can't see the strikethrough.

this probably happens in ReportParticipantsPage too

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

The simplest one is to use findLast to find the latest pending member.

to further improve the UX, we can disable the pending delete member

App/src/pages/RoomMembersPage.tsx

Line 207 in 191d932

isDisabled: accountID === session?.accountID,

isDisabled: accountID === session?.accountID || pendingChatMember?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,

What alternative solutions did you explore? (Optional)

Change the structure of pendingChatMembers to be an object, so we can only have one accountID in pendingChatMembers

pendingChatMembers: {
    123: 'add',
}

and if we merge it with
{
    123: 'delete',
}

then it became
pendingChatMembers: {
    123: 'delete',
}

and to get the pending action, we just need to do:

report?.pendingChatMembers?.[accountID]

or we can update getPendingChatMembers to remove any duplicate, for example by iterating from the back, keep track of found accountID, and skipping the item that the accountID is already found.

const foundAccountIDs: Record<string, boolean> = {};
const result = [...previousPendingChatMembers, ...pendingChatMembers];
const newResult: PendingChatMember[] = [];

for (let i=result.length-1; i>=0; i--) {
    if (!foundAccountIDs[result[i].accountID]) {
        foundAccountIDs[result[i].accountID] = true;
        newResult.push(result[i]);
    }
}

return newResult;

[bernhardoj]

@francoisl I don't think it's a regression of anything, but a missed case to be handled.

1. Fix/34671: Grey out pending member in report #35883 fixes the issue that the member isn't grayed out when adding it while offline, but at this time, there is another bug, that is when you remove a member while offline, the member completely disappears, so they have no way to test the delete while offline case.
2. Then, Room settings-Members added/deleted in a room in offline mode are not grayed out/crossed #35355 fixes the delete offline issue.

The fix from (2) already works correctly, but because of the imperfect implementation from (1), we get this issue where it fails when we add and delete the same user while offline.

EDIT: updated my proposal to include another alternative

[tienifr]

@francoisl Yes, it's not my regression

[melvin-bot]

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

[bernhardoj]

@francoisl I think you miss the External label?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011972614e24d84517

[melvin-bot]

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

[mollfpr]

Thanks @bernhardoj

I think it makes sense for now to use the simple solution to avoid any regressions.

The main solution from @bernhardoj's proposal looks good to me!

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

Current assignee @francoisl is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[laurenreidexpensify]

Waiting on @francoisl sign off

[melvin-bot]

📣 @mollfpr 🎉 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]

📣 @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]

PR is ready

cc: @mollfpr

[melvin-bot]

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

[melvin-bot]

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

• Fix pending delete room members are not strikethrough-ed #39231

If no regressions arise, payment will be issued on 2024-04-15. 🎊

For reference, here are some details about the assignees on this issue:

• @mollfpr requires payment automatic offer (Reviewer)
• @bernhardoj requires payment automatic offer (Contributor)

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

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

[laurenreidexpensify]

Payment Summary -

• C+ $500 @mollfpr - please request payment in newdot
• Contributor $500 @bernhardoj payment issued in upwork

[laurenreidexpensify]

@mollfpr pls confirm checklist steps above thank you

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:

#35883

[@mollfpr] 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:

https://github.com/Expensify/App/pull/35883/files#r1567706303

[@mollfpr] 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:

I think the regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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.

1. Create a new room if you don't have one
2. Go offline
3. Invite a new user to the room
4. Verify the user is grayed out
5. Remove the user from the room
6. Verify the user is grayed out and strikethrough-ed
7. 👍 or 👎

[JmillsExpensify]

$500 approved for @mollfpr