(feat) O3-5578: Validate Queue Location tags in QueueValidator

[LxndrKrchvtsv]

Requirements

• This PR has a title that briefly describes the work done including
  the ticket number (feat) O3-5578 with conventional commit label.
• My work is based on the existing validation patterns used throughout the queue module.
• My work includes tests or is validated by existing tests.

Summary

Found a TODO: Check if the location is tagged as a Queue Location? - Then created new task and implemented it.

• Preserved the fundamental null check for the location object.
• Leveraged Context.getLocationService() to retrieve the "Queue Location" tag.
• Added logic to verify that the location's assigned tags specifically include the "Queue Location" tag.
• Covered the task with tests

Screenshots

N/A

Related Issue

https://openmrs.atlassian.net/browse/O3-5578

Other

N/A

[sanks011]

Hey @LxndrKrchvtsv , nice work picking up this TODO. A few things I wanted to flag after looking at the existing codebase:

1. Hardcoded string "Queue Location" - should this be a constant?
The tag name "Queue Location" appears in the liquibase changeset (liquibase.xml, changeset queue_202301161833) and now in your validator code. The rest of the module uses QueueModuleConstants for things like QUEUE_STATUS, QUEUE_PRIORITY, QUEUE_SERVICE. It would be more consistent to add something like public static final String QUEUE_LOCATION_TAG = "Queue Location"; there, instead of having the raw string in the validator. Makes it easier to find and change if it ever needs updating.

2. Double error on null location
When queue.getLocation() is null, the existing ValidationUtils.rejectIfEmptyOrWhitespace(errors, "location", ...) on
line 42 already rejects it. Then your new null check on the location field adds a second error with the same code queue.location.null. So you'd get errors.getFieldErrorCount("location") == 2 for a null location - your test actually asserts this (assertThat(errors.getFieldErrorCount("location"), equalTo(2))), but that seems like a bug rather than intended behavior. You probably want to skip your new block entirely if location is already null, since the rejectIfEmptyOrWhitespace already handles that case. Something like:

if (queue.getLocation() != null) {
    LocationTag queueLocationTag = Context.getLocationService().getLocationTagByName("Queue Location");
    if (queueLocationTag == null || !location.getTags().contains(queueLocationTag)) {
        errors.rejectValue("location", "queue.location.invalid", "Location should be tagged as a Queue Location");
    }
}

3. What happens when the "Queue Location" tag doesn't exist in the system at all?
If getLocationTagByName("Queue Location") returns null, your code rejects with queue.location.invalid. But this could also mean the liquibase migration hasn't run or the tag was accidentally deleted - which is a system configuration issue, not a problem with the queue being created. The error message says "Location should be tagged as a Queue Location" which would be confusing to the user in that scenario. Might be worth either logging a warning separately when queueLocationTag == null or using a different error code/message to distinguish "tag doesn't exist in the system" from "location isn't tagged."

4. Context.getLocationService() vs existing pattern
The existing code in QueueValidator uses Context.getRegisteredComponents(QueueServicesWrapper.class).get(0) to
access services. In QueueEntryValidator, it uses Context.getRegisteredComponent(...). Your PR introduces a new call to Context.getLocationService() directly. This works fine, but I just wanted to flag that QueueServicesWrapper already wraps LocationService - so you could also go through queueServices.getLocationService().getLocationTagByName(...) to stay
consistent with how other validators in this module access services. Not a blocker, just a consistency thing.

5. Tests look solid overall - good coverage of the null case, missing tag, wrong tag, and happy path. One minor thing: in the null location test, you're not mocking Context.getLocationService(), which works because your code returns early on null, but it does rely on the execution order. If someone reorganizes the code later, that test might start throwing NPEs from the unmocked getLocationService().

Overall this is a clean change that addresses a real gap. The main thing I'd want to see fixed is the double-error on null location (#2) - the rest are suggestions.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud