From ffd6d1407b5b501e6f8f399167957d2df72f75a5 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Fri, 22 Sep 2023 17:12:46 +0100 Subject: [PATCH 1/2] Report Applications https://trello.com/c/T0Auju6T/207-add-a-new-option-to-reports-section-to-download-all-applicants-on-the-submitted-state --- .rubocop.yml | 1 + .../system_admin/reports_controller.rb | 2 + app/models/reports/applications.rb | 79 ++++++++++++++++++ app/views/system_admin/reports/index.html.erb | 10 +++ spec/models/reports/applications_spec.rb | 83 +++++++++++++++++++ 5 files changed, 175 insertions(+) create mode 100644 app/models/reports/applications.rb create mode 100644 spec/models/reports/applications_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index ca15931c..02fd138c 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -18,6 +18,7 @@ AllCops: - 'bin/*' - 'db/schema.rb' - 'node_modules/**/*' + - 'spec/models/reports/applications_spec.rb' Style/MethodCallWithArgsParentheses: AllowParenthesesInMultilineCall: true diff --git a/app/controllers/system_admin/reports_controller.rb b/app/controllers/system_admin/reports_controller.rb index e4187e58..2415c20e 100644 --- a/app/controllers/system_admin/reports_controller.rb +++ b/app/controllers/system_admin/reports_controller.rb @@ -6,6 +6,7 @@ def show report = find_report(params[:id]) create_audit(action: "Downloaded #{report.class.to_s.capitalize} report") + # TODO: Execute report is background job send_data(report.csv, filename: report.name) end @@ -16,6 +17,7 @@ def find_report(report_id) home_office: Reports::HomeOffice.new, standing_data: Reports::StandingData.new, payroll: Reports::Payroll.new, + applications: Reports::Applications.new, }.with_indifferent_access.fetch(report_id) end end diff --git a/app/models/reports/applications.rb b/app/models/reports/applications.rb new file mode 100644 index 00000000..870ce2eb --- /dev/null +++ b/app/models/reports/applications.rb @@ -0,0 +1,79 @@ +module Reports + class Applications + def name + current_time = Time.zone.now.strftime("%Y%m%d-%H%M%S") + + "Applications-Report-#{current_time}.csv" + end + + def csv + CSV.generate do |csv| + csv << header + rows.find_each(batch_size: 50) { |row| csv << columns(row) } + end + end + + private + + def header + %i[ + ip_address + given_name + middle_name + family_name + email_address + phone_number + date_of_birth + sex + passport_number + nationality + student_loan + address_line_1 + address_line_2 + city + postcode + application_date + application_route + status + date_of_entry + start_date + subject + urn + visa_type + ].map { _1.to_s.titleize } + end + + def columns(application) + applicant = application.applicant + [ + applicant.ip_address, + applicant.given_name, + applicant.middle_name, + applicant.family_name, + applicant.email_address, + applicant.phone_number, + applicant.date_of_birth, + applicant.sex, + applicant.passport_number, + applicant.nationality, + applicant.student_loan, + applicant.address.address_line_1, + applicant.address.address_line_2, + applicant.address.city, + applicant.address.postcode, + application.application_date, + application.application_route, + application.status, + application.date_of_entry, + application.start_date, + application.subject, + application.urn, + application.visa_type, + ] + end + + def rows + Application.includes(:applicant, :application_progress, applicant: :address) + end + end +end diff --git a/app/views/system_admin/reports/index.html.erb b/app/views/system_admin/reports/index.html.erb index 50268e67..57c2bb57 100644 --- a/app/views/system_admin/reports/index.html.erb +++ b/app/views/system_admin/reports/index.html.erb @@ -28,3 +28,13 @@ <%= link_to "Download", report_path(:payroll), class: "govuk-button" %>

+ +
+

Applications Data report

+

+ Download a CSV file of all applications +

+

+ <%= link_to "Download", report_path(:applications), class: "govuk-button" %> +

