From b40f9f7efa017a326a4c9cdf8d1629a580d2d10a Mon Sep 17 00:00:00 2001
From: Martin Zurowietz <martin@zurowietz.de>
Date: Mon, 12 Feb 2024 14:04:27 +0100
Subject: [PATCH] Fix bug where annotations could be deleted without warning

If an annotation was dismissed and then relabeled with the same
label than before, the (new) order of processing the changes would
leave without any label so it would be deleted. These cases are now
filtered out before the changes are processed.

See: https://github.com/orgs/biigle/discussions/774
---
 src/Jobs/ApplyLargoSession.php       | 39 +++++++++++++++++++++
 tests/Jobs/ApplyLargoSessionTest.php | 52 ++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/src/Jobs/ApplyLargoSession.php b/src/Jobs/ApplyLargoSession.php
index 3da07a6f..93d1524c 100644
--- a/src/Jobs/ApplyLargoSession.php
+++ b/src/Jobs/ApplyLargoSession.php
@@ -146,6 +146,11 @@ protected function handleImageAnnotations()
     {
         [$dismissed, $changed] = $this->ignoreDeletedLabels($this->dismissedImageAnnotations, $this->changedImageAnnotations);
 
+        // This is essential, otherwise annotations could be deleted without warning
+        // below (because changes are applied first, then labels are dismissed, then
+        // dangling annotations are deleted)!
+        [$dismissed, $changed] = $this->ignoreNullChanges($dismissed, $changed);
+
         // Change labels first, then dismiss to keep the opportunity to copy feature
         // vectors. If labels are deleted first, the feature vectors will be immediately
         // deleted, too, and nothing can be copied any more.
@@ -161,6 +166,11 @@ protected function handleVideoAnnotations()
     {
         [$dismissed, $changed] = $this->ignoreDeletedLabels($this->dismissedVideoAnnotations, $this->changedVideoAnnotations);
 
+        // This is essential, otherwise annotations could be deleted without warning
+        // below (because changes are applied first, then labels are dismissed, then
+        // dangling annotations are deleted)!
+        [$dismissed, $changed] = $this->ignoreNullChanges($dismissed, $changed);
+
         // Change labels first, then dismiss to keep the opportunity to copy feature
         // vectors. If labels are deleted first, the feature vectors will be immediately
         // deleted, too, and nothing can be copied any more.
@@ -207,6 +217,35 @@ protected function ignoreDeletedLabels($dismissed, $changed)
         return [$dismissed, $changed];
     }
 
+    /**
+     * Removes changes to annotations where the same label was dismissed than should be
+     * attached again later.
+     */
+    protected function ignoreNullChanges(array $dismissed, array $changed): array
+    {
+        foreach ($dismissed as $labelId => $annotationIds) {
+            if (array_key_exists($labelId, $changed)) {
+                $toIgnore = [];
+                foreach ($annotationIds as $id) {
+                    if (array_search($id, $changed[$labelId]) !== false) {
+                        $toIgnore[] = $id;
+                    }
+                }
+
+                $dismissed[$labelId] = array_filter($annotationIds,
+                        fn ($x) => !in_array($x, $toIgnore));
+                $changed[$labelId] = array_filter($changed[$labelId],
+                        fn ($x) => !in_array($x, $toIgnore));
+            }
+        }
+
+        // Remove elements that may now be emepty.
+        $dismissed = array_filter($dismissed);
+        $changed = array_filter($changed);
+
+        return [$dismissed, $changed];
+    }
+
     /**
      * Detach annotation labels that were dismissed in a Largo session.
      *
diff --git a/tests/Jobs/ApplyLargoSessionTest.php b/tests/Jobs/ApplyLargoSessionTest.php
index 1041e5f3..a05697f3 100644
--- a/tests/Jobs/ApplyLargoSessionTest.php
+++ b/tests/Jobs/ApplyLargoSessionTest.php
@@ -672,6 +672,58 @@ public function testChangeVideoAnnotationCopyFeatureVector()
         $this->assertEquals($l1->id, $vectors[0]->label_id);
         $this->assertEquals($vector1->vector, $vectors[0]->vector);
     }
+
+    public function testDismissButChangeBackToSameLabelImage()
+    {
+        $user = UserTest::create();
+        $image = ImageTest::create();
+        $a1 = ImageAnnotationTest::create(['image_id' => $image->id]);
+        $al1 = ImageAnnotationLabelTest::create([
+            'annotation_id' => $a1->id,
+            'user_id' => $user->id,
+        ]);
+
+        $a2 = ImageAnnotationTest::create(['image_id' => $image->id]);
+        $al2 = ImageAnnotationLabelTest::create([
+            'annotation_id' => $a2->id,
+            'user_id' => $user->id,
+            'label_id' => $al1->label_id,
+        ]);
+
+        $dismissed = [$al1->label_id => [$a1->id, $a2->id]];
+        $changed = [$al1->label_id => [$a1->id, $a2->id]];
+        $job = new ApplyLargoSession('job_id', $user, $dismissed, $changed, [], [], false);
+        $job->handle();
+
+        $this->assertNotNull($a1->fresh());
+        $this->assertNotNull($a2->fresh());
+    }
+
+    public function testDismissButChangeBackToSameLabelVideo()
+    {
+        $user = UserTest::create();
+        $video = VideoTest::create();
+        $a1 = VideoAnnotationTest::create(['video_id' => $video->id]);
+        $al1 = VideoAnnotationLabelTest::create([
+            'annotation_id' => $a1->id,
+            'user_id' => $user->id,
+        ]);
+
+        $a2 = VideoAnnotationTest::create(['video_id' => $video->id]);
+        $al2 = VideoAnnotationLabelTest::create([
+            'annotation_id' => $a2->id,
+            'user_id' => $user->id,
+            'label_id' => $al1->label_id,
+        ]);
+
+        $dismissed = [$al1->label_id => [$a1->id, $a2->id]];
+        $changed = [$al1->label_id => [$a1->id, $a2->id]];
+        $job = new ApplyLargoSession('job_id', $user, [], [], $dismissed, $changed, false);
+        $job->handle();
+
+        $this->assertNotNull($a1->fresh());
+        $this->assertNotNull($a2->fresh());
+    }
 }
 
 class ApplyLargoSessionStub extends ApplyLargoSession