From df9ce07d9ab8a67ad2a4f8c8cef1898426116c19 Mon Sep 17 00:00:00 2001 From: JOSHUA Date: Tue, 23 Jan 2024 10:29:03 +0300 Subject: [PATCH 1/6] O3-2761: Add ability to save user email --- .../controller/user/UserFormController.java | 47 +++++++++++++++++++ omod/src/main/webapp/admin/users/userForm.jsp | 4 ++ .../src/main/webapp/resources/css/openmrs.css | 2 +- .../user/UserFormControllerTest.java | 6 +-- 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java index 92caf0dc..8acf7709 100644 --- a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java +++ b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java @@ -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; @@ -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; @@ -112,6 +115,13 @@ public List 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("email", user); + model.addAttribute("email", emailField.get(user)); + } catch (IllegalArgumentException | IllegalAccessException | NullPointerException e) { + log.warn("Email field not available for setting", e); + } // the formBackingObject method above sets up user, depending on userId and personId parameters @@ -157,6 +167,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(); @@ -227,6 +238,19 @@ public String handleSubmission(WebRequest request, HttpSession httpSession, Mode } } + //check validity of email then save it + if (isEmailValid(email)) { + Field emailField; + try { + emailField = getEmailField("email", user); + emailField.set(user, email); + } catch (IllegalArgumentException | IllegalAccessException | NullPointerException e) { + log.error("Error while setting the email field", e); + } + } else { + log.warn("Invalid or unspecified email: " + (email != null ? "'" + email + "'" : "not provided")); + } + Set newRoles = new HashSet(); if (roles != null) { for (String r : roles) { @@ -321,4 +345,27 @@ 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 emailFieldName + * @param user + */ + private Field getEmailField(String emailFieldName, User user) { + try { + Field emailField = user.getClass().getDeclaredField(emailFieldName); + emailField.setAccessible(true); + return emailField; + } catch (NoSuchFieldException | SecurityException e) { + log.warn("Email field not available for setting", e); + return null; + } + } } diff --git a/omod/src/main/webapp/admin/users/userForm.jsp b/omod/src/main/webapp/admin/users/userForm.jsp index cbdbf748..ebd33af2 100644 --- a/omod/src/main/webapp/admin/users/userForm.jsp +++ b/omod/src/main/webapp/admin/users/userForm.jsp @@ -90,6 +90,10 @@ + + * + + * diff --git a/omod/src/main/webapp/resources/css/openmrs.css b/omod/src/main/webapp/resources/css/openmrs.css index 5e9846c0..cf1a231a 100644 --- a/omod/src/main/webapp/resources/css/openmrs.css +++ b/omod/src/main/webapp/resources/css/openmrs.css @@ -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; } diff --git a/omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java b/omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java index 8f79e6d1..b70becc4 100644 --- a/omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java +++ b/omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java @@ -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, "", user, new BindException(user, "user"), new MockHttpServletResponse()); } @@ -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", "", user, new BindException(user, "user"), response); Assert.assertFalse(Context.getProviderService().getProvidersByPerson(user.getPerson()).isEmpty()); Assert.assertEquals(200, response.getStatus()); } @@ -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", "", user, new BindException(user, "user"), response); Assert.assertEquals(400, response.getStatus()); } From 8626164a9e3d4e517e43df513fd6707815d95164 Mon Sep 17 00:00:00 2001 From: JOSHUA Date: Wed, 31 Jan 2024 17:22:20 +0300 Subject: [PATCH 2/6] should not show email field for platform versions that do not support it --- .../web/controller/user/UserFormController.java | 11 ++++++----- omod/src/main/webapp/admin/users/userForm.jsp | 10 ++++++---- .../web/controller/user/UserFormControllerTest.java | 6 +++--- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java index 8acf7709..f800e487 100644 --- a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java +++ b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java @@ -117,10 +117,12 @@ public String showForm(@RequestParam(required = false, value = "userId") Integer @ModelAttribute("user") User user, ModelMap model) { try { - Field emailField = getEmailField("email", user); + Field emailField = getEmailField(); 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 @@ -242,7 +244,7 @@ public String handleSubmission(WebRequest request, HttpSession httpSession, Mode if (isEmailValid(email)) { Field emailField; try { - emailField = getEmailField("email", user); + emailField = getEmailField(); emailField.set(user, email); } catch (IllegalArgumentException | IllegalAccessException | NullPointerException e) { log.error("Error while setting the email field", e); @@ -355,12 +357,11 @@ private boolean isEmailValid(String email) { /** * @return an email field - * @param emailFieldName * @param user */ - private Field getEmailField(String emailFieldName, User user) { + private Field getEmailField() { try { - Field emailField = user.getClass().getDeclaredField(emailFieldName); + Field emailField = User.class.getDeclaredField("email"); emailField.setAccessible(true); return emailField; } catch (NoSuchFieldException | SecurityException e) { diff --git a/omod/src/main/webapp/admin/users/userForm.jsp b/omod/src/main/webapp/admin/users/userForm.jsp index ebd33af2..1582ee31 100644 --- a/omod/src/main/webapp/admin/users/userForm.jsp +++ b/omod/src/main/webapp/admin/users/userForm.jsp @@ -90,10 +90,12 @@ - - * - - + + + * + + + * diff --git a/omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java b/omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java index b70becc4..cfb3e1d3 100644 --- a/omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java +++ b/omod/src/test/java/org/openmrs/web/controller/user/UserFormControllerTest.java @@ -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, "sample@email.com", user, new BindException(user, "user"), new MockHttpServletResponse()); } @@ -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", "sample@email.com", user, new BindException(user, "user"), response); Assert.assertFalse(Context.getProviderService().getProvidersByPerson(user.getPerson()).isEmpty()); Assert.assertEquals(200, response.getStatus()); } @@ -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", "sample@email.com", user, new BindException(user, "user"), response); Assert.assertEquals(400, response.getStatus()); } From c01867b96be3865439825ff8a0c90abfe797c66e Mon Sep 17 00:00:00 2001 From: JOSHUA Date: Thu, 1 Feb 2024 00:20:58 +0300 Subject: [PATCH 3/6] check platform version before setting email field --- .../controller/user/UserFormController.java | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java index f800e487..b32ec292 100644 --- a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java +++ b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java @@ -117,12 +117,14 @@ public String showForm(@RequestParam(required = false, value = "userId") Integer @ModelAttribute("user") User user, ModelMap model) { try { - Field emailField = getEmailField(); - model.addAttribute("email", emailField.get(user)); - model.addAttribute("hasEmailField", true); + boolean isPlatformTwoPointTwoOrAbove = isPlatform22OrNewer(); + model.addAttribute("hasEmailField", isPlatformTwoPointTwoOrAbove); + if (isPlatformTwoPointTwoOrAbove) { + Field emailField = getEmailField(); + model.addAttribute("email", emailField.get(user)); + } } 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 @@ -241,7 +243,7 @@ public String handleSubmission(WebRequest request, HttpSession httpSession, Mode } //check validity of email then save it - if (isEmailValid(email)) { + if (isPlatform22OrNewer() && isEmailValid(email)) { Field emailField; try { emailField = getEmailField(); @@ -369,4 +371,21 @@ private Field getEmailField() { return null; } } + + /** + * @return true if the platform version is 2.2 or higher + */ + private boolean isPlatform22OrNewer() { + 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; + } } From 873d3814c1d5e97145134756be15a44f667df621 Mon Sep 17 00:00:00 2001 From: JOSHUA Date: Thu, 1 Feb 2024 00:24:52 +0300 Subject: [PATCH 4/6] email field should noe be required --- omod/src/main/webapp/admin/users/userForm.jsp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/omod/src/main/webapp/admin/users/userForm.jsp b/omod/src/main/webapp/admin/users/userForm.jsp index 1582ee31..c01706b5 100644 --- a/omod/src/main/webapp/admin/users/userForm.jsp +++ b/omod/src/main/webapp/admin/users/userForm.jsp @@ -92,7 +92,7 @@ - * + From ebbe6bbcdc86e3246f3845df907f2166004d4536 Mon Sep 17 00:00:00 2001 From: JOSHUA Date: Thu, 1 Feb 2024 01:19:29 +0300 Subject: [PATCH 5/6] email field should noe be required --- .../org/openmrs/web/controller/user/UserFormController.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java index b32ec292..b2435a15 100644 --- a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java +++ b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java @@ -117,10 +117,10 @@ public String showForm(@RequestParam(required = false, value = "userId") Integer @ModelAttribute("user") User user, ModelMap model) { try { - boolean isPlatformTwoPointTwoOrAbove = isPlatform22OrNewer(); - model.addAttribute("hasEmailField", isPlatformTwoPointTwoOrAbove); - if (isPlatformTwoPointTwoOrAbove) { + boolean isPlatform22OrNewer = isPlatform22OrNewer(); + if (isPlatform22OrNewer) { Field emailField = getEmailField(); + model.addAttribute("hasEmailField", isPlatform22OrNewer); model.addAttribute("email", emailField.get(user)); } } catch (IllegalArgumentException | IllegalAccessException | NullPointerException e) { From 418c8675d3ca7441d1fa9cec58319f07a9d110f5 Mon Sep 17 00:00:00 2001 From: JOSHUA Date: Thu, 1 Feb 2024 01:45:28 +0300 Subject: [PATCH 6/6] remove unnecessary comments --- .../java/org/openmrs/web/controller/user/UserFormController.java | 1 - 1 file changed, 1 deletion(-) diff --git a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java index b2435a15..9eee003f 100644 --- a/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java +++ b/omod/src/main/java/org/openmrs/web/controller/user/UserFormController.java @@ -242,7 +242,6 @@ public String handleSubmission(WebRequest request, HttpSession httpSession, Mode } } - //check validity of email then save it if (isPlatform22OrNewer() && isEmailValid(email)) { Field emailField; try {