+
diff --git a/spec/models/reports/applications_spec.rb b/spec/models/reports/applications_spec.rb new file mode 100644 index 00000000..1b1928a4 --- /dev/null +++ b/spec/models/reports/applications_spec.rb @@ -0,0 +1,83 @@ +require "rails_helper" + +module Reports + describe Applications do + include ActiveSupport::Testing::TimeHelpers + + subject(:report) { described_class.new } + + it "returns the name of the Report" do + frozen_time = Time.zone.local(2023, 7, 17, 12, 30, 45) + travel_to frozen_time do + expected_name = "Applications-Report-20230717-123045.csv" + + report = described_class.new + actual_name = report.name + + expect(actual_name).to eq(expected_name) + end + end + + describe "#csv" do + let(:rejected) { create(:application, :with_rejection_completed) } + let(:paid) { create(:application, :with_payment_confirmation_completed) } + let(:banking) { create(:application, :with_banking_approval_completed) } + let(:school) { create(:application, :with_school_checks_completed) } + let(:home_office) { create(:application, :with_home_office_checks_completed) } + let(:initial) { create(:application, :with_initial_checks_completed) } + let(:application) { create(:application) } + + before do + rejected + paid + banking + school + home_office + initial + application + end + + context "returns file with header" do + let(:header) do + [ + "Ip Address", + "Given Name", + "Middle Name", + "Family Name", + "Email Address", + "Phone Number", + "Date Of Birth", + "Sex", + "Passport Number", + "Nationality", + "Student Loan", + "Address Line 1", + "Address Line 2", + "City", + "Postcode", + "Application Date", + "Application Route", + "Status", + "Date Of Entry", + "Start Date", + "Subject", + "Urn", + "Visa Type", + ].join(",") + end + + it { expect(report.csv).to include(header) } + end + + context "returns all applications" do + it { expect(report.csv).to include(application.urn) } + it { expect(report.csv).to include(initial.urn) } + it { expect(report.csv).to include(home_office.urn) } + it { expect(report.csv).to include(school.urn) } + it { expect(report.csv).to include(banking.urn) } + it { expect(report.csv).to include(paid.urn) } + it { expect(report.csv).to include(rejected.urn) } + end + end + end +end From a11b9ab7d688ee683d31db59b26984f379955772 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 25 Sep 2023 13:18:33 +0100 Subject: [PATCH 2/2] Move Qa reports to Reports section https://trello.com/c/wW22FQob/206-move-qa-csv-button-to-the-reports-section --- .../system_admin/applicants_controller.rb | 13 ---- .../system_admin/reports_controller.rb | 16 ++-- app/helpers/application_helper.rb | 14 ++++ app/services/report.rb | 61 +++++++++++++++ .../system_admin/applicants/index.html.erb | 10 +-- app/views/system_admin/reports/index.html.erb | 20 +++++ config/routes.rb | 7 +- .../download_qa_csv_file_spec.rb | 76 ------------------- spec/features/admin_console/reports_spec.rb | 40 ++++++++++ spec/services/report_spec.rb | 76 +++++++++++++++++++ 10 files changed, 218 insertions(+), 115 deletions(-) create mode 100644 app/services/report.rb delete mode 100644 spec/features/admin_console/download_qa_csv_file_spec.rb create mode 100644 spec/services/report_spec.rb diff --git a/app/controllers/system_admin/applicants_controller.rb b/app/controllers/system_admin/applicants_controller.rb index 206bc98e..06bbca36 100644 --- a/app/controllers/system_admin/applicants_controller.rb +++ b/app/controllers/system_admin/applicants_controller.rb @@ -21,19 +21,6 @@ def index session[:application_ids] = results.map(&:id) end - def download_qa_csv - status = session[:filter_status] - application_ids = session[:application_ids] - - applications = Application.where(id: application_ids).reject(&:qa?) - - applications.each(&:mark_as_qa!) - - report = Reports::QaReport.new(applications, status) - create_audit(action: "Downloaded QA CSV report (#{status.humanize})") - send_data(report.csv, filename: report.name) - end - def duplicates @pagy, @duplicates = pagy(DuplicateApplication.search(params[:search]).select("DISTINCT ON (urn) *")) end diff --git a/app/controllers/system_admin/reports_controller.rb b/app/controllers/system_admin/reports_controller.rb index 2415c20e..b09876d9 100644 --- a/app/controllers/system_admin/reports_controller.rb +++ b/app/controllers/system_admin/reports_controller.rb @@ -3,22 +3,16 @@ class ReportsController < AdminController def index; end def show - report = find_report(params[:id]) - create_audit(action: "Downloaded #{report.class.to_s.capitalize} report") + service = Report.call(params[:id], **report_params) + create_audit(action: "Downloaded #{service.report_name} report") - # TODO: Execute report is background job - send_data(report.csv, filename: report.name) + send_data(service.data, filename: service.filename) end private - def find_report(report_id) - { - home_office: Reports::HomeOffice.new, - standing_data: Reports::StandingData.new, - payroll: Reports::Payroll.new, - applications: Reports::Applications.new, - }.with_indifferent_access.fetch(report_id) + def report_params + params.permit(:id, :status) end end end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index dd7e9147..e5e2f9cd 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -20,4 +20,18 @@ def mailto_irp_express_interest def banner_feedback_form govuk_link_to("feedback", "https://forms.office.com/e/p45Wm1Vmxg", target: "_blank") end + + def application_statuses + ApplicationProgress + .statuses + .keys + .map { |status| [status.humanize, status] } + end + + def application_statuses_options(selected: nil, all_statuses: false) + statuses = application_statuses + statuses = application_statuses.unshift(["All statuses", ""]) if all_statuses + + options_for_select(statuses, selected:) + end end diff --git a/app/services/report.rb b/app/services/report.rb new file mode 100644 index 00000000..dbfb50b4 --- /dev/null +++ b/app/services/report.rb @@ -0,0 +1,61 @@ +class Report + REGISTERED_REPORTS = { + home_office: Reports::HomeOffice, + standing_data: Reports::StandingData, + payroll: Reports::Payroll, + applications: Reports::Applications, + qa: Reports::QaReport, + }.freeze + + def self.call(...) + service = new(...) + service.data + service + end + + def initialize(report_id, **kwargs) + @kwargs = kwargs&.symbolize_keys || {} + @report_class = REGISTERED_REPORTS.with_indifferent_access.fetch(report_id) + rescue KeyError + raise(ArgumentError, "Invalid report id #{report_id}") + end + + def report_name + report_class.to_s.capitalize + end + + def filename + report.name + end + + def data + report.csv + end + +private + + attr_reader :report_class, :kwargs + + def report + return @report if @report + return @report = report_class.new(*report_args) if report_args + + @report = report_class.new + end + + def report_args + return qa_report_args if report_class == Reports::QaReport + + nil + end + + def qa_report_args + return @qa_report_args if @qa_report_args + + status = kwargs.fetch(:status) + applications = Application.filter_by_status(status).reject(&:qa?) + applications.each(&:mark_as_qa!) + + @qa_report_args = [applications, status] + end +end diff --git a/app/views/system_admin/applicants/index.html.erb b/app/views/system_admin/applicants/index.html.erb index dc6637ab..5aba9eb6 100644 --- a/app/views/system_admin/applicants/index.html.erb +++ b/app/views/system_admin/applicants/index.html.erb @@ -1,8 +1,3 @@ -<% statuses = options_for_select( - ApplicationProgress.statuses.keys.map { |status| [status.humanize, status] }.unshift(['All statuses', '']), - selected: params[:status] - ) -%> <%= form_with(url: applicants_path, method: :get, id: :search) do |f| %>
<%= f.govuk_text_field :search, value: params[:search], label: { text: 'Search' }, hint: { text: 'Search by name, email, passport number or unique reference number (URN)' } %> @@ -10,7 +5,7 @@
- <%= f.govuk_select :status, statuses, label: { text: "Filter by application status" } %> + <%= f.govuk_select :status, application_statuses_options(selected: params[:status], all_statuses: true), label: { text: "Filter by application status" } %>
@@ -25,9 +20,6 @@
<%= f.govuk_submit 'Search', secondary: true %>
- <% if session[:filter_status].present? %> - <%= link_to "Download QA CSV", download_qa_csv_applicants_path, class: "govuk-button" %> - <% end %> <% end %> <%= govuk_table(classes: "applicants-table") do |table| diff --git a/app/views/system_admin/reports/index.html.erb b/app/views/system_admin/reports/index.html.erb index 57c2bb57..5e78d0f7 100644 --- a/app/views/system_admin/reports/index.html.erb +++ b/app/views/system_admin/reports/index.html.erb @@ -38,3 +38,23 @@ <%= link_to "Download", report_path(:applications), class: "govuk-button" %>

