Merge pull request #1069 from biigle/patch-1

Fix clearing of metadata parser when file is deleted
biigle · Feb 4, 2025 · 9ae6df4 · 9ae6df4
2 parents 6b11dc6 + 358b88a
commit 9ae6df4
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 11 deletions.
diff --git a/app/Services/Reports/Volumes/IfdoReportGenerator.php b/app/Services/Reports/Volumes/IfdoReportGenerator.php
@@ -171,7 +171,7 @@ abstract protected function getLabels();
      */
     protected function hasIfdo(Volume $source): bool
     {
-        return $source->metadata_parser === IfdoParser::class;
+        return $source->metadata_file_path && $source->metadata_parser === IfdoParser::class;
     }
 
     /**
@@ -184,10 +184,6 @@ abstract public function processFiles();
      */
     protected function getIfdo(Volume $source): ?Ifdo
     {
-        if (!$source->metadata_file_path) {
-            return null;
-        }
-
         $content = Storage::disk($source->getMetadataFileDisk())
             ->get($source->metadata_file_path);
 

diff --git a/app/Traits/HasMetadataFile.php b/app/Traits/HasMetadataFile.php
@@ -71,7 +71,10 @@ public function deleteMetadata($noUpdate = false): void
             $disk = $this->getMetadataFileDisk();
             Cache::store('array')->forget("metadata-{$disk}-{$this->metadata_file_path}");
             if (!$noUpdate) {
-                $this->update(['metadata_file_path' => null]);
+                $this->update([
+                    'metadata_file_path' => null,
+                    'metadata_parser' => null,
+                ]);
             }
         }
     }

diff --git a/database/migrations/2025_02_04_095139_fix_volume_metadata_parser_null.php b/database/migrations/2025_02_04_095139_fix_volume_metadata_parser_null.php
@@ -0,0 +1,25 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+
+return new class extends Migration {
+    /**
+     * Run the migrations.
+     */
+    public function up(): void
+    {
+        // When a metadata file was deleted, the file path was set to null but not the
+        // parser. This lead to inconsistent data.
+        Biigle\Volume::wherenotNull('metadata_parser')
+            ->whereNull('metadata_file_path')
+            ->update(['metadata_parser' => null]);
+    }
+
+    /**
+     * Reverse the migrations.
+     */
+    public function down(): void
+    {
+        //
+    }
+};
diff --git a/tests/php/Services/Reports/Volumes/ImageIfdoReportGeneratorTest.php b/tests/php/Services/Reports/Volumes/ImageIfdoReportGeneratorTest.php
@@ -769,9 +769,22 @@ public function testGenerateReportRestrictToLabels()
         $this->assertSame($expect, $generator->ifdo);
     }
 
-    public function testGenerateReportNoIfdo()
+    public function testGenerateReportNoIfdoParser()
     {
-        $volume = Volume::factory()->create();
+        $volume = Volume::factory()->create([
+            'metadata_file_path' => 'abc',
+        ]);
+        $generator = new ImageIfdoReportGeneratorStub();
+        $generator->setSource($volume);
+        $this->expectException(Exception::class);
+        $generator->generateReport('my/path');
+    }
+
+    public function testGenerateReportNoIfdoPath()
+    {
+        $volume = Volume::factory()->create([
+            'metadata_parser' => IfdoParser::class,
+        ]);
         $generator = new ImageIfdoReportGeneratorStub();
         $generator->setSource($volume);
         $this->expectException(Exception::class);

diff --git a/tests/php/Services/Reports/Volumes/VideoIfdoReportGeneratorTest.php b/tests/php/Services/Reports/Volumes/VideoIfdoReportGeneratorTest.php
@@ -710,9 +710,22 @@ public function testGenerateReportRestrictToLabels()
         $this->assertSame($expect, $generator->ifdo);
     }
 
-    public function testGenerateReportNoIfdo()
+    public function testGenerateReportNoIfdoParser()
     {
-        $volume = Volume::factory()->create();
+        $volume = Volume::factory()->create([
+            'metadata_file_path' => 'abc',
+        ]);
+        $generator = new VideoIfdoReportGeneratorStub();
+        $generator->setSource($volume);
+        $this->expectException(Exception::class);
+        $generator->generateReport('my/path');
+    }
+
+    public function testGenerateReportNoIfdoPath()
+    {
+        $volume = Volume::factory()->create([
+            'metadata_parser' => IfdoParser::class,
+        ]);
         $generator = new VideoIfdoReportGeneratorStub();
         $generator->setSource($volume);
         $this->expectException(Exception::class);

diff --git a/tests/php/VolumeTest.php b/tests/php/VolumeTest.php
@@ -580,10 +580,13 @@ public function testDeleteMetadata()
         $disk = Storage::fake('metadata');
         $disk->put($this->model->id.'.csv', 'abc');
         $this->model->metadata_file_path = $this->model->id.'.csv';
+        $this->model->metadata_parser = 'myparser';
         $this->model->save();
         $this->model->deleteMetadata();
         $disk->assertMissing($this->model->id.'.csv');
-        $this->assertNull($this->model->fresh()->metadata_file_path);
+        $this->model->refresh();
+        $this->assertNull($this->model->metadata_file_path);
+        $this->assertNull($this->model->metadata_parser);
     }
 
     public function testGetMetadata()