Skip to content
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

implimented requestpasswordreset endpoint to receive either email o… #334

Merged
merged 7 commits into from Aug 10, 2018
Copy path View file
@@ -11,7 +11,7 @@
<description>OpenMRS module project for Rest Web Services</description>

<properties>
<openmrs.version.2.2.0-SNAPSHOT>2.2.0-SNAPSHOT</openmrs.version.2.2.0-SNAPSHOT>
<openmrs.version.2.2.0-SNAPSHOT>2.2.0-passwordreset</openmrs.version.2.2.0-SNAPSHOT>
</properties>

<dependencies>
@@ -0,0 +1,79 @@
/**
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.webservices.rest.web.v1_0.controller.openmrs2_2;

import java.util.Map;

import org.openmrs.User;
import org.openmrs.api.UserService;
import org.openmrs.module.webservices.rest.web.RestConstants;
import org.openmrs.module.webservices.rest.web.v1_0.controller.BaseRestController;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.ResponseBody;
import org.springframework.web.bind.annotation.ResponseStatus;

@Controller
@RequestMapping(value = "/rest/" + RestConstants.VERSION_1 + "/passwordreset")
public class PasswordResetController2_2 extends BaseRestController {

@Qualifier("userService")
@Autowired
private UserService userService;

@RequestMapping(method = RequestMethod.POST)
@ResponseStatus(HttpStatus.OK)
public void requestPasswordReset(@RequestBody Map<String, String> body) {
User user = null;
String usernameOrEmail = body.get("usernameOrEmail");
if ((user = userService.getUserByEmail(usernameOrEmail)) == null) {

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

This code looks vague, you assign user in the if clause, I think you don't have to do the assignment again inside the if clause

user = userService.getUserByUsername(usernameOrEmail);

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 3, 2018

Member

I think in openmrs-core, you will need to rename this method to getUserByUsernameOrEmail just for clarity of what it does

}
//user = userService.getUserByEmail(usernameOrEmail);

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Please don't leave behind commented out code, just remove it

if (user != null) {
userService.setUserActivationKey(user);
}
}

@RequestMapping(value = "/{activationkey}", method = RequestMethod.POST)
@ResponseBody
public ResponseEntity<String> resetPassword(@PathVariable("activationkey") String activationkey,

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

We don't return ResponseEntity, we ONLY return SimpleObject

@RequestBody Map<String, String> body) {
String newPassword = body.get("newPassword");
if (userService.getUserByActivationKey(activationkey) == null) {

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

This check is already done userService.changePasswordUsingActivationKey(activationkey) so don't do it here again, it's the reason why you added the new exception class, remember?

return new ResponseEntity<String>("Invalid Activation Key", HttpStatus.BAD_REQUEST);

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

You should instead throw an exception

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Possibly a NPE

}
else {
userService.changePasswordUsingActivationKey(activationkey, newPassword);
return new ResponseEntity<String>(HttpStatus.OK);
}
}

@RequestMapping(value = "/{activationkey}", method = RequestMethod.GET)
@ResponseBody
public ResponseEntity<String> verifyActivationKey(@PathVariable("activationkey") String activationkey) {

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Why did you add this method?

This comment has been minimized.

Copy link
@fanyui

fanyui Aug 7, 2018

Author Contributor

@wluyima The sequence diagram Burke presented shows wee need a method to verify activationKey So this method is for that

User user = userService.getUserByActivationKey(activationkey);
if (user == null) {
return new ResponseEntity<String>("Invalid Activation Key", HttpStatus.BAD_REQUEST);
}
else {
return new ResponseEntity<String>(HttpStatus.OK);
}

}

}
@@ -0,0 +1,97 @@
/**
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.webservices.rest.web.v1_0.controller.openmrs2_2;

import static org.junit.Assert.assertEquals;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.openmrs.Person;
import org.openmrs.PersonName;
import org.openmrs.User;
import org.openmrs.api.APIException;
import org.openmrs.api.UserService;
import org.openmrs.api.context.Context;
import org.openmrs.messagesource.MessageSourceService;
import org.openmrs.module.webservices.rest.web.v1_0.controller.RestControllerTestUtils;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.mock.web.MockHttpServletResponse;

public class PasswordResetController2_2Test extends RestControllerTestUtils {

private static final String RESET_PASSWORD_URI = "passwordreset";

private MessageSourceService messages;

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Why is this needed?


@Rule
public ExpectedException expectedException = ExpectedException.none();

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 9, 2018

Member

Where are you using this?


@Autowired
@Qualifier("userService")
private UserService userService;

@Before
public void setup() {
userService = Context.getUserService();

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 9, 2018

Member

This is redundant and a duplication because you are already autowiring it

messages = Context.getMessageSourceService();
}

@Test
public void requestPasswordReset_shouldFailIfUsernameOrEmailIsBlanck() throws Exception {

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Why do you need this test in the weblayer? The logic is in the service layer, you don't need to duplicate the test here

User u = new User();

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

Why don't you use an existing user?

This comment has been minimized.

Copy link
@fanyui

fanyui Aug 8, 2018

Author Contributor

@wluyima I have been checking and the last dataset that contained an existing user was defined since 1.8 so i am thinking declaring a new user here is appropriate.

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

I'm not sure where you are looking, your test class is extending RestControllerTestUtils which has other superclasses form core that load this standardTestDataset

u.setPerson(new Person());
u.addName(new PersonName("Benjamin", "A", "Wolfe"));
u.setUsername("bwolfe");
u.getPerson().setGender("M");
userService.createUser(u, "Openmr5xy");
String usernameOrEmail = " ";

expectedException.expect(APIException.class);
expectedException.expectMessage(messages.getMessage("error.email.notNullOrBlank"));
handle(newPostRequest(RESET_PASSWORD_URI, "{\"usernameOrEmail\":\"" + usernameOrEmail + "\"}"));

}

@Test
public void requestPasswordReset_shouldFailIfUsernameOrEmailIsEmptyl() throws Exception {

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

This test isn't necessary in the web layer too

User u = new User();

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 8, 2018

Member

Why don't you use an existing user?

u.setPerson(new Person());
u.addName(new PersonName("Benjamin", "A", "Wolfe"));
u.setUsername("bwolfe");
u.getPerson().setGender("M");
userService.createUser(u, "Openmr5xy");
String usernameOrEmail = "";

expectedException.expect(APIException.class);
expectedException.expectMessage(messages.getMessage("error.email.notNullOrBlank"));
handle(newPostRequest(RESET_PASSWORD_URI, "{\"usernameOrEmail\":\"" + usernameOrEmail + "\"}"));

}

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Where is the test for a valid password reset request?

This comment has been minimized.

Copy link
@fanyui

fanyui Aug 7, 2018

Author Contributor

@wluyima activation key expires so am having trouble writing a test for successfully verifying and reseting the password. please for some insight on how to do it

@Test
public void verifyActivationKey_shouldFailWith400BadRequestIfActivationKeyIsInvalid() throws Exception {

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Where are the tests that test the setting a user activation key and changing the password?

This test won't be needed, because you don't need an endpoint to verify the activation key

String activationKey = "wrongActivationKey12";
MockHttpServletResponse response = handle(newGetRequest(RESET_PASSWORD_URI + "/" + activationKey));
assertEquals(400, response.getStatus());
}

@Test
public void resetPassword_shouldFailWith400BadRequestIfActivationKeyIsInvalid() throws Exception {

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

This behavior is in the service layer, you don't need to duplicate the test here

String activationkey = "wrongActivationKey12";
String newPassword = "newPassword9";
MockHttpServletResponse response = handle(newPostRequest(RESET_PASSWORD_URI + "/" + activationkey,
"{\"newPassword\":\"" + newPassword + "\"}"));
assertEquals(400, response.getStatus());

}
}

This comment has been minimized.

Copy link
@wluyima

wluyima Aug 7, 2018

Member

Where is the valid test for a valid change password request?

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.