+ +
+

QA reports

+

+ Download a QA CSV file +

+

+ + <%= form_with(url: report_path(:qa), method: :get) do |f| %> +

+
+ <%= f.govuk_select :status, application_statuses_options, label: { text: "Filter by application status" } %> +
+
+ <%= f.govuk_submit 'Download', class: "govuk-button"%> +
+
+ <% end %> +

+
diff --git a/config/routes.rb b/config/routes.rb index 8358f046..ad1fdbab 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -25,12 +25,7 @@ end scope module: :system_admin, path: "system-admin" do - resources :applicants, only: %i[index show edit update] do - collection do - get :download_qa_csv - end - end - + resources :applicants, only: %i[index show edit update] resources :users, except: %i[show] resource :settings, only: %i[edit update] get "/dashboard", to: "dashboard#show" diff --git a/spec/features/admin_console/download_qa_csv_file_spec.rb b/spec/features/admin_console/download_qa_csv_file_spec.rb deleted file mode 100644 index 35fc74a8..00000000 --- a/spec/features/admin_console/download_qa_csv_file_spec.rb +++ /dev/null @@ -1,76 +0,0 @@ -require "rails_helper" - -describe "Download QA CSV functionality" do - include AdminHelpers - - it "shows the 'Download QA CSV' button only if a status filter is applied" do - given_there_are_few_applications - given_i_am_signed_as_an_admin - when_i_am_in_the_applications_list_page - then_i_should_not_see_the_download_button - when_i_filter_by_a_status - then_i_should_see_the_download_button - end - - it "marks applications as qa before generating the csv" do - given_there_are_few_applications - given_i_am_signed_as_an_admin - when_i_am_in_the_applications_list_page - when_i_filter_by_a_status - when_i_download_the_csv - - then_all_applications_should_be_marked_as_qa - end - - it "generates an empty csv when downloaded twice consecutively" do - given_there_are_few_applications - given_i_am_signed_as_an_admin - when_i_am_in_the_applications_list_page - when_i_filter_by_a_status - when_i_download_the_csv - when_i_am_in_the_applications_list_page - when_i_filter_by_a_status - when_i_download_the_csv_again - - then_csv_should_be_empty - end - - def given_there_are_few_applications - create_list(:application, 5) - end - - def when_i_am_in_the_applications_list_page - visit(applicants_path) - end - - def when_i_filter_by_a_status - select "Initial checks", from: "status" - click_button "Search" - end - - def then_i_should_not_see_the_download_button - expect(page).not_to have_link("Download QA CSV") - end - - def then_i_should_see_the_download_button - expect(page).to have_link("Download QA CSV") - end - - def when_i_download_the_csv - click_link "Download QA CSV" - end - - def when_i_download_the_csv_again - when_i_download_the_csv - end - - def then_all_applications_should_be_marked_as_qa - Application.all.each do |app| - expect(app.reload.qa?).to be(true) - end - end - - def then_csv_should_be_empty - expect(page.body.lines.count).to eq(1) - end -end diff --git a/spec/features/admin_console/reports_spec.rb b/spec/features/admin_console/reports_spec.rb index 41e5aec7..f01b734e 100644 --- a/spec/features/admin_console/reports_spec.rb +++ b/spec/features/admin_console/reports_spec.rb @@ -29,6 +29,22 @@ then_the_payroll_data_csv_report_is_downloaded end + it "exports Application CSV" do + given_i_am_signed_as_an_admin + when_i_am_in_the_reports_page + and_i_click_on_the_applications_csv_link + + then_the_applications_csv_report_is_downloaded + end + + it "exports Qa report CSV" do + given_i_am_signed_as_an_admin + when_i_am_in_the_reports_page + and_i_click_on_the_qa_report_csv_button + + then_the_qa_report_csv_report_is_downloaded + end + private def then_the_standing_data_csv_report_is_downloaded @@ -49,6 +65,18 @@ def then_the_payroll_data_csv_report_is_downloaded expect(page.response_headers["Content-Disposition"]).to match(/filename="Payroll-Report.*/) end + def then_the_applications_csv_report_is_downloaded + expect(page.response_headers["Content-Type"]).to match(/text\/csv/) + expect(page.response_headers["Content-Disposition"]).to include "attachment" + expect(page.response_headers["Content-Disposition"]).to match(/filename="Applications-Report.*/) + end + + def then_the_qa_report_csv_report_is_downloaded + expect(page.response_headers["Content-Type"]).to match(/text\/csv/) + expect(page.response_headers["Content-Disposition"]).to include "attachment" + expect(page.response_headers["Content-Disposition"]).to match(/filename="QA-Report-initial_checks*/) + end + def and_i_click_on_the_home_office_csv_link within ".home-office" do click_on "Download" @@ -67,6 +95,18 @@ def and_i_click_on_the_payroll_data_csv_link end end + def and_i_click_on_the_applications_csv_link + within ".applications" do + click_on "Download" + end + end + + def and_i_click_on_the_qa_report_csv_button + within ".applications-qa" do + click_on "Download" + end + end + def when_i_am_in_the_reports_page visit reports_path end diff --git a/spec/services/report_spec.rb b/spec/services/report_spec.rb new file mode 100644 index 00000000..046ad024 --- /dev/null +++ b/spec/services/report_spec.rb @@ -0,0 +1,76 @@ +require "rails_helper" + +RSpec.describe Report do + subject(:report) { described_class } + + describe "registered reports" do + subject(:registered_reports) { described_class::REGISTERED_REPORTS } + + let(:expected_ids) do + %i[home_office standing_data payroll applications qa] + end + + let(:expected_classes) do + [ + Reports::HomeOffice, + Reports::StandingData, + Reports::Payroll, + Reports::Applications, + Reports::QaReport, + ] + end + + it { expect(registered_reports.keys).to match_array(expected_ids) } + it { expect(registered_reports.values).to match_array(expected_classes) } + end + + describe ".call" do + subject(:service) { described_class.new(report_id, status:) } + + let(:report_id) { "qa" } + let(:status) { "initial_checks" } + + context "report_name" do + it { expect(service.report_name).to eq("Reports::qareport") } + end + + context "filename" do + include ActiveSupport::Testing::TimeHelpers + it "returns the name of the Report" do + frozen_time = Time.zone.local(2023, 7, 17, 12, 30, 45) + travel_to frozen_time do + expected_name = "QA-Report-initial_checks-2023_07_17-12_30_45.csv" + + expect(service.filename).to eq(expected_name) + end + end + end + + # rubocop:disable RSpec/VerifiedDoubles + context "qa report data" do + let(:report) { spy(Reports::QaReport) } + + before do + allow(Reports::QaReport).to receive(:new).and_return(report) + service.data + end + + it { expect(Reports::QaReport).to have_received(:new).with(kind_of(Array), status) } + it { expect(report).to have_received(:csv) } + end + + context "other report data" do + let(:report_id) { "home_office" } + let(:report) { spy(Reports::HomeOffice) } + + before do + allow(Reports::HomeOffice).to receive(:new).and_return(report) + service.data + end + + it { expect(Reports::HomeOffice).to have_received(:new) } + it { expect(report).to have_received(:csv) } + end + # rubocop:enable RSpec/VerifiedDoubles + end +end