From 5b7bdf7ff02f82b1836a9724dce17f88a3b214d7 Mon Sep 17 00:00:00 2001 From: Tom Hallett Date: Fri, 8 Dec 2023 16:29:27 +0000 Subject: [PATCH] Second pass code review --- .../tdr/api/service/FileMetadataService.scala | 6 ---- .../tdr/api/routes/ConsignmentRouteSpec.scala | 29 +++++++++---------- .../tdr/api/routes/FileRouteSpec.scala | 16 +++++----- .../tdr/api/service/FileServiceSpec.scala | 14 ++++----- .../tdr/api/utils/TestUtils.scala | 2 +- 5 files changed, 28 insertions(+), 39 deletions(-) diff --git a/src/main/scala/uk/gov/nationalarchives/tdr/api/service/FileMetadataService.scala b/src/main/scala/uk/gov/nationalarchives/tdr/api/service/FileMetadataService.scala index 88bce408d..b14136124 100644 --- a/src/main/scala/uk/gov/nationalarchives/tdr/api/service/FileMetadataService.scala +++ b/src/main/scala/uk/gov/nationalarchives/tdr/api/service/FileMetadataService.scala @@ -145,10 +145,6 @@ object FileMetadataService { val Description = "description" val DescriptionAlternate = "DescriptionAlternate" - /** Default values for these properties stored in FilePropertyValues table. (TDR-2207) Save default values for these properties because TDR currently only supports records which - * are Open, in English, etc. Users agree to these conditions at a consignment level, so it's OK to save these as defaults for every file. They need to be saved so they can be - * included in the export package. The defaults may be removed in future once we let users upload a wider variety of records. - */ val RightsCopyright = "RightsCopyright" val LegalStatus = "LegalStatus" val HeldBy = "HeldBy" @@ -179,8 +175,6 @@ object FileMetadataService { ) } - case class StaticMetadata(name: String, value: String) - case class FileMetadataValue(name: String, value: String) case class AddFileMetadataInput(fileId: UUID, value: String, userId: UUID, filePropertyName: String) diff --git a/src/test/scala/uk/gov/nationalarchives/tdr/api/routes/ConsignmentRouteSpec.scala b/src/test/scala/uk/gov/nationalarchives/tdr/api/routes/ConsignmentRouteSpec.scala index 3b5d91a38..e9219c3c3 100644 --- a/src/test/scala/uk/gov/nationalarchives/tdr/api/routes/ConsignmentRouteSpec.scala +++ b/src/test/scala/uk/gov/nationalarchives/tdr/api/routes/ConsignmentRouteSpec.scala @@ -228,7 +228,7 @@ class ConsignmentRouteSpec extends TestContainerUtils with Matchers with TestReq utils.addFileMetadata("06209e0d-95d0-4f13-8933-e5b9d00eb435", fileOneId, SHA256ServerSideChecksum) utils.addFileMetadata("c4759aae-dc68-45ec-aee1-5a562c7b42cc", fileTwoId, SHA256ServerSideChecksum) - (clientSideProperties ++ staticMetadataProperties).foreach(propertyName => { + (clientSideProperties ++ defaultMetadataProperties).foreach(propertyName => { utils.addFileProperty(propertyName) val value = propertyName match { case ClientSideFileLastModifiedDate => "2021-03-11 12:30:30.592853" @@ -365,7 +365,7 @@ class ConsignmentRouteSpec extends TestContainerUtils with Matchers with TestReq val consignmentId = UUID.fromString("e72d94d5-ae79-4a05-bee9-86d9dea2bcc9") utils.createConsignment(consignmentId, userId) utils.addFileProperty(SHA256ServerSideChecksum) - staticMetadataProperties.foreach(utils.addFileProperty) + defaultMetadataProperties.foreach(utils.addFileProperty) clientSideProperties.foreach(utils.addFileProperty) val topDirectory = UUID.fromString("ce0a51a5-a224-474f-b3a4-df75effd5b34") val subDirectory = UUID.fromString("2753ceca-4df3-436b-8891-78ad38e2e8c5") @@ -374,12 +374,10 @@ class ConsignmentRouteSpec extends TestContainerUtils with Matchers with TestReq utils.createFile(topDirectory, consignmentId, NodeType.directoryTypeIdentifier, "directory", fileRef = Some("REF1")) utils.createFile(subDirectory, consignmentId, NodeType.directoryTypeIdentifier, "subDirectory", parentId = subDirectory.some, fileRef = Some("REF2"), parentRef = Some("REF1")) - // staticMetadataProperties.foreach(p => utils.addFileMetadata(UUID.randomUUID().toString, topDirectory.toString, p.name, p.value)) - addStaticMetaData(utils, topDirectory, false) + addDefaultMetaData(utils, topDirectory, false) utils.addFileMetadata(UUID.randomUUID.toString, topDirectory.toString, ClientSideOriginalFilepath, "directory") - // staticMetadataProperties.foreach(p => utils.addFileMetadata(UUID.randomUUID().toString, subDirectory.toString, p.name, p.value)) - addStaticMetaData(utils, subDirectory, false) + addDefaultMetaData(utils, subDirectory, false) utils.addFileMetadata(UUID.randomUUID.toString, subDirectory.toString, ClientSideOriginalFilepath, "directory") setUpFileAndStandardMetadata(consignmentId, fileId, utils, subDirectory.some, fileRef = Some("REF3")) @@ -521,7 +519,7 @@ class ConsignmentRouteSpec extends TestContainerUtils with Matchers with TestReq val consignmentId = UUID.fromString("e72d94d5-ae79-4a05-bee9-86d9dea2bcc9") utils.createConsignment(consignmentId, userId) utils.addFileProperty(SHA256ServerSideChecksum) - staticMetadataProperties.foreach(utils.addFileProperty) + defaultMetadataProperties.foreach(utils.addFileProperty) clientSideProperties.foreach(utils.addFileProperty) utils.addFileProperty(OriginalFilepath) val originalFilePath = "/an/original/file/path" @@ -837,7 +835,7 @@ class ConsignmentRouteSpec extends TestContainerUtils with Matchers with TestReq } private def setUpConsignments(consignmentParams: List[ConsignmentParams], utils: TestUtils, fileRef: String): Unit = { - (staticMetadataProperties ++ clientSideProperties).foreach(prop => utils.addFileProperty(prop)) + (defaultMetadataProperties ++ clientSideProperties).foreach(prop => utils.addFileProperty(prop)) setUpConsignmentsFor(consignmentParams, utils, userId, fileRef) } @@ -875,7 +873,7 @@ class ConsignmentRouteSpec extends TestContainerUtils with Matchers with TestReq } private def generateMetadataPropertiesForFile(fileId: UUID, utils: TestUtils, addProperty: Boolean): Unit = { - addStaticMetaData(utils, fileId, addProperty) + addDefaultMetaData(utils, fileId, addProperty) addClientSideProperties(utils, fileId, addProperty) } @@ -889,22 +887,21 @@ class ConsignmentRouteSpec extends TestContainerUtils with Matchers with TestReq parentRef: Option[Reference] = None ): UUID = { utils.createFile(fileId, consignmentId, NodeType.directoryTypeIdentifier, fileName, parentId = parentId, fileRef = fileRef, parentRef = parentRef) - addStaticMetaData(utils, fileId, false) - // staticMetadataProperties.foreach(p => utils.addFileMetadata(UUID.randomUUID().toString, fileId.toString, p.name, p.value)) + addDefaultMetaData(utils, fileId, false) utils.addFileMetadata(UUID.randomUUID.toString, fileId.toString, ClientSideOriginalFilepath, fileName) fileId } - private def addStaticMetaData(utils: TestUtils, parentId: UUID, addProperty: Boolean): Unit = { - staticMetadataProperties.foreach { staticMetadataProperty => + private def addDefaultMetaData(utils: TestUtils, parentId: UUID, addProperty: Boolean): Unit = { + defaultMetadataProperties.foreach { defaultMetadataProperty => if (addProperty) { - utils.addFileProperty(staticMetadataProperty) + utils.addFileProperty(defaultMetadataProperty) } utils.addFileMetadata( UUID.randomUUID().toString, parentId.toString, - staticMetadataProperty, - staticMetadataProperty match { + defaultMetadataProperty, + defaultMetadataProperty match { case RightsCopyright => defaultCopyright case LegalStatus => defaultLegalStatus case HeldBy => defaultHeldBy diff --git a/src/test/scala/uk/gov/nationalarchives/tdr/api/routes/FileRouteSpec.scala b/src/test/scala/uk/gov/nationalarchives/tdr/api/routes/FileRouteSpec.scala index ded29a23b..e5ea4ac60 100644 --- a/src/test/scala/uk/gov/nationalarchives/tdr/api/routes/FileRouteSpec.scala +++ b/src/test/scala/uk/gov/nationalarchives/tdr/api/routes/FileRouteSpec.scala @@ -4,9 +4,7 @@ import akka.http.scaladsl.model.headers.OAuth2BearerToken import com.dimafeng.testcontainers.PostgreSQLContainer import io.circe.generic.extras.Configuration import io.circe.generic.extras.auto._ -import org.scalatest.Assertion import org.scalatest.matchers.should.Matchers -import uk.gov.nationalarchives.tdr.api.service.FileMetadataService.{FileReference, FileType, Filename, ParentReference, clientSideProperties} import uk.gov.nationalarchives.tdr.api.utils.TestAuthUtils._ import uk.gov.nationalarchives.tdr.api.utils.TestContainerUtils._ import uk.gov.nationalarchives.tdr.api.utils.TestUtils._ @@ -51,13 +49,13 @@ class FileRouteSpec extends TestContainerUtils with Matchers with TestRequest { "The api" should "add files and metadata entries for files and directories" in withContainers { case container: PostgreSQLContainer => val consignmentId = UUID.fromString("c44f1b9b-1275-4bc3-831c-808c50a0222d") val utils = TestUtils(container.database) - (clientSideProperties ++ serverSideProperties ++ staticMetadataProperties).foreach(utils.addFileProperty) + (clientSideProperties ++ serverSideProperties ++ defaultMetadataProperties).foreach(utils.addFileProperty) utils.createConsignment(consignmentId, userId) - staticMetadataProperties.foreach { smp => + defaultMetadataProperties.foreach { defaultMetadataProperty => utils.createFilePropertyValues( - smp, - smp match { + defaultMetadataProperty, + defaultMetadataProperty match { case RightsCopyright => defaultCopyright case LegalStatus => defaultLegalStatus case HeldBy => defaultHeldBy @@ -72,8 +70,8 @@ class FileRouteSpec extends TestContainerUtils with Matchers with TestRequest { val res = runTestMutationFileMetadata("mutation_alldata_2", validUserToken()) val distinctDirectoryCount = 3 val fileCount = 5 - val expectedCount = ((Filename :: FileType :: FileReference :: ParentReference :: staticMetadataProperties).size * distinctDirectoryCount) + - (staticMetadataProperties.size * fileCount) + + val expectedCount = ((Filename :: FileType :: FileReference :: ParentReference :: defaultMetadataProperties).size * distinctDirectoryCount) + + (defaultMetadataProperties.size * fileCount) + (clientSideProperties.size * fileCount) + (serverSideProperties.size * fileCount) + distinctDirectoryCount @@ -92,7 +90,7 @@ class FileRouteSpec extends TestContainerUtils with Matchers with TestRequest { "The api" should "return file ids matched with sequence ids for addFilesAndMetadata" in withContainers { case container: PostgreSQLContainer => val consignmentId = UUID.fromString("1cd5e07a-34c8-4751-8e81-98edd17d1729") val utils = TestUtils(container.database) - (clientSideProperties ++ serverSideProperties ++ staticMetadataProperties).foreach(utils.addFileProperty) + (clientSideProperties ++ serverSideProperties ++ defaultMetadataProperties).foreach(utils.addFileProperty) utils.createConsignment(consignmentId, userId) val expectedResponse = expectedFilesAndMetadataMutationResponse("data_all") diff --git a/src/test/scala/uk/gov/nationalarchives/tdr/api/service/FileServiceSpec.scala b/src/test/scala/uk/gov/nationalarchives/tdr/api/service/FileServiceSpec.scala index 5d0758fb3..7c250945f 100644 --- a/src/test/scala/uk/gov/nationalarchives/tdr/api/service/FileServiceSpec.scala +++ b/src/test/scala/uk/gov/nationalarchives/tdr/api/service/FileServiceSpec.scala @@ -736,7 +736,7 @@ class FileServiceSpec extends AnyFlatSpec with MockitoSugar with Matchers with S }) val expectedSize = 56 metadataRows.size should equal(expectedSize) - staticMetadataProperties.foreach(prop => { + defaultMetadataProperties.foreach(prop => { metadataRows.count(_.propertyname == prop) should equal(5) }) @@ -822,7 +822,7 @@ class FileServiceSpec extends AnyFlatSpec with MockitoSugar with Matchers with S file.get.parentreference should equal(Some("ref2")) val expectedSize = 56 metadataRows.size should equal(expectedSize) - staticMetadataProperties.foreach(prop => { + defaultMetadataProperties.foreach(prop => { metadataRows.count(_.propertyname == prop) should equal(5) }) @@ -907,7 +907,7 @@ class FileServiceSpec extends AnyFlatSpec with MockitoSugar with Matchers with S }) val expectedSize = 36 metadataRows.size should equal(expectedSize) - staticMetadataProperties.foreach(prop => { + defaultMetadataProperties.foreach(prop => { metadataRows.count(_.propertyname == prop) should equal(3) }) @@ -1341,10 +1341,10 @@ class FileServiceSpec extends AnyFlatSpec with MockitoSugar with Matchers with S } private def mockCustomMetadataValuesResponse(customMetadataMock: CustomMetadataPropertiesRepository): ScalaOngoingStubbing[Future[Seq[FilepropertyvaluesRow]]] = { - val staticMetadataRows = staticMetadataProperties.map(staticMetadata => { + val defaultMetadataRows = defaultMetadataProperties.map(defaultMetadata => { FilepropertyvaluesRow( - staticMetadata, - staticMetadata match { + defaultMetadata, + defaultMetadata match { case RightsCopyright => defaultCopyright case LegalStatus => defaultLegalStatus case HeldBy => defaultHeldBy @@ -1355,6 +1355,6 @@ class FileServiceSpec extends AnyFlatSpec with MockitoSugar with Matchers with S ) }) - when(customMetadataMock.getCustomMetadataValuesWithDefault).thenReturn(Future(staticMetadataRows)) + when(customMetadataMock.getCustomMetadataValuesWithDefault).thenReturn(Future(defaultMetadataRows)) } } diff --git a/src/test/scala/uk/gov/nationalarchives/tdr/api/utils/TestUtils.scala b/src/test/scala/uk/gov/nationalarchives/tdr/api/utils/TestUtils.scala index 735201eba..a654d08e0 100644 --- a/src/test/scala/uk/gov/nationalarchives/tdr/api/utils/TestUtils.scala +++ b/src/test/scala/uk/gov/nationalarchives/tdr/api/utils/TestUtils.scala @@ -579,7 +579,7 @@ object TestUtils { val defaultFileId: UUID = UUID.fromString("07a3a4bd-0281-4a6d-a4c1-8fa3239e1313") val serverSideProperties: List[String] = List(FileReference, ParentReference) - val staticMetadataProperties: List[String] = List(RightsCopyright, LegalStatus, HeldBy, Language, FoiExemptionCode) + val defaultMetadataProperties: List[String] = List(RightsCopyright, LegalStatus, HeldBy, Language, FoiExemptionCode) val defaultCopyright: String = "Crown Copyright" val defaultLegalStatus: String = "Public Record" val defaultHeldBy: String = "TNA"