Fixes literal 'None' encoding of some empty Crop fields.

Treat all fields the same way then explicitly filter out None fields to protect ourselves from any misunderstandings about how collect treats values of type Option.
guardian · Nov 29, 2024 · c393106 · c393106
1 parent 6b57601
commit c393106
Show file tree

Hide file tree

Showing 2 changed files with 9 additions and 4 deletions.
diff --git a/cropper/app/lib/CropSpecMetadata.scala b/cropper/app/lib/CropSpecMetadata.scala
@@ -17,14 +17,19 @@ trait CropSpecMetadata {
       "date" -> crop.date.map(printDateTime),
       "width" -> dimensions.width,
       "height" -> dimensions.height,
-    ) ++ r.map("aspect-ratio" -> _)
+      "aspect-ratio" -> r)
 
-    val nonEmptyMetadata = metadata.collect {
+    val nonEmptyMetadata = metadata.filter {
+      case (_, None) => false
+      case _ => true
+    }
+
+    val flattenedMetadata = nonEmptyMetadata.collect {
       case (key, Some(value)) => key -> value
       case (key, value) => key -> value
     }.view.mapValues(_.toString).toMap
 
-    nonEmptyMetadata
+    flattenedMetadata
   }
 
   def cropSpecFromMetadata(userMetadata: Map[String, String]): Option[CropSpec] = {

diff --git a/cropper/test/lib/CropSpecMetadataTest.scala b/cropper/test/lib/CropSpecMetadataTest.scala
@@ -37,7 +37,7 @@ class CropSpecMetadataTest extends AnyFunSpec with Matchers with CropSpecMetadat
       val metadata = metadataForCrop(crop.copy(specification = withEmptyField, author = None), dimensions)
 
       metadata.get("aspect-ratio") shouldBe None
-      metadata.get("author") shouldBe Some("None")  // TODO this does not look intentional!
+      metadata.get("author") shouldBe None
     }
 
     it("should round trip metadata back to crop spec") {