From 93ff262c3b89357285dc16247ed147f9aff8a05d Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Fri, 22 Dec 2023 11:09:36 +0000 Subject: [PATCH 01/11] Gem install librecov This is a library which can convert DOC/DOCX/ODT in to PDFs using Libreoffice, which has been added as a dependency of the docker image. --- Dockerfile | 3 ++- Gemfile | 1 + Gemfile.lock | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index c2abc4f241..83cdbfb2ea 100644 --- a/Dockerfile +++ b/Dockerfile @@ -82,7 +82,8 @@ RUN apk upgrade --no-cache openssl libssl3 libcrypto3 curl # libpq: required to run postgres # vips-dev: dependencies for ruby-vips (image processing library) -RUN apk add --update --no-cache libpq vips-dev +# libreoffice-writer: for converting word documents to PDF +RUN apk add --update --no-cache libpq vips-dev libreoffice-writer # Copy files generated in the builder image COPY --from=builder /app /app diff --git a/Gemfile b/Gemfile index 06d66accb5..7c1ac4c395 100644 --- a/Gemfile +++ b/Gemfile @@ -17,6 +17,7 @@ gem "email_address" gem "faraday" gem "indefinite_article" gem "jsbundling-rails" +gem "libreconv" gem "matrix" gem "okcomputer" gem "omniauth-azure-activedirectory-v2" diff --git a/Gemfile.lock b/Gemfile.lock index e8ed3b4393..331ad7b4f0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -288,6 +288,7 @@ GEM json (2.7.1) jwt (2.7.1) language_server-protocol (3.17.0.3) + libreconv (0.9.5) loofah (2.22.0) crass (~> 1.0.2) nokogiri (>= 1.12.0) @@ -621,6 +622,7 @@ DEPENDENCIES govuk_markdown indefinite_article jsbundling-rails + libreconv mail-notify matrix okcomputer From 33ff011a394e4da8bd8f03193eae461307676d68 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Fri, 22 Dec 2023 11:24:05 +0000 Subject: [PATCH 02/11] Gem install rmagick This allows us to convert images to PDFs using ImageMagick which has been added as a dependency in the docker image. --- Dockerfile | 6 ++++-- Gemfile | 1 + Gemfile.lock | 4 ++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 83cdbfb2ea..ce8b914a5f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -20,7 +20,8 @@ RUN apk upgrade --no-cache openssl libssl3 libcrypto3 curl # postgresql-dev: postgres driver and libraries # git: dependencies for bundle # vips-dev: dependencies for ruby-vips (image processing library) -RUN apk add --update --no-cache build-base yarn postgresql14-dev git vips-dev +# imagemagick-dev: dependencies for rmagick (image conversion library) +RUN apk add --update --no-cache build-base yarn postgresql14-dev git vips-dev imagemagick-dev # Install gems defined in Gemfile COPY Gemfile Gemfile.lock ./ @@ -83,7 +84,8 @@ RUN apk upgrade --no-cache openssl libssl3 libcrypto3 curl # libpq: required to run postgres # vips-dev: dependencies for ruby-vips (image processing library) # libreoffice-writer: for converting word documents to PDF -RUN apk add --update --no-cache libpq vips-dev libreoffice-writer +# imagemagick-dev and imagemagick-pdf: for converting images to PDF +RUN apk add --update --no-cache libpq vips-dev libreoffice-writer imagemagick-dev imagemagick-pdf # Copy files generated in the builder image COPY --from=builder /app /app diff --git a/Gemfile b/Gemfile index 7c1ac4c395..0dc6228fec 100644 --- a/Gemfile +++ b/Gemfile @@ -29,6 +29,7 @@ gem "propshaft" gem "puma" gem "pundit" gem "rack-attack" +gem "rmagick" gem "ruby-vips" gem "sentry-rails" gem "sentry-ruby" diff --git a/Gemfile.lock b/Gemfile.lock index 331ad7b4f0..72bae97f92 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -363,6 +363,7 @@ GEM racc pdf-core (0.9.0) pg (1.5.4) + pkg-config (1.5.6) prawn (2.4.0) pdf-core (~> 0.9.0) ttfunk (~> 1.7) @@ -445,6 +446,8 @@ GEM retriable (3.1.2) rexml (3.2.6) rladr (1.2.0) + rmagick (5.3.0) + pkg-config (~> 1.4) rspec (3.12.0) rspec-core (~> 3.12.0) rspec-expectations (~> 3.12.0) @@ -638,6 +641,7 @@ DEPENDENCIES rack-attack rails (= 7.1.2) rladr + rmagick rspec rspec-rails rubocop-govuk From 92f2380955d07161c10c348b330c8edafa0fcaf2 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 10 Jan 2024 11:04:49 +0000 Subject: [PATCH 03/11] Gem install combine_pdf This allows us to combine individual PDF uploads in to a single document. --- Gemfile | 1 + Gemfile.lock | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/Gemfile b/Gemfile index 0dc6228fec..2c1a6cf8a3 100644 --- a/Gemfile +++ b/Gemfile @@ -9,6 +9,7 @@ gem "activerecord-session_store" gem "azure-storage-blob" gem "bootsnap", require: false gem "business" +gem "combine_pdf" gem "cssbundling-rails" gem "devise" gem "devise_invitable" diff --git a/Gemfile.lock b/Gemfile.lock index 72bae97f92..e4617b1428 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -142,6 +142,9 @@ GEM xpath (~> 3.2) cgi (0.4.1) climate_control (1.2.0) + combine_pdf (1.0.26) + matrix + ruby-rc4 (>= 0.1.5) concurrent-ruby (1.2.2) connection_pool (2.4.1) crack (0.4.5) @@ -504,6 +507,7 @@ GEM rubocop-capybara (~> 2.17) rubocop-factory_bot (~> 2.22) ruby-progressbar (1.13.0) + ruby-rc4 (0.1.5) ruby-vips (2.2.0) ffi (~> 1.12) ruby2_keywords (0.0.5) @@ -606,6 +610,7 @@ DEPENDENCIES business capybara climate_control + combine_pdf cssbundling-rails cuprite debug From 622763f009f5db43513a571d2314d39f11f339f2 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Tue, 9 Jan 2024 15:27:29 +0000 Subject: [PATCH 04/11] Install fonts in the Docker image This allows LibreOffice to read ODT or DOCX files so they can be converted to PDFs. --- Dockerfile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Dockerfile b/Dockerfile index ce8b914a5f..f7c8b4a81a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -87,6 +87,10 @@ RUN apk upgrade --no-cache openssl libssl3 libcrypto3 curl # imagemagick-dev and imagemagick-pdf: for converting images to PDF RUN apk add --update --no-cache libpq vips-dev libreoffice-writer imagemagick-dev imagemagick-pdf +# Install fonts suitable for rendering DOCX and ODT files to PDF +# https://wiki.alpinelinux.org/wiki/Fonts#Installation +RUN apk add font-terminus font-bitstream-100dpi font-bitstream-75dpi font-bitstream-type1 font-noto font-noto-extra font-arabic-misc font-misc-cyrillic font-mutt-misc font-screen-cyrillic font-winitzki-cyrillic font-cronyx-cyrillic font-noto-arabic font-noto-armenian font-noto-cherokee font-noto-devanagari font-noto-ethiopic font-noto-georgian font-noto-hebrew font-noto-lao font-noto-malayalam font-noto-tamil font-noto-thaana font-noto-thai + # Copy files generated in the builder image COPY --from=builder /app /app COPY --from=builder /usr/local/bundle/ /usr/local/bundle/ From 87877cf5207cb8857a75734a9dbaeeab7c489b7d Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Tue, 9 Jan 2024 10:02:56 +0000 Subject: [PATCH 05/11] Add convert_documents_to_pdf feature When enabled this feature will show the link to convert a document to a single PDF. --- config/feature_flags.yml | 6 ++++++ lib/tasks/review_app.rake | 3 +++ 2 files changed, 9 insertions(+) diff --git a/config/feature_flags.yml b/config/feature_flags.yml index f862f65e6a..bd00806908 100644 --- a/config/feature_flags.yml +++ b/config/feature_flags.yml @@ -1,4 +1,10 @@ feature_flags: + convert_documents_to_pdf: + author: Thomas Leese + description: > + Provide a way of automatically converting documents to a single PDF suitable for + uploading to the Ecctis portal. + fetch_malware_scan_result: author: Steve Laing description: > diff --git a/lib/tasks/review_app.rake b/lib/tasks/review_app.rake index 5d0358b8e6..bed2590306 100644 --- a/lib/tasks/review_app.rake +++ b/lib/tasks/review_app.rake @@ -1,3 +1,5 @@ +# frozen_string_literal: true + namespace :review_app do desc "Set some default configuration in the review environment" task configure: :environment do @@ -5,6 +7,7 @@ namespace :review_app do raise "THIS TASK CANNOT BE RUN IN PRODUCTION" end + FeatureFlags::FeatureFlag.activate(:convert_documents_to_pdf) FeatureFlags::FeatureFlag.activate(:personas) FeatureFlags::FeatureFlag.activate(:teacher_applications) end From 6c468e55c4feb8f6c49a68a01203abf7a7d67965 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 28 Dec 2023 10:26:27 +0000 Subject: [PATCH 06/11] Simplify malware scan result page This simplifies the page by moving the locale strings out of the teacher interface (since this page is also shown to assessors). --- app/views/shared/malware_scan.html.erb | 13 +++++++------ config/locales/en.yml | 10 ++++++++++ config/locales/teacher_interface.en.yml | 11 ----------- .../teacher_interface/malware_scanning_spec.rb | 13 ------------- 4 files changed, 17 insertions(+), 30 deletions(-) diff --git a/app/views/shared/malware_scan.html.erb b/app/views/shared/malware_scan.html.erb index 5ba2c1a57c..bb2f55a35b 100644 --- a/app/views/shared/malware_scan.html.erb +++ b/app/views/shared/malware_scan.html.erb @@ -1,8 +1,9 @@ -<% content_for :page_title, t("teacher_interface.uploads.malware_scan.#{@upload.malware_scan_result}.title") %> -<% content_for :back_link_url, url_for(:back) %> +<% content_for :page_title, t(@upload.malware_scan_result, scope: %i[malware_scan title]) %> -

