Fix #205: Add functionality to reset authentication token and logout #233
Conversation
There was a problem hiding this comment.
@nikochiko
With respect to changes proposed in this PR:
4, Add functionality in set_token command to clear existing token with:
evalai set_token clear_token
The use of evalai set_token just to set the token. To put it in other words, responsibility set_token is just to set the token. You are delegating it one more task to clear token using flag tokens. How about clear token only using logout command? I am trying to use Single Responsibility Principal to explain this.
|
@vkartik97 Got it. Yes now that I think about it, it makes a lot of sense. I will make that change. |
|
@vkartik97 Any comments on the rest of the task? |
tests/test_auth_utils.py
Outdated
| self.assertEqual(value, expected) | ||
|
|
||
| @mock.patch("evalai.utils.auth.os.remove") | ||
| def test_reset_user_auth_token_when_writing_to_file_fails(self, mock_remove): |
There was a problem hiding this comment.
@nikochiko What does this test signify?
There was a problem hiding this comment.
@pushkalkatara this is the test for the case when file deleting fails. And oops I think I named it incorrectly 😕 . How about changing the name to test_reset_user_auth_token_when_token_file_removal_fails ?
test_reset_user_auth_token_when_writing_to_file_fails -> test_reset_user_auth_token_when_token_file_removal_fails
Address #205
Objective:
To allow user to easily log out (i.e. reset existing authentication token) from the command line.
Changes proposed:
reset_user_auth_tokenfunction in auth utilsreset_user_auth_token4, Add functionality inset_tokencommand to clear existing token with:Edit: Removed 4: functionality to clear existing token from set_token command.
@RishabhJain2018 @vkartik97 @Ram81 Please review.