-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
TRUNK-6030 : The allergy field in the Allergy UI should validate the input string #4534
base: master
Are you sure you want to change the base?
Conversation
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
if (allergy.getAllergen() != null && StringUtils.isNumeric(allergy.getAllergen().getNonCodedAllergen())) { | ||
errors.rejectValue("allergen", "error.allergyapi.allergy.Allergen.cannotContainNumeric"); | ||
} |
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 better to move the if
block inside the else
block at line 79.
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.
@ManojLL done.
moved `if` inside the `else` block
@@ -82,6 +82,11 @@ public void validate(Object target, Errors errors) { | |||
} else if (!allergen.isCoded() && StringUtils.isBlank(allergen.getNonCodedAllergen())) { | |||
errors.rejectValue("allergen", "allergyapi.allergen.nonCodedAllergen.required"); | |||
} | |||
|
|||
// Additional validation: Ensure allergen does not contain numeric values |
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.
remove the comment
@@ -82,6 +82,11 @@ public void validate(Object target, Errors errors) { | |||
} else if (!allergen.isCoded() && StringUtils.isBlank(allergen.getNonCodedAllergen())) { | |||
errors.rejectValue("allergen", "allergyapi.allergen.nonCodedAllergen.required"); | |||
} | |||
|
|||
// Additional validation: Ensure allergen does not contain numeric values | |||
if (allergy.getAllergen() != null && StringUtils.isNumeric(allergy.getAllergen().getNonCodedAllergen())) { |
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.
Remove the condition allergy.getAllergen() != null &&
as it has already been checked
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.
@ManojLL Done
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.
@subhamkumarr When committing, include a meaningful commit message that references the relevant JIRA ticket ID,
ex:- TRUNK-6030 : validate the input string
Removed `allergy.getAllergen() != null &&` as it has already been checked
Comment removed
if (StringUtils.isNumeric(allergy.getAllergen().getNonCodedAllergen())) { | ||
errors.rejectValue("allergen", "error.allergyapi.allergy.Allergen.cannotContainNumeric"); | ||
} |
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.
Remove the extra tab space and configure your IDE using this Also, replace allergy.getAllergen()
with allergen.
@@ -191,4 +191,12 @@ public void validate_shouldRejectNumericReactionValue() { | |||
validator.validate(allergy, errors); | |||
assertTrue(errors.hasErrors()); | |||
} | |||
@Test |
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.
Insert a space between the method
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.
@subhamkumarr Did you see this comment?
TRUNK-6030: validate the input string
@@ -82,6 +82,10 @@ public void validate(Object target, Errors errors) { | |||
} else if (!allergen.isCoded() && StringUtils.isBlank(allergen.getNonCodedAllergen())) { | |||
errors.rejectValue("allergen", "allergyapi.allergen.nonCodedAllergen.required"); | |||
} | |||
|
|||
if (StringUtils.isNumeric(allergen.getNonCodedAllergen())) { | |||
errors.rejectValue("allergen", "error.allergyapi.allergy.Allergen.cannotContainNumeric"); |
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.
Notice, as above, this should probably not have the error prefix.
allergy.setAllergen(allergen); | ||
Errors errors = new BindException(allergy,"allergy"); | ||
validator.validate(allergy, errors); | ||
assertTrue(errors.hasErrors()); |
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.
This should actually validate the error message.
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 Sir, please check I made the changes.
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 could you please review my PR.
TRUNK-6030 : validate the input string
TRUNK-6030 : validate the input string
TRUNK-6030: validate the input string
TRUNK-6030 : validate the input string
@@ -191,4 +191,12 @@ public void validate_shouldRejectNumericReactionValue() { | |||
validator.validate(allergy, errors); | |||
assertTrue(errors.hasErrors()); | |||
} | |||
@Test |
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.
@subhamkumarr Did you see this comment?
validator.validate(allergy, errors); | ||
assertTrue(errors.hasErrors()); | ||
assertTrue(errors.getAllErrors().stream() | ||
.anyMatch(error -> "allergyapi.allergy.Allergen.cannotContainNumeric".equals(error.getCode()))); } |
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.
Indent here should be with tabs.
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.
Also, the curly brace belongs on a new line. Please ensure you're following the Java Conventions and see this to setup your IDE (basically, whatever mechanism will take an Eclipse formatting configuration).
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. 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 :) |
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
Description of what I changed
This pull request enhances the AllergyValidator in the Allergy UI to provide improved validation for the allergen field. Previously, the allergen field allowed users to enter numeric values, which is not a valid scenario for allergy names.
Changes Made:
Updated the AllergyValidator class to include validation for numeric values in the allergen field.
If the allergen field contains numeric values, a custom error is triggered to inform the user about the restriction on using
numeric values for allergy names.
How to Test:
Issue I worked on
https://openmrs.atlassian.net/issues/TRUNK-6030
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amend
I ran
mvn clean package
right before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master