<%= t("teacher_interface.uploads.malware_scan.#{@upload.malware_scan_result}.title") %>

+

+ <%= t(@upload.malware_scan_result, scope: %i[malware_scan title]) %> +

-

<%= t("teacher_interface.uploads.malware_scan.#{@upload.malware_scan_result}.body") %>

- -

<%= govuk_link_to "Back to application", :back %>

+

+ <%= t(@upload.malware_scan_result, scope: %i[malware_scan body]) %> +

diff --git a/config/locales/en.yml b/config/locales/en.yml index 4e70000ec9..847c7b4831 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -39,6 +39,16 @@ en: qualification_document: qualification written_statement: written statement + malware_scan: + title: + error: There’s a problem with your file + pending: We’re checking your file + suspect: There’s a problem with your file + body: + error: There’s a problem with this file. You’ll need to take another image of the document you want to upload, then try again. + pending: We’re carrying out some checks on the file you’ve uploaded. Once we’ve successfully completed these checks, you’ll be able to preview your file. + suspect: There’s a problem with this file. You’ll need to take another image of the document you want to upload, then try again. + staff: omniauth_callbacks: failure: There was a problem signing you in. Please try again. diff --git a/config/locales/teacher_interface.en.yml b/config/locales/teacher_interface.en.yml index c104ca07cf..69a2faa432 100644 --- a/config/locales/teacher_interface.en.yml +++ b/config/locales/teacher_interface.en.yml @@ -96,17 +96,6 @@ en: title: Check your answers edit_contact: title: About you - uploads: - malware_scan: - error: - title: There’s a problem with your file - body: There’s a problem with this file. You’ll need to take another image of the document you want to upload, then try again. - pending: - title: We’re checking your file - body: We’re carrying out some checks on the file you’ve uploaded. Once we’ve successfully completed these checks, you’ll be able to preview your file. - suspect: - title: There’s a problem with your file - body: There’s a problem with this file. You’ll need to take another image of the document you want to upload, then try again. work_histories: add_another: title: Add another role diff --git a/spec/system/teacher_interface/malware_scanning_spec.rb b/spec/system/teacher_interface/malware_scanning_spec.rb index b669eac927..d0f4ea40e2 100644 --- a/spec/system/teacher_interface/malware_scanning_spec.rb +++ b/spec/system/teacher_interface/malware_scanning_spec.rb @@ -22,9 +22,6 @@ when_i_click_on_a_file_marked_as_malware then_i_see_theres_a_problem_with_the_file - - when_i_click_back_to_application - then_i_see_the_check_your_uploaded_files_page end def given_there_is_an_application_form @@ -87,16 +84,6 @@ def then_i_see_theres_a_problem_with_the_file expect(page).to have_content("There’s a problem with this file") end - def when_i_click_back_to_application - click_on "Back to application" - end - - def then_i_see_the_check_your_uploaded_files_page - expect(teacher_check_uploaded_files_page).to have_title( - "Check your uploaded files", - ) - end - def teacher @teacher ||= create(:teacher) end From bbce8b62d7a137b04a8bea5079f8b4164b59faf2 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 28 Dec 2023 10:29:46 +0000 Subject: [PATCH 07/11] Refactor UploadHelper This simplifies the code in the helper and simplifies the logic by making it consistent whether or not the upload scan is available. --- .../assessor_interface/uploads_controller.rb | 2 +- .../teacher_interface/uploads_controller.rb | 2 +- app/helpers/upload_helper.rb | 60 ++++++-------- ...eck_your_answers_summary_component_spec.rb | 21 +++-- spec/helpers/upload_helper_spec.rb | 78 +++++++++---------- .../malware_scanning_spec.rb | 4 +- 6 files changed, 73 insertions(+), 94 deletions(-) diff --git a/app/controllers/assessor_interface/uploads_controller.rb b/app/controllers/assessor_interface/uploads_controller.rb index 754ca748a2..277a791755 100644 --- a/app/controllers/assessor_interface/uploads_controller.rb +++ b/app/controllers/assessor_interface/uploads_controller.rb @@ -13,7 +13,7 @@ class UploadsController < BaseController def show authorize %i[assessor_interface application_form] - if downloadable?(upload) + if upload_downloadable?(upload) send_blob_stream(upload.attachment, disposition: :inline) else render "shared/malware_scan" diff --git a/app/controllers/teacher_interface/uploads_controller.rb b/app/controllers/teacher_interface/uploads_controller.rb index d49ea2e93c..4606c65014 100644 --- a/app/controllers/teacher_interface/uploads_controller.rb +++ b/app/controllers/teacher_interface/uploads_controller.rb @@ -18,7 +18,7 @@ class UploadsController < BaseController before_action :load_upload, only: %i[delete destroy show] def show - if downloadable?(@upload) + if upload_downloadable?(@upload) send_blob_stream(@upload.attachment, disposition: :inline) else render "shared/malware_scan" diff --git a/app/helpers/upload_helper.rb b/app/helpers/upload_helper.rb index 3f712c5060..870f1ebef1 100644 --- a/app/helpers/upload_helper.rb +++ b/app/helpers/upload_helper.rb @@ -1,50 +1,40 @@ +# frozen_string_literal: true + module UploadHelper def upload_link_to(upload) - if downloadable?(upload) + href = govuk_link_to( "#{upload.name} (opens in a new tab)", - [interface, :application_form, upload.document, upload], + upload_path(upload), target: :_blank, rel: :noopener, ) - else - output = - govuk_link_to( - upload.name, - [interface, :application_form, upload.document, upload], - ) - if show_scan_result_error?(upload) - output += - tag.p( - "There’s a problem with this file", - class: "govuk-error-message", - ) - end - output - end - end - def interface - if request.path.start_with?("/assessor/") - :assessor_interface - else - :teacher_interface - end - end + scan_result_problem = + malware_scanning_enabled? && + (upload.scan_result_error? || upload.scan_result_suspect?) - def downloadable?(upload) - unless FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result) - return true - end + return href unless scan_result_problem - upload.scan_result_clean? + href + + tag.p("There’s a problem with this file", class: "govuk-error-message") end - def show_scan_result_error?(upload) - unless FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result) - return false - end + def upload_path(upload) + interface = + if request.path.start_with?("/assessor/") + :assessor_interface + else + :teacher_interface + end + [interface, :application_form, upload.document, upload] + end + + def upload_downloadable?(upload) + !malware_scanning_enabled? || upload.scan_result_clean? + end - upload.scan_result_error? || upload.scan_result_suspect? + def malware_scanning_enabled? + FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result) end end diff --git a/spec/components/check_your_answers_summary_component_spec.rb b/spec/components/check_your_answers_summary_component_spec.rb index e2d5e8d24c..68dcaa58f0 100644 --- a/spec/components/check_your_answers_summary_component_spec.rb +++ b/spec/components/check_your_answers_summary_component_spec.rb @@ -268,10 +268,7 @@ "upload.pdf (opens in a new tab)", ) expect(row.at_css(".govuk-summary-list__value a")[:href]).to eq( - teacher_interface_application_form_document_upload_path( - model.document, - model.document.uploads.last, - ), + "/teacher/application/documents/#{model.document.id}/uploads/#{model.document.uploads.last.id}", ) end @@ -335,11 +332,11 @@ expect(row.at_css(".govuk-summary-list__value a").text).to eq( "upload.pdf (opens in a new tab)", ) + + document = model.translatable_document + expect(row.at_css(".govuk-summary-list__value a")[:href]).to eq( - teacher_interface_application_form_document_upload_path( - model.translatable_document, - model.translatable_document.uploads.last, - ), + "/teacher/application/documents/#{document.id}/uploads/#{document.uploads.last.id}", ) end @@ -364,11 +361,11 @@ expect(row.at_css(".govuk-summary-list__value a").text).to eq( "translation_upload.pdf (opens in a new tab)", ) + + document = model.translatable_document + expect(row.at_css(".govuk-summary-list__value a")[:href]).to include( - teacher_interface_application_form_document_upload_path( - model.translatable_document, - model.translatable_document.uploads.first, - ), + "/teacher/application/documents/#{document.id}/uploads/#{document.uploads.first.id}", ) end diff --git a/spec/helpers/upload_helper_spec.rb b/spec/helpers/upload_helper_spec.rb index 18f507e6f1..a4f16a74fb 100644 --- a/spec/helpers/upload_helper_spec.rb +++ b/spec/helpers/upload_helper_spec.rb @@ -2,72 +2,64 @@ RSpec.describe UploadHelper do describe "#upload_link_to" do - let(:upload) { build_stubbed(:upload, :clean) } + subject(:upload_link_to) { helper.upload_link_to(upload) } - it "returns a link to the upload" do - link = helper.upload_link_to(upload) + context "when the upload is clean" do + let(:upload) { build_stubbed(:upload, :clean) } - expect(link).to have_link( - "#{upload.name} (opens in a new tab)", - href: - teacher_interface_application_form_document_upload_path( - upload.document, - upload, - ), - ) + it "returns a link to the upload" do + expect(upload_link_to).to have_link( + "#{upload.name} (opens in a new tab)", + href: + "/teacher/application/documents/#{upload.document.id}/uploads/#{upload.id}", + ) + end + + context "via the assessors interface" do + before do + allow(helper.request).to receive(:path).and_return( + "/assessor/application", + ) + end + + it "returns a link to the upload" do + expect(upload_link_to).to have_link( + "#{upload.name} (opens in a new tab)", + href: + "/assessor/application/documents/#{upload.document.id}/uploads/#{upload.id}", + ) + end + end end context "when the upload malware scan is pending" do let(:upload) { build_stubbed(:upload, :pending) } + before { FeatureFlags::FeatureFlag.activate(:fetch_malware_scan_result) } + it "returns text for a pending scan" do - link = helper.upload_link_to(upload) - expect(link).to have_link( + expect(upload_link_to).to have_link( upload.name, href: - teacher_interface_application_form_document_upload_path( - upload.document, - upload, - ), + "/teacher/application/documents/#{upload.document.id}/uploads/#{upload.id}", ) end end context "when the upload malware scan is suspect" do let(:upload) { build_stubbed(:upload, :suspect) } + before { FeatureFlags::FeatureFlag.activate(:fetch_malware_scan_result) } + it "appends an error for a suspect scan" do - link = helper.upload_link_to(upload) - expect(link).to have_content( - "#{upload.name}There’s a problem with this file", + expect(upload_link_to).to have_content( + "There’s a problem with this file", ) - expect(link).to have_css( + expect(upload_link_to).to have_css( "p.govuk-error-message", text: "There’s a problem with this file", ) end end - - context "via the assessors interface" do - it "returns a link to the upload" do - allow(helper.request).to receive(:path).and_return( - assessor_interface_application_form_document_upload_path( - upload.document, - upload, - ), - ) - - link = helper.upload_link_to(upload) - - expect(link).to have_link( - "#{upload.name} (opens in a new tab)", - href: - assessor_interface_application_form_document_upload_path( - upload.document, - upload, - ), - ) - end - end end end diff --git a/spec/system/teacher_interface/malware_scanning_spec.rb b/spec/system/teacher_interface/malware_scanning_spec.rb index d0f4ea40e2..746b96a167 100644 --- a/spec/system/teacher_interface/malware_scanning_spec.rb +++ b/spec/system/teacher_interface/malware_scanning_spec.rb @@ -60,10 +60,10 @@ def then_i_see_the_check_your_uploaded_files_page def and_the_uploaded_files_have_been_marked_as_malware expect(teacher_check_uploaded_files_page.files).to have_content( - "File 1\tFile upload error\nThere’s a problem with this file", + "File 1\tFile upload error (opens in a new tab)\nThere’s a problem with this file", ) expect(teacher_check_uploaded_files_page.files).to have_content( - "File 2\tFile upload error\nThere’s a problem with this file", + "File 2\tFile upload error (opens in a new tab)\nThere’s a problem with this file", ) end From c64310b3a54b8d2db2571b85a090b8d66b1e600e Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 28 Dec 2023 17:08:33 +0000 Subject: [PATCH 08/11] Add ConvertToPDF This adds a service which handles the conversion of an upload to a PDF. --- app/lib/convert_to_pdf.rb | 74 ++++++++++++++++++++++++++++++ app/models/upload.rb | 4 ++ config/initializers/inflections.rb | 3 +- spec/lib/convert_to_pdf_spec.rb | 17 +++++++ 4 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 app/lib/convert_to_pdf.rb create mode 100644 spec/lib/convert_to_pdf_spec.rb diff --git a/app/lib/convert_to_pdf.rb b/app/lib/convert_to_pdf.rb new file mode 100644 index 0000000000..0b1bbd4a29 --- /dev/null +++ b/app/lib/convert_to_pdf.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +class ConvertToPDF + include ServicePattern + + def initialize(document:, translation:) + @document = document + + scope = + (translation ? document.translated_uploads : document.original_uploads) + + @uploads = + scope.order(:created_at).select { |upload| upload.attachment.present? } + end + + def call + return nil if uploads.count == 1 && uploads.all?(&:is_pdf?) + + combine_pdf = + uploads.reduce(CombinePDF.new) do |accumulator, upload| + open_file(upload:) { |file| accumulator << CombinePDF.load(file.path) } + end + + combine_pdf.to_pdf + end + + private + + attr_reader :document, :uploads + + def open_file(upload:, &block) + blob = upload.attachment.blob + + if upload.is_pdf? + blob.open(&block) + elsif blob.image? + open_image_as_pdf(blob:, &block) + elsif is_document?(blob:) + open_document_as_pdf(blob:, &block) + end + end + + def is_document?(blob:) + %w[ + application/msword + application/vnd.oasis.opendocument.text + application/vnd.openxmlformats-officedocument.wordprocessingml.document + ].include?(blob.content_type) + end + + def open_document_as_pdf(blob:, &block) + blob.open do |original_file| + Tempfile.create( + "converted-blob-#{blob.id}.pdf", + binmode: true, + ) do |converted_file| + Libreconv.convert(original_file.path, converted_file.path) + block.call(converted_file) + end + end + end + + def open_image_as_pdf(blob:, &block) + image = Magick::Image.from_blob(blob.download).first + return if image.nil? + + image.format = "PDF" + + Tempfile.create("converted-image-#{blob.id}.pdf", binmode: true) do |file| + image.write(file.path) + block.call(file) + end + end +end diff --git a/app/models/upload.rb b/app/models/upload.rb index 0daeed6ba1..6fdb102746 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -46,4 +46,8 @@ def url end delegate :application_form, to: :document + + def is_pdf? + attachment.blob.content_type == "application/pdf" + end end diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb index 8f1eb34eea..82bc2ceae6 100644 --- a/config/initializers/inflections.rb +++ b/config/initializers/inflections.rb @@ -1,8 +1,9 @@ # Be sure to restart your server when you modify this file. ActiveSupport::Inflector.inflections(:en) do |inflect| - inflect.acronym "DfE" inflect.acronym "DQT" + inflect.acronym "DfE" + inflect.acronym "PDF" inflect.acronym "QTS" inflect.acronym "TRN" inflect.uncountable %w[staff] diff --git a/spec/lib/convert_to_pdf_spec.rb b/spec/lib/convert_to_pdf_spec.rb new file mode 100644 index 0000000000..b9a7e46a1a --- /dev/null +++ b/spec/lib/convert_to_pdf_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe ConvertToPDF do + let(:document) { create(:document) } + + before { create(:upload, document:) } + + describe "#call" do + subject(:call) { described_class.call(document:, translation: false) } + + it "doesn't raise an error" do + expect { call }.to_not raise_error + end + end +end From 893cdbd73b8134f5d4ffecfd9f9d54257ab05ab4 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 28 Dec 2023 17:17:48 +0000 Subject: [PATCH 09/11] Add a controller method for viewing a PDF This adds a controller method which converts a document in to a PDF. --- .../documents_controller.rb | 40 +++++++++++++++++++ .../assessor_interface/uploads_controller.rb | 2 +- .../application_form_policy.rb | 1 + config/routes.rb | 4 ++ .../application_form_policy_spec.rb | 5 +++ 5 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 app/controllers/assessor_interface/documents_controller.rb diff --git a/app/controllers/assessor_interface/documents_controller.rb b/app/controllers/assessor_interface/documents_controller.rb new file mode 100644 index 0000000000..023f5f43c7 --- /dev/null +++ b/app/controllers/assessor_interface/documents_controller.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module AssessorInterface + class DocumentsController < BaseController + include ActiveStorage::Streaming + include StreamedResponseAuthenticatable + include RescueActiveStorageErrors + include UploadHelper + + skip_before_action :authenticate_staff! + before_action { authenticate_or_redirect(:staff) } + + def show_pdf + authorize %i[assessor_interface application_form] + + unless all_uploads_downloadable? + render "shared/malware_scan" + return + end + + translation = params[:scope] == "translated" + + if (pdf_data = ConvertToPDF.call(document:, translation:)) + send_data(pdf_data, type: "application/pdf", disposition: :inline) + else + error_internal_server_error + end + end + + private + + def document + @document ||= Document.find(params[:id]) + end + + def all_uploads_downloadable? + document.uploads.all? { |upload| upload_downloadable?(upload) } + end + end +end diff --git a/app/controllers/assessor_interface/uploads_controller.rb b/app/controllers/assessor_interface/uploads_controller.rb index 277a791755..880b67d522 100644 --- a/app/controllers/assessor_interface/uploads_controller.rb +++ b/app/controllers/assessor_interface/uploads_controller.rb @@ -8,7 +8,7 @@ class UploadsController < BaseController include UploadHelper skip_before_action :authenticate_staff! - before_action -> { authenticate_or_redirect(:staff) } + before_action { authenticate_or_redirect(:staff) } def show authorize %i[assessor_interface application_form] diff --git a/app/policies/assessor_interface/application_form_policy.rb b/app/policies/assessor_interface/application_form_policy.rb index 5157cd6ecb..b168ba3040 100644 --- a/app/policies/assessor_interface/application_form_policy.rb +++ b/app/policies/assessor_interface/application_form_policy.rb @@ -14,6 +14,7 @@ def show? alias_method :status?, :show? alias_method :timeline?, :show? + alias_method :show_pdf?, :show? def update? user.change_name_permission diff --git a/config/routes.rb b/config/routes.rb index da62354924..28aeff5c73 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -158,6 +158,10 @@ end end + get "/application/documents/:id/:scope/pdf", + to: "documents#show_pdf", + as: :application_form_document_pdf + get "/application/documents/:document_id/uploads/:id", to: "uploads#show", as: :application_form_document_upload diff --git a/spec/policies/assessor_interface/application_form_policy_spec.rb b/spec/policies/assessor_interface/application_form_policy_spec.rb index 30a13f2fa5..0e3041f8f5 100644 --- a/spec/policies/assessor_interface/application_form_policy_spec.rb +++ b/spec/policies/assessor_interface/application_form_policy_spec.rb @@ -40,6 +40,11 @@ it_behaves_like "a policy method with permission" end + describe "#show_pdf?" do + subject(:show_pdf?) { policy.show_pdf? } + it_behaves_like "a policy method with permission" + end + describe "#create?" do subject(:create?) { policy.create? } it_behaves_like "a policy method without permission" From d40fc0496d59ce2c9265e6ff84cb5cdcc3314f9e Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Tue, 9 Jan 2024 11:31:13 +0000 Subject: [PATCH 10/11] Add link to download a PDF This updates the component which renders the links to the uploads to include a link to the document. --- .../check_your_answers_summary/component.rb | 38 +++++++++++++++---- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/app/components/check_your_answers_summary/component.rb b/app/components/check_your_answers_summary/component.rb index 840cf2fa62..db82805e39 100644 --- a/app/components/check_your_answers_summary/component.rb +++ b/app/components/check_your_answers_summary/component.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module CheckYourAnswersSummary class Component < ViewComponent::Base def initialize( @@ -63,11 +65,9 @@ def value_for(field) end def href_for(field) - return if (path = field[:href]).blank? - - return path if path.is_a?(String) - - Rails.application.routes.url_helpers.polymorphic_path(path) + if (path = field[:href]).present? + path.is_a?(String) ? path : url_helpers.polymorphic_path(path) + end end def row_for_field(field) @@ -135,13 +135,21 @@ def format_document(document, field) .order(:created_at) .select { |upload| upload.attachment.present? } - malware_scan_active = - FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result) - [ format_array(uploads, field), if malware_scan_active && scope.scan_result_suspect.exists? "One or more upload has been deleted by the virus scanner." + elsif request.path.starts_with?("/assessor") && + convert_to_pdf_active && !uploads.all?(&:is_pdf?) + helpers.govuk_link_to( + "Download as PDF (opens in a new tab)", + url_helpers.assessor_interface_application_form_document_pdf_path( + document, + field[:translation] ? "translated" : "original", + ), + target: :_blank, + rel: :noopener, + ) end, ].compact_blank.join("

