Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: get one trackedEntity via path and query param is identical DHIS2-18541 #19377

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

teleivo
Copy link
Contributor

@teleivo teleivo commented Dec 4, 2024

as already done for /tracker/events in #18943

  • Program program parameter was always null in DefaultTrackedEntityService.
  • Make controller test a postgres one. The controller test was only able to test the /tracker/trackedEntities/{uid} using H2 and not /tracker/trackedEntities.
  • Set default orgUnitMode in OperationParams to ACCESSIBLE as this is the safest/least surprising/mostly what we want value. null is not a valid value and makes the OperationParams harder to use.

Tests

  • removed getTrackedEntityByIdWithFieldsRelationshipsNoAccessToRelationshipType as we do not check data read access of the relationship type in /tracker/trackedEntities. This was only done for /tracker/trackedEntities/{uid}.

Found Bug

We only tested the /tracker/trackedEntities/{uid} using controller tests so far. Only now do these tests show this bug as we are now using the same code path for serving collections. That code path is extremely complicated 😢 (aggregate).

### fields * gets the tracked entity with enrollments.events.relationships
GET {{PROTOCOL}}://{{AUTH}}@{{HOST}}/api/tracker/trackedEntities?trackedEntities=Kj6vYde4LHh&fields=*
Accept: application/json

### fields enrollments did not get the enrollments.events.relationships
GET {{PROTOCOL}}://{{AUTH}}@{{HOST}}/api/tracker/trackedEntities?trackedEntities=Kj6vYde4LHh&fields=enrollments
Accept: application/json

Reason was

--- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/trackedentity/aggregates/EventAggregate.java
+++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/trackedentity/aggregates/EventAggregate.java
@@ -105,7 +105,7 @@ public class EventAggregate implements Aggregate {
               Multimap<String, RelationshipItem> relationships = relationshipAsync.join();

               for (Event event : events.values()) {
-                if (ctx.getParams().isIncludeRelationships()) {
+                if (ctx.getParams().getEventParams().isIncludeRelationships()) {
                   event.setRelationshipItems(new HashSet<>(relationships.get(event.getUid())));
                 }

We used the wrong param to detect whether relationships should be set on the event. TrackedEntityParams are another source of unnecessary complexity.

Next

  • migrate remaining tests in TrackedEntitiesExportControllerTest to use the json fixture instead of creating the setup using the manager
  • backport the bug fix to previous versions
  • improve test setup
    • We need to make our test code more composable. Extract a setup class to create metadata/tracker data so we are free to extend any test class and do not create the next TestBase 😬
    • We need to be able to share test code across controller and service integration tests. This is currently not possible due to our module setup. Unless we put test code into src/main in the tracker module. Should we merge the test-integration and test-web-api modules? We need to investigate the pros/cons.

@teleivo teleivo force-pushed the DHIS2-18541 branch 6 times, most recently from f731f93 to 040bbd3 Compare December 4, 2024 15:28
Copy link

sonarqubecloud bot commented Dec 4, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
6 New issues
3 New Blocker Issues (required ≤ 0)
6 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@teleivo teleivo force-pushed the DHIS2-18541 branch 5 times, most recently from 5515b49 to c6630f1 Compare January 8, 2025 09:51
muilpp and others added 2 commits January 8, 2025 12:32
so we return NotFound instead of BadRequest which is done in
the OperationsParamsValidator
Copy link

sonarqubecloud bot commented Jan 8, 2025

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.

2 participants