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

O3-2453 - remove where conditions from property JPA annotations #42

Merged
merged 2 commits into from
Sep 27, 2023
Merged

O3-2453 - remove where conditions from property JPA annotations #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cc7776
O3-2453 - remove where conditions from property JPA annotations
mseaton Sep 27, 2023
91405f6
Update api/src/main/java/org/openmrs/module/queue/model/Queue.java
mseaton Sep 27, 2023
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
55 changes: 52 additions & 3 deletions api/src/main/java/org/openmrs/module/queue/model/Queue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,14 @@
import javax.persistence.OneToMany;
import javax.persistence.Table;

import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.stream.Collectors;

import lombok.Data;
import lombok.EqualsAndHashCode;
import org.hibernate.annotations.Where;
import org.apache.commons.lang.BooleanUtils;
import org.openmrs.BaseChangeableOpenmrsMetadata;
import org.openmrs.Concept;
import org.openmrs.Location;
Expand All @@ -52,13 +55,59 @@ public class Queue extends BaseChangeableOpenmrsMetadata {
private Concept service;

@OneToMany(mappedBy = "queue", cascade = CascadeType.ALL, fetch = FetchType.LAZY)
@Where(clause = "voided = 0 and (started_at <= current_timestamp() and ended_at is null)")
private List<QueueEntry> queueEntries;

@OneToMany(mappedBy = "queue", cascade = CascadeType.ALL, fetch = FetchType.LAZY)
@Where(clause = "retired = 0")
private List<QueueRoom> queueRooms;

/**
* @return all non-voided QueueEntries that do not start in the future and are not already ended
*/
public List<QueueEntry> getActiveQueueEntries() {
List<QueueEntry> activeQueueEntries = new ArrayList<>();
Date now = new Date();
if (queueEntries != null) {
for (QueueEntry queueEntry : queueEntries) {
if (BooleanUtils.isNotTrue(queueEntry.getVoided())) {
if (!queueEntry.getStartedAt().after(now) && queueEntry.getEndedAt() == null) {
activeQueueEntries.add(queueEntry);
}
}
}
}
return activeQueueEntries;
}

/**
* @return all non-retired QueueRooms
*/
public List<QueueRoom> getActiveQueueRooms() {
if (queueRooms == null) {
return new ArrayList<>();
}
return queueRooms.stream().filter(r -> BooleanUtils.isNotTrue(r.getRetired())).collect(Collectors.toList());
}

/**
* @param queueEntry the QueueEntry to add
*/
public void addQueueEntry(QueueEntry queueEntry) {
if (queueEntries == null) {
queueEntries = new ArrayList<>();
}
queueEntries.add(queueEntry);
}

/**
* @param queueRoom the QueueRoom to add
*/
public void addQueueRoom(QueueRoom queueRoom) {
if (queueRooms == null) {
queueRooms = new ArrayList<>();
}
queueRooms.add(queueRoom);
}

@Override
public Integer getId() {
return getQueueId();
Expand Down
116 changes: 116 additions & 0 deletions api/src/test/java/org/openmrs/module/queue/model/QueueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.queue.model;

import org.apache.commons.lang.time.DateUtils;
import org.junit.Test;

import java.util.Date;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;

public class QueueTest {

Date now = new Date();

@Test
public void addQueueEntry_shouldAddANewQueueEntryEvenIfQueueEntriesIsNull() {
Queue queue = new Queue();
queue.setQueueEntries(null);
QueueEntry queueEntry = new QueueEntry();
queue.addQueueEntry(queueEntry);
assertThat(queue.getQueueEntries().size(), equalTo(1));
}

@Test
public void addQueueRoom_shouldAddANewQueueRoomEvenIfQueueRoomsIsNull() {
Queue queue = new Queue();
queue.setQueueRooms(null);
QueueRoom queueRoom = new QueueRoom();
queue.addQueueRoom(queueRoom);
assertThat(queue.getQueueRooms().size(), equalTo(1));
}

@Test
public void getActiveQueueEntries_shouldReturnAnEmptyListIfNoQueueEntriesExist() {
Queue queue = new Queue();
queue.setQueueEntries(null);
assertThat(queue.getQueueEntries(), nullValue());
assertThat(queue.getActiveQueueEntries(), notNullValue());
assertThat(queue.getActiveQueueEntries().size(), equalTo(0));
}

@Test
public void getActiveQueueEntries_shouldReturnOnlyNonVoidedEntries() {
Queue queue = new Queue();

QueueEntry queueEntry = new QueueEntry();
queueEntry.setStartedAt(DateUtils.addHours(now, -1));
queueEntry.setEndedAt(null);
queue.addQueueEntry(queueEntry);

assertThat(queue.getQueueEntries(), contains(queueEntry));
assertThat(queue.getActiveQueueEntries(), contains(queueEntry));
queueEntry.setVoided(true);
assertThat(queue.getQueueEntries(), contains(queueEntry));
assertThat(queue.getActiveQueueEntries(), not(contains(queueEntry)));
}

@Test
public void getActiveQueueEntries_shouldNotReturnFutureQueueEntries() {
Queue queue = new Queue();

QueueEntry queueEntry = new QueueEntry();
queueEntry.setStartedAt(DateUtils.addHours(now, -1));
queueEntry.setEndedAt(null);
queue.addQueueEntry(queueEntry);

assertThat(queue.getQueueEntries(), contains(queueEntry));
assertThat(queue.getActiveQueueEntries(), contains(queueEntry));
queueEntry.setStartedAt(DateUtils.addHours(now, 1));
assertThat(queue.getQueueEntries(), contains(queueEntry));
assertThat(queue.getActiveQueueEntries(), not(contains(queueEntry)));
}

@Test
public void getActiveQueueEntries_shouldNotReturnEndedQueueEntries() {
Queue queue = new Queue();

QueueEntry queueEntry = new QueueEntry();
queueEntry.setStartedAt(DateUtils.addHours(now, -2));
queueEntry.setEndedAt(null);
queue.addQueueEntry(queueEntry);

assertThat(queue.getQueueEntries(), contains(queueEntry));
assertThat(queue.getActiveQueueEntries(), contains(queueEntry));
queueEntry.setEndedAt(DateUtils.addHours(now, 1));
assertThat(queue.getQueueEntries(), contains(queueEntry));
assertThat(queue.getActiveQueueEntries(), not(contains(queueEntry)));
}

@Test
public void getActiveQueueRooms_shouldNotReturnRetiredQueueRooms() {
Queue queue = new Queue();

QueueRoom queueRoom = new QueueRoom();
queue.addQueueRoom(queueRoom);

assertThat(queue.getQueueRooms(), contains(queueRoom));
assertThat(queue.getActiveQueueRooms(), contains(queueRoom));
queueRoom.setRetired(true);
assertThat(queue.getQueueRooms(), contains(queueRoom));
assertThat(queue.getActiveQueueRooms(), not(contains(queueRoom)));
}
}
25 changes: 13 additions & 12 deletions ...del/AutoCloseVisitQueueEntryTaskTest.java → ...sks/AutoCloseVisitQueueEntryTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: All the changes in this file are just due to the fact that I had put this in the wrong package, and I noticed this while creating a unit test for the Queue class, so this just moves the task unit test from the "model" to the "tasks" package.

* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.queue.model;
package org.openmrs.module.queue.tasks;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.nullValue;
import org.junit.Test;
import org.openmrs.Visit;
import org.openmrs.module.queue.model.QueueEntry;
import org.openmrs.module.queue.model.VisitQueueEntry;

import java.text.DateFormat;
import java.text.SimpleDateFormat;
Expand All @@ -21,9 +22,9 @@
import java.util.List;
import java.util.stream.Collectors;

import org.junit.Test;
import org.openmrs.Visit;
import org.openmrs.module.queue.tasks.AutoCloseVisitQueueEntryTask;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.nullValue;

public class AutoCloseVisitQueueEntryTaskTest {

Expand All @@ -44,7 +45,7 @@ protected void saveQueueEntry(QueueEntry queueEntry) {

@Test
public void shouldAutoCloseVisitQueueEntriesIfVisitIsClosed() throws Exception {

Visit visit1 = new Visit();
visit1.setStartDatetime(getDate("2020-01-01 10:00"));
QueueEntry queueEntry1 = new QueueEntry();
Expand All @@ -53,7 +54,7 @@ public void shouldAutoCloseVisitQueueEntriesIfVisitIsClosed() throws Exception {
visitQueueEntry1.setVisit(visit1);
visitQueueEntry1.setQueueEntry(queueEntry1);
queueEntries.add(visitQueueEntry1);

Visit visit2 = new Visit();
visit2.setStartDatetime(getDate("2021-01-01 10:00"));
QueueEntry queueEntry2 = new QueueEntry();
Expand All @@ -62,17 +63,17 @@ public void shouldAutoCloseVisitQueueEntriesIfVisitIsClosed() throws Exception {
visitQueueEntry2.setVisit(visit2);
visitQueueEntry2.setQueueEntry(queueEntry2);
queueEntries.add(visitQueueEntry2);

TestAutoCloseVisitEntryTask task = new TestAutoCloseVisitEntryTask();
task.run();
assertThat(queueEntry1.getEndedAt(), nullValue());
assertThat(queueEntry2.getEndedAt(), nullValue());

visit1.setStopDatetime(getDate("2020-01-01 23:15"));
task.run();
assertThat(queueEntry1.getEndedAt(), equalTo(visit1.getStopDatetime()));
assertThat(queueEntry2.getEndedAt(), nullValue());

visit2.setStopDatetime(getDate("2021-01-05 11:30"));
task.run();
assertThat(queueEntry1.getEndedAt(), equalTo(visit1.getStopDatetime()));
Expand Down
2 changes: 1 addition & 1 deletion omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryCountSubResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public void setParent(QueueEntryCount queueEntryCount, Queue queue) {
@Override
public PageableResult doGetAll(Queue queue, RequestContext requestContext) throws ResponseException {
return new GenericSingleObjectResult(Arrays.asList(new PropValue("queueName", queue.getName()),
new PropValue("queueEntriesCount", queue.getQueueEntries().size())));
new PropValue("queueEntriesCount", queue.getActiveQueueEntries().size())));
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is needed for this to replicate the existing functionality, but is this what we want? Should the "getAll" resource limit itself to only active queue entries? Or should getAll really ...get all. And then we can limit this to only active entries by passing in a filter or search parameter.

Copy link
Member

@corneliouzbett corneliouzbett Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a weird resource. Just returns the counts. A response like:

{
  "QueueName": "Triage Queue",
  "count": "36"
}

The idea is to get the number of patients in a queue. Mainly used in the UI to display totals for queues. We don't about active or voided queueEntries.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... It doesn't seem like this resource is going to perform in an ideal way. I'd prefer it if we had one endpoint that returned "the current queue", i.e., queue name, all active entries, and counts. Then we could use the custom view code if someone really want to limit things down to just requesting, e.g., the name and count. This just encourages a pattern of access where we're making a lot of REST requests, which is bad for performance.

@mseaton In the context of a queue, I think it's fine if the default is to return active entries only. It seems reasonable to have a query property that be used to also request the queue with inactive entries, but I'm not clear on the use-case for that. This may be a little different from other domains, but I tend to think of inactive queue entries as similar to retired metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand the primary use case. A potentially more comprehensive endpoint might return an object that contains data that is a bit more complete and includes counts as the service/api deals with them, so it's clear. For example, if our "getActiveQueueEntries" filters out voided, started-in-future, and ended entries (which it does), then we could ensure that all of these are represented in this endpoint comprehensively like:

{
  "QueueName": "Triage Queue",
  "totalCount": "63",
  "activeCount": "36",
  "futureCount": "5",
  "voidedCount": "2",
  "endedCount": "20"
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I made this comment before I saw @ibacher response. Must have been typing at the same time. You can disregard my comment, especially if we get rid of this endpoint as Ian seems to be suggesting :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This may be a little different from other domains, but I tend to think of inactive queue entries as similar to retired metadata

I understand your point and I'm fine if the endpoint returns active entries by default. I just wanted to bring it up to make sure. As you indicate, we can always add support for query parameters like "includeEnded=true", "includeVoided=true", and "includeFuture=true" for those use cases if/when they arise.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, haven't dug into it in much detail but the use cases for get all "ended" queue members or a count of all of them seems relatively small. If I'm understand it correctly the "ended" queue entries will just continue to grow and grow indefinitely and the general need when asking for "all" would be "get me all currently in the queue". There might be a case to say, "get me all cases when patient X was in this queue in the past month" but "getAllEntires" seems about as useful as "getAllObs". (Again, If I'm understanding this all correctly).

}

@Override
Expand Down
2 changes: 1 addition & 1 deletion omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntrySubResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public void setParent(QueueEntry queueEntry, Queue queue) {

@Override
public PageableResult doGetAll(Queue queue, RequestContext requestContext) throws ResponseException {
Collection<QueueEntry> queueEntries = queue.getQueueEntries();
Collection<QueueEntry> queueEntries = queue.getActiveQueueEntries();
Copy link
Member Author

Choose a reason for hiding this comment

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

Same comment here as above. This change is needed for this to replicate the existing functionality, but is this what we want? Should the "getAll" resource limit itself to only active queue entries? Or should getAll really ...get all. And then we can limit this to only active entries by passing in a filter or search parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a use case for that already?
Yeah getAll should really get all queue entries and probably add more search parameters through search handlers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have an explicit use case yet. It was more just a matter of wanting to ensure our API was flexible enough to handle these use cases if/when they come up and also that it makes intuitive sense and matches what users would expect. Once we establish that this endpoint only returns active queue entries, if a use case does come up where we want all queue entries regardless of active or not, how would we handle that? Would we have to pass in a parameter saying "includeInactive=true" for example? Not the end of the world, just putting it out there for consideration as to how we'd want/expect our REST apis to work, and if there are existing conventions in OpenMRS that we honor those.

return new NeedsPaging<>(new ArrayList<>(queueEntries), requestContext);
}

Expand Down
Loading