").html_safe end @@ -150,5 +158,19 @@ def format_document(document, field) def format_array(list, field) list.map { |v| format_value(v, field) }.join("
").html_safe end + + def url_helpers + @url_helpers ||= Rails.application.routes.url_helpers + end + + def malware_scan_active + @malware_scan_active ||= + FeatureFlags::FeatureFlag.active?(:fetch_malware_scan_result) + end + + def convert_to_pdf_active + @convert_to_pdf_active = + FeatureFlags::FeatureFlag.active?(:convert_documents_to_pdf) + end end end From 14b17ed3920446467e574234405571eaa68d83df Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 14 Dec 2023 11:10:53 +0000 Subject: [PATCH 11/11] Simplify layout views This removes the base view and replaces it with the application view as it doesn't do anything extra. --- app/controllers/application_controller.rb | 3 +- .../assessor_interface/base_controller.rb | 2 - .../eligibility_interface/base_controller.rb | 2 - app/controllers/errors_controller.rb | 1 - app/controllers/performance_controller.rb | 2 + app/controllers/static_controller.rb | 2 - .../teacher_interface/base_controller.rb | 2 - app/views/layouts/application.html.erb | 92 ++++++++++++++++++- app/views/layouts/base.html.erb | 89 ------------------ app/views/layouts/full_from_desktop.html.erb | 2 +- app/views/layouts/two_thirds.html.erb | 2 +- .../autoload/page_objects/performance.rb | 2 +- 12 files changed, 95 insertions(+), 106 deletions(-) delete mode 100644 app/views/layouts/base.html.erb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5c851d9a75..3438b15992 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -5,8 +5,9 @@ class ApplicationController < ActionController::Base include DfE::Analytics::Requests include Pundit::Authorization - default_form_builder GOVUKDesignSystemFormBuilder::FormBuilder append_view_path Rails.root.join("app/components") + default_form_builder GOVUKDesignSystemFormBuilder::FormBuilder + layout "two_thirds" before_action :authenticate, unless: -> { FeatureFlags::FeatureFlag.active?(:service_open) } diff --git a/app/controllers/assessor_interface/base_controller.rb b/app/controllers/assessor_interface/base_controller.rb index a260b767fd..d138d4862c 100644 --- a/app/controllers/assessor_interface/base_controller.rb +++ b/app/controllers/assessor_interface/base_controller.rb @@ -4,7 +4,5 @@ class AssessorInterface::BaseController < ApplicationController include AssessorCurrentNamespace include StaffAuthenticatable - layout "two_thirds" - after_action :verify_authorized end diff --git a/app/controllers/eligibility_interface/base_controller.rb b/app/controllers/eligibility_interface/base_controller.rb index 8927846939..49c983527a 100644 --- a/app/controllers/eligibility_interface/base_controller.rb +++ b/app/controllers/eligibility_interface/base_controller.rb @@ -2,8 +2,6 @@ module EligibilityInterface class BaseController < ApplicationController include EligibilityCurrentNamespace - layout "two_thirds" - before_action :load_region after_action :save_eligibility_check_id diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index d392372b71..ef1cb2f1f7 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -2,7 +2,6 @@ class ErrorsController < ApplicationController include EligibilityCurrentNamespace skip_before_action :verify_authenticity_token - layout "two_thirds" def forbidden render "forbidden", formats: :html, status: :forbidden diff --git a/app/controllers/performance_controller.rb b/app/controllers/performance_controller.rb index e70bd9b721..bd6b7fe962 100644 --- a/app/controllers/performance_controller.rb +++ b/app/controllers/performance_controller.rb @@ -20,5 +20,7 @@ def index stats.live_service_usage @time_to_complete_data = stats.time_to_complete @usage_by_country_count, @usage_by_country_data = stats.usage_by_country + + render layout: "full_from_desktop" end end diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index 148514938e..40603a0176 100644 --- a/app/controllers/static_controller.rb +++ b/app/controllers/static_controller.rb @@ -2,6 +2,4 @@ class StaticController < ApplicationController include EligibilityCurrentNamespace - - layout "two_thirds" end diff --git a/app/controllers/teacher_interface/base_controller.rb b/app/controllers/teacher_interface/base_controller.rb index 7410b3c5e6..c5163739d4 100644 --- a/app/controllers/teacher_interface/base_controller.rb +++ b/app/controllers/teacher_interface/base_controller.rb @@ -3,8 +3,6 @@ class TeacherInterface::BaseController < ApplicationController include TeacherCurrentNamespace - layout "two_thirds" - before_action :authenticate_teacher! def load_application_form diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index a5e665ad5f..d1426b0fa8 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -1,5 +1,89 @@ -<% content_for :content do %> - <%= yield %> -<% end %> + + + + + <%= [yield(:page_title).presence, t(current_namespace, scope: %i[service name])].compact.join(' - ') %> -<%= render template: 'layouts/base' %> + <%= csrf_meta_tags %> + <%= csp_meta_tag %> + + <%= tag :meta, name: 'viewport', content: 'width=device-width, initial-scale=1' %> + <%= tag :meta, property: 'og:image', content: asset_path('images/govuk-opengraph-image.png') %> + <%= tag :meta, name: 'theme-color', content: '#0b0c0c' %> + <%= favicon_link_tag asset_path('images/favicon.ico') %> + <%= favicon_link_tag asset_path('images/govuk-mask-icon.svg'), rel: 'mask-icon', type: 'image/svg', color: "#0b0c0c" %> + <%= favicon_link_tag asset_path('images/govuk-apple-touch-icon.png'), rel: 'apple-touch-icon', type: 'image/png' %> + <%= favicon_link_tag asset_path('images/govuk-apple-touch-icon-152x152.png'), rel: 'apple-touch-icon', type: 'image/png', size: '152x152' %> + <%= favicon_link_tag asset_path('images/govuk-apple-touch-icon-167x167.png'), rel: 'apple-touch-icon', type: 'image/png', size: '167x167' %> + <%= favicon_link_tag asset_path('images/govuk-apple-touch-icon-180x180.png'), rel: 'apple-touch-icon', type: 'image/png', size: '180x180' %> + + <%= stylesheet_link_tag "application" %> + <%= javascript_include_tag "application", defer: true %> + + + + <%= javascript_tag nonce: true do -%> + document.body.className = ((document.body.className) ? document.body.className + ' js-enabled' : 'js-enabled'); + <% end -%> + + <%= govuk_skip_link %> + + <%= render partial: "shared/header" %> + +
+
+

