fix: Move converters into the bundle package [DHIS2-18093] #18807

enricocolasante · 2024-10-11T13:55:32Z

This PR moves the converters inside the bundle package and merge them in one single static mapper.

While at it, a few small fixes were performed:

Deleted object cannot be modified, setDeleted(false) was removed from the mapper.
EnrollmentStatus default is ACTIVE. Added default value in tracker model and removed it from the mapper.
EnrollmentStatus change to Active was missing logic. Bug created https://dhis2.atlassian.net/browse/DHIS2-18225
Event dv shouldn't be mapped. Task created https://dhis2.atlassian.net/browse/DHIS2-18223
Relationship cannot be updated so the mapper and the persisted got simplified.
Relationship lastUpdatedBy was never populated. Bug created https://dhis2.atlassian.net/browse/DHIS2-18226
Relationship field deleted was removed as it was never used.
A task to remove all useless fields in tracker model was created https://dhis2.atlassian.net/browse/DHIS2-18222
A task to review the logic for dates and statuses for enrollment and event was created https://dhis2.atlassian.net/browse/DHIS2-18224

...ices/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/domain/Enrollment.java

...s/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/domain/TrackedEntity.java

muilpp · 2024-10-15T04:24:20Z

...service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/TrackerObjectsMapper.java

+    Date occurredDate = DateUtils.fromInstant(enrollment.getOccurredAt());
+
+    dbEnrollment.setEnrollmentDate(enrollmentDate);
+    dbEnrollment.setOccurredDate(occurredDate != null ? occurredDate : enrollmentDate);


what's the difference between this one and enrollmentDate? and why do we set it to enrollmentDate if occurredDate is null?

Now the there is less code and it is easier to understand this logic I created https://dhis2.atlassian.net/browse/DHIS2-18224 to reason about this kind of stuff.
I am not sure what is the logic here

muilpp · 2024-10-15T04:39:50Z

