-
Notifications
You must be signed in to change notification settings - Fork 0
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
Make centralauth work, write tests #9
base: master
Are you sure you want to change the base?
Conversation
jade/centralauth.py
Outdated
|
||
class CentralAuth: | ||
|
||
class CentralAuth(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer necessary as of Python 3.0. https://stackoverflow.com/questions/15374857/should-all-python-classes-extend-object
@@ -45,17 +47,18 @@ def check_user_rights(self, gu_id, context, requirements): | |||
gui_doc = self.get_globaluser_info(gu_id) | |||
|
|||
if 'locked' in gui_doc: | |||
raise errors.UserLockedError( | |||
"The user account with gu_id={0} is locked".format(gu_id)) | |||
raise errors.UserLockedError(gu_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Much better
# def test_get_globaluser_info_invalid_user_id(): | ||
# ca = CentralAuth.from_config(test_config) | ||
# with pytest.raises(mwapi.errors.APIError): | ||
# ca.get_globaluser_info(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out, requesting non-existent users actually causes an APIError in mwapi, rather than the "missing" sad path. We probably need to handle this, and calling functions will be expecting differents types of jade.errors.UserPermissionError. We should consider wrapping the APIError... Also, the TODO is because I hadn't decided which code we should be checking. Now I think we should be mocking mwapi.Session.get to just throw an error, but first let's decide how the CentralAuth class will handle it.
A couple of comments, but otherwise, this looks great. Thanks for your work :) |
No description provided.