-
Notifications
You must be signed in to change notification settings - Fork 23
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.
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-2445: Queue Module - allow nullable priority and status on queue entry. #66
base: main
Are you sure you want to change the base?
Conversation
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())) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()
?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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🙂
There was a problem hiding this comment.
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:
validate
is a void method andreturn null;
won't work, maybe justreturn
.- if
queueEntry.getStatus() == null
passes, thenqueueEntry.getPriority()
will never be validated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure 🙂
Issue I worked on
see https://issues.openmrs.org/browse/O3-2445