-
Notifications
You must be signed in to change notification settings - Fork 689
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
3608 - Add admin checkbox clicks and test case for an admin to create… #3854
3608 - Add admin checkbox clicks and test case for an admin to create… #3854
Conversation
… a new admin user and the new admin user to be able to login and have admin privileges
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.
Hey, thanks for the PR. I have one comment that is mostly a nit about code structure than content.
@@ -352,6 +355,20 @@ def _new_user_can_log_in(self): | |||
with pytest.raises(NoSuchElementException): | |||
self.driver.find_element_by_id('link-admin-index') | |||
|
|||
def _new_admin_user_can_log_in(self): |
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.
This could be removed since it's a little too specific. I think just doing a logout
then login
would be sufficient.
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.
There's one additional check to ensure that admin link is available on login.
assert 'link-admin-index' in self.driver.page_source
Do you think its not necessary?
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.
I think a better solution would to to navigate to it. The text being present doesn't prove that the function works. We also already have a navigation helper for that, so your function could be strung together as 3 existing functions.
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.
I made it into a wrapper that calls the functions which does the login (with 'mocked' token) and clicks admin interface.
…into 3608-add-admin-user-functional-test
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.
This looks good to me. Thanks for the PR. 🎉
… a new admin user and the new admin user to be able to login and have admin privileges
Status
Ready for review
Description of Changes
Add support for clicking admin checkbox in functional tests.
Added a unit test to add a new admin user.
Testing
Running of functional tests if necessary
Deployment
Any special considerations for deployment? Consider both:
Its a test related change
Checklist
If you made changes to the server application code:
If you made changes to
securedrop-admin
:If you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation: