Merge pull request #2145 from DFE-Digital/virus-scan-feedback

Show virus scanning feedback to users

app/components/status_tag/component.rb

@@ -32,6 +32,7 @@ def tags
       cannot_start: "grey",
       declined: "red",
       draft: "grey",
+      error: "red",
       expired: "pink",
       in_progress: "blue",
       invalid: "red",

app/components/task_list/component.html.erb

@@ -1,6 +1,6 @@
 <ol class="app-task-list">
   <% sections.each do |section| %>
-    <li>
+    <li id="app-task-list-<%= section[:key] %>">
       <% if (title = section[:title]).present? %>
         <h2 class="app-task-list__section">
           <% if sections.count > 1 %>

app/controllers/assessor_interface/consent_requests_controller.rb

@@ -80,8 +80,7 @@ def update_upload
     end
 
     def check_upload
-      unless consent_request.unsigned_consent_document.completed? &&
-               consent_request.unsigned_consent_document.downloadable?
+      unless consent_request.unsigned_consent_document.completed?
         history_stack.pop
 
         redirect_to [

app/controllers/assessor_interface/documents_controller.rb

@@ -13,8 +13,8 @@ class DocumentsController < BaseController
     def show_pdf
       authorize %i[assessor_interface application_form]
 
-      unless document.downloadable?
-        render "shared/malware_scan"
+      unless document.completed?
+        render "errors/forbidden", status: :forbidden
         return
       end
 

app/controllers/assessor_interface/uploads_controller.rb

@@ -13,10 +13,10 @@ class UploadsController < BaseController
     def show
       authorize %i[assessor_interface application_form]
 
-      if upload.downloadable?
+      if upload.safe_to_link?
         send_blob_stream(upload.attachment, disposition: :inline)
       else
-        render "shared/malware_scan"
+        render "errors/forbidden", status: :forbidden
       end
     end
 

app/controllers/teacher_interface/documents_controller.rb

@@ -13,25 +13,34 @@ class DocumentsController < BaseController
 
     def show
       if document.uploads.empty? && !document.optional?
-        redirect_to new_teacher_interface_application_form_document_upload_path(
+        redirect_to [
+                      :new,
+                      :teacher_interface,
+                      :application_form,
                       document,
-                    )
+                      :upload,
+                    ]
       else
-        redirect_to edit_teacher_interface_application_form_document_path(
-                      document,
-                    )
+        redirect_to [:edit, :teacher_interface, :application_form, document]
       end
     end
 
     def edit
-      if show_available_form?
-        @form =
+      @form =
+        if show_available_form?
           DocumentAvailableForm.new(document:, available: document.available)
-        render :edit_available
-      else
-        @form = AddAnotherUploadForm.new
-        render :edit_uploads
+        else
+          AddAnotherUploadForm.new
+        end
+
+      if document.any_unsafe_to_link?
+        @form.errors.add(
+          :uploads,
+          I18n.t("teacher_interface.documents.unsafe_to_link"),
+        )
       end
+
+      render show_available_form? ? :edit_available : :edit_uploads
     end
 
     def update
@@ -46,7 +55,7 @@ def update
               ),
           )
         else
-          TeacherInterface::AddAnotherUploadForm.new(
+          AddAnotherUploadForm.new(
             add_another:
               params.dig(
                 :teacher_interface_add_another_upload_form,
@@ -55,6 +64,13 @@ def update
           )
         end
 
+      if document.any_unsafe_to_link?
+        @form.errors.add(
+          :uploads,
+          I18n.t("teacher_interface.documents.unsafe_to_link"),
+        )
+      end
+
       handle_application_form_section(
         form: @form,
         if_success_then_redirect: ->(check_path) do

app/controllers/teacher_interface/uploads_controller.rb

@@ -19,10 +19,10 @@ class UploadsController < BaseController
     before_action :load_upload, only: %i[delete destroy show]
 
     def show
-      if @upload.downloadable?
+      if @upload.safe_to_link?
         send_blob_stream(@upload.attachment, disposition: :inline)
       else
-        render "shared/malware_scan"
+        render "errors/forbidden", status: :forbidden
       end
     end
 

app/forms/concerns/uploadable_form.rb

@@ -24,25 +24,27 @@ module UploadableForm
   def create_uploads!
     document.uploads.each(&:destroy!) unless document.allow_multiple_uploads?
 
+    uploads = []
+
     if original_attachment.present?
-      document.uploads.create!(
+      uploads << document.uploads.create!(
         attachment: original_attachment,
         filename: original_attachment.original_filename,
+        malware_scan_result: malware_scan_result(original_attachment),
         translation: false,
       )
     end
 
     if translated_attachment.present?
-      document.uploads.create!(
+      uploads << document.uploads.create!(
         attachment: translated_attachment,
         filename: translated_attachment.original_filename,
+        malware_scan_result: malware_scan_result(translated_attachment),
         translation: true,
       )
     end
 
-    if FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result)
-      fetch_and_update_malware_scan_results
-    end
+    fetch_and_update_malware_scan_results(uploads)
   end
 
   private
@@ -61,9 +63,23 @@ def attachments_present
     end
   end
 
-  def fetch_and_update_malware_scan_results
-    document.uploads.each do |upload|
-      UpdateMalwareScanResultJob.set(wait: 2.seconds).perform_later(upload)
+  def fetch_and_update_malware_scan_results(uploads)
+    if FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result)
+      uploads
+        .select(&:malware_scan_pending?)
+        .each do |upload|
+          UpdateMalwareScanResultJob.set(wait: 2.seconds).perform_later(upload)
+        end
+    end
+  end
+
+  def malware_scan_result(attachment)
+    if FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result)
+      "pending"
+    elsif attachment.original_filename == "virus.pdf"
+      "suspect"
+    else
+      "clean"
     end
   end
 end

app/forms/teacher_interface/add_another_upload_form.rb

@@ -1,11 +1,9 @@
 # frozen_string_literal: true
 
-module TeacherInterface
-  class AddAnotherUploadForm < BaseForm
-    attribute :add_another, :boolean
-    validates :add_another, inclusion: { in: [true, false] }
+class TeacherInterface::AddAnotherUploadForm < TeacherInterface::BaseForm
+  attribute :add_another, :boolean
+  validates :add_another, inclusion: { in: [true, false] }
 
-    def update_model
-    end
+  def update_model
   end
 end

app/helpers/document_helper.rb

@@ -4,34 +4,26 @@ module DocumentHelper
   include UploadHelper
 
   def document_link_to(document, translated: false)
-    scope =
-      (
-        if translated
-          document.translated_uploads
-        else
-          document.original_uploads
-        end
-      )
-
     if document.optional? && !document.available
-      "<em>The applicant has indicated that they haven't done an induction period " \
-        "and don't have this document.</em>".html_safe
+      tag.em(
+        "The applicant has indicated that they haven't " \
+          "done an induction period and don't have this document.",
+      )
     else
       uploads =
-        scope.order(:created_at).select { |upload| upload.attachment.present? }
-
-      malware_scan_active =
-        FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result)
-
-      items = uploads.map { |upload| upload_link_to(upload) }
-
-      if malware_scan_active && scope.malware_scan_suspect.exists?
-        items << tag.em(
-          "#{scope.count} #{"file upload".pluralize(scope.count)} has been scanned as malware and deleted.",
+        (
+          if translated
+            document.translated_uploads
+          else
+            document.original_uploads
+          end
         )
-      end
 
-      tag.ul(class: "govuk-list") { items.map { |item| concat(tag.li(item)) } }
+      tag.ul(class: "govuk-list") do
+        uploads
+          .order(:created_at)
+          .each { |upload| concat(tag.li(upload_link_to(upload))) }
+      end
     end
   end
 end

app/helpers/upload_helper.rb

@@ -2,19 +2,23 @@
 
 module UploadHelper
   def upload_link_to(upload)
-    href =
+    if upload.unsafe_to_link?
+      tag.div(class: "govuk-form-group--error") do
+        tag.p(upload.filename, class: "govuk-!-font-weight-bold") +
+          tag.p(
+            I18n.t("teacher_interface.documents.unsafe_to_link"),
+            class: "govuk-error-message",
+          )
+      end
+    elsif upload.safe_to_link?
       govuk_link_to(
         "#{upload.filename} (opens in a new tab)",
         upload_path(upload),
         target: :_blank,
         rel: :noopener,
       )
-
-    if upload.malware_scan_error? || upload.malware_scan_suspect?
-      href +
-        tag.p("There’s a problem with this file", class: "govuk-error-message")
     else
-      href
+      upload.filename
     end
   end
 

app/lib/application_form_section_status_updater.rb

@@ -73,7 +73,7 @@ def personal_information_status
   end
 
   def identification_document_status
-    identification_document.completed? ? :completed : :not_started
+    identification_document.status
   end
 
   def qualifications_status
@@ -84,7 +84,13 @@ def qualifications_status
       return :in_progress
     end
 
-    qualifications.all?(&:complete?) ? :completed : :in_progress
+    if qualifications.any?(&:any_documents_unsafe_to_link?)
+      :error
+    elsif qualifications.all?(&:complete?)
+      :completed
+    else
+      :in_progress
+    end
   end
 
   def age_range_status
@@ -105,16 +111,20 @@ def english_language_status
 
     case english_language_proof_method
     when "medium_of_instruction"
-      status_for_document(
-        english_language_medium_of_instruction_document,
-        not_started: :in_progress,
-      )
+      if (status = english_language_medium_of_instruction_document.status) !=
+           "not_started"
+        status
+      else
+        "in_progress"
+      end
     when "provider"
       if english_language_provider_other
-        status_for_document(
-          english_language_proficiency_document,
-          not_started: :in_progress,
-        )
+        if (status = english_language_proficiency_document.status) !=
+             "not_started"
+          status
+        else
+          "in_progress"
+        end
       else
         status_for_values(
           english_language_proof_method,
@@ -167,7 +177,7 @@ def written_statement_status
     if teaching_authority_provides_written_statement
       written_statement_confirmation ? :completed : :not_started
     else
-      status_for_document(written_statement_document)
+      written_statement_document.status
     end
   end
 
@@ -176,14 +186,4 @@ def status_for_values(*values)
     return :completed if values.all?(&:present?)
     :in_progress
   end
-
-  def status_for_document(document, not_started: :not_started)
-    if document.completed?
-      :completed
-    elsif !document.available.nil?
-      :in_progress
-    else
-      not_started
-    end
-  end
 end

app/models/application_form.rb

@@ -155,9 +155,10 @@ class ApplicationForm < ApplicationRecord
   ].freeze
 
   STATUS_VALUES = {
-    not_started: "not_started",
-    in_progress: "in_progress",
     completed: "completed",
+    error: "error",
+    in_progress: "in_progress",
+    not_started: "not_started",
   }.freeze
 
   STATUS_COLUMNS.each { |column| enum column, STATUS_VALUES, prefix: column }