Merge pull request #149 from biigle/patch-1

Fix bug where annotations could be deleted without warning
biigle · Feb 12, 2024 · 7cbb5a0 · 7cbb5a0
2 parents b198fb0 + b40f9f7
commit 7cbb5a0
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 0 deletions.
diff --git 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
@@ -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