O3-2861 - Queue Module - support default and configurable ordering of… #60
Conversation
… queue entries on a queue
| List<Concept> allowedPriorities = services.getAllowedPriorities(queueEntry.getQueue()); | ||
| int ret = 0; // Default to the lowest sort weight | ||
| if (queueEntry.getPriority() != null && allowedPriorities.contains(queueEntry.getPriority())) { | ||
| ret = allowedPriorities.indexOf(queueEntry.getPriority()); |
There was a problem hiding this comment.
This seems backwards of how I'd expect a concept set to be sorted, I would figure the element that was sorted/displayed at the top would have index 0, not index length - 1, or am I reading this wrong. (But not suggesting we change the way sort orders of concept sets work, just wanted to double-check this was correct!).
There was a problem hiding this comment.
You mean queues, not concept sets @mogoodrich ?
For the set itself, we have tended to have the lower priority priorities listed first, as the default is often to use the first priority in the list, and we don't want the default to be an emergency. And our PIH Priority Set has "Normal" then "Emergency".
For the queue, I tend to agree that a queue entries would more intuitively sort ascending. And this is how I originally started implementing it. But I changed it largely due to the fact that the existing / legacy queue ESM code had set "Emergency = 1" and "Normal = 0" in the database. So if we want to keep in line with that, then we need to sort our sort orders descending.
There was a problem hiding this comment.
I meant concept sets, I guess my instinct would be that when viewing a concept set, the higher priority would be listed first, though based on what you said, this isn't the case.
So, what we are saying, is that if someone wanted to have a concept set "Low, Medium, HIgh" they'd have to arrange it (in the old concept set UI) like this:
Low
Medium
High
That just seems a little counter-intuitive, but maybe that's the standard. The one example I found in CIEL does seem to follow that pattern:
https://app.openconceptlab.org/#/orgs/CIEL/sources/CIEL/concepts/161238/
There was a problem hiding this comment.
Either way, we should probably ensure this is documented somewhere.
There was a problem hiding this comment.
Thanks. See my follow-up commit to document this better in the README @ibacher
|
@ibacher - I'm looking to merge but want to make sure you have a chance to weigh in if you have objections to this approach. |
|
LGTM |
| List<Concept> allowedPriorities = services.getAllowedPriorities(queueEntry.getQueue()); | ||
| int ret = 0; // Default to the lowest sort weight | ||
| if (queueEntry.getPriority() != null && allowedPriorities.contains(queueEntry.getPriority())) { | ||
| ret = allowedPriorities.indexOf(queueEntry.getPriority()); |
There was a problem hiding this comment.
Either way, we should probably ensure this is documented somewhere.
… queue entries on a queue
For a complete description of this change, please see most recent comments here:
https://openmrs.atlassian.net/browse/O3-2861