+ + <%= HostingEnvironment.phase %> + + + + <% if HostingEnvironment.production? %> + This is a new service - <%= link_to "your feedback will help us to improve it", yield(:feedback_url).presence || t("service.form.feedback") %>. + <% else %> + This is a ‘<%= HostingEnvironment.phase %>’ version of the service. + <% end %> + +

+
+
+ +
+ <%= govuk_back_link(href: yield(:back_link_url)) unless yield(:back_link_url).blank? %> +
+ <%= render(FlashMessage::Component.new(flash: flash)) %> + <%= content_for?(:content) ? yield(:content) : yield %> +
+
+ + <%= govuk_footer( + meta_items_title: 'Footer links', + meta_items: { + 'Accessibility': '/accessibility', + 'Cookies': '/cookies', + 'Privacy': '/privacy' + } ) do |footer| %> + <% footer.with_navigation do %> + <% if current_teacher %> +
+

Get help

+

+ If you’re having difficulty completing your application form, contact: + <%= govuk_link_to t("service.email.enquiries"), "mailto:#{t("service.email.enquiries")}" %> + for support. +

+

+ So that we can help you quickly, please include as much detail as possible + about the problem you‘re experiencing, for example: +

+
    +
  • any important details (such as application reference numbers etc)
    • +
  • screenshots that illustrate your problem
    • +
