O3-2761: Add ability to save user email #188
Conversation
omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java
Outdated
Show resolved
Hide resolved
omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java
Outdated
Show resolved
Hide resolved
| @@ -90,6 +90,12 @@ | |||
| <spring:nestedPath path="user.person.names[0]"> | |||
| <openmrs:portlet url="nameLayout" id="namePortlet" size="full" parameters="layoutMode=edit|layoutShowTable=false|layoutShowExtended=false" /> | |||
| </spring:nestedPath> | |||
| <c:if test="${hasEmailField}"> | |||
| <tr> | |||
| <td><openmrs:message code="Email" /><span class="required">*</span></td> | |||
There was a problem hiding this comment.
It is literally not required. However, since reseting password actually depends on it, i thought it would be a good practice if we emphasise users to always fill it
There was a problem hiding this comment.
Required means we cannot save the user without it.
There was a problem hiding this comment.
reseting password
I'm not sure I necessarily follow this. We don't (in the general case) have support for sending users email for password resets, though it's possible for a module to add this capability.
There was a problem hiding this comment.
@ibacher I am simply trying to make sure that rest/v1/passwordreset can get working so that we can use it in the frontend
This PR might be followed by many more until this works
There was a problem hiding this comment.
We don't (in the general case) have support for sending users email for password resets
I can see why you'd think this @ibacher , but in fact OpenMRS core does natively support sending emails for the "forgot password" feature, via the setUserActivationKey method: https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/api/impl/UserServiceImpl.java#L760
| @@ -112,6 +115,15 @@ public List<Role> getRoles(WebRequest request) { | |||
| public String showForm(@RequestParam(required = false, value = "userId") Integer userId, | |||
| @RequestParam(required = false, value = "createNewPerson") String createNewPerson, | |||
| @ModelAttribute("user") User user, ModelMap model) { | |||
|
|
|||
| try { | |||
| Field emailField = getEmailField(); | |||
There was a problem hiding this comment.
Why don't you get the email field only for the platform versions that support it?
There was a problem hiding this comment.
i was trying to avoid duplicating the code User.class.getDeclaredField("email") when; 1) determining if the platform version actually supports this field, and, 2) when i actually need access the email filed to set it
I have another way though. It might not be ideal though using OPENMRS_VERSION_SHORT
private boolean isPlatformTwoPointTwoOrAbove() {
String platformVersion = OpenmrsConstants.OPENMRS_VERSION_SHORT.substring(0, 3);
try {
float versionFloat = Float.parseFloat(platformVersion);
if (versionFloat>=2.2) {
return true;
}
} catch (NumberFormatException e) {
log.error("Unable to parse platform value text to float", e);
}
return false;
}
| try { | ||
| boolean isPlatformTwoPointTwoOrAbove = isPlatform22OrNewer(); | ||
| model.addAttribute("hasEmailField", isPlatformTwoPointTwoOrAbove); | ||
| if (isPlatformTwoPointTwoOrAbove) { |
There was a problem hiding this comment.
Why not just set the hasEmailField within this if statement?
| @@ -112,6 +115,17 @@ public List<Role> getRoles(WebRequest request) { | |||
| public String showForm(@RequestParam(required = false, value = "userId") Integer userId, | |||
| @RequestParam(required = false, value = "createNewPerson") String createNewPerson, | |||
| @ModelAttribute("user") User user, ModelMap model) { | |||
|
|
|||
| try { | |||
| boolean isPlatform22OrNewer = isPlatform22OrNewer(); | |||
There was a problem hiding this comment.
Do we need the above variable? Why not just directly call the method in the if statement?
| try { | ||
| emailField = getEmailField(); | ||
| emailField.set(user, email); | ||
| } catch (IllegalArgumentException | IllegalAccessException | NullPointerException e) { |
There was a problem hiding this comment.
Aren't these exceptions thrown and caught within getEmailField?
| @@ -227,6 +242,19 @@ public String handleSubmission(WebRequest request, HttpSession httpSession, Mode | |||
| } | |||
| } | |||
|
|
|||
| //check validity of email then save it | |||
There was a problem hiding this comment.
I do not see any value added by the above comment.
| * @return an email field | ||
| * @param user | ||
| */ | ||
| private Field getEmailField() { |
There was a problem hiding this comment.
Shouldn't this method return the email field value instead of the field object?
There was a problem hiding this comment.
i would find it hard to reuse this method in handleSubmission if it was returning the value
This has been done using java reflection API
Issue: https://openmrs.atlassian.net/browse/O3-2761
cc @dkayiwa @ibacher @michaelbontyes