BAH-4464 Fix for Recurring Appointments Generate Inconsistent Appoint… by lingeswaranTW · Pull Request #188 · Bahmni/openmrs-module-appointments

lingeswaranTW · 2026-02-20T07:10:03Z

…ment Numbers Across Occurrences

lingeswaranTW

Review for BAH-4464

Clean, well-scoped fix. All recurring appointment occurrences now correctly share the same appointment number. Production code change is minimal and surgical, test coverage is comprehensive.

No blocking issues found.

Acceptance Criteria: 3/3 met

AC	Status
All recurring instances share identical appointment number	✅
Single appointments generate unique numbers as before	✅
Retrieved series show consistent numbers	✅

What's done well:

Smart reuse of existing appointment numbers when adding to an existing recurring series
4 unit tests + 3 integration tests covering key scenarios
Only 1 production file changed — surgical fix with minimal blast radius

lingeswaranTW · 2026-02-20T09:56:00Z

QA Readiness Report: PR #188

PR: BAH-4464 Fix for Recurring Appointments Generate Inconsistent Appointment Numbers Across Occurrences
URL: #188
Author: lingeswaranTW
Branch: BAH-4464 → master
Checked: 2026-02-20
JIRA: BAH-4464
Build Status: ✅ SUCCESS (mvn clean package)
Report Updated: 2026-02-20 (All recommended improvements completed)

QA Readiness Status

✅ READY FOR QA - ALL IMPROVEMENTS COMPLETED

Test coverage meets QA readiness standards. The core bug fix is thoroughly tested at both unit and integration levels with 4 new unit tests and 3 new integration tests. All recommended improvements have been implemented, including database persistence verification in integration tests.

Improvements Completed

✅ Added shared-number contract assertion in update() overload test
✅ Added assertions to verify pre-existing appointments retain their numbers
✅ Empty appointment list edge case test implemented
✅ Added database re-fetch verification to integration test for persistence validation

Test Coverage Summary

Test Category	Status	Details
Unit Tests	✅ Good	4 new test cases in `AppointmentRecurringPatternServiceImplTest.java`
Snapshot Tests	N/A	Backend Java service — no UI components
Accessibility Tests	N/A	Backend Java service — no UI components
Integration Tests	✅ Good	3 new test cases in `RecurringAppointmentsControllerIT.java`

Test Pyramid Analysis

Current PR Test Distribution:

