Pull request feedback

.github/workflows/deploy.yml

@@ -5,7 +5,7 @@ jobs:
   deploy:
     uses: nationalarchives/tdr-github-actions/.github/workflows/sbt_release.yml@main
     with:
-      library-name: "AWS utils"
+      library-name: "Metadata validation"
     secrets:
       WORKFLOW_PAT: ${{ secrets.WORKFLOW_PAT }}
       SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }}

build.sbt

@@ -1,7 +1,6 @@
 import Dependencies._
 import sbt.url
-import sbtrelease.ReleaseStateTransformations.{checkSnapshotDependencies, commitNextVersion, commitReleaseVersion, inquireVersions, pushChanges, runClean, runTest, setNextVersion, setReleaseVersion, tagRelease}
-
+import sbtrelease.ReleaseStateTransformations._
 
 ThisBuild / organization := "uk.gov.nationalarchives"
 ThisBuild / organizationName := "National Archives"
@@ -26,7 +25,7 @@ developers := List(
   )
 )
 
-ThisBuild / description := "A library to validate input metadata for the Transfer Digital Records"
+ThisBuild / description := "A library to validate input metadata for Transfer Digital Records"
 ThisBuild / licenses := List("MIT" -> new URL("https://choosealicense.com/licenses/mit/"))
 ThisBuild / homepage := Some(url("https://github.com/nationalarchives/tdr-metadata-validation"))
 

src/main/scala/uk/gov/nationalarchives/tdr/validation/DataType.scala

@@ -5,9 +5,9 @@ import uk.gov.nationalarchives.tdr.validation.ErrorCode._
 import java.time.{LocalDateTime, Year}
 import scala.util.control.Exception.allCatch
 
-sealed trait DataType extends AnyRef
+sealed trait DataType
 
