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

Expose 4 more rows from the galaxy_user table to help out sys admins #4255

Merged
merged 2 commits into from Jul 5, 2017

Conversation

Projects
None yet
6 participants
@XDtim
Copy link
Contributor

commented Jun 28, 2017

Galaxy admins can use this extra data returned via the API to better automate user management and cleanup.

@galaxybot galaxybot added the triage label Jun 28, 2017

@galaxybot galaxybot added this to the 17.09 milestone Jun 28, 2017

@mvdbeek mvdbeek added area/API status/review and removed triage labels Jun 28, 2017

@XDtim

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

Humm, not sure why this test case broke. It isn't in the user part of the API.

Looking into it now

@XDtim

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

Humm, I think we should re-run this. I tested it locally on my branch with this commnd:

./run_tests.sh -api test/api/test_dataset_collections.py:DatasetCollectionApiTestCase.test_list_pair_download
And got this output:

ID: api.test_dataset_collections.DatasetCollectionApiTestCase.test_list_pair_download
Status: success

@galaxybot test this

@dannon

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

@XDtim I think this test may intermittently fail. I've restarted the test.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

@XDtim

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

Should we merge this?

@@ -152,7 +152,7 @@ class User( object, Dictifiable ):
histories, credentials, and roles.
"""
# attributes that will be accessed and returned when calling to_dict( view='collection' )
dict_collection_visible_keys = ( 'id', 'email', 'username' )
dict_collection_visible_keys = ( 'id', 'email', 'username', 'deleted', 'purged', 'active', 'last_password_change' )

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 3, 2017

Member

I'd probably remove 'purged', users cannot be purged at the moment as far as I know.

Also, can you please add 'deleted', 'active', 'last_password_change' to dict_element_visible_keys below? dict_collection_visible_keys is supposed to be a subset of it.

This comment has been minimized.

Copy link
@XDtim

XDtim Jul 3, 2017

Author Contributor

I will make it so

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

Thanks for the contribution - this is awesome stuff and I can definitely see this being useful to admins!

I'm a little uncomfortable with exposing these new details to non-admin users however - I don't think users should know for instance when other users last changed their passwords. I've reworked this in #4274 - if you merge my branch into this PR I will close mine - otherwise if someone does merge mine in the meantime this PR will automatically get marked as merged and your exact commits with your authorship will be included in Galaxy (it will be as if this was merged directly).

Thanks again!

@nsoranzo nsoranzo merged commit e320fd4 into galaxyproject:dev Jul 5, 2017

5 checks passed

api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.