RESTWS-953:Support setting values of type "Location" #617
Conversation
|
|
||
| //We want clients to be able to fetch a coded value in one rest call | ||
| //and set the returned payload as the obs value | ||
| return; |
There was a problem hiding this comment.
I was reluctant to refactor this too much because I wasn't sure how good the test coverage was, but the nested if/else block were confusing and this change was going to make them even more confusing, so I removed the top-level if/else and had each block just return instead.
| } | ||
|
|
||
| // if there is a potential uuid, see if there is a matching location and set | ||
| if (RestUtil.isUuid(potentialLocationUuid)) { |
There was a problem hiding this comment.
We don't strictly need to do a uuid check, but otherwise we'd we calling a getLocation.getLocationByUuid for any value text passed it, which seemed like it could be a performance hit.
| return; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
End of the stuff I added. The stuff below should be the same, just with one less if/then block.
| @@ -936,4 +940,8 @@ public static Class<?> getSupportedClass(Resource resource) { | |||
| .supportedClass(); | |||
| } | |||
| } | |||
|
|
|||
| public static boolean isUuid(String str) { | |||
| return StringUtils.isNotBlank(str) && UUID_REGEX.matcher(str).matches(); | |||
There was a problem hiding this comment.
I couldn't find any existing standard util methods for this (besides doing a UUID.toString() in a try/catch block, which was ugly) so I just grabbed a regex I found online.
There was a problem hiding this comment.
I've definitely seen other implementations of this in OpenMRS code, can't remember where off-hand...
There was a problem hiding this comment.
There are a few lying around, but they often run up against the issue that we've been allowing invalid UUIDs for years (e.g., every single "uuid" in CIEL is, in fact, not a "valid UUID"). Usually these implementations hang around until someone complains and we basically end up with a check that says "is this string 36 characters long"? Here is one version and a slight variant.
There was a problem hiding this comment.
Oh, right, @ibacher regarding the invalid uuid formats for concepts. Hopefully shouldn't matter for this case, since it is regarding to locations, but makes sense to standardized on a single util method, will fix.
| } | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the start of the stuff I added to handle location
| Location location = Context.getLocationService().getLocationByUuid(potentialLocationUuid); | ||
| if (location != null) { | ||
| obs.setValueText(location.getLocationId().toString()); | ||
| obs.setComment("org.openmrs.Location"); |
There was a problem hiding this comment.
Fine, but could also do "Location.class.getName()"
| @@ -936,4 +940,8 @@ public static Class<?> getSupportedClass(Resource resource) { | |||
| .supportedClass(); | |||
| } | |||
| } | |||
|
|
|||
| public static boolean isUuid(String str) { | |||
| return StringUtils.isNotBlank(str) && UUID_REGEX.matcher(str).matches(); | |||
There was a problem hiding this comment.
I've definitely seen other implementations of this in OpenMRS code, can't remember where off-hand...
Description of what I changed
Issue I worked on
see https://issues.openmrs.org/browse/RESTWS-
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 --amendI 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 --amendI ran
mvn clean packageright 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