Trunk 6124 Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing) #4533

subhamkumarr · 2024-01-17T05:18:08Z

Description of what I changed

Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing)

Issue I worked on

https://openmrs.atlassian.net/browse/TRUNK-6124

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.
All new and existing tests passed.
My pull request is based on the latest changes of the master branch.

Eliminate unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing)

tendomart · 2024-01-17T05:58:37Z

@subhamkumarr change the title to reflect what's being done in the ticket

subhamkumarr · 2024-01-17T06:07:34Z

@tendomart added

subhamkumarr · 2024-01-17T06:11:49Z

@tendomart should i also add test to the code?

import org.junit.Test;
import static org.junit.Assert.*;

public class ResultTest {

@Test
public void testToDatetimeWithNonNullValueDatetime() {
    // Create a Result with a non-null valueDatetime
    Result result = new Result(new Date(), Result.Datatype.DATETIME, null, null, new Date(), null, null, null);

    // Call toDatetime and assert the expected result
    assertEquals(result.getValueDatetime(), result.toDatetime());
}

@Test
public void testToDatetimeWithTextValue() {
    // Create a Result with datatype TEXT and a non-null valueText that can be parsed into a date
    String dateString = "2022-01-01";
    Result result = new Result(new Date(), Result.Datatype.TEXT, null, null, null, null, dateString, null);

    // Call toDatetime and assert the expected result
    assertEquals(DateUtils.parseDate(dateString), result.toDatetime());
}

@Test
public void testToDatetimeWithTextValueParsingError() {
    // Create a Result with datatype TEXT and a valueText that cannot be parsed into a date
    Result result = new Result(new Date(), Result.Datatype.TEXT, null, null, null, null, "invalidDate", null);

    // Call toDatetime and assert that it returns null
    assertNull(result.toDatetime());
}

@Test
public void testToDatetimeWithMultipleResults() {
    // Create a Result with multiple sub-results, each having a different valueDatetime
    Result subResult1 = new Result(new Date(), Result.Datatype.DATETIME, null, null, new Date(), null, null, null);
    Result subResult2 = new Result(new Date(), Result.Datatype.DATETIME, null, null, DateUtils.addDays(new Date(), 1), null, null, null);
    Result result = new Result(Arrays.asList(subResult1, subResult2));

    // Call toDatetime and assert that it returns the earliest valueDatetime among sub-results
    assertEquals(subResult1.toDatetime(), result.toDatetime());
}

}

tendomart · 2024-01-17T06:19:25Z

Yes please, just to make sure nothing is breaking.

subhamkumarr · 2024-01-17T09:22:41Z

@tendomart Sir everything is good from my end. Please merge it.

The allergy field in the Allergy UI allows the user to enter a numeric value, considering allergies cannot have numeric name, we must validate the input string before allowing the user to procced further

tendomart · 2024-01-17T15:05:29Z

api/src/main/java/org/openmrs/logic/result/Result.java

@@ -21,6  21,8 @@
 import org.openmrs.Obs;
 import org.openmrs.api.context.Context;
 import org.openmrs.logic.LogicException;
+import org.junit.Test;
+import static org.junit.Assert.*;


Please remove the wild card

@tendomart Wild card? i did not get you

tendomart · 2024-01-18T06:01:32Z

yes @subhamkumarr line 25

import static org.junit.Assert.*;

avoid imports like which end with .*, always import a specific class that you're using, avoid importing an entire package

look at this https://wiki.openmrs.org/display/docs/Java Conventions

in short , use onlly absolute paths

subhamkumarr · 2024-01-18T14:04:45Z

@tendomart done the changes. Could you please review and merge.

tendomart · 2024-01-18T20:17:54Z

api/src/test/java/org/openmrs/validator/AllergyValidatorTest.java

@@ -191,4  191,12 @@ public void validate_shouldRejectNumericReactionValue() {
 		validator.validate(allergy, errors);
 		assertTrue(errors.hasErrors());
 	}
