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: Remove getTrackedEntity(UID) [DHIS2-17714] #18316

Merged
merged 16 commits into from
Aug 20, 2024

Conversation

muilpp
Copy link
Contributor

@muilpp muilpp commented Aug 8, 2024

Big picture

Tracker has multiple services for each entity like tracked entity, enrollment, event and relationship. One is in dhis-api and one in dhis-service-tracker. Our goal is to provide one API (service) per entity that is part of the tracker domain/team.

Trackers architecture splits read/write into exporter services and an importer. So if you want to get data you use an exporter. If you want to insert, update or delete you have to go through the importer. Only then can we ensure integrity of tracker data.

Another goal is that code that provides tracker functionality and is thus owned by the tracker team lives in ideally one maven module (We can settle on a name later on maybe dhis-service-tracker or dhis-tracker).

This is a big task that we are going to implement in many small steps.

This PR

  • Removes the method getTrackedEntity(String uid) from TrackedEntityService.
  • It's replaced by the manager in all the tests, and by the getTrackedEntity method from the tracker module TrackedEntityService in the service layer. There are still usages of the manager in the service, they are explained below.
  • The use of the manager in the OperationParamsValidator needs to be revisited. I'm not sure if it is sufficient or if we need to use the tracker service to run the appropriate validations. Same goes for the DeduplicationController.
  • In DefaultProgramMessageService and DeliveryChannelStrategy we still use the manager because they both live in the service core module, so they have no access to the tracker module. These two services will audit the log directly using the TrackedEntityChangeLogService. Despite its name, this service actually generates audit logs, not change logs. However, this is only a temporary solution, as the service belongs to the tracker module and should be relocated to the appropriate module. Once we move it, neither of these two services will have access to it anymore.
  • DefaultTrackerObjectsDeletionService is also using the manager, because the proper validations are already run before we reach that point, it's not necessary to run them again.
  • The error messages of the SMS feature need to be rethought. Once we are able to mock the gateway server and test it properly we can redo that part as well.

Please note this is a very long PR, but there are a lot of changes that consist of a method signature refactor only.

@muilpp muilpp marked this pull request as ready for review August 13, 2024 09:16
@muilpp muilpp requested review from ameenhere, zubaira and a team August 13, 2024 09:17
@muilpp muilpp enabled auto-merge (squash) August 13, 2024 19:40
@@ -186,11 +191,14 @@ public TrackedEntity validateTrackedEntity(String trackedEntityUid, User user)
return null;
}

TrackedEntity trackedEntity = trackedEntityService.getTrackedEntity(trackedEntityUid);
// TODO(tracker) Are these validations enough? Should we check for ownership too?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably unify this method with the one present in RelationshipOperationParamsMapper when solving this TODO

Copy link

sonarcloud bot commented Aug 20, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
14 New issues
14 New Code Smells (required ≤ 0)

See analysis details on SonarCloud

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

@muilpp muilpp merged commit 5bd6cf8 into master Aug 20, 2024
14 of 15 checks passed
@muilpp muilpp deleted the DHIS2-17714-remove-getTrackedEntity branch August 20, 2024 08:15
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