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

O3-2445: Queue Module - allow nullable priority and status on queue entry. #66

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

O3-2445: Queue Module - allow nullable priority and status on queue entry. #66

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,14 @@ public class QueueEntry extends BaseChangeableOpenmrsData {
private Visit visit;

@ManyToOne
@JoinColumn(name = "priority", referencedColumnName = "concept_id", nullable = false)
@JoinColumn(name = "priority", referencedColumnName = "concept_id")
private Concept priority;

@Column(name = "priority_comment")
private String priorityComment;

@ManyToOne
@JoinColumn(name = "status", referencedColumnName = "concept_id", nullable = false)
@JoinColumn(name = "status", referencedColumnName = "concept_id")
private Concept status;

// Provides a means to indicate the relative order within a queue. Higher weight reflects higher priority.
Expand Down
21 changes: 7 additions & 14 deletions api/src/main/java/org/openmrs/module/queue/validators/QueueEntryValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,14 @@ public void validate(Object target, Errors errors) {
}

QueueServicesWrapper queueServices = Context.getRegisteredComponents(QueueServicesWrapper.class).get(0);
if (queueEntry.getStatus() == null) {
errors.rejectValue("status", "queueEntry.status.null", "The property status should not be null");
} else {
if (!queueServices.getAllowedStatuses(queue).contains(queueEntry.getStatus())) {
errors.rejectValue("status", "queueEntry.status.invalid",
"The property status should be a member of configured queue status conceptSet.");
}
if (queueEntry.getStatus() != null && !queueServices.getAllowedStatuses(queue).contains(queueEntry.getStatus())) {
Copy link

Choose a reason for hiding this comment

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

in all these cases, shouldn't we return early in-case of an npe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mherman22 thanks for the review but I need more light on that question, npe from queueServices.getAllowedStatuses() or from queueEntry.getStatus()?!

Copy link

Choose a reason for hiding this comment

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

both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

queueEntry is null checked at the beginning of the validator, maybe queueServices which I doubt can be null.

Copy link

Choose a reason for hiding this comment

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

lemi elaborate what i mean. you are making the change below.

		if (queueEntry.getStatus() != null && !queueServices.getAllowedStatuses(queue).contains(queueEntry.getStatus())) {
			errors.rejectValue("status", "queueEntry.status.invalid",
			    "The property status should be a member of configured queue status conceptSet.");
		}
		if (queueEntry.getPriority() != null
		        && !queueServices.getAllowedPriorities(queue).contains(queueEntry.getPriority())) {
			errors.rejectValue("priority", "queueEntry.priority.invalid",
			    "The property priority should be a member of configured queue priority conceptSet.");
		}

which is okay but why not return early which could be something like:

if (queueEntry.getStatus() == null) {
    return null; // Early return if status is null
}

if (!queueServices.getAllowedStatuses(queue).contains(queueEntry.getStatus())) {
    // your implementation
}

if (queueEntry.getPriority() == null) {
    return null; // Early return if priority is null
}

if (!queueServices.getAllowedPriorities(queue).contains(queueEntry.getPriority())) {
    // your implementation
}

Returning early

Copy link

Choose a reason for hiding this comment

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

just like it was done in the previous implementation though it instead rejected null values, you could just remove that and instead return null since we are allowing nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mherman22 thanx, now I get it, let me make the changes🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mherman22 I think early return is not applicable in this situation because:

  1. validate is a void method and return null; won't work, maybe just return.
  2. if queueEntry.getStatus() == null passes, then queueEntry.getPriority() will never be validated.

Copy link

Choose a reason for hiding this comment

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

Yeah sure 🙂

errors.rejectValue("status", "queueEntry.status.invalid",
"The property status should be a member of configured queue status conceptSet.");
}
if (queueEntry.getPriority() == null) {
errors.rejectValue("priority", "queueEntry.priority.null", "The property priority should not be null");
} else {
if (!queueServices.getAllowedPriorities(queue).contains(queueEntry.getPriority())) {
errors.rejectValue("priority", "queueEntry.priority.invalid",
"The property priority should be a member of configured queue priority conceptSet.");
}
if (queueEntry.getPriority() != null
&& !queueServices.getAllowedPriorities(queue).contains(queueEntry.getPriority())) {
errors.rejectValue("priority", "queueEntry.priority.invalid",
"The property priority should be a member of configured queue priority conceptSet.");
}
}
}
19 changes: 13 additions & 6 deletions api/src/main/resources/liquibase.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,9 @@
<column name="patient_id" type="int">
<constraints nullable="false"/>
</column>
<column name="priority" type="int">
<constraints nullable="false"/>
</column>
<column name="priority" type="int"/>
<column name="priority_comment" type="varchar(255)"/>
<column name="status" type="int">
<constraints nullable="false"/>
</column>
<column name="status" type="int"/>
<column name="sort_weight" type="double"/>
<column name="location_waiting_for" type="int"/>
<column name="provider_waiting_for" type="int"/>
Expand Down Expand Up @@ -460,4 +456,15 @@
</sql>
</changeSet>

<changeSet id="nullable_priority_and_status_on_queue_entry_20240329" author="mujuzi">
<preConditions onFail="MARK_RAN">
<columnExists tableName="queue_entry" columnName="priority"/>
<columnExists tableName="queue_entry" columnName="status"/>
</preConditions>
<comment>
Drop Not-Null constraint from columns queue_entry.priority and queue_entry.status
</comment>
<dropNotNullConstraint tableName="queue_entry" columnName="priority" columnDataType="int"/>
<dropNotNullConstraint tableName="queue_entry" columnName="status" columnDataType="int"/>
</changeSet>
</databaseChangeLog>
12 changes: 12 additions & 0 deletions ...tion-tests/src/test/java/org/openmrs/module/queue/validators/QueueEntryValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import org.apache.commons.lang.time.DateUtils;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.openmrs.Concept;
import org.openmrs.Visit;
Expand Down Expand Up @@ -75,6 +76,7 @@ public void shouldSupportQueueEntry() {
}

@Test
@Ignore("O3-2445: Allow nullable priority and status on queue entry")
public void shouldRejectQueueEntryWithNullStatus() {
validator.validate(queueEntry, errors);

Expand All @@ -85,6 +87,16 @@ public void shouldRejectQueueEntryWithNullStatus() {
assertThat(queueEntryStatusFieldError.getDefaultMessage(), is("The property status should not be null"));
}

@Test
public void shouldNotRejectQueueEntryWithNullStatusAndPriority() {
validator.validate(queueEntry, errors);

FieldError queueEntryStatusFieldError = errors.getFieldError("status");
FieldError queueEntryPriorityFieldError = errors.getFieldError("priority");
assertNull(queueEntryStatusFieldError);
assertNull(queueEntryPriorityFieldError);
}

@Test
public void shouldNotRejectQueueEntryWithValidConceptStatus() {
Concept validStatusConcept = Context.getConceptService().getConceptByUuid(VALID_STATUS_CONCEPT);
Expand Down