Strengthened Test Coverage for Organization Service - #4131
Conversation
Signed-off-by: Atwijukire Ariho Seth <120330466+Ariho-Seth@users.noreply.github.com>
Signed-off-by: Atwijukire Ariho Seth <120330466+Ariho-Seth@users.noreply.github.com>
Signed-off-by: Atwijukire Ariho Seth <120330466+Ariho-Seth@users.noreply.github.com>
Signed-off-by: Atwijukire Ariho Seth <120330466+Ariho-Seth@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves reliability and regression resistance in the organization module by expanding automated test coverage for organization-related services and hardening organization hierarchy initialization against cyclic parent relationships.
Changes:
- Added/expanded JUnit 4 tests for
OrganizationServiceandOrganizationTypeService, and introduced a Mockito-based unit test suite forOrganizationExportServiceImpl. - Hardened
OrganizationDAOImpl.searchOrganizationsWithTypes()hierarchy initialization to avoid infinite recursion when parent relationships contain cycles. - Normalized duplicate-organization name comparison in
duplicateOrganizationExists()to be consistent with the query’strim(lower(...))semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/test/java/org/openelisglobal/organisation/OrganizationTypeServiceTest.java |
Adds coverage for getOrganizationIdsForType() behavior using existing DBUnit fixtures. |
src/test/java/org/openelisglobal/organisation/OrganizationServiceTest.java |
Adds multiple service-level tests (including duplicate handling and soft delete), but includes commented-out/deleted test code and a misleading test name. |
src/test/java/org/openelisglobal/organisation/OrganizationExportServiceTest.java |
Adds unit tests for FHIR export flows; one test should be strengthened to avoid null-ID behavior and to verify bundling. Also, Import-service coverage from Issue #4130 is still missing. |
src/main/java/org/openelisglobal/organization/daoimpl/OrganizationDAOImpl.java |
Improves duplicate matching normalization and prevents recursive hierarchy initialization from looping on cyclic org graphs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pmanko
left a comment
There was a problem hiding this comment.
Reviewed commit: 78639f1fe963881a2f5ce8b490bc0b35aa67602d
Must fix before approval: this PR still does not test organization import, although #4130 requires it. Add those tests, or update the PR and issue scope so they do not claim that work. Also make the parent export test use stable FHIR IDs and verify that the bundle is built. The two smaller Copilot test-cleanup notes can be handled in the same update.
Verification: targeted organization import/export tests and the authoritative E2E checkpoint must pass.
Signed-off-by: Atwijukire Ariho Seth <120330466+Ariho-Seth@users.noreply.github.com>
|
Hello @pmanko, I've worked on the above suggested reviews. Hope it looks okay now> |
| @Test | ||
| public void testGetOrganizationIdsForType_ShouldReturnTypesMatchingThePassedID() { | ||
| List<String> returnedTypes = organizationTypeService.getOrganizationIdsForType("1"); | ||
| assertNotNull(returnedTypes); |
There was a problem hiding this comment.
assertNotNull(result) confirms something was returned, but says nothing about whether it's the right thing
can we get rid of it ans use an aggressive assertion
|
do you wanna checkout the CI ? |
Pull Requests Requirements
Issue number if applicable.
Styleguide and Design
documentation.
Guidelines of this
project.
Summary
This PR includes improves the test coverage of the core organization service classes to enhance functionality and ensure stability within the organization module.
Changes Made
OrganizationServiceImpl.java: Updated business logic and service methods related to organization management, queries, and hierarchy handling.OrganizationTypeServiceImpl.java: Refined methods and data handling for organization types.OrganizationExportServiceImpl.java: Added unit testsRelated Issue
Improve Test Coverage for the organization.service Package
Closes #4130