+	@Test
+	public void validate_shouldRejectNumericAllergenValue() {
+		Allergen allergen = new Allergen(AllergenType.DRUG, null , "45" );


remove the space after null and "45"

tendomart · 2024-01-18T20:24:00Z

api/src/main/java/org/openmrs/validator/AllergyValidator.java

@@ -64,6  64,11 @@ public void validate(Object target, Errors errors) {

 		Allergy allergy = (Allergy) target;

+		    // Additional validation: Ensure allergen does not contain numeric values
+			if (allergy.getAllergen() != null && StringUtils.isNumeric(allergy.getAllergen().getNonCodedAllergen())) {


avoid using null instead do something llike
if (StringUtils.isNotEmpty(allergy.getAllergen()) && StringUtils.isNumeric(allergy.getAllergen().getNonCodedAllergen()))

if (StringUtils.isNumeric(Objects.requireNonNull(allergy.getAllergen(), "Allergen must not be null").getNonCodedAllergen()))
Is this is ok?

tendomart · 2024-01-18T20:24:22Z

api/src/main/java/org/openmrs/validator/AllergyValidator.java

+			if (allergy.getAllergen() != null && StringUtils.isNumeric(allergy.getAllergen().getNonCodedAllergen())) {
+				errors.rejectValue("allergen", "error.allergyapi.allergy.Allergen.cannotContainNumeric");
+			}
+
 		if (allergy.getReactionNonCoded() != null) {


Do the same here

if (Objects.nonNull(allergy.getReactionNonCoded())) is this is OK?

try to do as described above or follow the the OpenMRS Java Conventions

tendomart · 2024-01-18T20:25:21Z

api/src/test/java/org/openmrs/validator/AllergyValidatorTest.java

+	public void validate_shouldRejectNumericAllergenValue() {
+		Allergen allergen = new Allergen(AllergenType.DRUG, null , "45" );
+		allergy.setAllergen(allergen);
+		Errors errors = new BindException(allergy,"allergy");


Add space between allergy and "allergy like
Errors errors = new BindException(allergy, "allergy");

tendomart · 2024-01-18T20:25:50Z

api/src/main/java/org/openmrs/logic/result/Result.java

@@ -543,9  543,13 @@ public Date toDatetime() {
 				}
 				catch (Exception e) {}
 			}
-			return valueDatetime;
+			else {
+


Do you need this extra space ?

tendomart · 2024-01-18T20:35:57Z

api/src/main/resources/messages.properties

@@ -94,6  94,7 @@ error.email.alreadyInUse=Email address is already assigned to another user accou
 error.activationkey.invalid =Invalid user activation Key


You can choose to remove the spaces before and after the Equal signs(=)

tendomart · 2024-01-19T10:00:00Z

@subhamkumarr have you responded to all the changes which I requested above ?

subhamkumarr · 2024-01-19T10:32:14Z

@tendomart I made all the changes you asked me to do. Please review.

tendomart · 2024-01-19T10:32:59Z

api/src/main/resources/messages.properties

@@ -91,7  91,7 @@ Context.DAO.only=This method can only be called from the ContextDAO class, not {
 error.rank.notPositiveInteger=Rank can only be a positive integer
 error.email.invalid =Invalid Email address
 error.email.alreadyInUse=Email address is already assigned to another user account
-error.activationkey.invalid =Invalid user activation Key
+error.activationkey.invalid=Invalid user activation Key
 error.usernameOrEmail.notNullOrBlank =Username Or email cannot be null or blank


remove extra space here

tendomart · 2024-01-19T10:34:52Z

api/src/test/java/org/openmrs/validator/AllergyValidatorTest.java

+	public void validate_shouldRejectNumericAllergenValue() {
+		Allergen allergen = new Allergen(AllergenType.DRUG, null, "45");
+		allergy.setAllergen(allergen);
+		Errors errors = new BindException (allergy,"allergy");


here too...extra spaces

@tendomart Done Please check

subhamkumarr · 2024-01-19T10:53:52Z

@tendomart is this good for merge?

tendomart · 2024-01-19T10:52:33Z

api/src/test/java/org/openmrs/validator/AllergyValidatorTest.java

@@ -191,4  191,12 @@ public void validate_shouldRejectNumericReactionValue() {
 		validator.validate(allergy, errors);
 		assertTrue(errors.hasErrors());
 	}
+	@Test
+	public void validate_shouldRejectNumericAllergenValue() {
+		Allergen allergen=new Allergen(AllergenType.DRUG, null, "45");


put spaces around the equal signs

tendomart · 2024-01-19T10:53:27Z

api/src/test/java/org/openmrs/validator/AllergyValidatorTest.java

+	public void validate_shouldRejectNumericAllergenValue() {
+		Allergen allergen=new Allergen(AllergenType.DRUG, null, "45");
+		allergy.setAllergen(allergen);
+		Errors errors=new BindException (allergy, "allergy");


this should be like this
Errors errors = new BindException(allergy, "allergy");

@tendomart Done

subhamkumarr · 2024-01-19T11:27:55Z

@dkayiwa could you please review my PR.

tendomart · 2024-01-19T12:23:38Z

You have some failing tests..

do you need help in that ?


 Error:  Errors: 
Error:    AllergyValidatorTest.validate_shouldFailIfAllergenIsNull:82 » NullPointer Allergen must not be null
Error:    AllergyValidatorTest.validate_shouldFailIfPatientIsNull:73 » NullPointer Allergen must not be null
Error:    AllergyValidatorTest.validate_shouldRejectNumericReactionValue:191 » NullPointer Allergen must not be null

subhamkumarr · 2024-01-19T12:28:06Z

@tendomart yes i need help

subhamkumarr · 2024-01-19T14:21:32Z

@tendomart what changes i need to make?

github-actions · 2024-08-04T00:34:51Z

tl;dr our action detected no activity on this PR and will close it in 30 days if the stale label is not removed.

OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare!

This PR has not seen any activity in the last 5 months. That is why we wanted to check whether you are still working on it or need assistance from our side.
Please note that this is an automated message and we might very well be the reason why there has not been any activity lately. We certainly do not want to discourage you from contributing. We do need to be honest in that OpenMRS has limited resources for reviewing PRs.

If you do not have time to continue the work or have moved on you don’t need to do anything. We will automatically close the PR in 30 days. We hope to see you back soon :)
If you would like to continue working on it or require help from us please remove the stale label and respond by commenting on the issue.

subhamkumarr added 2 commits January 17, 2024 10:16

TRUNK-6124

9e36062

Eliminate unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing)

TRUNK-6124

b5c5079

Eliminate unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing)

subhamkumarr changed the title ~~Trunk 6124~~ Trunk 6124 Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing) Jan 17, 2024

subhamkumarr added 2 commits January 17, 2024 16:17

TRUNK-6124

b534c0b

TRUNK-6030

b23c87c

The allergy field in the Allergy UI allows the user to enter a numeric value, considering allergies cannot have numeric name, we must validate the input string before allowing the user to procced further

tendomart reviewed Jan 17, 2024

View reviewed changes

TRUNK-6124

dc5d9bf

tendomart reviewed Jan 18, 2024

View reviewed changes

subhamkumarr requested a review from tendomart January 19, 2024 05:54

TRUNK 6124

98b0278

tendomart reviewed Jan 19, 2024

View reviewed changes

subhamkumarr added 2 commits January 19, 2024 16:18

TRUNK 6124

96fb7ce

TRUNK 6124

68aaeb2

subhamkumarr requested a review from tendomart January 19, 2024 10:52

tendomart reviewed Jan 19, 2024

View reviewed changes

TRUNK 6124

472984c

subhamkumarr requested a review from tendomart January 19, 2024 10:57

github-actions bot added the Stale label Aug 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trunk 6124 Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing) #4533

Trunk 6124 Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing) #4533

subhamkumarr commented Jan 17, 2024

tendomart commented Jan 17, 2024

subhamkumarr commented Jan 17, 2024

subhamkumarr commented Jan 17, 2024 •

edited

Loading

tendomart commented Jan 17, 2024

subhamkumarr commented Jan 17, 2024 •

edited

Loading

tendomart Jan 17, 2024

subhamkumarr Jan 17, 2024

tendomart commented Jan 18, 2024 •

edited

Loading

subhamkumarr commented Jan 18, 2024

tendomart Jan 18, 2024

tendomart Jan 18, 2024 •

edited

Loading

subhamkumarr Jan 18, 2024

tendomart Jan 18, 2024

subhamkumarr Jan 18, 2024

tendomart Jan 19, 2024 •

edited

Loading

tendomart Jan 18, 2024 •

edited

Loading

tendomart Jan 18, 2024

tendomart Jan 18, 2024

tendomart commented Jan 19, 2024

subhamkumarr commented Jan 19, 2024

tendomart Jan 19, 2024

tendomart Jan 19, 2024

subhamkumarr Jan 19, 2024

subhamkumarr commented Jan 19, 2024

tendomart Jan 19, 2024

tendomart Jan 19, 2024

subhamkumarr Jan 19, 2024

subhamkumarr commented Jan 19, 2024 •

edited

Loading

tendomart commented Jan 19, 2024

subhamkumarr commented Jan 19, 2024

subhamkumarr commented Jan 19, 2024

github-actions bot commented Aug 4, 2024

		@@ -94,6 94,7 @@ error.email.alreadyInUse=Email address is already assigned to another user accou
		error.activationkey.invalid =Invalid user activation Key

Trunk 6124 Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing) #4533

Are you sure you want to change the base?

Trunk 6124 Eliminated unnecessary methods (toDatetime(), getDatetime(), and getDate() should all do the same thing) #4533

Conversation

subhamkumarr commented Jan 17, 2024

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

tendomart commented Jan 17, 2024

subhamkumarr commented Jan 17, 2024

subhamkumarr commented Jan 17, 2024 • edited Loading

tendomart commented Jan 17, 2024

subhamkumarr commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tendomart commented Jan 18, 2024 • edited Loading

subhamkumarr commented Jan 18, 2024

Choose a reason for hiding this comment

tendomart Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tendomart Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

tendomart Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tendomart commented Jan 19, 2024

subhamkumarr commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhamkumarr commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhamkumarr commented Jan 19, 2024 • edited Loading

tendomart commented Jan 19, 2024

subhamkumarr commented Jan 19, 2024

subhamkumarr commented Jan 19, 2024

github-actions bot commented Aug 4, 2024

subhamkumarr commented Jan 17, 2024 •

edited

Loading

subhamkumarr commented Jan 17, 2024 •

edited

Loading

tendomart commented Jan 18, 2024 •

edited

Loading

tendomart Jan 18, 2024 •

edited

Loading

tendomart Jan 19, 2024 •

edited

Loading

tendomart Jan 18, 2024 •

edited

Loading

subhamkumarr commented Jan 19, 2024 •

edited

Loading