-
Notifications
You must be signed in to change notification settings - Fork 174
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
O3-2761: Add ability to save user email #188
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the email field required?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required means we cannot save the user without it.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you get the email field only for the platform versions that support it?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this method return the email field value instead of the field object?
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 would find it hard to reuse this method in handleSubmission
if it was returning the value
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!
This has been done using java reflection API
Issue: https://openmrs.atlassian.net/browse/O3-2761
cc @dkayiwa @ibacher @michaelbontyes