LUI-197 UI for saving concept reference ranges #196
Conversation
|
Do you have a screenshot of how this looks like? |
It's not showing up yet. I have am using the current built version. Am I missing something? |
|
Do you try compile and update the module install? |
|
|
omod/src/main/java/org/openmrs/web/controller/ConceptFormController.java
Outdated
Show resolved
Hide resolved
|
@dicksonmulli in the hibernate mapping file, did you try map concept reference ranges the same way as concept mappings? |
Yes, but I am still getting the same error : |
What value do you have in the |
I changed to |
dicksonmulli
changed the title
|
@dicksonmulli i think we have one more remaining bug here. |
Fixed now. |
|
@dicksonmulli did you forget to make the commit? |
|
@dicksonmulli i have noticed that the order of insertion of reference ranges is preserved only when they are added to an existing concept. But when you create a new one and immediately add the reference ranges before saving it, the order is not preserved. |
|
@dicksonmulli i also noticed that when i run this on platform versions before 2.7, i get this in the logs when i save a concept: https://pastebin.com/wcDCXc8P Secondly, i also see the |
Yes, this is is the expected behaviour because we are catching the exception incase the version is not compatible - so that the application will not fail to run. However, we can change the log from |
|
Two things:
|
| setMethodValue(cn, "removeReferenceRange", platformReferenceRange); | ||
| } | ||
| catch (Exception exception) { | ||
| logger.warn("Failed to remove concept reference range. " |
There was a problem hiding this comment.
The logs will be filled with many of these warnings for those running platform versions lower than 2.7
I do not find it a good practice to fill logs with such warnings when there is completely nothing wrong about running lower versions of the platform.
| * @return true if current version is greater or equal to 2.7.0 and false otherwise | ||
| * @since 1.17.0 | ||
| */ | ||
| public static boolean isTwoPointSevenAndAbove() { |
There was a problem hiding this comment.
This is cimplicated and brittle. Why don't you use the same trick of trying to load a class like concept reference range which was added in 2.7?
There was a problem hiding this comment.
This boolean is used in UI/JSP hence the need to check the version instead of loading a particular class. I think this will be reliable as long as we have the version constant(s).
There was a problem hiding this comment.
We can still have a boolean with loading a particular class.
| public static boolean isTwoPointSevenAndAbove() { | ||
| try { | ||
| Class<?> referenceRangeClass = Class.forName("org.openmrs.ConceptReferenceRange"); | ||
| referenceRangeClass.getMethod("getCriteria"); |
There was a problem hiding this comment.
What value does line 85 add? Do we have cases where that class exists but not the method?
|
@dicksonmulli i have noticed that with the changes in this pull request, when i am creating a new concept, the |
|
@dicksonmulli and FWIW, this incorrect display happens only on platform versions lower than 2.7 |
|
@dicksonmulli now when a concept is saved with the |
https://openmrs.atlassian.net/browse/LUI-197