AM-241 Enable Hibernate Envers auditing (upgrade platform to 2.8.3) - #202
Open
druchniewicz wants to merge 1 commit into
Open
AM-241 Enable Hibernate Envers auditing (upgrade platform to 2.8.3)#202druchniewicz wants to merge 1 commit into
druchniewicz wants to merge 1 commit into
Conversation
|
|
📝 WalkthroughWalkthroughThis PR replaces PowerMock-based static mocking with Mockito's MockedStatic/mockConstruction across api, atom-feed, and omod test suites, adds Hibernate Envers auditing annotations to appointment model classes, changes AppointmentDaoImpl search logic to use direct equality restrictions, wraps a global property read with proxy privileges, updates dependencies (mockito-inline, servlet API, OpenMRS platform version), and updates test datasets to use dedicated patient records. ChangesPowerMock removal, Envers auditing, and dependency upgrades
Estimated code review effort: 3 (Moderate) | ~40 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
@angshu @mohan-13 @mks-d @ShruthiPitta can you take a look at this PR? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/src/test/resources/appointmentTestData.xml (1)
93-102: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDuplicate
patient_appointment_id="14"overwrites the earlier row.executeDataSet()loads this fixture with REFRESH semantics, so the secondpatient_appointment_id="14"replaces thepatient_id="500"record. Renumber or remove one of the rows inapi/src/test/resources/appointmentTestData.xml:93-102.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/resources/appointmentTestData.xml` around lines 93 - 102, The fixture has a duplicate patient_appointment_id value, causing the later patient_appointment row to overwrite the earlier one when executeDataSet() loads the data. Update the patient_appointment entries in appointmentTestData.xml so each record has a unique patient_appointment_id, or remove the redundant row, and keep the identifiers in the patient_appointment dataset consistent to preserve both records.api/src/test/java/org/openmrs/module/appointments/service/impl/AppointmentsServiceImplTest.java (1)
559-573: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerification silently dropped after PowerMock migration.
Line 572's bare
Context.hasPrivilege(RESET_APPOINTMENT_STATUS);used to work as a PowerMockverifyStatic()follow-up call; withmockStatic, it just invokes the stub and discards the result — it verifies nothing. The test still passes, but the intended check thathasPrivilegewas called is now a no-op.🔧 Proposed fix
} finally { verify(messageSourceService).getMessage(exceptionCode, new Object[]{RESET_APPOINTMENT_STATUS}, null); - Context.hasPrivilege(RESET_APPOINTMENT_STATUS); + mockedContext.verify(() -> Context.hasPrivilege(RESET_APPOINTMENT_STATUS)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/org/openmrs/module/appointments/service/impl/AppointmentsServiceImplTest.java` around lines 559 - 573, The static privilege check in AppointmentsServiceImplTest is no longer being verified after the PowerMock migration because the bare Context.hasPrivilege(RESET_APPOINTMENT_STATUS) call is just invoking the stub. Update the test around appointmentsService.changeStatus to use the appropriate static verification for Context.hasPrivilege, keeping the existing expectedException and messageSourceService assertions intact, so the test actually confirms the privilege check occurred.
🧹 Nitpick comments (2)
api/src/main/java/org/openmrs/module/appointments/dao/impl/AppointmentDaoImpl.java (1)
92-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding DAO-level coverage for the
providerrestriction branch.
shouldSearchAppointmentsForAPatient(the only IT test for this method) only exercises patient/location/service; the newproviderequality branch has no direct test coverage at the DAO layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/main/java/org/openmrs/module/appointments/dao/impl/AppointmentDaoImpl.java` around lines 92 - 93, Add DAO-level test coverage for the new provider filter branch in AppointmentDaoImpl by exercising the appointment search path that builds the Criteria with Restrictions.eq("provider", appointment.getProvider()). Extend the existing DAO test suite around shouldSearchAppointmentsForAPatient or add a focused test in AppointmentDaoImpl to verify that when an Appointment has a non-null provider, the DAO includes the provider restriction and returns only matching appointments.api/src/test/java/org/openmrs/module/appointments/service/impl/DefaultTeleconsultationAppointmentPatientEmailNotifierTest.java (1)
51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace deprecated
MockitoAnnotations.initMocks.
initMocksis deprecated in favor ofopenMocks, which returns anAutoCloseablefor cleaner resource management. Since sibling tests in this PR already migrated to@RunWith(MockitoJUnitRunner.Silent.class), consider aligning this test with that pattern for consistency.♻️ Suggested fix
- `@Before` - public void init() { - MockitoAnnotations.initMocks(this); - mockedContext = mockStatic(Context.class); + private AutoCloseable mocks; + + `@Before` + public void init() { + mocks = MockitoAnnotations.openMocks(this); + mockedContext = mockStatic(Context.class);And close
mocksalongsidemockedContextintearDown().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/org/openmrs/module/appointments/service/impl/DefaultTeleconsultationAppointmentPatientEmailNotifierTest.java` around lines 51 - 53, Replace the deprecated MockitoAnnotations.initMocks usage in DefaultTeleconsultationAppointmentPatientEmailNotifierTest with openMocks or align the test with the same MockitoJUnitRunner.Silent pattern used by sibling tests. Update the test setup in init() to manage the returned AutoCloseable, and close that resource in tearDown() together with mockedContext to keep mocking cleanup consistent and avoid resource leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@omod/src/test/resources/appointmentServicesTestData.xml`:
- Around line 67-68: The appointmentServicesTestData fixture has a duplicate
primary key in the patient_appointment entries: both records in the
patient_appointment elements use the same patient_appointment_id. Update the
second patient_appointment row to a unique ID while keeping the rest of the
record unchanged so the test data loads without key collisions.
---
Outside diff comments:
In
`@api/src/test/java/org/openmrs/module/appointments/service/impl/AppointmentsServiceImplTest.java`:
- Around line 559-573: The static privilege check in AppointmentsServiceImplTest
is no longer being verified after the PowerMock migration because the bare
Context.hasPrivilege(RESET_APPOINTMENT_STATUS) call is just invoking the stub.
Update the test around appointmentsService.changeStatus to use the appropriate
static verification for Context.hasPrivilege, keeping the existing
expectedException and messageSourceService assertions intact, so the test
actually confirms the privilege check occurred.
In `@api/src/test/resources/appointmentTestData.xml`:
- Around line 93-102: The fixture has a duplicate patient_appointment_id value,
causing the later patient_appointment row to overwrite the earlier one when
executeDataSet() loads the data. Update the patient_appointment entries in
appointmentTestData.xml so each record has a unique patient_appointment_id, or
remove the redundant row, and keep the identifiers in the patient_appointment
dataset consistent to preserve both records.
---
Nitpick comments:
In
`@api/src/main/java/org/openmrs/module/appointments/dao/impl/AppointmentDaoImpl.java`:
- Around line 92-93: Add DAO-level test coverage for the new provider filter
branch in AppointmentDaoImpl by exercising the appointment search path that
builds the Criteria with Restrictions.eq("provider", appointment.getProvider()).
Extend the existing DAO test suite around shouldSearchAppointmentsForAPatient or
add a focused test in AppointmentDaoImpl to verify that when an Appointment has
a non-null provider, the DAO includes the provider restriction and returns only
matching appointments.
In
`@api/src/test/java/org/openmrs/module/appointments/service/impl/DefaultTeleconsultationAppointmentPatientEmailNotifierTest.java`:
- Around line 51-53: Replace the deprecated MockitoAnnotations.initMocks usage
in DefaultTeleconsultationAppointmentPatientEmailNotifierTest with openMocks or
align the test with the same MockitoJUnitRunner.Silent pattern used by sibling
tests. Update the test setup in init() to manage the returned AutoCloseable, and
close that resource in tearDown() together with mockedContext to keep mocking
cleanup consistent and avoid resource leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4bda721-5042-4d4a-bda9-2de911dbfffd
📒 Files selected for processing (51)
api/pom.xmlapi/src/main/java/org/openmrs/module/appointments/dao/impl/AppointmentDaoImpl.javaapi/src/main/java/org/openmrs/module/appointments/model/Appointment.javaapi/src/main/java/org/openmrs/module/appointments/model/AppointmentProvider.javaapi/src/main/java/org/openmrs/module/appointments/model/AppointmentReason.javaapi/src/main/java/org/openmrs/module/appointments/model/AppointmentRecurringPattern.javaapi/src/main/java/org/openmrs/module/appointments/model/AppointmentServiceAttribute.javaapi/src/main/java/org/openmrs/module/appointments/model/AppointmentServiceAttributeType.javaapi/src/main/java/org/openmrs/module/appointments/model/AppointmentServiceDefinition.javaapi/src/main/java/org/openmrs/module/appointments/model/AppointmentServiceType.javaapi/src/main/java/org/openmrs/module/appointments/model/AppointmentUnavailability.javaapi/src/main/java/org/openmrs/module/appointments/model/ServiceWeeklyAvailability.javaapi/src/main/java/org/openmrs/module/appointments/model/Speciality.javaapi/src/main/java/org/openmrs/module/appointments/validator/impl/DefaultAppointmentStatusChangeValidator.javaapi/src/test/java/org/openmrs/module/appointments/AppointmentsActivatorTest.javaapi/src/test/java/org/openmrs/module/appointments/dao/impl/AppointmentDaoImplIT.javaapi/src/test/java/org/openmrs/module/appointments/model/AppointmentRecurringPatternTest.javaapi/src/test/java/org/openmrs/module/appointments/notification/impl/DefaultMailSenderTest.javaapi/src/test/java/org/openmrs/module/appointments/scheduler/tasks/MarkAppointmentAsCompleteTaskTest.javaapi/src/test/java/org/openmrs/module/appointments/scheduler/tasks/MarkAppointmentAsMissedTaskTest.javaapi/src/test/java/org/openmrs/module/appointments/service/impl/AppointmentServiceAttributeTypeServiceImplTest.javaapi/src/test/java/org/openmrs/module/appointments/service/impl/AppointmentServiceDefinitionServiceImplTest.javaapi/src/test/java/org/openmrs/module/appointments/service/impl/AppointmentUnavailabilityServiceImplTest.javaapi/src/test/java/org/openmrs/module/appointments/service/impl/AppointmentsServiceImplTest.javaapi/src/test/java/org/openmrs/module/appointments/service/impl/DefaultTeleconsultationAppointmentPatientEmailNotifierTest.javaapi/src/test/java/org/openmrs/module/appointments/service/impl/PatientAppointmentNotifierServiceTest.javaapi/src/test/java/org/openmrs/module/appointments/service/impl/SpecialityServiceImplTest.javaapi/src/test/java/org/openmrs/module/appointments/service/impl/TeleconsultationAppointmentServiceTest.javaapi/src/test/java/org/openmrs/module/appointments/validator/impl/DefaultAppointmentStatusChangeValidatorTest.javaapi/src/test/java/org/openmrs/module/appointments/validator/impl/DefaultAppointmentValidatorTest.javaapi/src/test/resources/appointmentTestData.xmlatom-feed/pom.xmlatom-feed/src/test/java/org/openmrs/module/appointments/advice/AppointmentAdviceTest.javaatom-feed/src/test/java/org/openmrs/module/appointments/advice/AppointmentServiceDefinitionAdviceTest.javaatom-feed/src/test/java/org/openmrs/module/appointments/advice/AtomFeedAdviceActivatorComponentTest.javaatom-feed/src/test/java/org/openmrs/module/appointments/advice/RecurringAppointmentsAdviceTest.javaomod/pom.xmlomod/src/test/java/org/openmrs/module/appointments/web/controller/AdhocTeleconsultationControllerTest.javaomod/src/test/java/org/openmrs/module/appointments/web/controller/AppointmentControllerTest.javaomod/src/test/java/org/openmrs/module/appointments/web/controller/AppointmentServiceDefinitionControllerTest.javaomod/src/test/java/org/openmrs/module/appointments/web/controller/AppointmentServicesControllerTest.javaomod/src/test/java/org/openmrs/module/appointments/web/controller/AppointmentsControllerTest.javaomod/src/test/java/org/openmrs/module/appointments/web/controller/RecurringAppointmentsControllerTest.javaomod/src/test/java/org/openmrs/module/appointments/web/controller/SpecialityControllerTest.javaomod/src/test/java/org/openmrs/module/appointments/web/mapper/AppointmentMapperTest.javaomod/src/test/java/org/openmrs/module/appointments/web/mapper/AppointmentServiceDefinitionMapperTest.javaomod/src/test/java/org/openmrs/module/appointments/web/service/impl/AllAppointmentRecurringPatternUpdateServiceTest.javaomod/src/test/java/org/openmrs/module/appointments/web/service/impl/SingleAppointmentRecurringPatternUpdateServiceTest.javaomod/src/test/resources/appointmentServicesTestData.xmlomod/src/test/resources/appointmentTestData.xmlpom.xml
💤 Files with no reviewable changes (4)
- api/src/test/java/org/openmrs/module/appointments/notification/impl/DefaultMailSenderTest.java
- api/src/test/java/org/openmrs/module/appointments/service/impl/PatientAppointmentNotifierServiceTest.java
- omod/src/test/java/org/openmrs/module/appointments/web/service/impl/AllAppointmentRecurringPatternUpdateServiceTest.java
- api/src/test/java/org/openmrs/module/appointments/service/impl/SpecialityServiceImplTest.java
Comment on lines
+67
to
+68
| <patient_appointment patient_appointment_id="16" patient_id ="501" appointment_service_id="4" start_date_time="2108-08-15 13:00:00.0" end_date_time="2108-08-15 14:00:00.0" status="Cancelled" creator="1" date_created="2108-08-10 15:57:09.0" voided="false" void_reason="" uuid="75504r42-3ca8-11e3-bf2b-0800271c13355"/> | ||
| <patient_appointment patient_appointment_id="16" patient_id ="501" appointment_service_id="4" start_date_time="2108-08-15 13:00:00.0" end_date_time="2108-08-15 14:00:00.0" status="Missed" creator="1" date_created="2108-08-10 15:57:09.0" voided="false" void_reason="" uuid="75504r42-3ca8-11e3-bf2b-0800271b13355"/> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'patient_appointment_id="1[0-9]"' omod/src/test/resources/appointmentServicesTestData.xmlRepository: Bahmni/openmrs-module-appointments
Length of output: 1787
🏁 Script executed:
#!/bin/bash
rg -n --hidden --glob '!**/target/**' --glob '!**/build/**' 'patient_appointment(_id)?|create table .*patient_appointment|PRIMARY KEY.*patient_appointment' omod srcRepository: Bahmni/openmrs-module-appointments
Length of output: 14871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate schema / mapping / entity definitions for patient_appointment
fd -a -H -t f '.*(sql|xml|hbm\.xml|java|gsp|md)$' . \
| rg 'patient_appointment|Appointment' \
| sed -n '1,200p'
printf '\n---\n'
# Search for direct references to the ID column / primary key
rg -n -S 'patient_appointment_id|patient_appointment\b|PRIMARY KEY|unique' omod/src/main src/main . \
--glob '!**/target/**' --glob '!**/build/**' \
| sed -n '1,240p'Repository: Bahmni/openmrs-module-appointments
Length of output: 50390
Duplicate patient_appointment_id in this fixture — lines 67-68 both use patient_appointment_id="16", but patient_appointment_id is the primary key on patient_appointment, so this will collide on load. Renumber the second row (for example to 17).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@omod/src/test/resources/appointmentServicesTestData.xml` around lines 67 -
68, The appointmentServicesTestData fixture has a duplicate primary key in the
patient_appointment entries: both records in the patient_appointment elements
use the same patient_appointment_id. Update the second patient_appointment row
to a unique ID while keeping the rest of the record unchanged so the test data
loads without key collisions.
Member
|
Hello @druchniewicz Thanks for your PR. A couple of things.
|
Author
hi @mohan-13
|
Member
Alright. Thanks for responding. Will take a look over the weekend |
Author
|
@mohan-13 will you be able to take a look at it? |
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket - https://openmrs.atlassian.net/browse/AM-241
We want to enable Hibernate Envers auditing in this module to make appointment tables auditable. Apart from marking appointment tables as @ Audited we need to make appointments module compatible with the newer versions of OpenMRS first (it requires OMRS Core 2.7+)
This PR includes changes for making appointments module compatible with OpenMRS 2.8.3 and supporting Hibernate envers auditing. After these changes we can see appointments tables in Audit Logs tables and we can track their history:
Changes made: