RESTWS-885: Reset password REST endpoint requires privileges #590
Conversation
| Context.removeProxyPrivilege(PrivilegeConstants.GET_USERS); | ||
| } | ||
|
|
||
| if (user == null || user.getUserId() == null ) { |
There was a problem hiding this comment.
If the previous implementation did not throw an exception if user is null, we shouldn't do so here either. For example, if you try to hit this, and get an exception, you know that username / email doesn't exist. That may not be information we want to expose.
Just simplify this method and do everything inside of the try/finally block. No need to declare the user outside of this block, and then use this user outside of this block. Something like:
try {
Context.addProxyPrivilege(PrivilegeConstants.GET_USERS);
User user = userService.getUserByUsernameOrEmail(usernameOrEmail);
if (user != null {
userService.setUserActivationKey(user);
}
}
finally {
Context.removeProxyPrivilege(PrivilegeConstants.GET_USERS);
}
| @@ -68,6 +68,16 @@ public void requestPasswordReset_shouldCreateUserActivationKeyGivenEmail() throw | |||
| handle(newPostRequest(RESET_PASSWORD_URI, "{\"usernameOrEmail\":\"" + user.getEmail() + "\"}")); | |||
| assertNotNull(dao.getLoginCredential(user).getActivationKey()); | |||
| } | |||
|
|
|||
| @Test(expected = MessageException.class) | |||
There was a problem hiding this comment.
Why is there a MessageException expected here? Are you sure this method will even get to the final assertion? If a MessageException is thrown from posting to the URL, then wrap this in a try/catch block, confirm that this is a message exception that is expected, and then move onto the other assertions.
| @Test | ||
| public void requestPasswordReset_shouldCreateUserActivationKeyGivenUsernameOrEmail() throws Exception { | ||
| User user = userService.getUserByUuid("c98a1558-e131-11de-babe-001e378eb67e"); | ||
| user.setEmail("[email protected]"); |
There was a problem hiding this comment.
The test method name says username or email. But the test body is dealing with only email.
| exception = me; | ||
| } | ||
| assertNotNull(exception); | ||
| assertNotNull(dao.getLoginCredential(user).getActivationKey()); |
There was a problem hiding this comment.
Can you assert that this was null before the REST call?
|
@IamMujuziMoses your test is not testing the changes in |
IamMujuziMoses
force-pushed
the
RESTWS-885
branch
from
00d9dd0
to
38c2b4e
Compare
|
@IamMujuziMoses is this still on your radar? |
|
@mherman22, you can pick it up and work on it |
@dkayiwa I have reverted the And when i add the changes in that class, the tests pass. |
mherman22
force-pushed
the
RESTWS-885
branch
from
62fe2f4
to
0069c6b
Compare
Issue I worked on
see https://issues.openmrs.org/browse/RESTWS-885
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master