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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

O3-2761: Add ability to save user email #188

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
df9ce07
O3-2761: Add ability to save user email
jnsereko Jan 23, 2024
8626164
should not show email field for platform versions that do not support it
jnsereko Jan 31, 2024
c01867b
check platform version before setting email field
jnsereko Jan 31, 2024
873d381
email field should noe be required
jnsereko Jan 31, 2024
ebbe6bb
email field should noe be required
jnsereko Jan 31, 2024
418c867
remove unnecessary comments
jnsereko Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.commons.validator.EmailValidator;
import org.openmrs.Person;
import org.openmrs.PersonName;
import org.openmrs.Provider;
Expand Down Expand Up @@ -43,6 +44,8 @@

import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;

import java.lang.reflect.Field;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -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();
Copy link
Member

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?

Copy link
Contributor Author

@jnsereko jnsereko Jan 31, 2024 •
edited
Loading

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;
}

model.addAttribute("email", emailField.get(user));
model.addAttribute("hasEmailField", true);
} catch (IllegalArgumentException | IllegalAccessException | NullPointerException e) {
log.warn("Email field not available for setting", e);
model.addAttribute("hasEmailField", false);
}

// the formBackingObject method above sets up user, depending on userId and personId parameters

Expand Down Expand Up @@ -157,6 +169,7 @@ public String handleSubmission(WebRequest request, HttpSession httpSession, Mode
@RequestParam(required = false, value = "roleStrings") String[] roles,
@RequestParam(required = false, value = "createNewPerson") String createNewPerson,
@RequestParam(required = false, value = "providerCheckBox") String addToProviderTableOption,
@RequestParam(required = false, value = "email") String email,
@ModelAttribute("user") User user, BindingResult errors, HttpServletResponse response) {

UserService us = Context.getUserService();
Expand Down Expand Up @@ -227,6 +240,19 @@ public String handleSubmission(WebRequest request, HttpSession httpSession, Mode
}
}

//check validity of email then save it
Copy link
Member

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.

if (isEmailValid(email)) {
Field emailField;
try {
emailField = getEmailField();
emailField.set(user, email);
} catch (IllegalArgumentException | IllegalAccessException | NullPointerException e) {
Copy link
Member

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?

log.error("Error while setting the email field", e);
}
} else {
log.warn("Invalid or unspecified email: " + (email != null ? "'" + email + "'" : "not provided"));
}

Set<Role> newRoles = new HashSet<Role>();
if (roles != null) {
for (String r : roles) {
Expand Down Expand Up @@ -321,4 +347,26 @@ private Boolean isNewUser(User user) {
return user == null ? true : user.getUserId() == null;
}

/**
* @return true if email is valid or false otherwise
* @param email
*/
private boolean isEmailValid(String email) {
return StringUtils.hasLength(email) && EmailValidator.getInstance().isValid(email);
}

/**
* @return an email field
* @param user
*/
private Field getEmailField() {
Copy link
Member

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?

Copy link
Contributor Author

@jnsereko jnsereko Jan 31, 2024 •
edited
Loading

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh i see!

try {
Field emailField = User.class.getDeclaredField("email");
emailField.setAccessible(true);
return emailField;
} catch (NoSuchFieldException | SecurityException e) {
log.warn("Email field not available for setting", e);
return null;
}
}
}
6 changes: 6 additions & 0 deletions omod/src/main/webapp/admin/users/userForm.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

@ibacher ibacher Jan 31, 2024 •
edited
Loading

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.

Copy link
Contributor Author

@jnsereko jnsereko Jan 31, 2024 •
edited
Loading

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

Copy link
Member

@mseaton mseaton Jan 31, 2024 •
edited
Loading

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

<td><input type="email" size="30" name="email" value="${email}"/></td>
</tr>
</c:if>
<tr>
<td><openmrs:message code="Person.gender"/><span class="required">*</span></td>
<td><spring:bind path="user.person.gender">
Expand Down
2 changes: 1 addition & 1 deletion omod/src/main/webapp/resources/css/openmrs.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,7 @@ tr.searchHighlight {
overflow-y: auto;
}

input[type="text"], input[type="password"] {
input[type="text"], input[type="email"], input[type="password"] {
padding: 1px;
border: 1px solid cadetblue;
}
Expand Down
6 changes: 3 additions & 3 deletions omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public void handleSubmission_shouldWorkForAnExample() throws Exception {
user.addName(new PersonName("This", "is", "Test"));
user.getPerson().setGender("F");
controller.handleSubmission(request, new MockHttpSession(), new ModelMap(), "", "Save User", "pass123", "pass123",
null, null, null, new String[0], "true", null, user, new BindException(user, "user"),
null, null, null, new String[0], "true", null, "[email protected]", user, new BindException(user, "user"),
new MockHttpServletResponse());
}

Expand All @@ -71,7 +71,7 @@ public void handleSubmission_createUserProviderAccountWhenProviderAccountCheckbo
controller.showForm(2, "true", user, model);
controller.handleSubmission(request, new MockHttpSession(), new ModelMap(), "", null, "Test1234",
"valid secret question", "valid secret answer", "Test1234", false, new String[] { "Provider" }, "true",
"addToProviderTable", user, new BindException(user, "user"), response);
"addToProviderTable", "[email protected]", user, new BindException(user, "user"), response);
Assert.assertFalse(Context.getProviderService().getProvidersByPerson(user.getPerson()).isEmpty());
Assert.assertEquals(200, response.getStatus());
}
Expand All @@ -85,7 +85,7 @@ public void shouldSetResponseStatusToBadRequestOnError() throws Exception {
MockHttpServletResponse response = new MockHttpServletResponse();
controller.handleSubmission(request, new MockHttpSession(), new ModelMap(), "", null, "Test123",
"valid secret question", "valid secret answer", "Test1234", false, new String[] { "Provider" }, "true",
"addToProviderTable", user, new BindException(user, "user"), response);
"addToProviderTable", "[email protected]", user, new BindException(user, "user"), response);
Assert.assertEquals(400, response.getStatus());
}

Expand Down
Loading