Unit Tests:        [##########] 57%  (4 tests)
Integration Tests: [########--] 43%  (3 tests)
E2E Tests:         [----------]  0%  (0 tests)

Pyramid Status: ✅ Healthy

The test pyramid is healthy. Unit tests form the base with focused, fast tests covering the core getOrGenerateSharedAppointmentNumber() logic. Integration tests validate the same behavior through the full REST API stack (Controller → Service → DAO → DB). No E2E tests are needed — the integration tests already exercise the full HTTP stack with real in-memory databases.

Findings by Severity

Severity	Count
🚫 BLOCKER	0
🔴 CRITICAL	0
🟠 MAJOR	0
🟡 MINOR	0
🔵 INFO	3

Note: All 4 MINOR issues identified have been addressed and fixed. See details below.

🚫 BLOCKER Issues

None

🔴 CRITICAL Issues

None

🟠 MAJOR Issues

None

🟡 MINOR Issues (All Resolved)

🔵 INFO

5. Generator exception propagation not tested

Category: Unit
Source File: api/src/main/java/org/openmrs/module/appointments/service/impl/AppointmentRecurringPatternServiceImpl.java

The call to appointmentNumberGenerator.generateAppointmentNumber() is not wrapped in try-catch. If the generator throws a RuntimeException, it propagates unchecked. No test verifies this behavior. This is acceptable as-is since the caller can handle exceptions at a higher level.

6. Weekly integration test uses loose assertion

Category: Integration
Source File: omod/src/test/java/org/openmrs/module/appointments/web/controller/RecurringAppointmentsControllerIT.java

shouldAssignSameAppointmentNumberToWeeklyRecurringAppointments uses assertTrue(appointmentResponses.size() > 1) instead of an exact count assertion. Consider using assertEquals(expectedCount, ...) for deterministic verification.

7. AC #2 (non-recurring appointments) relates to unchanged code

Category: Integration
Source File: N/A (code not changed by this PR)

JIRA AC #2 states "Non-recurring appointment creation: A unique appointment number is generated." This PR only changes AppointmentRecurringPatternServiceImpl, not the non-recurring path in AppointmentsServiceImpl. No new test was added for this AC, but the existing test coverage for non-recurring appointments remains valid since that code path was not modified.

Acceptance Criteria Coverage

Criterion	Tested	Notes
AC #1: Recurring appointment creation → all occurrences same number	✅	Unit: `shouldAssignSameAppointmentNumberToAllNewRecurringAppointments`. IT: `shouldAssignSameAppointmentNumberToDailyRecurringAppointments`, `shouldAssignSameAppointmentNumberToWeeklyRecurringAppointments`
AC #2: Non-recurring appointment → unique number generated	⚠️	Code path not changed by this PR. Existing tests cover unchanged behavior. No new test added (not required).
AC #3: Data validation → all occurrences identical number	✅	Unit: same as AC #1 plus `shouldReuseExistingAppointmentNumberWhenAddingAppointmentsToExistingSeries`, `shouldPreserveSingleAppointmentNumberWhenAlreadyAssigned`. IT: `shouldPreserveAppointmentNumberWhenIncreasingFrequency`

Checklist for Developers

All recommended improvements have been implemented:

Add assertions in shouldReuseExistingAppointmentNumberWhenAddingAppointmentsToExistingSeries to verify pre-existing appointments retain their numbers
Add assertEquals for voided and new appointment numbers in shouldAddAuditAppointmentNumberToOnlyUpdatedAppointments
Consider adding empty appointment list edge case test
Re-fetch from DB in at least one integration test to verify persistence

What's Done Well

Thorough unit test coverage: 4 new unit tests cover all key branches of getOrGenerateSharedAppointmentNumber() — new series, existing series extension, single appointment preservation, and null generator handling.
Strong integration test coverage: 3 new integration tests exercise the fix through the full HTTP stack with both daily and weekly recurring patterns, plus the update flow.
Proper mock isolation: Unit tests correctly mock all external dependencies (AppointmentNumberGenerator, AppointmentServiceHelper, AppointmentRecurringPatternDao) ensuring true unit-level isolation.
Generator call verification: Tests use verify(generator, times(1)) and verify(generator, never()) to confirm the number is generated exactly once and reused — not generated per-appointment (the root cause of the original bug).
Both create and update flows tested: Integration tests cover both POST (new recurring series) and PUT (updating existing series) endpoints, ensuring the fix works for both creation and modification scenarios.

…ment Numbers Across Occurrences

github-advanced-security · 2026-02-26T11:00:32Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

sumaztwcode

Review for BAH-4464

Good fix for the recurring appointment number inconsistency! The functional implementation is correct and the test coverage is thorough.

Found 0 blocking issues, 1 suggestion, and 2 nits.

Acceptance Criteria: 3/3 met

sumaztwcode · 2026-02-26T12:44:22Z

api/src/main/java/org/openmrs/module/appointments/service/AppointmentNumberGenerator.java

+     * Default behavior: generate one number and apply to all (shared number strategy).*
+     * @param appointments List of appointments to apply numbers to. Null or empty lists are ignored.
+     */
+    default void applyAppointmentNumbers(@NotNull List<Appointment> appointments) {


suggestion: This default method places recurring-series batch policy (generate once, share across all) directly in the single-appointment SPI interface. Two concerns:

Spring AOP proxy limitation: The interface is annotated @Transactional, but Spring's JDK dynamic proxies do not intercept default interface methods — they're invoked directly, bypassing the proxy chain. So @Transactional has no runtime effect for this method. If a custom generator's generateAppointmentNumber() interacts with the database (e.g., sequence-based), it would execute outside the active transaction.

Separation of concerns: The pre-existing checkAndAssignAppointmentNumber lived entirely in AppointmentRecurringPatternServiceImpl — the service that understands recurring patterns owned the assignment policy. Moving this policy into the generator interface conflates generation strategy (SPI concern) with distribution policy (service concern).

Consider keeping this logic in AppointmentRecurringPatternServiceImpl as a private helper instead — call generateAppointmentNumber(appointments.get(0)) directly from the service and distribute the result. This keeps everything within the service's @Transactional boundary and preserves the interface as a pure single-appointment generation strategy.

Revamped the changes

sumaztwcode · 2026-02-26T12:44:22Z

...org/openmrs/module/appointments/service/impl/AppointmentRecurringPatternServiceImplTest.java

+        assertNull(appointmentTwo.getAppointmentNumber());
+    }
+
+    @Test(expected = IndexOutOfBoundsException.class)


nit: This codifies IndexOutOfBoundsException (a leaked implementation detail from appointments.get(0) in validateAndSave) as expected behavior. Consider adding an explicit guard at the top of validateAndSave with a more descriptive exception:

if (appointments.isEmpty()) { throw new APIException("Cannot save a recurring pattern with no appointments"); }

Then update this test to expect APIException instead.

sumaztwcode · 2026-02-26T12:44:22Z

api/src/main/java/org/openmrs/module/appointments/service/AppointmentNumberGenerator.java

+     * @param appointments List of appointments to apply numbers to. Null or empty lists are ignored.
+     */
+    default void applyAppointmentNumbers(@NotNull List<Appointment> appointments) {
+        if (appointments == null || appointments.isEmpty()) {


nit: The @NotNull annotation on the parameter declares that null is a contract violation, but this line silently accepts null. These send contradictory signals. For consistency with generateAppointmentNumber(@NotNull Appointment) on the same interface (which has no internal null check), consider removing the null branch and keeping only appointments.isEmpty().

sumaztwcode · 2026-02-26T12:44:22Z

api/src/main/java/org/openmrs/module/appointments/service/AppointmentNumberGenerator.java

+            return;
+        }
+
+        String sharedNumber = generateAppointmentNumber(appointments.get(0));


question: The appointments list is constructed from new ArrayList<>(appointmentRecurringPattern.getAppointments()) where getAppointments() returns a HashSet with no guaranteed iteration order. So get(0) selects a non-deterministic appointment.

For the current DefaultAppointmentNumberGeneratorImpl (timestamp-based, ignores the appointment parameter) this is harmless. But if a custom generator uses appointment-specific properties to influence the number, the result would vary across runs. Worth documenting this assumption?

Pre-existing issue so we can ignore this for now.Its a valid one it cannot guarantee order.

sonarqubecloud · 2026-02-27T07:42:55Z

Quality Gate passed

Issues
5 New issues
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

lingeswaranTW commented Feb 20, 2026

View reviewed changes

lingeswaranTW force-pushed the BAH-4464 branch from 37bd308 to 1a055fb Compare February 20, 2026 09:23

BAH-4464 Fix for Recurring Appointments Generate Inconsistent Appoint…

7e0346c

…ment Numbers Across Occurrences

lingeswaranTW force-pushed the BAH-4464 branch from 1a055fb to 7e0346c Compare February 20, 2026 09:57

BAH-4464 changed into default method for extensibility

031e87d

lingeswaranTW force-pushed the BAH-4464 branch from dd65e4f to 031e87d Compare February 26, 2026 11:03

lingeswaranTW requested review from angshu, mohan-13, rahu1ramesh, sumanmaity112 and sumaztwcode and removed request for sumanmaity112 February 26, 2026 11:04

sumaztwcode reviewed Feb 26, 2026

View reviewed changes

lingeswaranTW force-pushed the BAH-4464 branch 2 times, most recently from a823554 to 7be714c Compare February 27, 2026 07:34

BAH-4464 refactored code

031bef2

lingeswaranTW force-pushed the BAH-4464 branch from 7be714c to 031bef2 Compare February 27, 2026 07:42

Conversation

lingeswaranTW commented Feb 20, 2026

Uh oh!

lingeswaranTW left a comment

Choose a reason for hiding this comment

Review for BAH-4464

Uh oh!

lingeswaranTW commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

QA Readiness Report: PR #188

QA Readiness Status

✅ READY FOR QA - ALL IMPROVEMENTS COMPLETED

Improvements Completed

Test Coverage Summary

Test Pyramid Analysis

Findings by Severity

🚫 BLOCKER Issues

🔴 CRITICAL Issues

🟠 MAJOR Issues

🟡 MINOR Issues (All Resolved)

🔵 INFO

5. Generator exception propagation not tested

6. Weekly integration test uses loose assertion

7. AC #2 (non-recurring appointments) relates to unchanged code

Acceptance Criteria Coverage

Checklist for Developers

What's Done Well

Uh oh!

github-advanced-security bot commented Feb 26, 2026

Uh oh!

sumaztwcode left a comment

Choose a reason for hiding this comment

Review for BAH-4464

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 27, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lingeswaranTW commented Feb 20, 2026 •

edited

Loading