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
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/f7946840-dd12-4b5d-abb1-12e0d27c90fb

View all open jobs on GitHub

[melvin-bot[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.

https://github.com/user-attachments/assets/56203355-aaab-48f8-8d26-fecf3a635aa6

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

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

[melvin-bot[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: https://github.com/Expensify/App/blob/8981081bed8fb8a07f8797d49af2797332c9259d/src/libs/actions/Delegate.ts#L358-L362 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:

https://github.com/Expensify/App/blob/8981081bed8fb8a07f8797d49af2797332c9259d/src/pages/settings/Security/SecuritySettingsPage.tsx#L144

we only display the errorFields?.addDelegate.

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

• We need to remove:

https://github.com/Expensify/App/blob/8981081bed8fb8a07f8797d49af2797332c9259d/src/libs/actions/Delegate.ts#L358-L362

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

https://github.com/Expensify/App/blob/8981081bed8fb8a07f8797d49af2797332c9259d/src/pages/settings/Security/SecuritySettingsPage.tsx#L144-L145

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:

https://github.com/Expensify/App/blob/8981081bed8fb8a07f8797d49af2797332c9259d/src/pages/settings/Security/SecuritySettingsPage.tsx#L178

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

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

[melvin-bot[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!

[mountiny]

cc @dangrous

[melvin-bot[bot]]

@mollfpr, @VictoriaExpensify Still overdue 6 days?! Let's take care of this!

[melvin-bot[bot]]

@mollfpr, @VictoriaExpensify Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[melvin-bot[bot]]

@mollfpr, @VictoriaExpensify 12 days overdue now... This issue's end is nigh!

[melvin-bot[bot]]

This issue has not been updated in over 14 days. @mollfpr, @VictoriaExpensify eroding to Weekly issue.

[dangrous]

Hi all! hopping in in the new year. Can you confirm what we need to do on the backend? Reading through, it sounded like we needed to send an error from the BE, but I think we're already doing that, right? Let me know what I'm missing, though, and happy to tackle the BE update.

[dangrous]

Hi! @mollfpr bumping the question above when you have a moment - let me know what we should be updating in the BE and I can take a stab at it

[dangrous]

Hi all! hopping in in the new year. Can you confirm what we need to do on the backend? Reading through, it sounded like we needed to send an error from the BE, but I think we're already doing that, right? Let me know what I'm missing, though, and happy to tackle the BE update.

@mollfpr Bumping this - let me know what action I can take to help move this along. Thanks!

[mollfpr]

Sorry @dangrous I lost track of the issue 🙏

Here's what I think we can do in BE.

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.

[dangrous]

Ah okay so something sort of like the openPolicyCompanyCardsPage we already have? Do we do anything like that for profile info yet? Not a deal-breaker, but just wondering. I guess that's probably the easiest way, though we'd need to add a loader for the page.

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