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

[IamMujuziMoses]

Issue I worked on

see https://issues.openmrs.org/browse/O3-2445

[mherman22]

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

[IamMujuziMoses]

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

[mherman22]

both

[IamMujuziMoses]

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

[mherman22]

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

[mherman22]

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

[IamMujuziMoses]

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

[IamMujuziMoses]

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

[mherman22]

Yeah sure 🙂