[$1000] Closed accounts aren't removed from workspace members list

[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. Invite new user (that doesn't have a NewDot account)
2. Valifdate this user, then login with this new user account in ND in private mode
3. Close this new user account
4. Switch to Desktop App with admin account and check members list

Expected Result:

5. Closed account was removed from the members list

Actual Result:

7. Closed accounts are still displayed in the members list

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.24.4

Reproducible in staging?: Yes

Reproducible in production?: No

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

Closed account

Bug6081613_ws-member-closed-account.mp4

Member's list

Bug6081613_ws-members-list.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0190db70a7a3eb2f5b
• Upwork Job ID: 1672152459568193536
• Last Price Increase: 2023-06-30

[OSBotify]

👋 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 @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Beamanator]

Hopinggggg this isn't due to my PR: https://github.com/Expensify/Web-Expensify/pull/37723

@Julesssss let me know if you want help investigating

[Julesssss]

Regardless it doesn't seem like a blocker. Testing locally now.

[Julesssss]

It seems very weird that this only occurs on Desktop staging:

• Desktop prod: ✅
• Desktop staging: ❌ -- I can reproduce
• Web staging: ✅
• Web prod: ✅

@Beamanator I doubt your changes would lead to this Desktop only issue, right?

[Beamanator]

Ooh ya I have no idea how mu changes would only cause this on desktop....

[roryabraham]

I think the problem is that desktop deploys are broke

[Julesssss]

Yep, good call:

Web version: v1.3.24-4
Desktop version: v1.3.24-3

Definately not a blocker, but I'll keep this open until I can test against the Desktop build

[roryabraham]

1.3.24-4 successfully deployed on desktop. 5th time's the charm! Looks like this is NAB but let's retest and close if not reproducible.

[roryabraham]

@kbecciv Can we please retest?

[Julesssss]

I'm going to close this myself after retesting Desktop on v1.3.25-1

[kbecciv]

Hello @roryabraham! Do you want QA team to re-test this issue? Let me know

[Julesssss]

Hi @kbecciv, yes please do!

[kbecciv]

Checking

[kbecciv]

@Julesssss Results below:

Member's list is fixed

20264.Desktop.mp4

Closed account is NOT fixed

20264.closed.account.mp4

[Julesssss]

Thanks @kbecciv, is that just Desktop or all platforms?

[kbecciv]

Tester checked only Desktop

[Julesssss]

Could you please test all platforms? I think it was only reported on Desktip due to the release missmatch.

[melvin-bot]

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

[Julesssss]

Bumping @aimane-chnaif for a review of this proposal. Or let me know if I should reassign 👍

[aimane-chnaif]

reviewing today

[melvin-bot]

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

[aimane-chnaif]

BhuvaneshPatil's proposal is not complete.
Need explanation of how to remove accountID key of the closed account using permal link.
Still open for proposals.

[BhuvaneshPatil]

@aimane-chnaif

My idea for implementing that is to modify the closeAccount() in User.js.
And make something similar to this -

function closeAccount(message) {
    // Note: successData does not need to set isLoading to false because if the CloseAccount
    // command succeeds, a Pusher response will clear all Onyx data.
    API.write(
        'CloseAccount',
        {message},
        {
            optimisticData: [
                {
                    onyxMethod: Onyx.METHOD.MERGE,
                    key: ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM,
                    value: {isLoading: true},
                },
                {
                    onyxMethod: Onyx.METHOD.MERGE,
                    key: ONYXKEYS.PERSONAL_DETAILS_LIST,
                    value: {
                        [currentUserAccountID]: undefined,
                    },
                },
            ],
            successData: [
                {
                    onyxMethod: Onyx.METHOD.MERGE,
                    key: ONYXKEYS.PERSONAL_DETAILS_LIST,
                    value: {
                        [currentUserAccountID]: undefined,
                    },
                },
            ],
            failureData: [
                {
                    onyxMethod: Onyx.METHOD.MERGE,
                    key: ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM,
                    value: {isLoading: false},
                },
            ],
        },
    );
}

But I think there are some changes that need to be done on backend as well.

Because for update avatar - this is what response looks like

But for close account -

The response doesn't contains successData or the key that we have changed.

My guess is that there are some changes that we need to do on backend(expensify team can confirm that), because there is something mentioned in the comments for closeAccount().

What's your thought @aimane-chnaif

[muttmuure]

@Julesssss do you mind confirming if @BhuvaneshPatil's theory about the backend changes is correct

@aimane-chnaif could you confirm if the front end fix proposal looks complete now

[aimane-chnaif]

I think this should be fully fixed in backend. So when close account, need to remove from all workspaces and rooms where user had joined.
After closing account, user is signed out and redirected to login page. So no need to fix frontend.
@BhuvaneshPatil please explain if you disagree and still need frontend fix.

[Julesssss]

So when close account, need to remove from all workspaces and rooms where user had joined.

I agree, but as this would notify all accounts within a workspace/room and because close account is such a rare action I'm not even sure we should fix this. Leaning towards just closing it, what do you think @muttmuure?

[melvin-bot]

@twisterdotcom @Julesssss @muttmuure @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

[twisterdotcom]

but as this would notify all accounts within a workspace/room and because close account is such a rare action I'm not even sure we should fix this.

I disagree. Just because something is rare, does not mean we shouldn't fix it. Ideally, every case is polished, edge or not. If we think this does need to be Internal then we should make it that, and prioritise it accordingly. That sounds like what we'll do.

If you agree @Julesssss, let's just leave you assigned and make it a Monthly?

[Julesssss]

Sure, but it's probably going to just sit in my backlog for the rest of the year tbh.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[twisterdotcom]

If this is not reproducible, can we close?