fix: Remove unused check on delete persistables [DHIS2-17402] (2.39)

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();
+  }
 }

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<BundlePreProcessor> preProcessors) {
   @Override
   public TrackerBundle preprocess(TrackerBundle bundle) {
     for (BundlePreProcessor preProcessor : preProcessors) {
-      preProcessor.process(bundle);
+      if (preProcessor.needsToRun(bundle.getImportStrategy())) {
+        preProcessor.process(bundle);
+      }
     }
 
     return bundle;

dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/preprocess/EventProgramPreProcessor.java

@@ -27,8 +27,9 @@
  */
 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;
@@ -51,14 +52,19 @@ public class EventProgramPreProcessor implements BundlePreProcessor {
   public void process(TrackerBundle bundle) {
     List<Event> eventsToPreprocess =
         bundle.getEvents().stream()
-            .filter(e -> e.getProgram().isBlank() || e.getProgramStage().isBlank())
+            .filter(
+                e ->
+                    e.getProgram() == null
+                        || e.getProgram().isBlank()
+                        || e.getProgramStage() == null
+                        || e.getProgramStage().isBlank())
             .collect(Collectors.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 +90,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> programStage = program.getProgramStages().stream().findFirst();
           if (programStage.isPresent()) {
             TrackerIdSchemeParams idSchemes = bundle.getPreheat().getIdSchemes();

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;
+  }
 }

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;

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!
  *
  * <p>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)}.
  *
  * <p>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 @@
  *   <li>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}.
- *   <li>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.
+ *   <li>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.
  *   <li>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.
- *   <li>{@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.
+ *   <li>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:
+ *       <ul>
+ *         <li>if it doesn't exist there are no children to check
+ *         <li>if user has no access to it, then it will not have access to its children
+ *       </ul>
  * </ul>
  */
 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 <T extends TrackerDto> void collectDeletables(
-      Class<T> type, List<Function<T, TrackerDto>> parents, List<T> entities) {
+  private <T extends TrackerDto> void collectDeletables(Class<T> type, List<T> entities) {
     for (T entity : entities) {
-      if (isValid(entity) && isDeletable(entity)) {
+      if (isValid(entity)) {
         collectPersistable(type, entity);
-        continue;
       }
-
-      List<TrackerErrorReport> errors = addErrorsForParents(parents, entity);
-      markAsNonDeletable(errors);
     }
   }
 
@@ -211,10 +207,6 @@ private boolean isContained(EnumMap<TrackerType, Set<String>> map, TrackerDto en
     return map.get(entity.getTrackerType()).contains(entity.getUid());
   }
 
-  private <T extends TrackerDto> 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 <T extends TrackerDto> 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 <T extends TrackerDto> 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<TrackerErrorReport> 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 <T extends TrackerDto> Predicate<T> 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 <T extends TrackerDto> List<TrackerErrorReport> addErrorsForParents(
-      List<Function<T, TrackerDto>> parents, T entity) {
-    List<TrackerErrorReport> 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.

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))