Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update UserDeleteService to use roles not users #9049

Open
wants to merge 1 commit into
base: missing-test-coverage
Choose a base branch
from

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Oct 31, 2024

When deleting a user we also delete any groups that user created that don't contain any annotations by other users. Change this to ignore whether the user created a group and instead delete any groups that the user is the only owner of, again as long as the group doesn't contain any annotations by other users.

With the current version of the code the SQL query that's ultimately generated is:

SELECT "group".id 
FROM "group"
LEFT OUTER JOIN annotation ON annotation.groupid = "group".pubid AND annotation.deleted IS false 
WHERE "group".id IN (
  SELECT user_group.group_id 
  FROM user_group 
  WHERE user_group.group_id IN (
    SELECT user_group.group_id 
    FROM user_group 
    WHERE (user_group.roles @> '"owner"'::jsonb)
    GROUP BY user_group.group_id 
    HAVING count(user_group.group_id) = 1
  )
  AND (user_group.roles @> '"owner"'::jsonb)
  AND user_group.user_id = 22562
)
GROUP BY "group".id 
HAVING count(annotation.id) = 0;

It's not currently possible to test that query in production because the roles column hasn't been deployed to production yet.

@seanh seanh changed the title delete users roles Update UserDeleteService to use roles not users Oct 31, 2024
@seanh seanh changed the base branch from main to roles-model October 31, 2024 19:01
@seanh seanh changed the base branch from roles-model to main October 31, 2024 19:21
@seanh seanh force-pushed the delete-users-roles branch 2 times, most recently from c04f32a to cab6bdd Compare November 1, 2024 09:39
seanh added a commit that referenced this pull request Nov 1, 2024
`TestLimitedWorker` is missing unit test coverage for a couple of branches in
`LimitedWorker`. The reason why coverage isn't failing on `main` is that
`TestUserPurger` (the unit tests for `UserPurger`) calls through to the
real `LimitedWorker` code (doesn't mock it) and those `UserPurger` unit
tests were adding coverage to `LimitedWorker`.

In a future PR (#9049) I want to
make some changes to `TestUserPurger` that would cause them to no longer
cover certain branches of `LimitedWorker` and would cause a coverage
failure. It isn't `TestUserPurger`'s job to cover every line and branch
of `LimitedWorker`, that's `TestLimitedWorker`'s job, so I'm adding the
missing coverage to `TestLimitedWorker` now to prevent those coverage
failures in future.

This illustrates some of the risks of integrated tests:

* You make changes to the tests for one unit and that causes coverage
  failures in *other* units, creating extra work to fix them. If we used
  integrated tests frequently this could happen often.
* You can have missing unit tests and not now it because the lines and
  branches are accidentally covered by the tests for *other* units.
* You might have a unit test that you think is covering a branch but
  actually isn't going down that branch. Even though the test is wrong
  it could still be passing. Missing coverage could save you but won't
  if tests for *other* units are covering the branch that your broken
  test is supposed to be covering.

In this case I wouldn't want `TestUserPurger` to mock `LimitedWorker`:
`UserPurger` contains lots of complex SQL queries that it executes
through `LimitedWorker` rather than directly. We want these queries to
be executed against the real DB so we can test that various objects are
deleted or not-deleted correctly.
seanh added a commit that referenced this pull request Nov 1, 2024
`TestLimitedWorker` is missing unit test coverage for a couple of
branches in `LimitedWorker`. The reason why coverage isn't failing on
`main` is that `TestUserPurger` (the unit tests for `UserPurger`) calls
through to the real `LimitedWorker` code (doesn't mock it) and those
`UserPurger` unit tests were adding coverage to `LimitedWorker`.

In a future PR (#9049) I want to
make some changes to `TestUserPurger` that would cause them to no longer
cover certain branches of `LimitedWorker` and would cause a coverage
failure. It isn't `TestUserPurger`'s job to cover every line and branch
of `LimitedWorker`, that's `TestLimitedWorker`'s job, so I'm adding the
missing coverage to `TestLimitedWorker` now to prevent those coverage
failures in future.

This illustrates some of the risks of integrated tests:

* You make changes to the tests for one unit and that causes coverage
  failures in *other* units, creating extra work to fix them. If we used
  integrated tests frequently this could happen often.
* You can have missing unit tests and not know it because the lines and
  branches are accidentally covered by the tests for *other* units.
* You might have a unit test that you think is covering a branch but
  actually isn't going down that branch. Even though the test is wrong
  it could still be passing. Missing coverage could save you but won't
  if tests for *other* units are covering the branch that your broken
  test is supposed to be covering.

In this case I wouldn't want `TestUserPurger` to mock `LimitedWorker`:
`UserPurger` contains lots of complex SQL queries that it executes
through `LimitedWorker` rather than directly. We want these queries to
be executed against the real DB so we can test that various objects are
deleted or not-deleted correctly.

I think the ideal solution might be for `TestUserPurger` to mock
`LimitedWorker`, intercept the SQLAlchemy queries that `UserPurger`
passes to the mock `LimitedWorker`, and for the _tests_ to then execute
those queries against the DB and assert that they return the right
results. But I'm not going to bite off that relatively major change
right now.
When deleting a user account we currently delete any groups that the
user created and that don't contain any annotations by other users.

This made sense when the creator was the only person who could edit the
group (changing its name, description and type) or moderate annotations
in the group: groups with no creator were kind of broken and may as well
be deleted (as long as they didn't contain any annotations).

But we've now added the concept of group membership roles and in
particular a group can have multiple "owners" who all have full
permissions to edit and moderate the group. Even if the group's creator
no longer exists, the group may still have other owners who have full
control of the group.

So instead of deleting groups that the user created, change
`UserDeleteService` to delete groups that the user is the only owner of
(and that don't contain any annotations by other users).
@seanh seanh changed the base branch from main to missing-test-coverage November 1, 2024 13:27
@seanh seanh marked this pull request as ready for review November 1, 2024 13:58
@seanh seanh requested a review from marcospri November 1, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant