From 03c38af75b8d163e0bbcba14531ec97f8262e3f4 Mon Sep 17 00:00:00 2001 From: Enrico Colasante Date: Tue, 22 Oct 2024 10:24:12 +0200 Subject: [PATCH] chore: Clean up data values persistence [DHIS2-18223] (#18871) * chore: Clean up data values persistence [DHIS2-18223] * Fix e2e test * Fix formatting * Fix review comments --- .../imports/bundle/TrackerObjectsMapper.java | 19 -- .../bundle/persister/EventPersister.java | 220 +++++++++--------- .../tracker/imports/domain/DataValue.java | 5 - .../event/DataValuesValidatorTest.java | 51 ---- .../tracker/importer/events/events.json | 2 - 5 files changed, 115 insertions(+), 182 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/TrackerObjectsMapper.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/TrackerObjectsMapper.java index 37c0cb4e510f..f15f756a7a0d 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/TrackerObjectsMapper.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/TrackerObjectsMapper.java @@ -34,9 +34,7 @@ import java.util.stream.Collectors; import javax.annotation.Nonnull; import org.hisp.dhis.category.CategoryOptionCombo; -import org.hisp.dhis.dataelement.DataElement; import org.hisp.dhis.event.EventStatus; -import org.hisp.dhis.eventdatavalue.EventDataValue; import org.hisp.dhis.note.Note; import org.hisp.dhis.organisationunit.OrganisationUnit; import org.hisp.dhis.program.Enrollment; @@ -50,7 +48,6 @@ import org.hisp.dhis.relationship.RelationshipType; import org.hisp.dhis.trackedentity.TrackedEntity; import org.hisp.dhis.trackedentity.TrackedEntityType; -import org.hisp.dhis.tracker.imports.domain.DataValue; import org.hisp.dhis.tracker.imports.preheat.TrackerPreheat; import org.hisp.dhis.tracker.imports.util.RelationshipKeySupport; import org.hisp.dhis.user.User; @@ -236,22 +233,6 @@ private TrackerObjectsMapper() { assignedUser.ifPresent(dbEvent::setAssignedUser); } - // TODO(DHIS2-18223): Remove data value mapping and fix changelog logic - for (DataValue dataValue : event.getDataValues()) { - DataElement dataElement = preheat.getDataElement(dataValue.getDataElement()); - - EventDataValue eventDataValue = new EventDataValue(); - eventDataValue.setDataElement(dataElement.getUid()); - eventDataValue.setCreated(DateUtils.fromInstant(dataValue.getCreatedAt())); - eventDataValue.setCreatedByUserInfo(UserInfoSnapshot.from(user)); - eventDataValue.setValue(dataValue.getValue()); - eventDataValue.setLastUpdated(now); - eventDataValue.setProvidedElsewhere(dataValue.isProvidedElsewhere()); - eventDataValue.setLastUpdatedByUserInfo(UserInfoSnapshot.from(user)); - - dbEvent.getEventDataValues().add(eventDataValue); - } - if (isNotEmpty(event.getNotes())) { dbEvent .getNotes() diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/EventPersister.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/EventPersister.java index fcf9ae0fff1e..2b512f3db051 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/EventPersister.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/EventPersister.java @@ -27,10 +27,7 @@ */ package org.hisp.dhis.tracker.imports.bundle.persister; -import static com.google.common.base.Preconditions.checkNotNull; - import jakarta.persistence.EntityManager; -import java.time.Instant; import java.util.ArrayList; import java.util.Collections; import java.util.Date; @@ -41,8 +38,8 @@ import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; -import lombok.Builder; -import lombok.Data; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; import org.apache.commons.lang3.StringUtils; import org.hisp.dhis.changelog.ChangeLogType; import org.hisp.dhis.common.UID; @@ -50,6 +47,7 @@ import org.hisp.dhis.event.EventStatus; import org.hisp.dhis.eventdatavalue.EventDataValue; import org.hisp.dhis.program.Event; +import org.hisp.dhis.program.UserInfoSnapshot; import org.hisp.dhis.reservedvalue.ReservedValueService; import org.hisp.dhis.tracker.TrackerType; import org.hisp.dhis.tracker.export.event.EventChangeLogService; @@ -62,7 +60,6 @@ import org.hisp.dhis.tracker.imports.job.TrackerNotificationDataBundle; import org.hisp.dhis.tracker.imports.preheat.TrackerPreheat; import org.hisp.dhis.user.UserDetails; -import org.hisp.dhis.util.DateUtils; import org.springframework.stereotype.Component; /** @@ -170,65 +167,119 @@ private void handleDataValues( .orElse(new HashMap<>()); payloadDataValues.forEach( - dv -> { - DataElement dataElement = preheat.getDataElement(dv.getDataElement()); - checkNotNull( - dataElement, - "Data element should never be NULL here if validation is enforced before commit."); - - // EventDataValue.dataElement contains a UID - EventDataValue eventDataValue = dataValueDBMap.get(dataElement.getUid()); + dataValue -> { + DataElement dataElement = preheat.getDataElement(dataValue.getDataElement()); + EventDataValue dbDataValue = dataValueDBMap.get(dataElement.getUid()); + + if (isNewDataValue(dbDataValue, dataValue)) { + logDataValueChange( + user.getUsername(), + dataElement, + event, + dataValue.getValue(), + dataValue.isProvidedElsewhere(), + ChangeLogType.CREATE); + saveDataValue(dataValue, event, dataElement, user, entityManager, preheat); + } else if (isUpdate(dbDataValue, dataValue)) { + logDataValueChange( + user.getUsername(), + dataElement, + event, + dbDataValue.getValue(), + dbDataValue.getProvidedElsewhere(), + ChangeLogType.UPDATE); + updateDataValue( + dbDataValue, dataValue, event, dataElement, user, entityManager, preheat); + } else if (isDeletion(dbDataValue, dataValue)) { + logDataValueChange( + user.getUsername(), + dataElement, + event, + dbDataValue.getValue(), + dbDataValue.getProvidedElsewhere(), + ChangeLogType.DELETE); + deleteDataValue(dbDataValue, event, dataElement, entityManager, preheat); + } + }); + } - ValuesHolder valuesHolder = getAuditAndDateParameters(eventDataValue, dv); + private void saveDataValue( + DataValue dv, + Event event, + DataElement dataElement, + UserDetails user, + EntityManager entityManager, + TrackerPreheat preheat) { + EventDataValue eventDataValue = new EventDataValue(); + eventDataValue.setDataElement(dataElement.getUid()); + eventDataValue.setCreated(new Date()); + eventDataValue.setCreatedByUserInfo(UserInfoSnapshot.from(user)); + eventDataValue.setStoredBy(user.getUsername()); - eventDataValue = valuesHolder.getEventDataValue(); + eventDataValue.setLastUpdated(new Date()); + eventDataValue.setLastUpdatedByUserInfo(UserInfoSnapshot.from(user)); - eventDataValue.setDataElement(dataElement.getUid()); - eventDataValue.setStoredBy(dv.getStoredBy()); + eventDataValue.setValue(dv.getValue()); + eventDataValue.setProvidedElsewhere(dv.isProvidedElsewhere()); - if (StringUtils.isEmpty(dv.getValue())) { - if (dataElement.isFileType()) { - unassignFileResource( - entityManager, preheat, event.getUid(), eventDataValue.getValue()); - } + if (dataElement.isFileType()) { + assignFileResource(entityManager, preheat, event.getUid(), eventDataValue.getValue()); + } - event.getEventDataValues().remove(eventDataValue); - } else { - eventDataValue.setValue(dv.getValue()); + event.getEventDataValues().add(eventDataValue); + } - if (dataElement.isFileType()) { - assignFileResource(entityManager, preheat, event.getUid(), eventDataValue.getValue()); - } + private void updateDataValue( + EventDataValue eventDataValue, + DataValue dv, + Event event, + DataElement dataElement, + UserDetails user, + EntityManager entityManager, + TrackerPreheat preheat) { + eventDataValue.setLastUpdated(new Date()); + eventDataValue.setLastUpdatedByUserInfo(UserInfoSnapshot.from(user)); - event.getEventDataValues().remove(eventDataValue); - event.getEventDataValues().add(eventDataValue); - } + if (dataElement.isFileType()) { + unassignFileResource(entityManager, preheat, event.getUid(), eventDataValue.getValue()); + assignFileResource(entityManager, preheat, event.getUid(), dv.getValue()); + } - logTrackedEntityDataValueHistory( - user.getUsername(), dataElement, event, new Date(), valuesHolder); - }); + eventDataValue.setProvidedElsewhere(dv.isProvidedElsewhere()); + eventDataValue.setValue(dv.getValue()); } - private Date getFromOrNewDate(DataValue dv, Function dateGetter) { - return Optional.of(dv).map(dateGetter).map(DateUtils::fromInstant).orElseGet(Date::new); + private void deleteDataValue( + EventDataValue eventDataValue, + Event event, + DataElement dataElement, + EntityManager entityManager, + TrackerPreheat preheat) { + if (dataElement.isFileType()) { + unassignFileResource(entityManager, preheat, event.getUid(), eventDataValue.getValue()); + } + + event.getEventDataValues().remove(eventDataValue); } - private void logTrackedEntityDataValueHistory( - String userName, DataElement de, Event event, Date created, ValuesHolder valuesHolder) { - ChangeLogType changeLogType = valuesHolder.getChangeLogType(); - - if (changeLogType != null) { - TrackedEntityDataValueChangeLog valueAudit = new TrackedEntityDataValueChangeLog(); - valueAudit.setEvent(event); - valueAudit.setValue(valuesHolder.getValue()); - valueAudit.setAuditType(changeLogType); - valueAudit.setDataElement(de); - valueAudit.setModifiedBy(userName); - valueAudit.setProvidedElsewhere(valuesHolder.isProvidedElseWhere()); - valueAudit.setCreated(created); - - eventChangeLogService.addTrackedEntityDataValueChangeLog(valueAudit); - } + private void logDataValueChange( + String userName, + DataElement de, + Event event, + String value, + boolean providedElsewhere, + ChangeLogType changeLogType) { + + TrackedEntityDataValueChangeLog changeLog = new TrackedEntityDataValueChangeLog(); + changeLog.setEvent(event); + changeLog.setValue(value); + changeLog.setAuditType(changeLogType); + changeLog.setDataElement(de); + changeLog.setModifiedBy(userName); + changeLog.setProvidedElsewhere(providedElsewhere); + changeLog.setCreated(new Date()); + + eventChangeLogService.addTrackedEntityDataValueChangeLog(changeLog); } @Override @@ -245,61 +296,20 @@ protected String getUpdatedTrackedEntity(Event entity) { .orElse(null); } - private boolean isNewDataValue(EventDataValue eventDataValue, DataValue dv) { - return eventDataValue == null - || (eventDataValue.getCreated() == null && StringUtils.isNotBlank(dv.getValue())); - } - - private boolean isDeletion(EventDataValue eventDataValue, DataValue dv) { - return StringUtils.isNotBlank(eventDataValue.getValue()) && StringUtils.isBlank(dv.getValue()); + private boolean isNewDataValue( + @CheckForNull EventDataValue eventDataValue, @Nonnull DataValue dv) { + return eventDataValue == null && !StringUtils.isBlank(dv.getValue()); } - private boolean isUpdate(EventDataValue eventDataValue, DataValue dv) { - return !StringUtils.equals(dv.getValue(), eventDataValue.getValue()); + private boolean isDeletion(@CheckForNull EventDataValue eventDataValue, @Nonnull DataValue dv) { + return eventDataValue != null + && StringUtils.isNotBlank(eventDataValue.getValue()) + && StringUtils.isBlank(dv.getValue()); } - private ValuesHolder getAuditAndDateParameters(EventDataValue eventDataValue, DataValue dv) { - String persistedValue; - - ChangeLogType changeLogType = null; - - if (isNewDataValue(eventDataValue, dv)) { - eventDataValue = new EventDataValue(); - eventDataValue.setCreated(getFromOrNewDate(dv, DataValue::getCreatedAt)); - eventDataValue.setLastUpdated(getFromOrNewDate(dv, DataValue::getUpdatedAt)); - persistedValue = dv.getValue(); - changeLogType = ChangeLogType.CREATE; - } else { - persistedValue = eventDataValue.getValue(); - - if (isUpdate(eventDataValue, dv)) { - changeLogType = ChangeLogType.UPDATE; - eventDataValue.setLastUpdated(getFromOrNewDate(dv, DataValue::getUpdatedAt)); - } - - if (isDeletion(eventDataValue, dv)) { - changeLogType = ChangeLogType.DELETE; - eventDataValue.setLastUpdated(getFromOrNewDate(dv, DataValue::getUpdatedAt)); - } - } - - return ValuesHolder.builder() - .value(persistedValue) - .providedElseWhere(dv.isProvidedElsewhere()) - .changeLogType(changeLogType) - .eventDataValue(eventDataValue) - .build(); - } - - @Data - @Builder - static class ValuesHolder { - private final String value; - - private final boolean providedElseWhere; - - private final ChangeLogType changeLogType; - - private final EventDataValue eventDataValue; + private boolean isUpdate(@CheckForNull EventDataValue eventDataValue, @Nonnull DataValue dv) { + return eventDataValue != null + && !StringUtils.isBlank(dv.getValue()) + && !StringUtils.equals(dv.getValue(), eventDataValue.getValue()); } } diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/domain/DataValue.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/domain/DataValue.java index 78a933a777e5..2a5a9fbbc22b 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/domain/DataValue.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/domain/DataValue.java @@ -29,7 +29,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import java.io.Serializable; -import java.time.Instant; import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; @@ -44,10 +43,6 @@ @AllArgsConstructor // TODO(DHIS2-18223) Remove unused fields when fixing data values logic public class DataValue implements Serializable { - @JsonProperty private Instant createdAt; // remove - - @JsonProperty private Instant updatedAt; // remove - @JsonProperty private String storedBy; @JsonProperty private boolean providedElsewhere; diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/imports/validation/validator/event/DataValuesValidatorTest.java b/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/imports/validation/validator/event/DataValuesValidatorTest.java index 1d487cbbc179..3d887dea4d06 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/imports/validation/validator/event/DataValuesValidatorTest.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/imports/validation/validator/event/DataValuesValidatorTest.java @@ -58,7 +58,6 @@ import org.hisp.dhis.tracker.imports.preheat.TrackerPreheat; import org.hisp.dhis.tracker.imports.validation.Reporter; import org.hisp.dhis.tracker.imports.validation.ValidationCode; -import org.hisp.dhis.util.DateUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -149,54 +148,6 @@ void successValidationWhenDataElementIsValid() { assertIsEmpty(reporter.getErrors()); } - @Test - void successValidationWhenCreatedAtIsNull() { - DataElement dataElement = dataElement(); - when(preheat.getDataElement(MetadataIdentifier.ofUid(DATA_ELEMENT_UID))) - .thenReturn(dataElement); - - ProgramStage programStage = programStage(dataElement); - when(preheat.getProgramStage(MetadataIdentifier.ofUid(PROGRAM_STAGE_UID))) - .thenReturn(programStage); - - DataValue validDataValue = dataValue(); - validDataValue.setCreatedAt(null); - Event event = - Event.builder() - .programStage(idSchemes.toMetadataIdentifier(programStage)) - .status(EventStatus.ACTIVE) - .dataValues(Set.of(validDataValue)) - .build(); - - validator.validate(reporter, bundle, event); - - assertIsEmpty(reporter.getErrors()); - } - - @Test - void failValidationWhenUpdatedAtIsNull() { - DataElement dataElement = dataElement(); - when(preheat.getDataElement(MetadataIdentifier.ofUid(DATA_ELEMENT_UID))) - .thenReturn(dataElement); - - ProgramStage programStage = programStage(dataElement); - when(preheat.getProgramStage(MetadataIdentifier.ofUid(PROGRAM_STAGE_UID))) - .thenReturn(programStage); - - DataValue validDataValue = dataValue(); - validDataValue.setUpdatedAt(null); - Event event = - Event.builder() - .programStage(idSchemes.toMetadataIdentifier(programStage)) - .status(EventStatus.ACTIVE) - .dataValues(Set.of(validDataValue)) - .build(); - - validator.validate(reporter, bundle, event); - - assertIsEmpty(reporter.getErrors()); - } - @Test void failValidationWhenDataElementIsInvalid() { DataElement dataElement = dataElement(); @@ -1140,8 +1091,6 @@ private DataValue dataValue(String value) { private DataValue dataValue() { DataValue dataValue = new DataValue(); - dataValue.setCreatedAt(DateUtils.instantFromDateAsString("2020-10-10")); - dataValue.setUpdatedAt(DateUtils.instantFromDateAsString("2020-10-10")); dataValue.setValue("text"); dataValue.setDataElement(MetadataIdentifier.ofUid(DATA_ELEMENT_UID)); return dataValue; diff --git a/dhis-2/dhis-test-e2e/src/test/resources/tracker/importer/events/events.json b/dhis-2/dhis-test-e2e/src/test/resources/tracker/importer/events/events.json index a682f85a6830..76760b187cff 100644 --- a/dhis-2/dhis-test-e2e/src/test/resources/tracker/importer/events/events.json +++ b/dhis-2/dhis-test-e2e/src/test/resources/tracker/importer/events/events.json @@ -117,8 +117,6 @@ "providedElsewhere": false }, { - "updatedAt": "2019-03-04T15:12:59.209", - "createdAt": "2019-03-04T15:01:29.793", "dataElement": "z3Z4TD3oBCP", "value": "true", "providedElsewhere": false