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.

Sign up for GitHub

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

Jump to bottom

(feat) O3-3184 allow changing of LocationWaitingFor and ProviderWaiti… #74

Closed
wants to merge 1 commit into from
Closed

(feat) O3-3184 allow changing of LocationWaitingFor and ProviderWaiti… #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ private void endQueueEntry(@NotNull QueueEntry queueEntry) {
@Transactional(readOnly = true)
public QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry) {
Queue queueComingFrom = queueEntry.getQueueComingFrom();
if(queueComingFrom == null) {
if (queueComingFrom == null) {
return null;
}
QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria();
Expand Down
10 changes: 8 additions & 2 deletions api/src/main/java/org/openmrs/module/queue/model/QueueEntryTransition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

import lombok.Data;
import org.openmrs.Concept;
import org.openmrs.Location;
import org.openmrs.Provider;

/**
* Bean definition that encapsulates the supported criteria for saving a direct transition from one
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Typo in both comments "obtrained" -> "obtained"


/**
* @return a new queue entry representing what one intends to transition into
*/
Expand All @@ -49,8 +55,8 @@ public QueueEntry constructNewQueueEntry() {
newPriorityComment == null ? queueEntryToTransition.getPriorityComment() : newPriorityComment);
queueEntry.setStatus(newStatus == null ? queueEntryToTransition.getStatus() : newStatus);
queueEntry.setSortWeight(queueEntryToTransition.getSortWeight());
queueEntry.setLocationWaitingFor(queueEntryToTransition.getLocationWaitingFor());
queueEntry.setProviderWaitingFor(queueEntryToTransition.getProviderWaitingFor());
queueEntry.setLocationWaitingFor(newLocationWaitingFor);
queueEntry.setProviderWaitingFor(newProviderWaitingFor);
Copy link
Member

Choose a reason for hiding this comment

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

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?

queueEntry.setQueueComingFrom(queueEntryToTransition.getQueue());
queueEntry.setStartedAt(transitionDate);
return queueEntry;
Expand Down
8 changes: 5 additions & 3 deletions api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,8 @@ public void shouldTransitionQueueEntry() {
QueueEntryTransition transition1 = new QueueEntryTransition();
transition1.setQueueEntryToTransition(queueEntry1);
transition1.setTransitionDate(date2);
transition1.setNewLocationWaitingFor(location1);
transition1.setNewProviderWaitingFor(provider1);
QueueEntry queueEntry2 = queueEntryService.transitionQueueEntry(transition1);
assertThat(queueEntry1.getEndedAt(), equalTo(date2));
assertThat(queueEntry2.getQueue(), equalTo(queue1));
Expand Down Expand Up @@ -280,8 +282,8 @@ public void shouldTransitionQueueEntry() {
assertThat(queueEntry3.getPriorityComment(), equalTo(string2));
assertThat(queueEntry3.getStatus(), equalTo(concept2));
assertThat(queueEntry3.getSortWeight(), equalTo(double1));
assertThat(queueEntry3.getLocationWaitingFor(), equalTo(location1));
assertThat(queueEntry3.getProviderWaitingFor(), equalTo(provider1));
assertThat(queueEntry3.getLocationWaitingFor(), equalTo(null));
assertThat(queueEntry3.getProviderWaitingFor(), equalTo(null));
assertThat(queueEntry3.getQueueComingFrom(), equalTo(queue1));
assertThat(queueEntry3.getStartedAt(), equalTo(date3));
assertNull(queueEntry3.getEndedAt());
Expand Down Expand Up @@ -332,7 +334,7 @@ public void shouldUndoTransitionQueueEntry() {
criteria.setEndedOn(date2);
criteria.setQueues(Arrays.asList(queueEntry2.getQueueComingFrom()));
when(dao.getQueueEntries(criteria)).thenReturn(Arrays.asList(queueEntry1));

queueEntryService.undoTransition(queueEntry2);
assertThat(queueEntry2.getVoided(), equalTo(true));
assertNull(queueEntry1.getEndedAt());
Expand Down
37 changes: 36 additions & 1 deletion omod/src/main/java/org/openmrs/module/queue/web/QueueEntryTransitionRestController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,13 @@
package org.openmrs.module.queue.web;

import java.util.Date;
import java.util.Map;
import java.util.Optional;

import com.fasterxml.jackson.databind.ObjectMapper;
import org.openmrs.Concept;
import org.openmrs.Location;
import org.openmrs.Provider;
import org.openmrs.api.APIException;
import org.openmrs.module.queue.api.QueueEntryService;
import org.openmrs.module.queue.api.QueueServicesWrapper;
Expand Down Expand Up @@ -48,7 +52,8 @@ public QueueEntryTransitionRestController(QueueServicesWrapper services) {
@RequestMapping(value = "/rest/" + RestConstants.VERSION_1 + "/queue-entry/transition", method = { RequestMethod.PUT,
RequestMethod.POST })
@ResponseBody
public Object transitionQueueEntry(@RequestBody QueueEntryTransitionRequest body) {
public Object transitionQueueEntry(@RequestBody Map<String, Object> rawBody) {
QueueEntryTransitionRequest body = new ObjectMapper().convertValue(rawBody, QueueEntryTransitionRequest.class);
QueueEntryTransition transition = new QueueEntryTransition();

// Queue Entry to Transition
Expand Down Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

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

Was there a reason these couldn't just be added as properties for the QueueEntryTransitionRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@chibongho - isn't that also the case for some of the other, already existing properties you are setting? For example priorityComment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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).

if (body.getNewLocationWaitingFor() == null) {
transition.setNewLocationWaitingFor(null);
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessary. Isn't it always null already in this case?

} else {
Location location = services.getLocationService().getLocationByUuid(body.getNewLocationWaitingFor());
if (location == null) {
throw new APIException("Invalid locationWaitingFor specified: " + body.getNewLocationWaitingFor());
}
transition.setNewLocationWaitingFor(location);
}
} else {
transition.setNewLocationWaitingFor(queueEntry.getLocationWaitingFor());
}

// Provider waiting for
if (rawBody.containsKey(QueueEntryTransitionRequest.NEW_PROVIDER_WAITING_FOR_FIELD)) {
if (body.getNewProviderWaitingFor() == null) {
transition.setNewProviderWaitingFor(null);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

} else {
Provider provider = services.getProviderService().getProviderByUuid(body.getNewLocationWaitingFor());
Copy link
Member

Choose a reason for hiding this comment

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

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?

if (provider == null) {
throw new APIException("Invalid providerWaitingFor specified: " + body.getNewProviderWaitingFor());
}
transition.setNewProviderWaitingFor(provider);
}
} else {
transition.setNewProviderWaitingFor(queueEntry.getProviderWaitingFor());
}

// Execute transition
QueueEntry newQueueEntry = services.getQueueEntryService().transitionQueueEntry(transition);
return ConversionUtil.convertToRepresentation(newQueueEntry, Representation.REF);
Expand Down
8 changes: 8 additions & 0 deletions omod/src/main/java/org/openmrs/module/queue/web/dto/QueueEntryTransitionRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,12 @@ public class QueueEntryTransitionRequest {
private String newPriority;

private String newPriorityComment;

private String newLocationWaitingFor;

private String newProviderWaitingFor;

public static final String NEW_LOCATION_WAITING_FOR_FIELD = "newLocationWaitingFor";

public static final String NEW_PROVIDER_WAITING_FOR_FIELD = "newProviderWaitingFor";
}
Loading