-
Notifications
You must be signed in to change notification settings - Fork 427
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
seanh
wants to merge
1
commit into
missing-test-coverage
Choose a base branch
from
delete-users-roles
base: missing-test-coverage
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seanh
changed the title
delete users roles
Update Oct 31, 2024
UserDeleteService
to use roles not users
seanh
force-pushed
the
delete-users-roles
branch
from
October 31, 2024 19:21
e1c5830
to
b984b82
Compare
seanh
force-pushed
the
delete-users-roles
branch
2 times, most recently
from
November 1, 2024 09:39
c04f32a
to
cab6bdd
Compare
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
force-pushed
the
delete-users-roles
branch
from
November 1, 2024 13:27
cab6bdd
to
a8c4bf6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
It's not currently possible to test that query in production because the
roles
column hasn't been deployed to production yet.