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

Added WebUtil.escapeHTML function to ProviderListItem #157

Merged
merged 2 commits into from
May 11, 2021

Conversation

Parth59
Copy link
Contributor

@Parth59 Parth59 commented Apr 9, 2021

Had raised #141 for the following. After which @isears requested to modify the server-side code instead of the JS files. So I tried solving EMPT64 with changes in the following server File:- omod/src/main/java/org/openmrs/web/dwr/ProviderListItem.java. Had to revert the changes of the following PR #152 to make it error free.

Steps to reproduce can be found here.
https://docs.google.com/document/d/1GJNMRlGTRV45St6WSjnD_FjW84aoNxY9HzGbpwoBrro/edit?usp=sharing

Link to ticket:
https://issues.openmrs.org/browse/RA-1865

Copy link
Member

@isears isears left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I remember this now.

In general, I definitely prefer this server-side approach to patching. The reason #152 got merged in, however, is because it patched EMPT-46 as well as EMPT-64, even if the JS-based patch was less desirable.

@Parth59 if you can include a server-side patch for EMPT-46 in this PR, I'll merge it in.

@HerbertYiga
Copy link
Contributor

can include a server-side patch for EMPT-46 in this PR

beautiful

@Parth59
Copy link
Contributor Author

Parth59 commented Apr 14, 2021

Hi Team,
Thanks for the review @isears
I worked with @maxhark for the same and have raised the following PR's with their respected serverside fixes.

i) EMPT60 :- #158
ii) EMPT46 :-#159

@isears
Copy link
Member

isears commented Apr 15, 2021

Ok thanks @Parth59 and @maxhark for working together to figure this out. I've merged #158 and #159 so I'm going to go ahead and close this one

@isears isears closed this Apr 15, 2021
@Parth59
Copy link
Contributor Author

Parth59 commented May 9, 2021

Hi @isears .

This PR's fixes EMPT64. Whereas the below PR's fixes EMPT60 and EMPT46. Kindly review it to fix EMPT64.
i) EMPT60 :- #158
ii) EMPT46 :-#159

@isears isears reopened this May 11, 2021
Copy link
Member

@isears isears left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry must have misinterpreted, thanks for the reminder @Parth59 !

@isears isears merged commit 7f1d5bb into openmrs:master May 11, 2021
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.

3 participants