Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moj multi upload #7448

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -77,7 +77,7 @@ Capybara/CurrentPathExpectation:
- 'spec/system/other/users_can_view_help_guides_spec.rb'
- 'spec/system/publishers/publishers_are_reminded_of_application_functionality_spec.rb'
- 'spec/system/publishers/publishers_are_shown_new_features_page_spec.rb'
- 'spec/system/publishers/publishers_can_add_aditional_documents_to_a_vacancy_spec.rb'
- 'spec/system/publishers/publishers_can_add_additional_documents_to_a_vacancy_spec.rb'
- 'spec/system/publishers/publishers_can_add_notes_to_a_job_application_spec.rb'
- 'spec/system/publishers/publishers_can_copy_a_vacancy_spec.rb'
- 'spec/system/publishers/publishers_can_edit_a_draft_vacancy_spec.rb'
@@ -1247,7 +1247,7 @@ RSpec/VerifiedDoubles:
- 'spec/services/zendesk_spec.rb'
- 'spec/system/jobseekers/jobseekers_can_create_a_job_alert_from_a_search_spec.rb'
- 'spec/system/jobseekers/jobseekers_can_manage_a_profile_spec.rb'
- 'spec/system/publishers/publishers_can_add_aditional_documents_to_a_vacancy_spec.rb'
- 'spec/system/publishers/publishers_can_add_additional_documents_to_a_vacancy_spec.rb'
- 'spec/system/publishers/publishers_can_edit_a_published_vacancy_as_a_school_spec.rb'
- 'spec/system/publishers/publishers_can_end_a_job_listing_early_spec.rb'

@@ -1812,7 +1812,7 @@ Style/WordArray:
- 'spec/system/jobseekers/jobseekers_can_create_a_job_alert_from_a_listing_spec.rb'
- 'spec/system/jobseekers/jobseekers_can_search_for_jobs_spec.rb'
- 'spec/system/jobseekers/jobseekers_can_view_a_job_application_spec.rb'
- 'spec/system/publishers/publishers_can_add_aditional_documents_to_a_vacancy_spec.rb'
- 'spec/system/publishers/publishers_can_add_additional_documents_to_a_vacancy_spec.rb'
- 'spec/system/publishers/publishers_can_edit_a_draft_vacancy_spec.rb'
- 'spec/system/publishers/publishers_can_edit_a_published_vacancy_as_a_school_group_spec.rb'
- 'spec/system/publishers/publishers_can_edit_a_published_vacancy_as_a_school_spec.rb'
1 change: 1 addition & 0 deletions .undercover
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-c origin/main
5 changes: 4 additions & 1 deletion Gemfile
Original file line number Diff line number Diff line change
@@ -20,6 +20,7 @@ gem "railties", RAILS_VERSION
gem "activerecord-import"
gem "activerecord-postgis-adapter", ">= 10.0.1"
gem "activerecord-session_store"
gem "active_storage_validations"
gem "addressable"
gem "array_enum"
gem "aws-sdk-s3", require: false
@@ -117,7 +118,9 @@ group :development, :test do
gem "rspec-rails"
gem "rswag-specs"
gem "slim_lint", require: false
gem "undercover", require: false
# https://github.com/grodowski/undercover/issues/220
# v 0.6 doesn't respect :nocov: tags properly
gem "undercover", "< 0.6", require: false
end

group :test do
17 changes: 11 additions & 6 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -66,6 +66,12 @@ GEM
erubi (~> 1.11)
rails-dom-testing (~> 2.2)
rails-html-sanitizer (~> 1.6)
active_storage_validations (2.0.0)
activejob (>= 6.1.4)
activemodel (>= 6.1.4)
activestorage (>= 6.1.4)
activesupport (>= 6.1.4)
marcel (>= 1.0.3)
activejob (7.2.2.1)
activesupport (= 7.2.2.1)
globalid (>= 0.3.6)
@@ -322,8 +328,6 @@ GEM
json-schema (5.0.1)
addressable (~> 2.8)
jwt (2.10.1)
jwt (2.9.3)
base64
kramdown (2.5.1)
rexml (>= 3.3.9)
kramdown-parser-gfm (1.1.0)
@@ -432,7 +436,7 @@ GEM
parallel (1.26.3)
parallel_tests (4.9.0)
parallel
parser (3.3.7.0)
parser (3.3.7.1)
ast (~> 2.4.1)
racc
parslet (2.0.0)
@@ -741,9 +745,9 @@ GEM
tzinfo (2.0.6)
concurrent-ruby (~> 1.0)
uber (0.1.0)
undercover (0.6.3)
undercover (0.5.0)
bigdecimal
imagen (>= 0.2.0)
imagen (>= 0.1.8)
rainbow (>= 2.1, < 4.0)
rugged (>= 0.27, < 1.8)
unicode (0.4.4.5)
@@ -813,6 +817,7 @@ DEPENDENCIES
actionpack (~> 7.2)
actionpack-action_caching
actiontext (~> 7.2)
active_storage_validations
activejob (~> 7.2)
activemodel (~> 7.2)
activerecord (~> 7.2)
@@ -919,7 +924,7 @@ DEPENDENCIES
slim_lint
solargraph
tzinfo-data
undercover
undercover (< 0.6)
uri-query_params
valid_email2
validate_url
12 changes: 12 additions & 0 deletions app/assets/javascript/application.js
Original file line number Diff line number Diff line change
@@ -71,3 +71,15 @@ Rails.start();
govukFrontend.initAll();
window.$ = $;
mojFrontend.initAll();

