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.

Sign up for GitHub

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

Jump to bottom

Implement undelete and purge API for users #8309

Merged
merged 14 commits into from
Aug 27, 2019
Merged

Implement undelete and purge API for users #8309

merged 14 commits into from
Aug 27, 2019

Conversation

cganote
Copy link
Contributor

@cganote cganote commented Jul 8, 2019

Refactored the controller vs. manager for the deletion and purge of users. Purge now wipes out the username (regardless of gdrp setting) and takes out histories. Undelete should now work.

@galaxybot galaxybot added this to the 19.09 milestone Jul 8, 2019
@martenson

This comment has been minimized.

Sign in to view
@hexylena
Copy link
Member

hexylena commented Jul 8, 2019

Purge now wipes out the username...and takes out histories.

🎉❤️🎉❤️🎉❤️

cganote
cganote commented Jul 11, 2019
View reviewed changes
Copy link
Contributor Author

@cganote cganote left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@martenson martenson added area/API status/review kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes and removed triage labels Jul 11, 2019
@martenson
Copy link
Member

just now noticed #8299 :/

@martenson
Copy link
Member

I cherry picked the test commits from #8299 and fixed them. This PR now supersedes the other one.

@martenson

This comment has been minimized.

Sign in to view
@martenson martenson mentioned this pull request Jul 12, 2019
Closed
@martenson martenson changed the title PR for user purge doing real purgey things! implement undelete and purge API for users, refactor Jul 12, 2019
@hexylena
Copy link
Member

hexylena commented Aug 9, 2019

Fixes #930

@nsoranzo nsoranzo changed the title implement undelete and purge API for users, refactor Implement undelete and purge API for users Aug 9, 2019
nsoranzo
nsoranzo reviewed Aug 9, 2019
View reviewed changes
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
@nsoranzo nsoranzo requested a review from hexylena August 9, 2019 18:35
@bernt-matthias
Copy link
Contributor

Any chance to cover this old one by me #3843

@martenson
Copy link
Member

@bernt-matthias I wouldn't tie it to this PR though

@bernt-matthias
Copy link
Contributor

I wouldn't tie it to this PR though

sure

@hexylena hexylena mentioned this pull request Aug 15, 2019
Closed
juleengraham and others added 6 commits August 22, 2019 15:04
@juleengraham @martenson
Addressed lint issues
4e7371f
@martenson
fix tests
2fa7eba
@martenson
bubble up exception without masking, adjust docstring
2ae4737
@martenson
use the superclass for basic operations
32b1d1f 
suggested by nsoranzo
@martenson
remove unused imports
b218f36
@martenson
avoid calling superclass method for purging
addfeff 
Because multiple inheritance will result in calling this class 'delete' method
which is not the correct target. This also avoid a database operation
I believe, since the PurgableManagerMixin.purge also calls delete before purging.
nsoranzo
nsoranzo reviewed Aug 27, 2019
View reviewed changes
test/api/test_users.py Outdated Show resolved Hide resolved
test/api/test_users.py Outdated Show resolved Hide resolved
nsoranzo
nsoranzo reviewed Aug 27, 2019
View reviewed changes
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
lib/galaxy/managers/users.py Outdated Show resolved Hide resolved
@martenson
Copy link
Member

@nsoranzo I think I'm not a fan of going from api to manager then to mixin, then same manager again, then same mixin again for one method. It feels confusing so I'd like to avoid that.

@nsoranzo
Copy link
Member

@nsoranzo I think I'm not a fan of going from api to manager then to mixin, then same manager again, then same mixin again for one method. It feels confusing so I'd like to avoid that.

I think these are good abstractions. For reference:

  • the API takes care of the communication with the client, checks the inputs and permissions, then invokes the methods of the right manager
  • the manager inherits and specialises some methods from superclasses and mixins, ensuring a consistent interface
  • the PurgableManagerMixin mixin implements a generic purge() method, which just calls the delete() method and sets the purged attribute of the item
  • for UserManager, we need to specialise both the delete() and purge() method, inherited from PurgableManagerMixin

@martenson
Copy link
Member

I don't think the path manager>mixin>manager>mixin>done is better than manager>done but at this point it seems like a matter of opinion and I will defer to yours.

hexylena
hexylena approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

Fine for me :)

@martenson martenson merged commit 94e08eb into galaxyproject:dev Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martenson martenson martenson left review comments

@nsoranzo nsoranzo nsoranzo approved these changes

@hexylena hexylena hexylena approved these changes

Assignees
No one assigned
Labels
area/API kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Milestone
19.09
Development

Successfully merging this pull request may close these issues.
8 participants
@cganote @martenson @hexylena @bernt-matthias @nsoranzo @galaxybot @mvdbeek @juleengraham