+

We’ll aim to respond to you within 2 working days.

+
+ <% end %> + <% end %> + <% end %> + + diff --git a/app/views/layouts/base.html.erb b/app/views/layouts/base.html.erb deleted file mode 100644 index d1426b0fa8..0000000000 --- a/app/views/layouts/base.html.erb +++ /dev/null @@ -1,89 +0,0 @@ - - - - - <%= [yield(:page_title).presence, t(current_namespace, scope: %i[service name])].compact.join(' - ') %> - - <%= csrf_meta_tags %> - <%= csp_meta_tag %> - - <%= tag :meta, name: 'viewport', content: 'width=device-width, initial-scale=1' %> - <%= tag :meta, property: 'og:image', content: asset_path('images/govuk-opengraph-image.png') %> - <%= tag :meta, name: 'theme-color', content: '#0b0c0c' %> - <%= favicon_link_tag asset_path('images/favicon.ico') %> - <%= favicon_link_tag asset_path('images/govuk-mask-icon.svg'), rel: 'mask-icon', type: 'image/svg', color: "#0b0c0c" %> - <%= favicon_link_tag asset_path('images/govuk-apple-touch-icon.png'), rel: 'apple-touch-icon', type: 'image/png' %> - <%= favicon_link_tag asset_path('images/govuk-apple-touch-icon-152x152.png'), rel: 'apple-touch-icon', type: 'image/png', size: '152x152' %> - <%= favicon_link_tag asset_path('images/govuk-apple-touch-icon-167x167.png'), rel: 'apple-touch-icon', type: 'image/png', size: '167x167' %> - <%= favicon_link_tag asset_path('images/govuk-apple-touch-icon-180x180.png'), rel: 'apple-touch-icon', type: 'image/png', size: '180x180' %> - - <%= stylesheet_link_tag "application" %> - <%= javascript_include_tag "application", defer: true %> - - - - <%= javascript_tag nonce: true do -%> - document.body.className = ((document.body.className) ? document.body.className + ' js-enabled' : 'js-enabled'); - <% end -%> - - <%= govuk_skip_link %> - - <%= render partial: "shared/header" %> - -
-
-