if (typeof mojFrontend.MultiFileUpload !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is the right place?
Could we add a multifile_upload.js or add it to utils.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure - it ought to be in the MoJ library init...

const uploadSelectors = document.querySelectorAll('.moj-multi-file-upload');
uploadSelectors.forEach((container) => {
// eslint-disable-next-line no-new
new mojFrontend.MultiFileUpload({
container,
uploadUrl: container.dataset.upload,
deleteUrl: container.dataset.delete,
});
});
}
2 changes: 1 addition & 1 deletion app/components/vacancy_form_page_heading_component.rb
Original file line number Diff line number Diff line change
@@ -26,7 +26,7 @@ def caption

private

attr_reader :vacancy, :copy, :step_process, :back_path
attr_reader :vacancy, :step_process, :back_path

def page_title_from_vacancy_organisations
return current_organisation.name if vacancy.organisations.none?
48 changes: 36 additions & 12 deletions app/controllers/publishers/vacancies/documents_controller.rb
Original file line number Diff line number Diff line change
@@ -1,16 +1,21 @@
require "google/apis/drive_v3"

class Publishers::Vacancies::DocumentsController < Publishers::Vacancies::BaseController
helper_method :documents_form, :confirmation_form
helper_method :confirmation_form

def create
if documents_form.valid?
documents_form.supporting_documents.each do |document|
vacancy.supporting_documents.attach(document)
send_dfe_analytics_event(:supporting_document_created, document.original_filename, document.size, document.content_type)
end
skip_before_action :verify_authenticity_token,
only: %i[upload_file delete_uploaded_file]

vacancy.update(documents_form.params_to_save)
def index; end

def new
@documents_form = Publishers::JobListing::DocumentsForm.new(documents_form_params, vacancy)
end

def create
@documents_form = Publishers::JobListing::DocumentsForm.new(documents_form_params.merge(supporting_documents: vacancy.supporting_documents), vacancy)
if @documents_form.valid?
vacancy.update(@documents_form.params_to_save)

render :index
else
@@ -26,6 +31,29 @@ def destroy
redirect_to after_document_delete_path, flash: { success: t("jobs.file_delete_success_message", filename: document.filename) }
end

def upload_file
@document = params.require(:documents)

respond_to do |format|
if vacancy.supporting_documents.attach(@document)
send_dfe_analytics_event(:supporting_document_created, @document.original_filename, @document.size, @document.content_type)
format.json { render "upload_success" }
else
format.json { render "upload_error" }
end
end
end

def delete_uploaded_file
filename = params.require(:delete)
vacancy.supporting_documents.select { |d| d.filename == filename }.each do |document|
document.purge
send_dfe_analytics_event(:supporting_document_deleted, document.filename, document.byte_size, document.content_type)
end

respond_to(&:json)
end

def confirm
return render :index unless confirmation_form.valid?

@@ -42,10 +70,6 @@ def step
:documents
end

def documents_form
@documents_form ||= Publishers::JobListing::DocumentsForm.new(documents_form_params, vacancy)
end

def documents_form_params
(params[:publishers_job_listing_documents_form] || params)
.permit(supporting_documents: [])
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class Publishers::JobListing::ApplicationFormForm < Publishers::JobListing::VacancyForm
validate :application_form_presence
validates :application_form, form_file: DOCUMENT_VALIDATION_OPTIONS
validates :application_form, form_file: Vacancy::DOCUMENT_VALIDATION_OPTIONS

validates :application_email, presence: true
validate :other_application_email_presence
12 changes: 1 addition & 11 deletions app/form_models/publishers/job_listing/documents_form.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
class Publishers::JobListing::DocumentsForm < Publishers::JobListing::VacancyForm
validates :supporting_documents, form_file: DOCUMENT_VALIDATION_OPTIONS
validate :document_presence
validates :supporting_documents, presence: true, if: -> { vacancy.include_additional_documents }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice


attr_accessor :supporting_documents

@@ -11,13 +10,4 @@ def self.fields
def params_to_save
{ completed_steps: params[:completed_steps] }
end

private

