(feat) O3-3184 allow changing of LocationWaitingFor and ProviderWaiti… #74
Conversation
…ngFor during a transition
chibongho
requested review from
mseaton,
mogoodrich,
cioan,
denniskigen and
ibacher
| @@ -96,6 +101,36 @@ public Object transitionQueueEntry(@RequestBody QueueEntryTransitionRequest body | |||
|
|
|||
| transition.setNewPriorityComment(body.getNewPriorityComment()); | |||
|
|
|||
| // Location waiting for | |||
| if (rawBody.containsKey(QueueEntryTransitionRequest.NEW_LOCATION_WAITING_FOR_FIELD)) { | |||
There was a problem hiding this comment.
Was there a reason these couldn't just be added as properties for the QueueEntryTransitionRequest?
There was a problem hiding this comment.
They are properties already, but having those properties be null is not sufficient to distinguish between "we intend to set this to null" vs "we don't want to change this property". So we check the rawBody to see if those optional properties are provided at all, and only set the corresponding queue entry field if so.
There was a problem hiding this comment.
@chibongho - isn't that also the case for some of the other, already existing properties you are setting? For example priorityComment?
There was a problem hiding this comment.
@mseaton that's a good point about priorityComment (and other nullable fields). We got away with it because the frontend defaults the QueueEntryTransitionRequest.newPriorityComment field to whatever the original queue entry's priorityComment is, or empty string if its null. The UI renders null or empty string the same way anyway.
I originally intended the QueueEntryTransitionRequest to have all fields be optional and the request would only modify the fields that are provided. That seems to not work as well for nullable fields. I can just change the API so that all fields are required, and the request would have to populate all non-modified fields with values from the original queue entry. (We already do that in the fronend).
| @@ -36,6 +38,10 @@ public class QueueEntryTransition implements Serializable { | |||
|
|
|||
| private String newPriorityComment; | |||
|
|
|||
| private Location newLocationWaitingFor; // value obtrained from queueEntryToTransition if not provided by request | |||
|
|
|||
| private Provider newProviderWaitingFor; // value obtrained from queueEntryToTransition if not provided by request | |||
There was a problem hiding this comment.
Typo in both comments "obtrained" -> "obtained"
| @@ -96,6 +101,36 @@ public Object transitionQueueEntry(@RequestBody QueueEntryTransitionRequest body | |||
|
|
|||
| transition.setNewPriorityComment(body.getNewPriorityComment()); | |||
|
|
|||
| // Location waiting for | |||
| if (rawBody.containsKey(QueueEntryTransitionRequest.NEW_LOCATION_WAITING_FOR_FIELD)) { | |||
There was a problem hiding this comment.
@chibongho - isn't that also the case for some of the other, already existing properties you are setting? For example priorityComment?
| queueEntry.setLocationWaitingFor(queueEntryToTransition.getLocationWaitingFor()); | ||
| queueEntry.setProviderWaitingFor(queueEntryToTransition.getProviderWaitingFor()); | ||
| queueEntry.setLocationWaitingFor(newLocationWaitingFor); | ||
| queueEntry.setProviderWaitingFor(newProviderWaitingFor); |
There was a problem hiding this comment.
It seems a little inconsistent here to do this vs. what we do for status, priority, etc. For the existing fields, we handle the logic here for setting them to existing fields. For these new properties, we handle that logic in the REST controller. Would it make sense to be consistent?
| // Location waiting for | ||
| if (rawBody.containsKey(QueueEntryTransitionRequest.NEW_LOCATION_WAITING_FOR_FIELD)) { | ||
| if (body.getNewLocationWaitingFor() == null) { | ||
| transition.setNewLocationWaitingFor(null); |
There was a problem hiding this comment.
This seems unnecessary. Isn't it always null already in this case?
| // Provider waiting for | ||
| if (rawBody.containsKey(QueueEntryTransitionRequest.NEW_PROVIDER_WAITING_FOR_FIELD)) { | ||
| if (body.getNewProviderWaitingFor() == null) { | ||
| transition.setNewProviderWaitingFor(null); |
| if (body.getNewProviderWaitingFor() == null) { | ||
| transition.setNewProviderWaitingFor(null); | ||
| } else { | ||
| Provider provider = services.getProviderService().getProviderByUuid(body.getNewLocationWaitingFor()); |
There was a problem hiding this comment.
This is a bug, probably related to copying the code block. This should be body.getNewProviderWaitingFor(). I'm surprised the tests didn't catch this - can you modify the tests to ensure they do?
|
Closing this PR for now due to ticket reprioritization. |
…ngFor during a transition
This PR add
locationWaitingForandproviderWaitingForas attributes that are changeable during a transition.The logic here is a bit strange since these fields may be set to null, and we need to way to distinguish between "changing the field to null" vs "keeping the field as it was". It does that by determining whether the optional attributes
newLocationWaitingForandnewProviderWaitingForexists in the/queue-entry/transitionrequest body. If not, those fields do not get changed.