[$250] Security - Unable to remove closed account from copilot

[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: 9.0.73-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Precondition: copilot account B is added to account A. Account B was closed.

1. Account A: go to Security
2. Click on three dots near copilot account B
3. Click on Remove copilot > Remove copilot

Expected Result:

Account B is remover from the list

Actual Result:

Account B is no removed from the list

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6689200_1733770674968.copilot_remove.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021866325124686695861
• Upwork Job ID: 1866325124686695861
• Last Price Increase: 2024-12-17

Issue Owner

Current Issue Owner: @mollfpr

[melvin-bot]

Triggered auto assignment to @VictoriaExpensify (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.

[Shahidullah-Muffakir]

I couldn't reproduce it.

Screen.20Recording.202024-12-10.20at.201.mp4

[lanitochka17]

@Shahidullah-Muffakir please check again preconditions have been added:
Copilot account B is added to account A. Account B was closed.

[Shahidullah-Muffakir]

@Shahidullah-Muffakir please check again preconditions have been added: Copilot account B is added to account A. Account B was closed.

Thank you, Yes I could reproduce it now.
It seems like a BE issue:

[VictoriaExpensify]

Yeah this is an issue we should fix.

[melvin-bot]

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

[melvin-bot]

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

[mollfpr]

I agree this is a BE issue. The ND should send a pusher message to update the account.delegates to their delegators when closing an account. But this doesn't solve this issue, until the user re-login.

The other thing we can do in the ND is, call an API that contains fresh account data when the user opens the security page. It does solve the issue and the user does not need to re-login to get the correct delegate data.

@VictoriaExpensify We might need an internal engineer to choose the right path.

[daledah]

Proposal

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

• The error message is not displayed when API encountered error.

What is the root cause of that problem?

• In PR we already display the error message from server, but we didn't handle remove delegate case. The issue is just one of the cases the API throws error.

• In here:
  

  App/src/libs/actions/Delegate.ts

  Lines 358 to 362 in 8981081

  	errorFields: {
  	removeDelegate: {
  	[email]: null,
  	},
  	},

  
  so even if the request encountered an error, the error is always cleared.

• Even when we remove the above logic, the error message still cannot be displayed since in here:

App/src/pages/settings/Security/SecuritySettingsPage.tsx

Line 144 in 8981081

const addDelegateErrors = errorFields?.addDelegate?.[email];

we only display the errorFields?.addDelegate.

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

• We need to remove:

App/src/libs/actions/Delegate.ts

Lines 358 to 362 in 8981081

	errorFields: {
	removeDelegate: {
	[email]: null,
	},
	},

• Then, need to consider displaying removeDelegate error as well. So:

App/src/pages/settings/Security/SecuritySettingsPage.tsx

Lines 144 to 145 in 8981081

	const addDelegateErrors = errorFields?.addDelegate?.[email];
	const error = ErrorUtils.getLatestError(addDelegateErrors);

can be:

                   const addOrRemoveDelegateErrors = errorFields?.addDelegate?.[email] ?? errorFields?.removeDelegate?.[email];
                    const error = ErrorUtils.getLatestError(addOrRemoveDelegateErrors);
                    const isAddError = !!errorFields?.addDelegate?.[email];

• Last, we need to update onPendingActionDismiss:

App/src/pages/settings/Security/SecuritySettingsPage.tsx

Line 178 in 8981081

onPendingActionDismiss: () => clearDelegateErrorsByField(email, 'addDelegate'),

                        onPendingActionDismiss: () => clearDelegateErrorsByField(email, isAddError ? 'addDelegate' : 'removeDelegate'),

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

[daledah]

I believe the issue here is that we fail to display an error message when the API to remove a delegate encounters an error. There are many scenarios where the API might throw an error, and this is just one of them—specifically, when the pusher doesn't update the account.delegates data after closing an account.

To address this, we should:

1. Fix the pusher issue on the backend to ensure it updates correctly.
2. Ensure that an error message is displayed when an error occurs, as we already implemented in this PR.

[mollfpr]

@daledah That's a good suggestion to fix the show error message, but I think the issue persists. The user still can't remove the copilot until re-login.

The pusher fix only solved the issue when the copilot closed their account.

[daledah]

Thanks, @mollfpr! Could you tag the relevant internal team here to provide the final decision? In my opinion, we can address the error message display and the pusher issue simultaneously.

[melvin-bot]

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

[mollfpr]

@VictoriaExpensify Could you add the internal label? Thanks!

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

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

[melvin-bot]

@mollfpr @VictoriaExpensify 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!

[mollfpr]

Yup, similar to that or OpenPaymentsPage!

Do we do anything like that for profile info yet?

We only have OpenInitialSettingsPage but it doesn't return any data related to account delegates.

[dangrous]

Okay so I've made a draft command that the App will be able to call - OpenSecuritySettingsPage - that will return the following onyx update:

            [
                'onyxMethod' => Onyx::METHOD_MERGE,
                'key' => OnyxKeys::ACCOUNT,
                'value' => [
                    'delegatedAccess' => [up to date delegated access data],
                ],
            ],

Does that feel right? If so, are you up for making an App PR that calls that on opening the security page, and then I can test locally to see if that works out?

[dangrous]

bump on this ^^ when you have a moment @mollfpr - thanks!

[VictoriaExpensify]

Not overdue, waiting for @mollfpr to chime in

[mollfpr]

@dangrous Yeah, the API looks right!

If so, are you up for making an App PR that calls that on opening the security page, and then I can test locally to see if that works out?

Sure! I'll open up a draft PR for you to test now.

[mollfpr]

@dangrous Here's the draft PR #55709

[dangrous]

Perfect, thanks! Looks like BE is working well. Going to clean up that PR and get that merged, then we can move ahead with yours!

[dangrous]

hey @mollfpr! That command is now live on staging, and ready to be tested with your PR

[mollfpr]

@dangrous Thank you! I'll try on my PR!

[dangrous]

How's this one looking?

[mollfpr]

@dangrous I found a case that make the issue still reproduced.

While the account A is open the security page, it will show the account B as the copilot. Open the account B on other device and close the account. In this step the account B is still appear on account A and will fail if try to remove the account B from copilot.

I'm thinking to calling the API again when it happen, so it will update the copilot list. What do you think?

[dangrous]

hrm, that's a pretty edge case for the front end... So you're thinking just recall OpenSecuritySettingsPage before calling RemoveDelegate? I think that breaks our 1:1:1 policy.

I could potentially update the backend to just "succeed" if the account is closed? Not sure what that would take but might be okay...

[mollfpr]

@dangrous Actually, after the remove delegate is failing.

I could potentially update the backend to just "succeed" if the account is closed?

I'm onboard with this idea!

[dangrous]

Cool - backend PR with that change is in review! If a user tries to remove a delegate with a closed account, it won't error, and it'll queue an onyx update sending the updated delegatedaccess

[dangrous]

Okay, that change is live on prod - let me know if we need to adjust anything else, @mollfpr

[mollfpr]

@dangrous Thank you! The test looks great and I'll finish the PR now.

[mollfpr]

@dangrous On second thoughts, I think we don't need ND PR. There's no error in removing the close account as described in the issue.

Screen.Recording.2025-02-12.at.23.25.49.mp4

[dangrous]

I think we should still update the list on opening the security settings page, though - that'll provide the best experience, since it'll lessen the amount of times people even have to remove a delegate.

[mollfpr]

@dangrous I open a new PR #56900