def document_presence
return unless vacancy.include_additional_documents
return if supporting_documents.present?

errors.add(:supporting_documents, :blank)
end
end
14 changes: 14 additions & 0 deletions app/models/vacancy.rb
Original file line number Diff line number Diff line change
@@ -61,7 +61,21 @@ class Vacancy < ApplicationRecord
belongs_to :publisher_organisation, class_name: "Organisation", optional: true
belongs_to :publisher_ats_api_client, optional: true

DOCUMENT_FILE_SIZE_LIMIT = 20.megabytes
DOCUMENT_CONTENT_TYPES = %w[application/pdf application/msword application/vnd.openxmlformats-officedocument.wordprocessingml.document].freeze

DOCUMENT_VALIDATION_OPTIONS = {
file_type: :document,
content_types_allowed: DOCUMENT_CONTENT_TYPES,
file_size_limit: DOCUMENT_FILE_SIZE_LIMIT,
valid_file_types: %i[PDF DOC DOCX],
}.freeze

has_many_attached :supporting_documents, service: :amazon_s3_documents

validates :supporting_documents, content_type: DOCUMENT_CONTENT_TYPES,
size: { less_than: DOCUMENT_FILE_SIZE_LIMIT }, virus_free: true, if: -> { include_additional_documents }

has_one_attached :application_form, service: :amazon_s3_documents

has_many :saved_jobs, dependent: :destroy
14 changes: 6 additions & 8 deletions app/services/publishers/document_virus_check.rb
Original file line number Diff line number Diff line change
@@ -11,6 +11,12 @@ def initialize(file)
end

def safe?
uploaded_file = drive_service.create_file(
{ alt: "media", name: "virus-check-#{Time.zone.now.strftime('%F-%H.%M.%S.%3N')}" },
fields: "id, web_view_link, web_content_link, mime_type",
upload_source: file.path,
)

