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: align fetching a single vs multiple TEs more DHIS2-18541 #19647

Merged
merged 4 commits into from
Jan 15, 2025

Conversation

teleivo
Copy link
Contributor

@teleivo teleivo commented Jan 13, 2025

continuing on #19633

The goal is to ultimately use the same code to fetch a single vs multiple TEs. This PR makes another step towards that goal by

  • using the same methods to set the RelationshipItems
  • using the same method to set the TrackedEntityAttributeValues. /trackedEntities?program=uidreturned all TEAV so far while /trackedEntities/uid?program=uid only returned TET+program specific TEAV. That behavior was introduced only to /trackedEntities/uid?program=uid in fix: Return TEA from specified program only [DHIS2-14300] #17572
  • fixed bug in dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/trackedentity/aggregates/EventAggregate.java which used the wrong param

misc

  • prefix methods that mutate with set and those that map with map
  • order parameters similarly
  • only audit if a TE was found as there is nothing to audit otherwise

Open question

https://github.com/dhis2/dhis2-core/pull/19647/files#diff-069b27403f4f267ecb5e14378a2e9c32d339e92acb494e688b821df93de33acbL323-L326

was only applied to relationships of the TE and only on /trackedEntities/uid. Should this be applied to all relationships like for example TE.enrollments.relationships? Note the TrackedEntityAggregate store does not fetch relationships. So relationships are fetched in loops per tracked entity.

Next

/trackedEntities/uid vs /trackedEntities

  • The logic in setCollections should be applied to both paths.
  private void setCollections(
      TrackedEntity trackedEntity,
      Program program,
      TrackedEntityParams params,
      UserDetails user,
      boolean includeDeleted)
      throws NotFoundException {
    setRelationshipItems(trackedEntity, params, includeDeleted);
    if (params.isIncludeEnrollments()) {
      trackedEntity.setEnrollments(getEnrollments(trackedEntity, user, includeDeleted, program));
    }
    if (params.isIncludeProgramOwners()) {
      trackedEntity.setProgramOwners(getTrackedEntityProgramOwners(trackedEntity, program));
    }
    trackedEntity.setTrackedEntityAttributeValues(
        getTrackedEntityAttributeValues(trackedEntity, program));
  }

All but the two if statements are already applied with this PR. Adding them makes more tests fail which I'll fix separately.

  • Ideally align auditing as both do audit but different things. Not sure if these accesses need to be audited differently.

@teleivo teleivo requested review from enricocolasante and removed request for enricocolasante January 13, 2025 14:25
@teleivo teleivo changed the title chore: align fetching a single vs multiple TEs more chore: align fetching a single vs multiple TEs more DHIS2-18541 Jan 14, 2025
@teleivo teleivo marked this pull request as ready for review January 15, 2025 10:29
@teleivo teleivo requested a review from a team as a code owner January 15, 2025 10:29
@teleivo teleivo requested review from ameenhere, muilpp and a team January 15, 2025 12:25
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
4 New issues
C Reliability Rating on New Code (required ≥ A)
1 New Bugs (required ≤ 0)
3 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 requested a review from muilpp January 15, 2025 13:57
@teleivo teleivo merged commit 8cc485b into master Jan 15, 2025
16 of 17 checks passed
@teleivo teleivo deleted the DHIS2-18541-pre branch January 15, 2025 14:04
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.

3 participants