- - <%= HostingEnvironment.phase %> - - - - <% if HostingEnvironment.production? %> - This is a new service - <%= link_to "your feedback will help us to improve it", yield(:feedback_url).presence || t("service.form.feedback") %>. - <% else %> - This is a ‘<%= HostingEnvironment.phase %>’ version of the service. - <% end %> - -

-
-
- -
- <%= govuk_back_link(href: yield(:back_link_url)) unless yield(:back_link_url).blank? %> -
- <%= render(FlashMessage::Component.new(flash: flash)) %> - <%= content_for?(:content) ? yield(:content) : yield %> -
-
- - <%= govuk_footer( - meta_items_title: 'Footer links', - meta_items: { - 'Accessibility': '/accessibility', - 'Cookies': '/cookies', - 'Privacy': '/privacy' - } ) do |footer| %> - <% footer.with_navigation do %> - <% if current_teacher %> -
-

Get help

-

- If you’re having difficulty completing your application form, contact: - <%= govuk_link_to t("service.email.enquiries"), "mailto:#{t("service.email.enquiries")}" %> - for support. -

-

- So that we can help you quickly, please include as much detail as possible - about the problem you‘re experiencing, for example: -

-
    -
  • any important details (such as application reference numbers etc)
    • -
  • screenshots that illustrate your problem
    • -
