(BREAKING) O3-2748 add endpoint for un-doing transition of queue entries; ren… #64
Conversation
chibongho
requested review from
mseaton,
mogoodrich,
ojwanganto,
jecihjoy,
donaldkibet and
ibacher
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Great job @chibongho . See my review comments for some changes requested.
api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Show resolved
Hide resolved
omod/src/main/java/org/openmrs/module/queue/web/QueueEntryTransitionRestController.java
Outdated
Show resolved
Hide resolved
omod/src/main/java/org/openmrs/module/queue/web/QueueEntryTransitionRestController.java
Show resolved
Hide resolved
chibongho
force-pushed
the
undo-transition
branch
from
774edad
to
71d283d
Compare
chibongho
changed the title
…ies; rename endpoints related to transitions
chibongho
force-pushed
the
undo-transition
branch
from
71d283d
to
41a9ceb
Compare
|
|
||
| /** | ||
| * Given a specified queue entry Q, return its previous queue entry P, where P has same patient and | ||
| * visit as Q, and P.endedAt time is same as Q.startAt time. |
There was a problem hiding this comment.
startAt -> startedAt. Also, we should check the Queue. The queueComingFrom associated with Q should match the queue associated with P.
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
I resolved some comments that had been addressed, and added some additional @chibongho . Ping me with the "Re-review" button when you think it's ready to go...
chibongho
force-pushed
the
undo-transition
branch
from
e1dd8f4
to
c4d0006
Compare
| criteria.setPatient(queueEntry.getPatient()); | ||
| criteria.setVisit(queueEntry.getVisit()); | ||
| criteria.setEndedOn(queueEntry.getStartedAt()); | ||
| criteria.setQueues(Arrays.asList(queueEntry.getQueueComingFrom())); |
There was a problem hiding this comment.
@chibongho this needs to be able to handle the situation where the queueEntry.getQueueComingFrom() is null.
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Show resolved
Hide resolved
| criteria.setPatient(queueEntry.getPatient()); | ||
| criteria.setVisit(queueEntry.getVisit()); | ||
| criteria.setEndedOn(queueEntry.getStartedAt()); | ||
| criteria.setQueues(queueComingFrom == null ? Arrays.asList() : Arrays.asList(queueComingFrom)); |
There was a problem hiding this comment.
What exactly do you want to happen here? If you set this to an empty list, then the DAO will only return results in which the queue property of QueueEntry is null. Since this is not (currently) nullable, this will effectively mean that no results are returned. If that is actually what we want, then we should just do a check earlier in the method and return null without bothering to hit the DB. If it is not what we want, and we want to get all queue entries that match regardless of queue (and if there is only one then use it), then we need the if statement to be around this entire critieria inclusion call.
There was a problem hiding this comment.
Yes, I want it to return null if queueComingFrom is null. I'll change it to a null check earlier.
…ame endpoints related to transitions
This PR add the ability to undo queue entry transitions. Undoing a transition voids the current active queue entry and un-ends its previous queue entry.
I changed the route to REST endpoints related to queue entry transitions to be under
/queue-entry/*, namely/queue-entry/transitionandqueue-entry/undo-transition, hence the breaking change.