From 24769c8f01a78976b81b302fc02a0a14ba150ff3 Mon Sep 17 00:00:00 2001 From: Enrico Colasante Date: Fri, 14 Jun 2024 11:00:12 +0200 Subject: [PATCH 1/2] fix: Run most preprocessors only for CREATE and UPDATE [DHIS2-17402] (#17773) --- .../preprocess/BundlePreProcessor.java | 5 ++ .../DefaultTrackerPreprocessService.java | 4 +- .../preprocess/EventProgramPreProcessor.java | 23 +++++--- .../preprocess/StrategyPreProcessor.java | 5 ++ .../EventProgramPreProcessorTest.java | 23 ++++++++ .../enrollment_basic_data_for_deletion.json | 12 +--- .../event_basic_data_for_deletion.json | 50 +---------------- ...nt_enrollment_basic_data_for_deletion.json | 22 +------- .../tracker/tracker_data_for_deletion.json | 55 +------------------ 9 files changed, 55 insertions(+), 144 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/BundlePreProcessor.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/BundlePreProcessor.java index f8b0fd74dab8..6bd4d42f6acb 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/BundlePreProcessor.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/BundlePreProcessor.java @@ -27,6 +27,7 @@ */ package org.hisp.dhis.tracker.preprocess; +import org.hisp.dhis.tracker.TrackerImportStrategy; import org.hisp.dhis.tracker.bundle.TrackerBundle; /** @@ -40,4 +41,8 @@ */ public interface BundlePreProcessor { void process(TrackerBundle bundle); + + default boolean needsToRun(TrackerImportStrategy strategy) { + return !strategy.isDelete(); + } } diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/DefaultTrackerPreprocessService.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/DefaultTrackerPreprocessService.java index 22ef611e01f8..f599fbb5d196 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/DefaultTrackerPreprocessService.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/DefaultTrackerPreprocessService.java @@ -48,7 +48,9 @@ public void setPreProcessors(List preProcessors) { @Override public TrackerBundle preprocess(TrackerBundle bundle) { for (BundlePreProcessor preProcessor : preProcessors) { - preProcessor.process(bundle); + if (preProcessor.needsToRun(bundle.getImportStrategy())) { + preProcessor.process(bundle); + } } return bundle; diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java index 812f7a45c1d6..d88ee9b12f7f 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java @@ -27,10 +27,10 @@ */ package org.hisp.dhis.tracker.preprocess; +import static java.util.Objects.nonNull; + import java.util.List; -import java.util.Objects; import java.util.Optional; -import java.util.stream.Collectors; import org.hisp.dhis.program.Program; import org.hisp.dhis.program.ProgramStage; import org.hisp.dhis.tracker.TrackerIdSchemeParams; @@ -51,14 +51,19 @@ public class EventProgramPreProcessor implements BundlePreProcessor { public void process(TrackerBundle bundle) { List eventsToPreprocess = bundle.getEvents().stream() - .filter(e -> e.getProgram().isBlank() || e.getProgramStage().isBlank()) - .collect(Collectors.toList()); + .filter( + e -> + e.getProgram() == null + || e.getProgram().isBlank() + || e.getProgramStage() == null + || e.getProgramStage().isBlank()) + .toList(); for (Event event : eventsToPreprocess) { // Extract program from program stage - if (event.getProgramStage().isNotBlank()) { + if (nonNull(event.getProgramStage()) && event.getProgramStage().isNotBlank()) { ProgramStage programStage = bundle.getPreheat().getProgramStage(event.getProgramStage()); - if (Objects.nonNull(programStage)) { + if (nonNull(programStage)) { // TODO remove if once metadata import is fixed if (programStage.getProgram() == null) { // Program stages should always have a program! Due to @@ -84,9 +89,9 @@ public void process(TrackerBundle bundle) { } } // If it is a program event, extract program stage from program - else if (event.getProgram().isNotBlank()) { + else if (nonNull(event.getProgram()) && event.getProgram().isNotBlank()) { Program program = bundle.getPreheat().getProgram(event.getProgram()); - if (Objects.nonNull(program) && program.isWithoutRegistration()) { + if (nonNull(program) && program.isWithoutRegistration()) { Optional programStage = program.getProgramStages().stream().findFirst(); if (programStage.isPresent()) { TrackerIdSchemeParams idSchemes = bundle.getPreheat().getIdSchemes(); @@ -109,7 +114,7 @@ private void setAttributeOptionCombo(TrackerBundle bundle) { e.getAttributeOptionCombo().isBlank() && !e.getAttributeCategoryOptions().isEmpty()) .filter(e -> preheat.getProgram(e.getProgram()) != null) - .collect(Collectors.toList()); + .toList(); for (Event e : events) { Program program = preheat.getProgram(e.getProgram()); diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/StrategyPreProcessor.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/StrategyPreProcessor.java index e26223909753..71387503b8ac 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/StrategyPreProcessor.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/StrategyPreProcessor.java @@ -125,4 +125,9 @@ public void preProcessRelationships(TrackerBundle bundle) { } } } + + @Override + public boolean needsToRun(TrackerImportStrategy strategy) { + return true; + } } diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessorTest.java b/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessorTest.java index cad923180b4e..2ca01581e03b 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessorTest.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessorTest.java @@ -32,6 +32,7 @@ import static org.hisp.dhis.DhisConvenienceTest.createProgram; import static org.hisp.dhis.DhisConvenienceTest.createProgramStage; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -201,6 +202,20 @@ void testTrackerEventIsNotEnhancedWithProgramStage() { assertEquals(MetadataIdentifier.EMPTY_UID, bundle.getEvents().get(0).getProgramStage()); } + @Test + void shouldNotProcessEventWhenEventIsInvalidWithNoProgramAndNoProgramStage() { + Event event = invalidProgramEventWithNoProgramAndNoProgramStage(); + TrackerBundle bundle = + TrackerBundle.builder().events(Collections.singletonList(event)).preheat(preheat).build(); + + preprocessor.process(bundle); + + verify(preheat, never()).getProgram(PROGRAM_WITH_REGISTRATION); + verify(preheat, never()).getProgramStage(PROGRAM_STAGE_WITH_REGISTRATION); + assertNull(bundle.getEvents().get(0).getProgram()); + assertNull(bundle.getEvents().get(0).getProgramStage()); + } + @Test void testProgramEventWithProgramAndProgramStageIsNotProcessed() { TrackerIdSchemeParams identifierParams = TrackerIdSchemeParams.builder().build(); @@ -442,6 +457,14 @@ private Program programWithoutRegistration() { return program; } + private Event invalidProgramEventWithNoProgramAndNoProgramStage() { + return Event.builder() + .program(null) + .programStage(null) + .attributeOptionCombo(MetadataIdentifier.EMPTY_UID) + .build(); + } + private Event programEventWithProgram() { return Event.builder() .program(MetadataIdentifier.ofUid(PROGRAM_WITHOUT_REGISTRATION)) diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/enrollment_basic_data_for_deletion.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/enrollment_basic_data_for_deletion.json index 8086f299b4c8..26574bae12d5 100644 --- a/dhis-2/dhis-test-integration/src/test/resources/tracker/enrollment_basic_data_for_deletion.json +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/enrollment_basic_data_for_deletion.json @@ -33,17 +33,7 @@ "trackedEntities": [], "enrollments": [ { - "enrollment": "TvctPPhpD8u", - "createdAtClient": "2017-01-26T13:48:13.363", - "trackedEntity": "NCc1rCEOKaY", - "program": { - "idScheme": "UID", - "identifier": "E8o1E9tAppy" - }, - "orgUnit": { - "idScheme": "UID", - "identifier": "QfUVllTs6cS" - } + "enrollment": "TvctPPhpD8u" } ], "events": [], diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/event_basic_data_for_deletion.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/event_basic_data_for_deletion.json index f69178371f5d..6b0d10cfef90 100644 --- a/dhis-2/dhis-test-integration/src/test/resources/tracker/event_basic_data_for_deletion.json +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/event_basic_data_for_deletion.json @@ -34,55 +34,7 @@ "enrollments": [], "events": [ { - "event": "D9PbzJY8bJ1", - "status": "COMPLETED", - "program": { - "idScheme": "UID", - "identifier": "E8o1E9tAppy" - }, - "programStage": { - "idScheme": "UID", - "identifier": "Qmqxq907VNz" - }, - "enrollment": "TvctPPhpD8v", - "orgUnit": { - "idScheme": "UID", - "identifier": "QfUVllTs6cZ" - }, - "relationships": [], - "occurredAt": "2019-01-28T00:00:00.000", - "scheduledAt": "2019-01-28T12:10:38.100", - "storedBy": "admin", - "followup": false, - "deleted": false, - "createdAt": "2019-01-28T12:10:38.108", - "updatedAt": "2019-01-28T12:10:38.109", - "attributeOptionCombo": { - "idScheme": "UID", - "identifier": "KKKKX50cXC0" - }, - "attributeCategoryOptions": [ - { - "idScheme": "UID", - "identifier": "xYerKDKCefk" - } - ], - "completedBy": "admin", - "completedAt": "2019-01-28T00:00:00.000", - "dataValues": [ - { - "createdAt": "2019-01-28T12:10:38.113", - "updatedAt": "2019-01-28T12:10:38.113", - "storedBy": "admin", - "providedElsewhere": false, - "dataElement": { - "idScheme": "UID", - "identifier": "DSKTW8qFP0z" - }, - "value": "2017-01-27T17:00:00.000Z" - } - ], - "notes": [] + "event": "D9PbzJY8bJ1" } ], "relationships": [], diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/non_existent_enrollment_basic_data_for_deletion.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/non_existent_enrollment_basic_data_for_deletion.json index 4844d903137e..d22180cdfc69 100644 --- a/dhis-2/dhis-test-integration/src/test/resources/tracker/non_existent_enrollment_basic_data_for_deletion.json +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/non_existent_enrollment_basic_data_for_deletion.json @@ -33,27 +33,7 @@ "trackedEntities": [], "enrollments": [ { - "enrollment": "TvctPPhpD8w", - "createdAtClient": "2017-01-26T13:48:13.363", - "trackedEntity": "NCc1rCEOKaY", - "program": { - "idScheme": "UID", - "identifier": "E8o1E9tAppy" - }, - "status": "ACTIVE", - "orgUnit": { - "idScheme": "UID", - "identifier": "QfUVllTs6cS" - }, - "orgUnitName": "Mbokie CHP", - "enrolledAt": "2021-03-28T12:05:00.000", - "occurredAt": "2021-03-28T12:05:00.000", - "followUp": false, - "deleted": false, - "events": [], - "relationships": [], - "attributes": [], - "notes": [] + "enrollment": "TvctPPhpD8w" } ], "events": [], diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_data_for_deletion.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_data_for_deletion.json index 65b688a9dd26..17581e443dae 100644 --- a/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_data_for_deletion.json +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_data_for_deletion.json @@ -32,63 +32,12 @@ "skipRuleEngine": false, "trackedEntities": [ { - "trackedEntity": "PB9VMezGkwA", - "trackedEntityType": { - "idScheme": "UID", - "identifier": "bPJ0FMtcnEh" - }, - "orgUnit": { - "idScheme": "UID", - "identifier": "QfUVllTs6cS" - }, - "inactive": false, - "deleted": false, - "potentialDuplicate": false, - "relationships": [], - "attributes": [ - { - "attribute": { - "idScheme": "UID", - "identifier": "fmBIpOStKkF" - }, - "storedBy": "admin", - "value": "PersonA" - }, - { - "attribute": { - "idScheme": "UID", - "identifier": "sTJvSLN7Kcb" - }, - "storedBy": "admin", - "value": "AddressA" - } - ], - "enrollments": [] + "trackedEntity": "PB9VMezGkwA" } ], "enrollments": [ { - "enrollment": "TvctPPhpD8u", - "createdAtClient": "2017-01-26T13:48:13.363", - "trackedEntity": "NCc1rCEOKaY", - "program": { - "idScheme": "UID", - "identifier": "E8o1E9tAppy" - }, - "status": "ACTIVE", - "orgUnit": { - "idScheme": "UID", - "identifier": "QfUVllTs6cS" - }, - "orgUnitName": "Mbokie CHP", - "enrolledAt": "2021-03-28T12:05:00.000", - "occurredAt": "2021-03-28T12:05:00.000", - "followUp": false, - "deleted": false, - "events": [], - "relationships": [], - "attributes": [], - "notes": [] + "enrollment": "TvctPPhpD8u" } ], "events": [], From e3ead178c161829299d44e76baf04d09e9844c72 Mon Sep 17 00:00:00 2001 From: Enrico Colasante Date: Tue, 18 Jun 2024 15:39:59 +0200 Subject: [PATCH 2/2] fix: Remove unused check on delete persistables [DHIS2-17402] (#17784) * fix: Remove unused check on delete persistables [DHIS2-17402] * Code review comments * Code review comments --- .../preprocess/EventProgramPreProcessor.java | 5 +- .../dhis/tracker/report/TrackerErrorCode.java | 2 - .../validation/PersistablesFilter.java | 75 ++------ .../validation/PersistablesFilterTest.java | 174 +----------------- .../ReportSummaryDeleteIntegrationTest.java | 45 +++++ ...relationships_basic_data_for_deletion.json | 45 +++++ ...xistent_and_not_existent_for_deletion.json | 45 +++++ ...naccessible_relationship_for_deletion.json | 46 +++++ .../tracker_basic_data_before_deletion.json | 35 +++- .../tracker/tracker_basic_metadata.json | 35 ++++ 10 files changed, 271 insertions(+), 236 deletions(-) create mode 100644 dhis-2/dhis-test-integration/src/test/resources/tracker/relationships_basic_data_for_deletion.json create mode 100644 dhis-2/dhis-test-integration/src/test/resources/tracker/relationships_existent_and_not_existent_for_deletion.json create mode 100644 dhis-2/dhis-test-integration/src/test/resources/tracker/te_with_inaccessible_relationship_for_deletion.json diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java index d88ee9b12f7f..2254192ac8ba 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java @@ -31,6 +31,7 @@ import java.util.List; import java.util.Optional; +import java.util.stream.Collectors; import org.hisp.dhis.program.Program; import org.hisp.dhis.program.ProgramStage; import org.hisp.dhis.tracker.TrackerIdSchemeParams; @@ -57,7 +58,7 @@ public void process(TrackerBundle bundle) { || e.getProgram().isBlank() || e.getProgramStage() == null || e.getProgramStage().isBlank()) - .toList(); + .collect(Collectors.toList()); for (Event event : eventsToPreprocess) { // Extract program from program stage @@ -114,7 +115,7 @@ private void setAttributeOptionCombo(TrackerBundle bundle) { e.getAttributeOptionCombo().isBlank() && !e.getAttributeCategoryOptions().isEmpty()) .filter(e -> preheat.getProgram(e.getProgram()) != null) - .toList(); + .collect(Collectors.toList()); for (Event e : events) { Program program = preheat.getProgram(e.getProgram()); diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/report/TrackerErrorCode.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/report/TrackerErrorCode.java index c2d1ef332d54..3c55526ed36f 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/report/TrackerErrorCode.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/report/TrackerErrorCode.java @@ -181,8 +181,6 @@ public enum TrackerErrorCode { E4020("User: `{0}`, has no write access to relationship: `{1}`."), E5000( "\"{0}\" `{1}` cannot be persisted because \"{2}\" `{3}` referenced by it cannot be persisted."), - E5001( - "\"{0}\" `{1}` cannot be deleted because \"{2}\" `{3}` referenced by it cannot be deleted."), E9999("N/A"); private final String message; diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/validation/PersistablesFilter.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/validation/PersistablesFilter.java index 88beb6639a0b..3acfe8dc5b0d 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/validation/PersistablesFilter.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/validation/PersistablesFilter.java @@ -71,10 +71,9 @@ * TrackerBundle} is not mutated! * *

Errors are only added to {@link Result#errors} if they add information. For example a valid - * entity with invalid children cannot be deleted because of its children. Since the valid parent - * did not already have an error added during validation on will be added here. For more information - * see {@link #addErrorsForParents(List, TrackerDto)} and {@link #addErrorsForChildren(List, - * TrackerDto)}. + * entity with invalid parent cannot be created because of its parent. Since the valid children did + * not already have an error added during validation on will be added here. For more information see + * {@link #addErrorsForChildren(List, TrackerDto)}. * *

This filter relies on preprocessing and all {@link * org.hisp.dhis.tracker.validation.TrackerValidationHook}s having run beforehand. The following are @@ -84,17 +83,18 @@ *

  • This does not validate whether the {@link TrackerImportStrategy} matches the state of a * given entity. In case a non existing entity is updated it is expected to already be flagged * in the {@code invalidEntities}. - *
  • Existence is only checked in relation to a parent or child. During {@link - * TrackerImportStrategy#UPDATE} a valid enrollment can be updated if its parent the TEI is - * invalid but exists. Same applies to {@link TrackerImportStrategy#DELETE} as you cannot - * delete an entity that does not yet exist. + *
  • Existence is only checked in relation to a parent. During {@link + * TrackerImportStrategy#UPDATE} a valid enrollment can be updated if its parent the TE is + * invalid but exists. *
  • An {@link Event} in an event program does not have an {@link Event#getEnrollment()} * (parent) set in the payload. This expects one to be set during preprocessing. So events in * program with or without registration do not get any special treatment. - *
  • {@link TrackerImportStrategy#DELETE} with {@link - * org.hisp.dhis.security.Authorities#F_TEI_CASCADE_DELETE} is not treated differently than a - * delete without such authority. Validation should have already flagged entities in {@code - * invalidEntities} if they have children that cannot be deleted by that user. + *
  • It is not possible to have a valid parent with invalid children in {@link + * TrackerImportStrategy#DELETE} as an entity can be invalid only in 2 cases: + *
      + *
    • if it doesn't exist there are no children to check + *
    • if user has no access to it, then it will not have access to its children + *
    * */ class PersistablesFilter { @@ -160,11 +160,12 @@ private PersistablesFilter( private void filter() { if (onDelete()) { // bottom-up - collectDeletables(Relationship.class, RELATIONSHIP_PARENTS, bundle.getRelationships()); - collectDeletables(Event.class, EVENT_PARENTS, bundle.getEvents()); - collectDeletables(Enrollment.class, ENROLLMENT_PARENTS, bundle.getEnrollments()); - collectDeletables(TrackedEntity.class, TRACKED_ENTITY_PARENTS, bundle.getTrackedEntities()); + collectDeletables(Relationship.class, bundle.getRelationships()); + collectDeletables(Event.class, bundle.getEvents()); + collectDeletables(Enrollment.class, bundle.getEnrollments()); + collectDeletables(TrackedEntity.class, bundle.getTrackedEntities()); } else { + // top-down collectPersistables(TrackedEntity.class, TRACKED_ENTITY_PARENTS, bundle.getTrackedEntities()); collectPersistables(Enrollment.class, ENROLLMENT_PARENTS, bundle.getEnrollments()); @@ -173,16 +174,11 @@ private void filter() { } } - private void collectDeletables( - Class type, List> parents, List entities) { + private void collectDeletables(Class type, List entities) { for (T entity : entities) { - if (isValid(entity) && isDeletable(entity)) { + if (isValid(entity)) { collectPersistable(type, entity); - continue; } - - List errors = addErrorsForParents(parents, entity); - markAsNonDeletable(errors); } } @@ -211,10 +207,6 @@ private boolean isContained(EnumMap> map, TrackerDto en return map.get(entity.getTrackerType()).contains(entity.getUid()); } - private boolean isDeletable(T entity) { - return !isMarked(entity); - } - /** * Collect given entity in the persistables result. This entity can be created, updated or deleted * (depending on context and {@link TrackerImportStrategy}. @@ -240,22 +232,10 @@ private void mark(T entity) { this.markedEntities.get(entity.getTrackerType()).add(entity.getUid()); } - private void mark(TrackerErrorReport error) { - this.markedEntities.get(error.getTrackerType()).add(error.getUid()); - } - private boolean isMarked(T entity) { return isContained(this.markedEntities, entity); } - /** - * Mark parents as non-deletable for potential children. For example an invalid relationship - * (child) referencing a valid tracked entity (parent). - */ - private void markAsNonDeletable(List errors) { - errors.forEach(this::mark); - } - /** * Mark as persistable for later children. For example a tracked entity (parent) referenced by an * enrollment (child). @@ -291,23 +271,6 @@ private Predicate parentConditions( // entities without parents } - /** - * Add error for valid parents in the payload with invalid child as the reason. So users know why - * a valid entity could not be deleted. - */ - private List addErrorsForParents( - List> parents, T entity) { - List errors = - parents.stream() - .map(p -> p.apply(entity)) - .filter(this::isValid) // remove invalid parents - .filter(this.bundle::exists) // remove parents not in payload - .map(p -> error(TrackerErrorCode.E5001, p, entity)) - .collect(Collectors.toList()); - this.result.errors.addAll(errors); - return errors; - } - /** * Add error for valid child entity with invalid parent as a reason. If a child is invalid that is * enough information for a user to know why it could not be persisted. diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/validation/PersistablesFilterTest.java b/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/validation/PersistablesFilterTest.java index 0512ad8f0aa4..316cfe58d538 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/validation/PersistablesFilterTest.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/test/java/org/hisp/dhis/tracker/validation/PersistablesFilterTest.java @@ -32,7 +32,6 @@ import static org.hisp.dhis.tracker.TrackerType.RELATIONSHIP; import static org.hisp.dhis.tracker.TrackerType.TRACKED_ENTITY; import static org.hisp.dhis.tracker.report.TrackerErrorCode.E5000; -import static org.hisp.dhis.tracker.report.TrackerErrorCode.E5001; import static org.hisp.dhis.tracker.validation.PersistablesFilter.filter; import static org.hisp.dhis.tracker.validation.hooks.AssertTrackerValidationReport.assertHasError; import static org.hisp.dhis.utils.Assertions.assertIsEmpty; @@ -374,7 +373,7 @@ void testDeleteValidEntitiesCanBeDeleted() { .enrollment("Ok4Fe5moc3N") .event("Ox1qBWsnVwE") .event("jNyGqnwryNi") - .relationship("Te3IC6TpnBB", trackedEntity("xK7H53f4Hc2"), trackedEntity("QxGbKYwChDM")) + .relationship("Te3IC6TpnBB", null, null) .build(); PersistablesFilter.Result persistable = @@ -390,177 +389,6 @@ void testDeleteValidEntitiesCanBeDeleted() { () -> assertIsEmpty(persistable.getErrors())); } - @Test - void testDeleteInvalidTrackedEntityCannotBeDeletedButItsChildrenCan() { - Setup setup = - new Setup.Builder() - .trackedEntity("xK7H53f4Hc2") - .isNotValid() - .enrollment("t1zaUjKgT3p") - .event("Qck4PQ7TMun") - .event("Ox1qBWsnVwE") - .build(); - - PersistablesFilter.Result persistable = - filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE); - - assertAll( - () -> assertIsEmpty(persistable.get(TrackedEntity.class)), - () -> assertContainsOnly(persistable, Enrollment.class, "t1zaUjKgT3p"), - () -> assertContainsOnly(persistable, Event.class, "Qck4PQ7TMun", "Ox1qBWsnVwE"), - () -> assertIsEmpty(persistable.getErrors())); - } - - @Test - void testDeleteOnlyReportErrorsIfItAddsNewInformation() { - // If entities are found to be invalid during the validation an error for the entity will - // already be in the - // validation report. Only add errors if it would not be clear why an entity cannot be - // persisted. - - Setup setup = - new Setup.Builder() - .trackedEntity("xK7H53f4Hc2") - .isNotValid() - .enrollment("t1zaUjKgT3p") - .isNotValid() - .relationship("Te3IC6TpnBB", trackedEntity("xK7H53f4Hc2"), enrollment("t1zaUjKgT3p")) - .isNotValid() - .build(); - - PersistablesFilter.Result persistable = - filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE); - - assertAll( - () -> assertIsEmpty(persistable.get(TrackedEntity.class)), - () -> assertIsEmpty(persistable.get(Enrollment.class)), - () -> assertIsEmpty(persistable.get(Relationship.class)), - () -> assertIsEmpty(persistable.getErrors())); - } - - @Test - void testDeleteInvalidRelationshipPreventsDeletionOfTrackedEntityAndEvent() { - Setup setup = - new Setup.Builder() - .trackedEntity("xK7H53f4Hc2") - .enrollment("t1zaUjKgT3p") - .event("Qck4PQ7TMun") - .trackedEntity("QxGbKYwChDM") - .enrollment("Ok4Fe5moc3N") - .relationship("Te3IC6TpnBB", trackedEntity("xK7H53f4Hc2"), event("Qck4PQ7TMun")) - .isNotValid() - .build(); - - PersistablesFilter.Result persistable = - filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE); - - assertAll( - () -> assertContainsOnly(persistable, TrackedEntity.class, "QxGbKYwChDM"), - () -> assertContainsOnly(persistable, Enrollment.class, "Ok4Fe5moc3N"), - () -> assertIsEmpty(persistable.get(Event.class)), - () -> assertIsEmpty(persistable.get(Relationship.class)), - () -> - assertError( - persistable, - E5001, - TRACKED_ENTITY, - "xK7H53f4Hc2", - "because \"relationship\" `Te3IC6TpnBB`"), - () -> - assertError( - persistable, E5001, ENROLLMENT, "t1zaUjKgT3p", "because \"event\" `Qck4PQ7TMun`"), - () -> - assertError( - persistable, - E5001, - EVENT, - "Qck4PQ7TMun", - "because \"relationship\" `Te3IC6TpnBB`")); - } - - @Test - void testDeleteInvalidRelationshipPreventsDeletionOfEnrollmentAndEvent() { - Setup setup = - new Setup.Builder() - .trackedEntity("xK7H53f4Hc2") - .enrollment("t1zaUjKgT3p") - .event("QxGbKYwChDM") - .event("Ox1qBWsnVwE") - .enrollment("Ok4Fe5moc3N") - .relationship("Te3IC6TpnBB", event("QxGbKYwChDM"), enrollment("t1zaUjKgT3p")) - .isNotValid() - .build(); - - PersistablesFilter.Result persistable = - filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE); - - assertAll( - () -> assertIsEmpty(persistable.get(TrackedEntity.class)), - () -> assertContainsOnly(persistable, Enrollment.class, "Ok4Fe5moc3N"), - () -> assertContainsOnly(persistable, Event.class, "Ox1qBWsnVwE"), - () -> assertIsEmpty(persistable.get(Relationship.class)), - () -> - assertError( - persistable, - E5001, - ENROLLMENT, - "t1zaUjKgT3p", - "because \"relationship\" `Te3IC6TpnBB`"), - () -> - assertError( - persistable, - E5001, - EVENT, - "QxGbKYwChDM", - "because \"relationship\" `Te3IC6TpnBB`")); - } - - @Test - void testDeleteValidRelationshipWithInvalidFrom() { - Setup setup = - new Setup.Builder() - .trackedEntity("QxGbKYwChDM") - .trackedEntity("xK7H53f4Hc2") - .enrollment("t1zaUjKgT3p") - .isNotValid() - .relationship("Te3IC6TpnBB", enrollment("t1zaUjKgT3p"), trackedEntity("QxGbKYwChDM")) - .build(); - - PersistablesFilter.Result persistable = - filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE); - - assertAll( - () -> assertContainsOnly(persistable, TrackedEntity.class, "QxGbKYwChDM"), - () -> assertIsEmpty(persistable.get(Enrollment.class)), - () -> assertContainsOnly(persistable, Relationship.class, "Te3IC6TpnBB")); - } - - @Test - void testDeleteValidEntitiesReferencingParentsNotInPayload() { - Setup setup = - new Setup.Builder() - .trackedEntity("xK7H53f4Hc2") - .isInDB() - .isNotInPayload() - .enrollment("t1zaUjKgT3p") - .enrollment("Ok4Fe5moc3N") - .isInDB() - .isNotInPayload() - .event("Ox1qBWsnVwE") - .relationship("Te3IC6TpnBB", trackedEntity("xK7H53f4Hc2"), enrollment("Ok4Fe5moc3N")) - .build(); - - PersistablesFilter.Result persistable = - filter(setup.bundle, setup.invalidEntities, TrackerImportStrategy.DELETE); - - assertAll( - () -> assertIsEmpty(persistable.get(TrackedEntity.class)), - () -> assertContainsOnly(persistable, Enrollment.class, "t1zaUjKgT3p"), - () -> assertContainsOnly(persistable, Event.class, "Ox1qBWsnVwE"), - () -> assertContainsOnly(persistable, Relationship.class, "Te3IC6TpnBB"), - () -> assertIsEmpty(persistable.getErrors())); - } - @RequiredArgsConstructor private static class Entity { private boolean valid = true; diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/bundle/ReportSummaryDeleteIntegrationTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/bundle/ReportSummaryDeleteIntegrationTest.java index ef827a61680c..e1ec4da01409 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/bundle/ReportSummaryDeleteIntegrationTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/bundle/ReportSummaryDeleteIntegrationTest.java @@ -28,7 +28,11 @@ package org.hisp.dhis.tracker.bundle; import static org.hisp.dhis.tracker.Assertions.assertHasError; +import static org.hisp.dhis.tracker.Assertions.assertHasOnlyErrors; import static org.hisp.dhis.tracker.Assertions.assertNoErrors; +import static org.hisp.dhis.tracker.report.TrackerErrorCode.E4016; +import static org.hisp.dhis.tracker.report.TrackerErrorCode.E4020; +import static org.hisp.dhis.tracker.validation.Users.USER_10; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -36,6 +40,7 @@ import org.hisp.dhis.program.ProgramInstance; import org.hisp.dhis.program.ProgramStageInstance; import org.hisp.dhis.trackedentity.TrackedEntityInstance; +import org.hisp.dhis.tracker.AtomicMode; import org.hisp.dhis.tracker.TrackerImportParams; import org.hisp.dhis.tracker.TrackerImportService; import org.hisp.dhis.tracker.TrackerImportStrategy; @@ -69,12 +74,52 @@ protected void initTest() throws IOException { assertImportedObjects(13, bundleReport, TrackerType.TRACKED_ENTITY); assertImportedObjects(2, bundleReport, TrackerType.ENROLLMENT); assertImportedObjects(2, bundleReport, TrackerType.EVENT); + assertImportedObjects(2, bundleReport, TrackerType.RELATIONSHIP); assertEquals(6, manager.getAll(ProgramInstance.class).size()); assertEquals( bundleReport.getTypeReportMap().get(TrackerType.EVENT).getStats().getCreated(), manager.getAll(ProgramStageInstance.class).size()); } + @Test + void shouldFailToDeleteNotExistentRelationship() throws IOException { + TrackerImportParams params = + fromJson("tracker/relationships_existent_and_not_existent_for_deletion.json"); + params.setImportStrategy(TrackerImportStrategy.DELETE); + params.setAtomicMode(AtomicMode.OBJECT); + assertEquals(2, params.getRelationships().size()); + + TrackerImportReport trackerImportReport = trackerImportService.importTracker(params); + assertDeletedObjects(1, trackerImportReport.getBundleReport(), TrackerType.RELATIONSHIP); + assertHasOnlyErrors(trackerImportReport, E4016); + } + + @Test + void shouldSuccessfullyDeleteRelationships() throws IOException { + TrackerImportParams params = fromJson("tracker/relationships_basic_data_for_deletion.json"); + params.setImportStrategy(TrackerImportStrategy.DELETE); + assertEquals(2, params.getRelationships().size()); + + TrackerImportReport importReport = trackerImportService.importTracker(params); + assertNoErrors(importReport); + assertDeletedObjects(2, importReport.getBundleReport(), TrackerType.RELATIONSHIP); + } + + @Test + void shouldSuccessfullyDeleteTrackedEntityAndNotAccessibleRelationshipLinkedToIt() + throws IOException { + TrackerImportParams params = + fromJson("tracker/te_with_inaccessible_relationship_for_deletion.json"); + params.setImportStrategy(TrackerImportStrategy.DELETE); + params.setAtomicMode(AtomicMode.OBJECT); + params.setUserId(USER_10); + assertEquals(1, params.getTrackedEntities().size()); + + TrackerImportReport importReport = trackerImportService.importTracker(params); + assertDeletedObjects(1, importReport.getBundleReport(), TrackerType.TRACKED_ENTITY); + assertHasOnlyErrors(importReport, E4020); + } + @Test void testTrackedEntityInstanceDeletion() throws IOException { TrackerImportParams params = fromJson("tracker/tracked_entity_basic_data_for_deletion.json"); diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/relationships_basic_data_for_deletion.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/relationships_basic_data_for_deletion.json new file mode 100644 index 000000000000..906daa90b3eb --- /dev/null +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/relationships_basic_data_for_deletion.json @@ -0,0 +1,45 @@ +{ + "importMode": "COMMIT", + "idSchemes": { + "dataElementIdScheme": { + "idScheme": "UID" + }, + "orgUnitIdScheme": { + "idScheme": "UID" + }, + "programIdScheme": { + "idScheme": "UID" + }, + "programStageIdScheme": { + "idScheme": "UID" + }, + "idScheme": { + "idScheme": "UID" + }, + "categoryOptionComboIdScheme": { + "idScheme": "UID" + }, + "categoryOptionIdScheme": { + "idScheme": "UID" + } + }, + "importStrategy": "CREATE", + "atomicMode": "ALL", + "flushMode": "AUTO", + "validationMode": "FULL", + "skipPatternValidation": false, + "skipSideEffects": false, + "skipRuleEngine": false, + "trackedEntities": [], + "enrollments": [], + "events": [], + "relationships": [ + { + "relationship": "Nva3Xj2j75W" + }, + { + "relationship": "HiXiipNGsxT" + } + ], + "username": "system-process" +} \ No newline at end of file diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/relationships_existent_and_not_existent_for_deletion.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/relationships_existent_and_not_existent_for_deletion.json new file mode 100644 index 000000000000..8d91e56f27bd --- /dev/null +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/relationships_existent_and_not_existent_for_deletion.json @@ -0,0 +1,45 @@ +{ + "importMode": "COMMIT", + "idSchemes": { + "dataElementIdScheme": { + "idScheme": "UID" + }, + "orgUnitIdScheme": { + "idScheme": "UID" + }, + "programIdScheme": { + "idScheme": "UID" + }, + "programStageIdScheme": { + "idScheme": "UID" + }, + "idScheme": { + "idScheme": "UID" + }, + "categoryOptionComboIdScheme": { + "idScheme": "UID" + }, + "categoryOptionIdScheme": { + "idScheme": "UID" + } + }, + "importStrategy": "CREATE", + "atomicMode": "ALL", + "flushMode": "AUTO", + "validationMode": "FULL", + "skipPatternValidation": false, + "skipSideEffects": false, + "skipRuleEngine": false, + "trackedEntities": [], + "enrollments": [], + "events": [], + "relationships": [ + { + "relationship": "Nva3Xj2j79U" + }, + { + "relationship": "Nva3Xj2j75W" + } + ], + "username": "system-process" +} \ No newline at end of file diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/te_with_inaccessible_relationship_for_deletion.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/te_with_inaccessible_relationship_for_deletion.json new file mode 100644 index 000000000000..b92f7b7b71fb --- /dev/null +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/te_with_inaccessible_relationship_for_deletion.json @@ -0,0 +1,46 @@ +{ + "importMode": "COMMIT", + "idSchemes": { + "dataElementIdScheme": { + "idScheme": "UID" + }, + "orgUnitIdScheme": { + "idScheme": "UID" + }, + "programIdScheme": { + "idScheme": "UID" + }, + "programStageIdScheme": { + "idScheme": "UID" + }, + "idScheme": { + "idScheme": "UID" + }, + "categoryOptionComboIdScheme": { + "idScheme": "UID" + }, + "categoryOptionIdScheme": { + "idScheme": "UID" + } + }, + "importStrategy": "CREATE", + "atomicMode": "ALL", + "flushMode": "AUTO", + "validationMode": "FULL", + "skipPatternValidation": false, + "skipSideEffects": false, + "skipRuleEngine": false, + "trackedEntities": [ + { + "trackedEntity": "PB9VMezGkwA" + } + ], + "enrollments": [], + "events": [], + "relationships": [ + { + "relationship": "Nva3Xj2j75W" + } + ], + "username": "system-process" +} \ No newline at end of file diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_basic_data_before_deletion.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_basic_data_before_deletion.json index ed1bcfaea88a..4db573d7d220 100644 --- a/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_basic_data_before_deletion.json +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_basic_data_before_deletion.json @@ -39,7 +39,7 @@ }, "orgUnit": { "idScheme": "UID", - "identifier": "QfUVllTs6cS" + "identifier": "QfUVllTs6cW" }, "inactive": false, "deleted": false, @@ -73,7 +73,7 @@ }, "orgUnit": { "idScheme": "UID", - "identifier": "QfUVllTs6cS" + "identifier": "QfUVllTs6cZ" }, "inactive": false, "deleted": false, @@ -396,6 +396,35 @@ "notes": [] } ], - "relationships": [], + "relationships": [ + { + "relationship": "Nva3Xj2j75W", + "relationshipType": { + "idScheme": "UID", + "identifier": "xLmPUYJX9I7" + }, + "deleted": false, + "from": { + "trackedEntity": "NCc1rCEOKaY" + }, + "to": { + "trackedEntity": "PB9VMezGkwA" + } + }, + { + "relationship": "HiXiipNGsxT", + "relationshipType": { + "idScheme": "UID", + "identifier": "xLmPUYJX9I7" + }, + "deleted": false, + "from": { + "trackedEntity": "NCc1rCEOKaY" + }, + "to": { + "trackedEntity": "N0wuhEwexUW" + } + } + ], "username": "system-process" } \ No newline at end of file diff --git a/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_basic_metadata.json b/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_basic_metadata.json index ca261a1584a3..232c13ac0c17 100644 --- a/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_basic_metadata.json +++ b/dhis-2/dhis-test-integration/src/test/resources/tracker/tracker_basic_metadata.json @@ -382,6 +382,41 @@ } ], "relationshipTypes": [ + { + "created": "2019-04-23T09:20:41.834", + "lastUpdated": "2019-04-23T09:21:18.063", + "name": "Parent", + "code": "PARENT", + "id": "xLmPUYJX9I7", + "bidirectional": false, + "toFromName": "Sibling of", + "fromToName": "Sibling of", + "fromConstraint": { + "relationshipEntity": "TRACKED_ENTITY_INSTANCE", + "trackedEntityType": { + "id": "bPJ0FMtcnEh", + "code": "TA_PERSON_TET" + } + }, + "toConstraint": { + "relationshipEntity": "TRACKED_ENTITY_INSTANCE", + "trackedEntityType": { + "id": "bPJ0FMtcnEh", + "code": "TA_PERSON_TET" + } + }, + "sharing": { + "public": "rw------", + "external": false, + "users": { + "xkMgMi7Ci2D": { + "id": "xkMgMi7Ci2D", + "access": "rwrw----" + } + } + }, + "translations": [] + }, { "created": "2019-04-23T09:20:41.834", "lastUpdated": "2019-04-23T09:21:18.063",