Added Changes to UserFormController.java to redirect error message back to user. #164
Conversation
Updated messages.properties to add User.DemographicInfo.length message notification Please refer to this PR for all the relevant details. i) openmrs/openmrs-module-legacyui#164 Link to ticket RA-1875 (https://issues.openmrs.org/browse/RA-1875)
Added required error message for EMPT70. Kindly refer to the following PR for all relevant details :- openmrs/openmrs-module-legacyui#164
@isears Added required error message for EMPT70. Kindly refer to the following PR for all relevant details :- openmrs/openmrs-module-legacyui#164
| String[] errorCodes = fieldError.getCodes(); | ||
| for (String value : errorCodes) { | ||
| if (value.contains("error.exceededMaxLengthOfField")) { | ||
| httpSession.setAttribute(WebConstants.OPENMRS_ERROR_ATTR, "User.DemographicInfo.length"); |
There was a problem hiding this comment.
thanks @Parth59 ,some times sessions get disturbing more so when they are not destroyed after use,why not use model .addAttribute( )
There was a problem hiding this comment.
Hi @HerbertYiga , I have started reading about the same. Will try to implement those changes in the code !
There was a problem hiding this comment.
Hi @HerbertYiga
I tried using the model.addAttrribute() and it seems like using those with redirect causes them to be added to the URL. Additionally, the <c:if> tag in the view doesn't load up the error messages as can be seen in the screenshot referenced below. [isLengthExceeded in the url comes to be true but isn't loaded in the page]
Infact, I also double checked for places that utilize model.addAttribute() in the same function. Turns out that the entire function utilizes httpSession for passing response back to the user.
So requesting you to kindly let this security PR pass . If needed I raise an issue and then will refactor to modify the entire function in a separate PR. Sounds good ?
isears
left a comment
There was a problem hiding this comment.
Thanks @Parth59
If this PR depends on Parth59/openmrs-core#1, then we will have to add the next release of openmrs-core as a dependency.
I wonder would it be possible to just add your message here (https://github.com/openmrs/openmrs-module-legacyui/blob/5f8d1d3ea5844cedb535b1634d9848ba0ca3b320/api/src/main/resources/messages.properties) so that this change doesn't depend on updates to external modules?
Sure. I will test it out if this approach works. |
fe904bb to
4253cd2
Compare
|
Hi Issac, |
|
|
||
| userValidator.validate(user, errors); | ||
|
|
||
| if (errors.hasErrors()) { |
There was a problem hiding this comment.
Can we combine the logic of this if-clause with the if-clause immediately below it (line 290)?
It looks like the condition is the same and collapsing them into a single if-clause might make the error-handling logic of this section a little more readable.
There was a problem hiding this comment.
Hi Issac, I have refactored the code as per suggestions.
| for (String value : errorCodes) { | ||
| if (value.contains("error.exceededMaxLengthOfField")) { | ||
| httpSession.setAttribute(WebConstants.OPENMRS_ERROR_ATTR,"legacyui.manageuser.DemographicInfo.lengthExceeded"); | ||
| httpSession.setAttribute(WebConstants.OPENMRS_ERROR_ARGS, "50"); |
Hi Team, @isears
This change is for fixing EMPT70. I have performed minor test runs to the best of my abilities. Additionally, there are minor one line addition to message.properties file in openmrs-core. I will shortly raise them and link this PR in those comments.
Link to ticket
RA-1875 (https://issues.openmrs.org/browse/RA-1875)
Steps to reproduce :
i) Go to Advance Administration ‹ Manage Users
ii) Enter admin in find user on name text field
iii)Click Search
iv)Click admin in the search result
v)Enter javascript:/--></title></style></textarea></script></xmp><svg/onlo ad='+/"/+/onmouseover=1/+/[/[]/+alert(1)//'> in the first name, middle name and last name input fields
vi) Click Save
Before fix:
After fix :