-

We’ll aim to respond to you within 2 working days.

-
- <% end %> - <% end %> - <% end %> - - diff --git a/app/views/layouts/full_from_desktop.html.erb b/app/views/layouts/full_from_desktop.html.erb index 08ed4ca592..e148ccb5a1 100644 --- a/app/views/layouts/full_from_desktop.html.erb +++ b/app/views/layouts/full_from_desktop.html.erb @@ -6,4 +6,4 @@ <% end %> -<%= render template: 'layouts/base' %> +<%= render template: "layouts/application" %> diff --git a/app/views/layouts/two_thirds.html.erb b/app/views/layouts/two_thirds.html.erb index 167aa779d1..3587a3d05b 100644 --- a/app/views/layouts/two_thirds.html.erb +++ b/app/views/layouts/two_thirds.html.erb @@ -6,4 +6,4 @@ <% end %> -<%= render template: 'layouts/base' %> +<%= render template: "layouts/application" %> diff --git a/spec/support/autoload/page_objects/performance.rb b/spec/support/autoload/page_objects/performance.rb index 87b6dd2577..20709de8ac 100644 --- a/spec/support/autoload/page_objects/performance.rb +++ b/spec/support/autoload/page_objects/performance.rb @@ -2,7 +2,7 @@ module PageObjects class Performance < SitePrism::Page set_url "/performance{?since_launch*}" - section :live_service_usage, "main > div:nth-of-type(2)" do + section :live_service_usage, "main > div > div > div:nth-of-type(2)" do elements :stats, "div > div" element :table, "table"