-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
------- | ||
|
||
To run the webserver locally, first run the following commands to insert a user with administrative access:: | ||
|
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.
Can you also add the SQL commands to select the right database.
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.
Done!
**User:** master | ||
|
||
**Password:** password | ||
|
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.
Maybe add a note about the test behaviour, i.e. the access of the "test" user is re-set after each unit 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.
Done!
|
||
**Password:** password | ||
|
||
Note that whenever a unittest is run, the user `test` is granted admin rights, which is reset once the unittest is completed. |
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 believe this is only the case if mock_admin_login
is used
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.
Right
How is the change on the README.md changing the coverage to +6.1%? |
It was a very well tested readme :D On Tue, Sep 20, 2016 at 4:12 PM, Jose Navas notifications@github.com
|
All comments have been addressed. Anything else left here? |
|
||
**Password:** password | ||
|
||
Note that whenever a unittest is run, the user `test` is granted admin rights, which is reset once the unittest is completed if `mock_admin_login` is called. |
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 is still inaccurate. The user test
is not implicitly granted admin rights
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.
Can you clarify exactly how this should be corrected?
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.
Delete it? The statement is inaccurate
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.
To me, Jamie's description seems to be accurate, since tearDown() alters the 'test' user access level:
def tearDown(self): BaseHandler.get_current_user = self.orig_func # Remove all access privileges user may hve been given by a test db.alter_access_levels('test', []) super(TestHandlerBase, self).tearDown()
Which means that a manual change of the access rights for this user are overwritten by any unit 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.
oh. I see. The first part is not precise enough. 'test' only get admin rights if mock_login_admin
or its manual equivalent db.alter_access_levels('test', [7])
is used, but not per default for every test in general.
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.
Let us rephrase the sentence like that:
"After executing our existing unit test suite, access level for the user 'test' will be reset to '', i.e. he won't be able to see most of the main menu items. Thus, adding a second user 'master' with admin privileges is quite useful."
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.
Done. Thanks @sjanssen2 ! @wasade do you have any thoughts on this current phrasing?
@josenavas @wasade would be cool to come to a decision rather we want to merge or not. |
No description provided.