drive_service.get_file(
uploaded_file.id,
acknowledge_abuse: false,
@@ -30,14 +36,6 @@ def safe?

attr_reader :file

def uploaded_file
@uploaded_file ||= drive_service.create_file(
{ alt: "media", name: "virus-check-#{Time.zone.now.strftime('%F-%H.%M.%S.%3N')}" },
fields: "id, web_view_link, web_content_link, mime_type",
upload_source: file.path,
)
end

def drive_service
@drive_service ||= Google::Apis::DriveV3::DriveService.new
end
16 changes: 16 additions & 0 deletions app/validators/virus_free_validator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# TODO: - this could be deleted when we move over to Azure storage, which will do the virus check for us
class VirusFreeValidator < ActiveModel::EachValidator
def validate_each(record, attribute, _value)
changes = record.attachment_changes[attribute.to_s]
if changes
changes.attachables.select { |d| d.instance_of?(ActionDispatch::Http::UploadedFile) }.each do |upload_file|
# :nocov:
unless Publishers::DocumentVirusCheck.new(upload_file.tempfile).safe?
Rails.logger.warn("Attempted to upload '#{upload_file.original_filename}' but Google Drive virus check failed")
record.errors.add(attribute, :contains_a_virus)
end
# :nocov:
end
end
end
end
4 changes: 4 additions & 0 deletions app/views/publishers/vacancies/documents/_file.json.jbuilder
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
json.file do
json.filename document.original_filename
json.orginalname document.original_filename
end
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
json.success true
25 changes: 14 additions & 11 deletions app/views/publishers/vacancies/documents/new.html.slim
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
- content_for :page_title_prefix, page_title_prefix(step_process, documents_form)
- content_for :page_title_prefix, page_title_prefix(step_process, @documents_form)

.govuk-grid-row
.govuk-grid-column-two-thirds data-controller="upload-documents"
= form_for documents_form, url: organisation_job_documents_path(vacancy.id) do |f|
div
div data-controller="upload-documents"
= form_for @documents_form, url: organisation_job_documents_path(vacancy.id) do |f|
= f.govuk_error_summary

- if params["back_to_#{action_name}"]
= f.hidden_field "back_to_#{action_name}", value: "true"

= f.govuk_file_field :supporting_documents,
label: { text: vacancy_form_page_heading(vacancy, step_process, back_path: back_path), tag: "h1", size: "l" },
hint: { text: t("helpers.hint.publishers_job_listing_documents_form.documents") },
accept: ".doc, .docx, .pdf",
multiple: true,
include_hidden: false,
enctype: "multipart/form-data"
= tag.div(class: "moj-multi-file-upload", data: { upload: upload_organisation_job_documents_path(vacancy.id), delete: remove_organisation_job_documents_path(vacancy.id) }) do
.moj-multi-file__uploaded-files
h2 class="govuk-heading-m" = "Files added"
.govuk-summary-list class="moj-multi-file-upload__list"

.moj-multi-file-upload__upload
= f.govuk_file_field :supporting_documents,
label: { text: vacancy_form_page_heading(vacancy, step_process, back_path: back_path), tag: "h1", size: "l" },
class: "moj-multi-file-upload__input",
accept: ".doc, .docx, .pdf"

= render "publishers/vacancies/vacancy_form_partials/submit", f: f
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
json.error do
json.message "#{@document.original_filename} #{vacancy.errors[:supporting_documents].first}"
end
json.partial! "file", document: @document
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
json.success do
json.messageHtml t("jobs.file_upload_success_message", filename: @document.original_filename)
json.messageText t("jobs.file_upload_success_message", filename: @document.original_filename)
end
json.partial! "file", document: @document
1 change: 1 addition & 0 deletions config/locales/activerecord.yml
Original file line number Diff line number Diff line change
@@ -336,6 +336,7 @@ en:
taken: has already been taken
blank: can't be blank
incomplete_qualification_result: Enter a %{attribute} for Subject %{result_idx}
contains_a_virus: contains a virus
activemodel:
errors:
models:
7 changes: 5 additions & 2 deletions config/locales/forms.yml
Original file line number Diff line number Diff line change
@@ -234,9 +234,12 @@ en:
publishers_job_listing_application_link_form:
application_link: For example, https://www.example.com
publishers_job_listing_application_form_form:
application_form: You can only upload a PDF or DOCX file under 10MB
application_form: You can only upload a PDF or DOCX file under 20MB
publishers_job_listing_applying_for_the_job_form:
enable_job_applications_options:
"false": You'll need to provide additional details about how to apply.
publishers_job_listing_documents_form:
documents: You can only upload PDF or DOCX files under 10MB
supporting_documents: You can only upload PDF or DOCX files under 20MB
publishers_job_listing_feedback_form:
email: >-
We'll only use this to tell you about opportunities to take part in user research that helps us improve
1 change: 1 addition & 0 deletions config/locales/jobs.yml
Original file line number Diff line number Diff line change
@@ -246,6 +246,7 @@ en:
supporting_documents_accessibility: If you need these documents in an accessible format, please contact the school.
no_files_message: No files selected
no_supporting_documents: No documents uploaded
file_upload_success_message: '%{filename} uploaded'
file_delete_success_message: '%{filename} deleted'
file_google_error_message: '%{filename} could not be uploaded - try again'
file_input_error_message: The selected file(s) could not be uploaded
3 changes: 3 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
@@ -365,6 +365,9 @@
resources :build, only: %i[show update], controller: "publishers/vacancies/build"
resources :documents, only: %i[index new create destroy], controller: "publishers/vacancies/documents" do
post :confirm, on: :collection
# API for MoJ multi-select component
post :upload, on: :collection, to: "publishers/vacancies/documents#upload_file", format: "json"
post :remove, on: :collection, to: "publishers/vacancies/documents#delete_uploaded_file", format: "json"
end
resource :application_forms, only: %i[create destroy], controller: "publishers/vacancies/application_forms"

Loading

Unchanged files with check annotations Beta

# configuring it using the ENV variables we provide in storage.yml. However, at this point, these ENV vars have not been loaded,
# causing the error. Below we define two throaway ENV vars to prevent the error from being thrown. These are then later overwritten,
# when all of the ENV vars are loaded.
ARG RAILS_MASTER_KEY

Check warning on line 35 in Dockerfile

GitHub Actions / Build docker image

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "RAILS_MASTER_KEY") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/

Check warning on line 35 in Dockerfile

GitHub Actions / Build docker image

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ARG "RAILS_MASTER_KEY") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/
ENV DOCUMENTS_S3_BUCKET=throwaway_value
ENV SCHOOLS_IMAGES_LOGOS_S3_BUCKET=throwaway_value
ENV RAILS_MASTER_KEY=$RAILS_MASTER_KEY

Check warning on line 39 in Dockerfile

GitHub Actions / Build docker image

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ENV "RAILS_MASTER_KEY") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/

Check warning on line 39 in Dockerfile

GitHub Actions / Build docker image

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ENV "RAILS_MASTER_KEY") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/
RUN RAILS_ENV=production SECRET_KEY_BASE=required-to-run-but-not-used RAILS_SERVE_STATIC_FILES=1 bundle exec rake assets:precompile
ENV COMMIT_SHA=$COMMIT_SHA
EXPOSE 3000
CMD bundle exec rails db:migrate:ignore_concurrent_migration_exceptions && bundle exec rails s

Check warning on line 72 in Dockerfile

GitHub Actions / Build docker image

JSON arguments recommended for ENTRYPOINT/CMD to prevent unintended behavior related to OS signals

JSONArgsRecommended: JSON arguments recommended for CMD to prevent unintended behavior related to OS signals More info: https://docs.docker.com/go/dockerfile/rule/json-args-recommended/