...cker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/RelationshipPersister.java

  }

  @Override
  protected org.hisp.dhis.relationship.Relationship convert(
      TrackerBundle bundle, Relationship trackerDto) {
-    return relationshipConverter.from(bundle.getPreheat(), trackerDto, bundle.getUser());
+    if (bundle.getStrategy(trackerDto) == TrackerImportStrategy.UPDATE) {
+      return null;


why do we not map the object in case of UPDATE?

Relationships cannot be updated.
If this is the case, we are ignoring the object in the payload and send a warning.
Ivo created a bug https://dhis2.atlassian.net/browse/DHIS2-18110 where we should improve the way we set the ignored objects to always provide an error/warning. Here it is happening but it is not enforced by the code.

muilpp · 2024-10-15T04:48:00Z

...service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/TrackerObjectsMapper.java

+          dbEnrollment.setCompletedDate(now);
+          dbEnrollment.setCompletedBy(user.getUsername());
+        }
+        case CANCELLED -> {


unrelated to this PR, but I've seen that when the enrollment is CANCELLED, the related event(s) are not modified, so it could happen that an event is ACTIVE while the parent enrollment is CANCELLED, does that make sense?

Same as before, I just didn't change the logic, but in https://dhis2.atlassian.net/browse/DHIS2-18224 we should try to understand if it makes sense or not.

teleivo · 2024-10-15T06:43:02Z

EnrollmentStatus default is ACTIVE. Added default value in the view and the model and removed it from the mapper.

Why is a default needed in both places?

enricocolasante · 2024-10-15T06:50:37Z

EnrollmentStatus default is ACTIVE. Added default value in the view and the model and removed it from the mapper.

Why is a default needed in both places?

Yeah, I don't think it is.
I started adding the default in the view and then I realized that I needed in the service, because tracker importer endpoint is not the only client, but then I didn't remove it from the view.

teleivo

Is the TrackerObjectsMapper only used by the persisters? if so move it their and make package private

...service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/TrackerObjectsMapper.java

...r/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/AbstractTrackerPersister.java

...racker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/EnrollmentPersister.java

enricocolasante · 2024-10-15T08:24:03Z

Is the TrackerObjectsMapper only used by the persisters? if so move it their and make package private

Unfortunately it is used in here

dhis2-core/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/validation/validator/relationship/SecurityOwnershipValidator.java

Line 63 in 7f59931

    
           user, TrackerObjectsMapper.map(bundle.getPreheat(), relationship, bundle.getUser()))

.

I would like to remove it at some point but it is out of the scope of this PR.

enricocolasante · 2024-10-15T08:54:25Z

EnrollmentStatus default is ACTIVE. Added default value in the view and the model and removed it from the mapper.

Why is a default needed in both places?

Yeah, I don't think it is. I started adding the default in the view and then I realized that I needed in the service, because tracker importer endpoint is not the only client, but then I didn't remove it from the view.

It is actually needed in both places.
If I don't add it in the view, than the null will be propagated.
If I don't set it in the service, than any other client will not have a default value.

sonarcloud · 2024-10-15T09:00:05Z

Quality Gate failed

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

See analysis details on SonarCloud

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

enricocolasante force-pushed the DHIS2-18093-converters branch 3 times, most recently from 9069b97 to 114cacb Compare October 11, 2024 14:03

github-advanced-security bot found potential problems Oct 11, 2024

View reviewed changes

...ices/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/domain/Enrollment.java Dismissed Show dismissed Hide dismissed

...s/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/domain/TrackedEntity.java Dismissed Show dismissed Hide dismissed

enricocolasante force-pushed the DHIS2-18093-converters branch 3 times, most recently from 460ea37 to 13fc56d Compare October 14, 2024 09:53

fix: Move converters into the bundle package [DHIS2-18093]

7f59931

enricocolasante force-pushed the DHIS2-18093-converters branch from 13fc56d to 7f59931 Compare October 14, 2024 12:52

enricocolasante marked this pull request as ready for review October 14, 2024 13:16

enricocolasante requested a review from a team as a code owner October 14, 2024 13:16

This was referenced Oct 14, 2024

fix: Fix status update logic for enrollment [DHIS2-18225](2.41) #18818

Merged

fix: Fix status update logic for enrollment [DHIS2-18225](2.40) #18819

Merged

fix: Fix status update logic for enrollment [DHIS2-18225](2.39) #18820

Merged

muilpp approved these changes Oct 15, 2024

View reviewed changes

enricocolasante requested review from ameenhere, teleivo and zubaira October 15, 2024 06:40

This was referenced Oct 15, 2024

fix: Set lastUpdatedBy when creating relationships [DHIS2-18226](2.41) #18826

Merged

fix: Set lastUpdatedBy when creating relationships [DHIS2-18226](2.40) #18827

Merged

fix: Set lastUpdatedBy when creating relationships [DHIS2-18226](2.39) #18828

Merged

teleivo reviewed Oct 15, 2024

View reviewed changes

Fix review comments

907bffc

enricocolasante requested a review from teleivo October 15, 2024 08:25

Fix review comments

e11b547

teleivo approved these changes Oct 15, 2024

View reviewed changes

enricocolasante merged commit be67efb into master Oct 15, 2024
14 of 15 checks passed

enricocolasante deleted the DHIS2-18093-converters branch October 15, 2024 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Move converters into the bundle package [DHIS2-18093] #18807

fix: Move converters into the bundle package [DHIS2-18093] #18807

enricocolasante commented Oct 11, 2024 •

edited

Loading

muilpp Oct 15, 2024

enricocolasante Oct 15, 2024

muilpp Oct 15, 2024

enricocolasante Oct 15, 2024

muilpp Oct 15, 2024

enricocolasante Oct 15, 2024

teleivo commented Oct 15, 2024

enricocolasante commented Oct 15, 2024

teleivo left a comment

enricocolasante commented Oct 15, 2024

enricocolasante commented Oct 15, 2024

sonarcloud bot commented Oct 15, 2024

fix: Move converters into the bundle package [DHIS2-18093] #18807

fix: Move converters into the bundle package [DHIS2-18093] #18807

Conversation

enricocolasante commented Oct 11, 2024 • edited Loading

muilpp Oct 15, 2024

Choose a reason for hiding this comment

enricocolasante Oct 15, 2024

Choose a reason for hiding this comment

muilpp Oct 15, 2024

Choose a reason for hiding this comment

enricocolasante Oct 15, 2024

Choose a reason for hiding this comment

muilpp Oct 15, 2024

Choose a reason for hiding this comment

enricocolasante Oct 15, 2024

Choose a reason for hiding this comment

teleivo commented Oct 15, 2024

enricocolasante commented Oct 15, 2024

teleivo left a comment

Choose a reason for hiding this comment

enricocolasante commented Oct 15, 2024

enricocolasante commented Oct 15, 2024

sonarcloud bot commented Oct 15, 2024

Quality Gate failed

enricocolasante commented Oct 11, 2024 •

edited

Loading