-case object Integer extends AnyRef with DataType with Product with Serializable {
+case object Integer extends DataType with Product with Serializable {
   def checkValue(value: String, criteria: MetadataCriteria): Option[String] = {
     value match {
       case "" if criteria.required            => Some(EMPTY_VALUE_ERROR)
@@ -18,14 +18,18 @@ case object Integer extends AnyRef with DataType with Product with Serializable
   }
 }
 
-case object DateTime extends AnyRef with DataType with Product with Serializable {
+case object DateTime extends DataType with Product with Serializable {
   def checkValue(value: String, criteria: MetadataCriteria): Option[String] = {
     value match {
       case "" if criteria.required  => Some(EMPTY_VALUE_ERROR)
       case "" if !criteria.required => None
       case v =>
         val date = v.replace("T", "-").split("[-:]")
-        validate(date(2), date(1), date(0), criteria)
+        if (date.length < 6) {
+          Some(INVALID_DATE_FORMAT_ERROR)
+        } else {
+          validate(date(2), date(1), date(0), criteria)
+        }
     }
   }
 
@@ -110,7 +114,7 @@ case object DateTime extends AnyRef with DataType with Product with Serializable
     }
 }
 
-case object Text extends AnyRef with DataType with Product with Serializable {
+case object Text extends DataType with Product with Serializable {
 
   def checkValue(value: String, criteria: MetadataCriteria): Option[String] = {
     val definedValues = criteria.definedValues
@@ -123,7 +127,7 @@ case object Text extends AnyRef with DataType with Product with Serializable {
   }
 }
 
-case object Boolean extends AnyRef with DataType with Product with Serializable {
+case object Boolean extends DataType with Product with Serializable {
   def checkValue(value: String, criteria: MetadataCriteria, requiredMetadata: Option[Metadata]): Option[String] = {
     value match {
       case "" if criteria.required =>
@@ -142,7 +146,7 @@ case object Boolean extends AnyRef with DataType with Product with Serializable
     criteria.requiredProperty.isDefined && requiredMetadata.exists(_.value.isEmpty)
   }
 }
-case object Decimal extends AnyRef with DataType with Product with Serializable
+case object Decimal extends DataType with Product with Serializable
 
 object DataType {
   def get(dataType: String): DataType = {

src/main/scala/uk/gov/nationalarchives/tdr/validation/MetadataCriteria.scala

@@ -26,7 +26,6 @@ object ErrorCode {
   val EMPTY_VALUE_ERROR = "EMPTY_VALUE_ERROR"
   val NO_OPTION_SELECTED_ERROR = "NO_OPTION_SELECTED_ERROR"
   val INVALID_DATE_FORMAT_ERROR = "INVALID_DATE_FORMAT_ERROR"
-  val INVALID_NUMBER_ERROR = "INVALID_NUMBER_ERROR"
   val EMPTY_VALUE_ERROR_FOR_DAY = "EMPTY_VALUE_ERROR_FOR_DAY"
   val NUMBER_ERROR_FOR_DAY = "NUMBER_ERROR_FOR_DAY"
   val NEGATIVE_NUMBER_ERROR_FOR_DAY = "NEGATIVE_NUMBER_ERROR_FOR_DAY"

src/test/scala/uk/gov/nationalarchives/tdr/validation/DataTypeSpec.scala

@@ -8,7 +8,7 @@ class DataTypeSpec extends AnyWordSpec {
 
   "Integer" should {
 
-    val criteria = MetadataCriteria("Property1", Integer, true, false, false, Nil, None, None)
+    val criteria = MetadataCriteria("Property1", Integer, required = true, isFutureDateAllowed = false, isMultiValueAllowed = false, Nil, None, None)
 
     "checkValue should return an error if the value is empty" in {
       Integer.checkValue("", criteria) should be(Some(EMPTY_VALUE_ERROR))
@@ -22,30 +22,35 @@ class DataTypeSpec extends AnyWordSpec {
       Integer.checkValue("-1", criteria) should be(Some(NEGATIVE_NUMBER_ERROR))
     }
 
-    "checkValue should not return any error if the value is a valid number" in {
+    "checkValue should not return any errors if the value is a valid number" in {
       Integer.checkValue("1", criteria) should be(None)
     }
   }
 
   "DateTime" should {
 
-    val criteria = MetadataCriteria("Property1", DateTime, true, false, false, Nil, None, None)
+    val criteria = MetadataCriteria("Property1", DateTime, required = true, isFutureDateAllowed = false, isMultiValueAllowed = false, Nil, None, None)
 
-    "checkValue should not return amy error if the date is valid" in {
+    "checkValue should not return any errors if the date is valid" in {
       DateTime.checkValue("1990-12-10T00:00:00", criteria) should be(None)
       DateTime.checkValue("2000-2-29T00:00:00", criteria) should be(None)
     }
 
-    "checkValue should not return amy error if the value is empty but it is not mandatory" in {
+    "checkValue should not return any errors if the value is empty but it is not mandatory" in {
       DateTime.checkValue("", criteria.copy(required = false)) should be(None)
     }
 
-    "checkValue should not return amy error if future date is allowed" in {
+    "checkValue should not return any errors if future date is allowed" in {
       DateTime.checkValue("1990-12-10T00:00:00", criteria.copy(isFutureDateAllowed = true)) should be(None)
       DateTime.checkValue("2090-12-10T00:00:00", criteria.copy(isFutureDateAllowed = true)) should be(None)
     }
 
-    "checkValue should return an error if the value is empty" in {
+    "checkValue should return an error if the date format is invalid" in {
+      DateTime.checkValue("1990/12/10T00:00:00", criteria) should be(Some(INVALID_DATE_FORMAT_ERROR))
+      DateTime.checkValue("2000-2-29", criteria) should be(Some(INVALID_DATE_FORMAT_ERROR))
+    }
+
+    "checkValue should return an error if the value is empty and mandatory" in {
       DateTime.checkValue("", criteria) should be(Some(EMPTY_VALUE_ERROR))
       DateTime.checkValue("--T00:00:00", criteria) should be(Some(EMPTY_VALUE_ERROR))
     }
@@ -78,42 +83,42 @@ class DataTypeSpec extends AnyWordSpec {
 
   "Text" should {
 
-    val criteria = MetadataCriteria("Property1", Text, true, false, false, Nil, None, None)
+    val criteria = MetadataCriteria("Property1", Text, required = true, isFutureDateAllowed = false, isMultiValueAllowed = false, Nil, None, None)
 
-    "checkValue should not return any error if the value is valid" in {
+    "checkValue should not return any errors if the value is valid" in {
       Text.checkValue("hello", criteria) should be(None)
     }
 
-    "checkValue should not return any error if the value is empty but it is not mandatory" in {
+    "checkValue should not return any errors if the value is empty but it is not mandatory" in {
       Text.checkValue("", criteria.copy(required = false)) should be(None)
     }
 
     "checkValue should return an error if the value is empty" in {
       Text.checkValue("", criteria) should be(Some(EMPTY_VALUE_ERROR))
     }
 
-    "checkValue should not return any error if the value has multiple values but multiple values are allowed" in {
+    "checkValue should not return any errors if the value has multiple values but multiple values are allowed" in {
       Text.checkValue("22,44", criteria.copy(definedValues = List("22", "44"), isMultiValueAllowed = true)) should be(None)
     }
 
-    "checkValue should return an error if the value has multiple value but multiple values are not allowed" in {
+    "checkValue should return an error if the value has multiple values but multiple values are not allowed" in {
       Text.checkValue("22,44", criteria.copy(definedValues = List("22", "44"))) should be(Some(MULTI_VALUE_ERROR))
     }
 
-    "checkValue should return an error if the value is not matching with defined values" in {
+    "checkValue should return an error if the value is not matching its defined values" in {
       Text.checkValue("22,44", criteria.copy(definedValues = List("22", "33"), isMultiValueAllowed = true)) should be(Some(UNDEFINED_VALUE_ERROR))
     }
   }
 
   "Boolean" should {
 
-    val criteria = MetadataCriteria("Property1", Boolean, true, false, false, List("yes", "no"), None, None)
+    val criteria = MetadataCriteria("Property1", Boolean, required = true, isFutureDateAllowed = false, isMultiValueAllowed = false, List("yes", "no"), None, None)
 
-    "checkValue should not return any error if the value is valid" in {
+    "checkValue should not return any errors if the value is valid" in {
       Boolean.checkValue("yes", criteria, None) should be(None)
     }
 
-    "checkValue should not return any error if the value is empty but required property is empty too" in {
+    "checkValue should not return any errors if the value is empty but required property is empty too" in {
       Boolean.checkValue("", criteria.copy(requiredProperty = Some("property2")), Some(Metadata("property2", ""))) should be(None)
     }
 
@@ -125,13 +130,12 @@ class DataTypeSpec extends AnyWordSpec {
       Boolean.checkValue("", criteria.copy(requiredProperty = Some("property2")), Some(Metadata("property2", "value"))) should be(Some(NO_OPTION_SELECTED_ERROR))
     }
 
-    "checkValue should return an error if the value is not matching with the defined values" in {
+    "checkValue should return an error if the value doesn't match its defined values" in {
       Boolean.checkValue("true", criteria, None) should be(Some(UNDEFINED_VALUE_ERROR))
     }
 
     "checkValue should return an error if the required property value is empty" in {
       Boolean.checkValue("yes", criteria.copy(requiredProperty = Some("property2")), Some(Metadata("property2", ""))) should be(Some(REQUIRED_PROPERTY_IS_EMPTY))
     }
-
   }
 }

src/test/scala/uk/gov/nationalarchives/tdr/validation/MetadataValidationSpec.scala

@@ -7,7 +7,7 @@ import uk.gov.nationalarchives.tdr.validation.MetadataProperty.closureType
 
 class MetadataValidationSpec extends AnyFlatSpec {
 
-  "validateClosureMetadata" should "validate closure metadata with valid values" in {
+  "validateClosureMetadata" should "validate closure metadata" in {
 
     val dependentMetadataCriteria = List(
       MetadataCriteria("Property1", Boolean, true, false, false, List("yes", "no"), None, None),
@@ -57,7 +57,7 @@ class MetadataValidationSpec extends AnyFlatSpec {
     error should be(List(Error(closureType, CLOSURE_STATUS_IS_MISSING)))
   }
 
-  "validateClosureMetadata" should "not return any error if closure status is open and closure metadata has empty or default value" in {
+  "validateClosureMetadata" should "not return any errors if closure status is open and closure metadata has empty or default values" in {
 
     val dependentMetadataCriteria = List(
       MetadataCriteria("Property1", Boolean, true, false, false, List("yes", "no"), defaultValue = Some("no")),
@@ -153,7 +153,7 @@ class MetadataValidationSpec extends AnyFlatSpec {
     )
   }
 
-  "validateClosureMetadata" should "not return an error if a closure metadata dependent by a descriptive metadata which is empty" in {
+  "validateClosureMetadata" should "not return an error if closure metadata is dependent on descriptive metadata which is empty" in {
 
     val dependentMetadataCriteria = List(
       MetadataCriteria(
@@ -179,7 +179,7 @@ class MetadataValidationSpec extends AnyFlatSpec {
     error should be(Nil)
   }
 
-  "validateDescriptiveMetadata" should "validate descriptive metadata with valid values" in {
+  "validateDescriptiveMetadata" should "validate descriptive metadata" in {
 
     val descriptiveMetadataCriteria = List(
       MetadataCriteria("Property1", Text, false, false, false, Nil, None, None),
@@ -197,7 +197,7 @@ class MetadataValidationSpec extends AnyFlatSpec {
     error should be(Nil)
   }
 
-  "validateMetadata" should "validate closure & descriptive metadata" in {
+  "validateMetadata" should "validate closure and descriptive metadata" in {
 
     val descriptiveMetadataCriteria = List(
       MetadataCriteria("Property21", Text, false, false, false, Nil, None, None),