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

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

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

Conversation

jnsereko
Copy link
Contributor

@jnsereko jnsereko commented Jan 23, 2024

This has been done using java reflection API

Issue: https://openmrs.atlassian.net/browse/O3-2761

cc @dkayiwa @ibacher @michaelbontyes

@jnsereko jnsereko requested a review from dkayiwa January 31, 2024 17:35
@@ -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

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

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

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();
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

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) {
Copy link
Member

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?

@jnsereko jnsereko requested a review from dkayiwa January 31, 2024 22:20
@@ -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();
Copy link
Member

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) {
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?

@@ -227,6 +242,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.

* @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

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!

@jnsereko jnsereko requested a review from dkayiwa January 31, 2024 23:34
@jnsereko
Copy link
Contributor Author

jnsereko commented Feb 5, 2024

hey @dkayiwa @ibacher anything else left here?

@dkayiwa
Copy link
Member

dkayiwa commented Feb 5, 2024

hey @dkayiwa @ibacher anything else left here?

Did you respond to my review comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants