Merge pull request #4252 from nationalarchives/TDRD-563-missing-colum…

…ns-error

Tdrd 563 missing columns error

README.md

@@ -260,5 +260,29 @@ If you need to add a new query:
 * Run `sbt package publishLocal`
 * Set the version for `tdr-generated-graphql` in this projects build.sbt to be the snapshot version.
 
+## Draft Metadata Feature: Running and Testing Locally
+
+### Uploading from localhost
+
+When uploading a metadata csv from `localhost` ensure have a valid set of AWS credentials for the TDR integration environment, or other environment if connecting your localhost to a different TDR environment.
+
+### CSV UTF-8 on Linux Machines
+
+The draft metadata feature uses the BOM to check that the uploaded metadata CSV is UTF-8 encoded.
+
+For software such a Libre office the BOM is not added to CSV files and therefore any file uploaded from a Linux machine will fail this validation check.
+
+See here for further details: [Metadata CSV file UTF-8 Validation](https://github.com/nationalarchives/tdr-dev-documentation/blob/master/architecture-decision-records/0035-metadata-csv-utf-8-file-validation.md)
+
+To add the BOM to a CSV file run the following commands:
+
+```
+  printf '\xEF\xBB\xBF' > with_bom.txt
+  cat {file name of csv}.csv >> with_bom.txt
+  mv with_bom.txt {file name of csv}.csv
+```
+
+These commands need to be run each time the file is saved.
+
 ## Notes
 * Each environment has its own secret for the auth server. These cannot be generated inside AWS in any way and so it's difficult to get them into the Terraform scripts. At the moment, these are stored in a parameter store variable called /${env}/auth/secret although this may change.

app/controllers/DraftMetadataChecksResultsController.scala

@@ -10,7 +10,7 @@ import play.api.i18n.{I18nSupport, Lang, MessagesApi}
 import play.api.mvc.{Action, AnyContent, Request}
 import services.FileError.SCHEMA_VALIDATION
 import services.Statuses._
-import services.{FileError, _}
+import services._
 import viewsapi.Caching.preventCaching
 
 import java.time.LocalDateTime
@@ -40,7 +40,9 @@ class DraftMetadataChecksResultsController @Inject() (
       for {
         reference <- consignmentService.getConsignmentRef(consignmentId, request.token.bearerAccessToken)
         consignmentStatuses <- consignmentStatusService.getConsignmentStatuses(consignmentId, token)
-        errorType <- getErrorType(consignmentStatuses, consignmentId)
+        draftMetadataStatus = consignmentStatuses.find(_.statusType == DraftMetadataType.id).map(_.value)
+        errorReport <- getErrorReport(draftMetadataStatus, consignmentId)
+        errorType = getErrorType(errorReport, draftMetadataStatus)
       } yield {
         val resultsPage = {
           // leaving original page for no errors
@@ -64,7 +66,8 @@ class DraftMetadataChecksResultsController @Inject() (
                   reference,
                   request.token.name,
                   actionMessage(errorType),
-                  detailsMessage(errorType)
+                  detailsMessage(errorType),
+                  getAffectedProperties(errorReport)
                 )
             }
           }
@@ -92,23 +95,34 @@ class DraftMetadataChecksResultsController @Inject() (
       s"Require details message for $key"
   }
 
+  private def getAffectedProperties(errorReport: Option[ErrorFileData]): Set[String] = {
+    errorReport match {
+      case Some(er) if er.fileError == FileError.SCHEMA_REQUIRED => er.validationErrors.flatMap(_.errors.map(_.property)).toSet
+      case _                                                     => Set()
+    }
+  }
+
   private def isErrorReportAvailable(fileError: FileError.FileError): Boolean = {
     fileError match {
       case SCHEMA_VALIDATION => true
       case _                 => false
     }
   }
 
-  private def getErrorType(consignmentStatuses: List[GetConsignment.ConsignmentStatuses], consignmentId: UUID): Future[FileError.Value] = {
-    val draftMetadataStatus = consignmentStatuses.find(_.statusType == DraftMetadataType.id).map(_.value)
-    if (draftMetadataStatus.isDefined) {
-      draftMetadataStatus.get match {
-        case CompletedValue.value => Future.successful(FileError.NONE)
-        case FailedValue.value    => Future.successful(FileError.UNKNOWN)
-        case _                    => draftMetadataService.getErrorTypeFromErrorJson(consignmentId)
-      }
-    } else {
-      Future.successful(FileError.UNKNOWN)
+  private def getErrorReport(draftMetadataStatus: Option[String], consignmentId: UUID): Future[Option[ErrorFileData]] = {
+    draftMetadataStatus match {
+      case Some(s) if s == CompletedValue.value || s == FailedValue.value => Future.successful(None)
+      case Some(_)                                                        => draftMetadataService.getErrorReport(consignmentId).map(Some(_))
+      case None                                                           => Future.successful(None)
+    }
+  }
+
+  private def getErrorType(errorReport: Option[ErrorFileData], draftMetadataStatus: Option[String]): FileError.Value = {
+    draftMetadataStatus match {
+      case Some(s) if s == CompletedValue.value => FileError.NONE
+      case Some(s) if s == FailedValue.value    => FileError.UNKNOWN
+      case Some(_) if errorReport.isDefined     => errorReport.get.fileError
+      case _                                    => FileError.UNKNOWN
     }
   }
 

app/views/draftmetadata/draftMetadataChecksErrorsNoDownload.scala.html

@@ -1,12 +1,12 @@
 @import views.html.partials._
 
 @import java.util.UUID
-@(consignmentId: UUID, consignmentRef: String, name: String, actionMessage: String, detailsMessage: String)(implicit request: RequestHeader, messages: Messages)
+@(consignmentId: UUID, consignmentRef: String, name: String, actionMessage: String, detailsMessage: String, affectedProperties: Set[String])(implicit request: RequestHeader, messages: Messages)
 
 @main("Results of CSV Checks", name = name) {
     <div class="govuk-grid-row">
         <div class="govuk-grid-column-two-thirds">
-            @draftMetadataChecksActionProcess(actionMessage, detailsMessage)
+            @draftMetadataChecksActionProcess(actionMessage, detailsMessage, affectedProperties)
             <p class="govuk-body">Once you have addressed this issue upload a revised metadata file.</p>
 
             <div class="govuk-button-group">

app/views/partials/draftMetadataChecksActionProcess.scala.html

@@ -1,4 +1,4 @@
-@(actionMessage: String, detailsMessage: String)(implicit request: RequestHeader, messages: Messages)
+@(actionMessage: String, detailsMessage: String, affectedProperties: Set[String] = Set())(implicit request: RequestHeader, messages: Messages)
 
 <h1 class="govuk-heading-l">
     Results of your metadata checks
@@ -31,7 +31,14 @@ <h1 class="govuk-heading-l">
             Action
         </dt>
         <dd class="govuk-summary-list__value">
-            @Html(actionMessage)
+            <p>@Html(actionMessage)</p>
+            @if(affectedProperties.nonEmpty) {
+                <ul class="govuk-list govuk-list--bullet">
+                    @for(property <- affectedProperties) {
+                        <li>@property</li>
+                    }
+                </ul>
+            }
         </dd>
     </div>
 </dl>

conf/messages

@@ -44,3 +44,5 @@ draftMetadata.validation.details.INVALID_CSV=The metadata file you uploaded was
 draftMetadata.validation.action.INVALID_CSV=Ensure that you save your Excel file as file type ''CSV UTF-8 (comma separated)''.
 draftMetadata.validation.details.VIRUS=A virus was found in your metadata file.
 draftMetadata.validation.action.VIRUS=Please get in touch on <a href="mailto:tdr@nationalarchives.gov.uk">tdr@nationalarchives.gov.uk</a>, so that we can validate this before contacting your own internal support.
+draftMetadata.validation.details.SCHEMA_REQUIRED=There was at least one missing column in your metadata file. The metadata file must contain specific column headers.
+draftMetadata.validation.action.SCHEMA_REQUIRED=Add the following column headers to your metadata file and re-load.

test/controllers/DraftMetadataChecksResultsControllerSpec.scala

@@ -16,6 +16,7 @@ import play.api.i18n.DefaultMessagesApi
 import play.api.test.CSRFTokenHelper._
 import play.api.test.FakeRequest
 import play.api.test.Helpers.{contentAsBytes, contentAsString, defaultAwaitTimeout, status => playStatus, _}
+import services.FileError.FileError
 import services.Statuses.{CompletedValue, CompletedWithIssuesValue, DraftMetadataType, FailedValue}
 import services.{ConsignmentService, ConsignmentStatusService, DraftMetadataService, Error, ErrorFileData, FileError, ValidationErrors}
 import testUtils.FrontEndTestHelper
@@ -50,7 +51,8 @@ class DraftMetadataChecksResultsControllerSpec extends FrontEndTestHelper {
   "DraftMetadataChecksResultsController GET" should {
     "render page not found error when 'blockDraftMetadataUpload' set to 'true'" in {
       val controller = instantiateController()
-      val additionalMetadataEntryMethodPage = controller.draftMetadataChecksResultsPage(consignmentId).apply(FakeRequest(GET, "/draft-metadata/checks-results").withCSRFToken)
+      val additionalMetadataEntryMethodPage =
+        controller.draftMetadataChecksResultsPage(consignmentId).apply(FakeRequest(GET, "/draft-metadata/checks-results").withCSRFToken)
       setConsignmentTypeResponse(wiremockServer, "standard")
 
       val pageAsString = contentAsString(additionalMetadataEntryMethodPage)
@@ -62,7 +64,8 @@ class DraftMetadataChecksResultsControllerSpec extends FrontEndTestHelper {
 
     "return forbidden for a TNA user" in {
       val controller = instantiateController(blockDraftMetadataUpload = false, keycloakConfiguration = getValidTNAUserKeycloakConfiguration())
-      val additionalMetadataEntryMethodPage = controller.draftMetadataChecksResultsPage(consignmentId).apply(FakeRequest(GET, "/draft-metadata/checks-results").withCSRFToken)
+      val additionalMetadataEntryMethodPage =
+        controller.draftMetadataChecksResultsPage(consignmentId).apply(FakeRequest(GET, "/draft-metadata/checks-results").withCSRFToken)
       setConsignmentTypeResponse(wiremockServer, "standard")
 
       playStatus(additionalMetadataEntryMethodPage) mustBe FORBIDDEN
@@ -174,20 +177,33 @@ class DraftMetadataChecksResultsControllerSpec extends FrontEndTestHelper {
 
   "DraftMetadataChecksResultsController should render the error page with no error download for some errors" should {
     val draftMetadataStatuses = Table(
-      ("status", "fileError", "detailsMessage", "actionMessage"),
-      (FailedValue.value, FileError.UNKNOWN, "Require details message for draftMetadata.validation.details.", "Require action message for draftMetadata.validation.action."),
+      ("status", "fileError", "detailsMessage", "actionMessage", "affectedProperties"),
+      (
+        FailedValue.value,
+        FileError.UNKNOWN,
+        "Require details message for draftMetadata.validation.details.",
+        "Require action message for draftMetadata.validation.action.",
+        Set[String]()
+      ),
       (
         CompletedWithIssuesValue.value,
         FileError.UTF_8,
         "The metadata file was not a CSV in UTF-8 format.",
-        s"Ensure that you save your Excel file as file type 'CSV UTF-8 (comma separated)'."
+        s"Ensure that you save your Excel file as file type 'CSV UTF-8 (comma separated)'.",
+        Set[String]()
+      ),
+      (
+        CompletedWithIssuesValue.value,
+        FileError.SCHEMA_REQUIRED,
+        "There was at least one missing column in your metadata file. The metadata file must contain specific column headers.",
+        "Add the following column headers to your metadata file and re-load.",
+        Set("Closure Status", "Description")
       )
     )
-    forAll(draftMetadataStatuses) { (statusValue, fileError, detailsMessage, actionMessage) =>
+    forAll(draftMetadataStatuses) { (statusValue, fileError, detailsMessage, actionMessage, affectedProperties) =>
       {
         s"render the draftMetadataResults page when the status is $statusValue and error is $fileError" in {
-          val controller = instantiateController(blockDraftMetadataUpload = false, fileError = fileError)
-          when(draftMetaDataService.getErrorTypeFromErrorJson(any[UUID])).thenReturn(Future.successful(fileError))
+          val controller = instantiateController(blockDraftMetadataUpload = false, fileError = fileError, affectedProperties = affectedProperties)
           val additionalMetadataEntryMethodPage = controller
             .draftMetadataChecksResultsPage(consignmentId)
             .apply(FakeRequest(GET, "/draft-metadata/checks-results").withCSRFToken)
@@ -212,6 +228,7 @@ class DraftMetadataChecksResultsControllerSpec extends FrontEndTestHelper {
           )
           pageAsString must include(detailsMessage)
           pageAsString must include(actionMessage)
+          affectedProperties.foreach(p => pageAsString must include(s"<li>$p</li>"))
         }
       }
     }
@@ -224,10 +241,8 @@ class DraftMetadataChecksResultsControllerSpec extends FrontEndTestHelper {
         val error = Error("BASE_SCHEMA", "FOI exemption code", "enum", "BASE_SCHEMA.foi_exmption_code.enum")
         val metadata = List(Metadata("FOI exemption code", "abcd"), Metadata("Filepath", "/aa/bb/faq"))
         val errorFileData = ErrorFileData(consignmentId, date = "2024-12-12", FileError.SCHEMA_VALIDATION, List(ValidationErrors("assetId", Set(error), metadata)))
-
-        when(draftMetaDataService.getErrorReport(any[UUID])).thenReturn(Future.successful(errorFileData))
-
-        val response = instantiateController().downloadErrorReport(consignmentId)(FakeRequest(GET, s"/consignment/$consignmentId/draft-metadata/download-report"))
+        val response = instantiateController(errorFileData = Some(errorFileData))
+          .downloadErrorReport(consignmentId)(FakeRequest(GET, s"/consignment/$consignmentId/draft-metadata/download-report"))
 
         val responseByteArray: ByteString = contentAsBytes(response)
         val bufferedSource = new ByteArrayInputStream(responseByteArray.toArray)
@@ -279,14 +294,18 @@ class DraftMetadataChecksResultsControllerSpec extends FrontEndTestHelper {
       securityComponents: SecurityComponents = getAuthorisedSecurityComponents,
       keycloakConfiguration: KeycloakConfiguration = getValidStandardUserKeycloakConfiguration,
       blockDraftMetadataUpload: Boolean = true,
-      fileError: FileError.FileError = FileError.UNKNOWN
+      fileError: FileError.FileError = FileError.UNKNOWN,
+      affectedProperties: Set[String] = Set(),
+      errorFileData: Option[ErrorFileData] = None
   ): DraftMetadataChecksResultsController = {
     when(configuration.get[Boolean]("featureAccessBlock.blockDraftMetadataUpload")).thenReturn(blockDraftMetadataUpload)
     val applicationConfig: ApplicationConfig = new ApplicationConfig(configuration)
     val graphQLConfiguration = new GraphQLConfiguration(app.configuration)
     val consignmentService = new ConsignmentService(graphQLConfiguration)
     val consignmentStatusService = new ConsignmentStatusService(graphQLConfiguration)
-    when(draftMetaDataService.getErrorTypeFromErrorJson(any[UUID])).thenReturn(Future.successful(fileError))
+    val mockedErrorFileData = if (errorFileData.isDefined) { errorFileData.get }
+    else mockErrorFileData(fileError, affectedProperties)
+    when(draftMetaDataService.getErrorReport(any[UUID])).thenReturn(Future.successful(mockedErrorFileData))
 
     val properties = new Properties()
     Using(Source.fromFile("conf/messages")) { source =>
@@ -309,4 +328,20 @@ class DraftMetadataChecksResultsControllerSpec extends FrontEndTestHelper {
       messagesApi
     )
   }
+
+  private def mockErrorFileData(fileError: FileError = FileError.UNKNOWN, affectedProperties: Set[String] = Set()): ErrorFileData = {
+    if (affectedProperties.nonEmpty) {
+      val validationErrors = {
+        val errors = affectedProperties.map(p => {
+          val convertedProperty = p.replaceAll(" ", "_").toLowerCase()
+          Error("SCHEMA_REQUIRED", p, "required", s"SCHEMA_REQUIRED.$convertedProperty.required")
+        })
+        ValidationErrors(UUID.randomUUID().toString, errors)
+      }
+      ErrorFileData(consignmentId, "", fileError, List(validationErrors))
+
+    } else {
+      ErrorFileData(consignmentId, "", fileError, List())
+    }
+  }
 }