Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

code-monitors: handle soft-deleted users#60405

Merged
stefanhengl merged 5 commits intomainfrom
sh/code-monitors/fix-soft-deleted-user
Feb 13, 2024
Merged

code-monitors: handle soft-deleted users#60405
stefanhengl merged 5 commits intomainfrom
sh/code-monitors/fix-soft-deleted-user

Conversation

@stefanhengl
Copy link
Member

@stefanhengl stefanhengl commented Feb 12, 2024

Code monitors from soft-deleted users cause errors in the client. For example, if an admin opens the code monitors page, they see a "user not found" error.

image

With this change we filter out code monitors of soft-deleted users. This means, the system effectively behaves as if the user was hard-deleted.

For users that were actually hard-deleted, we already delete their code monitors, which avoids this issue.

I also confirmed that we don't run code monitors of soft-deleted users.

Alternatives considered:

I first went down the path to fix this on the level of the resolver and in the client. However it turned out that this would require a lot of small changes of the GraphQL API paired with some fallback logic in the client. Overall the approach taken in this PR is much smaller and easier to understand.

Test plan:

  • new unit test
  • manual testing:
    • I created a code monitor as user Alice and then, as admin, soft-deleted Alice
    • I confirmed that there is no error and that, as admin, I don't see the code monitor
    • Undeleting Alice makes the code monitor reappear in the list

Code monitors from soft-deleted users cause errors in the client. For
example, if an admin opens the code monitors page, they see a "user not
found" error.

With this change we filter code monitors of soft-deleted users. This
means the system effectively behaves as if the user was hard-deleted.

For users that were actually hard-deleted, we already delete their code
monitors, which avoids this issue.

Test plan:
new unit test
@cla-bot cla-bot bot added the cla-signed label Feb 12, 2024
@stefanhengl stefanhengl requested a review from a team February 12, 2024 15:25
@stefanhengl stefanhengl marked this pull request as ready for review February 12, 2024 15:25
@jtibshirani jtibshirani requested a review from a team February 12, 2024 17:16
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach makes sense to me.

Copy link
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! This has come up a few times and I've never found the time to look into it

@keegancsmith
Copy link
Member

Backport? Probably best left for the next patch release. Also don't forget changelog :)

@stefanhengl stefanhengl merged commit 2beb52a into main Feb 13, 2024
@stefanhengl stefanhengl deleted the sh/code-monitors/fix-soft-deleted-user branch February 13, 2024 13:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants