Skip to content

(fix) O3-5552: Reject future transition dates in queue entry transitions#106

Open
Brijesh-0106 wants to merge 4 commits into
openmrs:mainfrom
Brijesh-0106:O3-5552
Open

(fix) O3-5552: Reject future transition dates in queue entry transitions#106
Brijesh-0106 wants to merge 4 commits into
openmrs:mainfrom
Brijesh-0106:O3-5552

Conversation

@Brijesh-0106
Copy link
Copy Markdown

@Brijesh-0106 Brijesh-0106 commented Mar 29, 2026

Requirements

  • This PR has a title that briefly describes the work done including the ticket number with conventional commit label.
  • My work is based on the existing validation patterns used throughout the queue module.
  • My work includes tests. I have tested and it is working fine.

Summary

The queue-entry transition endpoint accepted future dates as valid transition dates. This allowed transitions to be recorded as happening in the future, making queue metrics and wait times unreliable for clinical staff.

This PR adds a server-side validation check that rejects any transition request where the provided transitionDate is after the current server time.

Related Issue

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

@Brijesh-0106 Brijesh-0106 changed the title (feat) O3-5552: Service Queue Transition-selecting future time (or changing AM/PM) allows transition and shows incorrect “waited for” time (e.g., -11 hours) (feat) O3-5552: validate transition time to prevent negative wait duration Mar 29, 2026
@Brijesh-0106
Copy link
Copy Markdown
Author

Brijesh-0106 commented Apr 9, 2026

Hey, @denniskigen as my frontend side PR is merged but as we discussed in 1 coffee break session that we need to put backend side validation as well so I have added backend side validation as well. So, @denniskigen Could you please take a look and merge it when you get a chance? Let me know if you need any additional context. 🙏

@denniskigen denniskigen requested review from ibacher and mseaton April 9, 2026 18:52
Copy link
Copy Markdown
Member

@mseaton mseaton left a comment

Choose a reason for hiding this comment

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

LGTM

@denniskigen
Copy link
Copy Markdown
Member

denniskigen commented Apr 9, 2026

@mseaton should this check live in the service layer in QueueEntryServiceImpl.transitionQueueEntry() alongside existing guards (voided check, already-ended check, optimistic locking) instead?

@denniskigen denniskigen changed the title (feat) O3-5552: validate transition time to prevent negative wait duration (fix) O3-5552: Reject future transition dates in queue entry transitions Apr 9, 2026
Comment on lines +68 to +70
if (transitionDate.after(new Date())) {
throw new APIException("Transition cannot be in the future");
}
Copy link
Copy Markdown
Member

@ibacher ibacher Apr 9, 2026

Choose a reason for hiding this comment

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

Two things:

  1. Ideally this would live in the validation (i.e., in the service layer), not in the REST layer
  2. Ideally, we'd have a little but of tolerance, e.g., new Date() + 1 minute or so to account for small clock drifts.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @denniskigen and @ibacher , thanks for the suggestions!

I’ve updated the implementation accordingly:

  • Moved the validation logic to the service layer

  • Added a small tolerance window (+1 minute) to handle potential clock drift

Could you please review the changes when you have a moment? If everything looks good, feel free to proceed with the merge.

Thanks again for the guidance!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd still think this belongs with the rest of the validations, i.e., as part of the QueueEntryValidator.

@ibacher
Copy link
Copy Markdown
Member

ibacher commented Apr 10, 2026

@Brijesh-0106 A test or two would be nice for this.

@Brijesh-0106
Copy link
Copy Markdown
Author

@Brijesh-0106 A test or two would be nice for this.

Added two unit tests in QueueEntryServiceTest:

  • shouldThrowWhenTransitionDateIsInTheFuture
  • shouldAllowTransitionDateWithinOneMinuteClockDriftTolerance
mvn test -pl api -Dtest=QueueEntryServiceTest#shouldThrowWhenTransitionDateIsInTheFuture+shouldAllowTransitionDateWithinOneMinuteClockDriftTolerance

Also verified manually via API calls on a local OpenMRS instance. Both pass as expected.

@Brijesh-0106 Brijesh-0106 requested a review from ibacher April 16, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants