From fb3a6967791b76b7753716487a9ed9da1b158926 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 7 May 2025 13:41:57 +0300 Subject: [PATCH 01/15] Refactor specs for feature reports - Add form with routing to fixture - Add more request specs for report CSV downloads - Refactor view specs to remove explicit template - Make it clear what method is being described in Reports::FormDocumentsService specs --- .../files/form_documents_response.json | 96 ++++++++++++ spec/requests/reports_controller_spec.rb | 146 +++++++++++++++--- .../reports/csv_reports_service_spec.rb | 13 +- .../reports/feature_report_service_spec.rb | 31 ++-- .../reports/form_documents_service_spec.rb | 127 +++++++-------- ...th_csv_submission_enabled.html.erb_spec.rb | 4 +- .../forms_with_payments.html.erb_spec.rb | 4 +- .../forms_with_routes.html.erb_spec.rb | 4 +- ...s_with_add_another_answer.html.erb_spec.rb | 4 +- ...uestions_with_answer_type.html.erb_spec.rb | 4 +- 10 files changed, 319 insertions(+), 114 deletions(-) diff --git a/spec/fixtures/files/form_documents_response.json b/spec/fixtures/files/form_documents_response.json index 80a585bd7..d05c906c3 100644 --- a/spec/fixtures/files/form_documents_response.json +++ b/spec/fixtures/files/form_documents_response.json @@ -425,5 +425,101 @@ }, "created_at": "2025-01-02T16:24:31.445Z", "updated_at": "2025-01-02T16:24:31.445Z" + }, + { + "id": 4, + "form_id": 4, + "tag": "live", + "content": { + "name": "Skip route form", + "steps": [ + { + "id": 17, + "data": { + "hint_text": null, + "answer_type": "selection", + "is_optional": false, + "page_heading": null, + "is_repeatable": false, + "question_text": "Would you like to submit anonymously?", + "answer_settings": { + "only_one_option": "true", + "selection_options": [ + { + "name": "Yes" + }, + { + "name": "No" + } + ] + }, + "guidance_markdown": null + }, + "type": "question_page", + "position": 1, + "next_step_id": 18, + "routing_conditions": [ + { + "id": 4, + "created_at": "2025-05-07T10:28:31.149Z", + "updated_at": "2025-05-07T10:28:31.149Z", + "skip_to_end": true, + "answer_value": "Yes", + "goto_page_id": null, + "check_page_id": 17, + "routing_page_id": 17, + "exit_page_heading": null, + "validation_errors": [], + "exit_page_markdown": null + } + ] + }, + { + "id": 18, + "data": { + "hint_text": null, + "answer_type": "name", + "is_optional": false, + "page_heading": null, + "is_repeatable": false, + "question_text": "What’s your name?", + "answer_settings": { + "input_type": "full_name", + "title_needed": false + }, + "guidance_markdown": null + }, + "type": "question_page", + "position": 2, + "next_step_id": null, + "routing_conditions": [] + } + ], + "form_id": "4", + "live_at": "2025-05-07T10:28:31.206Z", + "form_slug": "skip-route-form", + "created_at": "2025-05-07T10:28:31.128Z", + "creator_id": null, + "start_page": 17, + "updated_at": "2025-05-07T10:28:31.206Z", + "payment_url": null, + "support_url": null, + "support_email": "your.email+fakedata84701@gmail.com.gov.uk", + "support_phone": null, + "s3_bucket_name": null, + "submission_type": "email", + "declaration_text": "", + "s3_bucket_region": null, + "submission_email": "", + "support_url_text": null, + "privacy_policy_url": "https://www.gov.uk/help/privacy-notice", + "share_preview_completed": true, + "s3_bucket_aws_account_id": null, + "question_section_completed": true, + "what_happens_next_markdown": "This is a test form", + "declaration_section_completed": true + }, + "created_at": "2025-05-07T10:28:31.274Z", + "updated_at": "2025-05-07T10:28:31.274Z" } ] diff --git a/spec/requests/reports_controller_spec.rb b/spec/requests/reports_controller_spec.rb index 4407cd411..5fc9dc1a8 100644 --- a/spec/requests/reports_controller_spec.rb +++ b/spec/requests/reports_controller_spec.rb @@ -695,62 +695,158 @@ expect(response.body).to include "A question" end end + end - describe "#live_forms_csv" do - let(:csv_reports_service_mock) { instance_double(Reports::CsvReportsService) } - let(:dummy_csv) { '"Column 1", "Column 2"\n"Value 1", "Value 2"' } + describe "csv downloads" do + shared_examples_for "csv response" do + it "returns http code 200" do + expect(response).to have_http_status(:ok) + end - before do - allow(Reports::CsvReportsService).to receive(:new).and_return(csv_reports_service_mock) - allow(csv_reports_service_mock).to receive(:live_forms_csv).and_return(dummy_csv) + it "has content-type text/csv" do + expect(response.headers["content-type"]).to eq "text/csv; charset=iso-8859-1" + end + end + describe "#live_forms_csv" do + before do login_as_super_admin_user + + travel_to Time.utc(2025, 5, 15, 15, 31, 57) + get report_live_forms_csv_path end - it "returns http code 200" do - expect(response).to have_http_status(:ok) - end + it_behaves_like "csv response" it "responds with an attachment content-disposition header" do - expect(response.headers["content-disposition"]).to match(/attachment; filename=live_forms_report-.*?\.csv/) + expect(response.headers["content-disposition"]).to match("attachment; filename=live_forms_report-2025-05-15 15:31:57 UTC.csv") end - it "has content-type text/csv" do - expect(response.headers["content-type"]).to eq "text/csv; charset=iso-8859-1" + it "has expected response body" do + csv = CSV.parse(response.body, headers: true) + expect(csv.headers).to eq Reports::CsvReportsService::FORM_CSV_HEADERS + expect(csv.length).to eq 4 + end + end + + describe "#live_forms_with_routes_csv" do + before do + login_as_super_admin_user + + travel_to Time.utc(2025, 5, 15, 15, 31, 57) + + get report_live_forms_with_routes_csv_path + end + + it_behaves_like "csv response" + + it "responds with an attachment content-disposition header" do + expect(response.headers["content-disposition"]).to match("attachment; filename=live_forms_with_routes_report-2025-05-15 15:31:57 UTC.csv") end it "has expected response body" do - expect(response.body).to eq(dummy_csv) + csv = CSV.parse(response.body, headers: true) + expect(csv.headers).to eq Reports::CsvReportsService::FORM_CSV_HEADERS + expect(csv.length).to eq 2 + expect(csv.by_col["Form name"]).to eq [ + "Branch route form", + "Skip route form", + ] end end - describe "#live_questions_csv" do - let(:csv_reports_service_mock) { instance_double(Reports::CsvReportsService) } - let(:dummy_csv) { '"Column 1", "Column 2"\n"Value 1", "Value 2"' } + describe "#live_forms_with_payments_csv" do + before do + login_as_super_admin_user + + travel_to Time.utc(2025, 5, 15, 15, 31, 57) + + get report_live_forms_with_payments_csv_path + end + + it_behaves_like "csv response" + + it "responds with an attachment content-disposition header" do + expect(response.headers["content-disposition"]).to match("attachment; filename=live_forms_with_payments_report-2025-05-15 15:31:57 UTC.csv") + end + + it "has expected response body" do + csv = CSV.parse(response.body, headers: true) + expect(csv.headers).to eq Reports::CsvReportsService::FORM_CSV_HEADERS + expect(csv.length).to eq 1 + expect(csv.by_col["Form name"]).to eq [ + "All question types form", + ] + end + end + describe "#live_forms_with_csv_submission_enabled_csv" do before do - allow(Reports::CsvReportsService).to receive(:new).and_return(csv_reports_service_mock) - allow(csv_reports_service_mock).to receive(:live_questions_csv).and_return(dummy_csv) + login_as_super_admin_user + + travel_to Time.utc(2025, 5, 15, 15, 31, 57) + + get report_live_forms_with_csv_submission_enabled_csv_path + end + it_behaves_like "csv response" + + it "responds with an attachment content-disposition header" do + expect(response.headers["content-disposition"]).to match("attachment; filename=live_forms_with_csv_submission_enabled_report-2025-05-15 15:31:57 UTC.csv") + end + + it "has expected response body" do + csv = CSV.parse(response.body, headers: true) + expect(csv.headers).to eq Reports::CsvReportsService::FORM_CSV_HEADERS + expect(csv.length).to eq 1 + expect(csv.by_col["Form name"]).to eq [ + "All question types form", + ] + end + end + + describe "#live_questions_csv" do + before do login_as_super_admin_user + + travel_to Time.utc(2025, 5, 15, 15, 31, 57) + get report_live_questions_csv_path end - it "returns http code 200" do - expect(response).to have_http_status(:ok) - end + it_behaves_like "csv response" it "responds with an attachment content-disposition header" do - expect(response.headers["content-disposition"]).to match(/attachment; filename=live_questions_report-.*?\.csv/) + expect(response.headers["content-disposition"]).to match("attachment; filename=live_questions_report-2025-05-15 15:31:57 UTC.csv") end - it "has content-type text/csv" do - expect(response.headers["content-type"]).to eq "text/csv; charset=iso-8859-1" + it "has expected response body" do + csv = CSV.parse(response.body, headers: true) + expect(csv.headers).to eq Reports::CsvReportsService::QUESTIONS_CSV_HEADERS + expect(csv.length).to eq 17 + end + end + + describe "#live_questions_with_add_another_answer_csv" do + before do + login_as_super_admin_user + + travel_to Time.utc(2025, 5, 15, 15, 31, 57) + + get report_live_questions_with_add_another_answer_csv_path + end + + it_behaves_like "csv response" + + it "responds with an attachment content-disposition header" do + expect(response.headers["content-disposition"]).to match("attachment; filename=live_questions_with_add_another_answer_report-2025-05-15 15:31:57 UTC.csv") end it "has expected response body" do - expect(response.body).to eq(dummy_csv) + csv = CSV.parse(response.body, headers: true) + expect(csv.headers).to eq Reports::CsvReportsService::QUESTIONS_CSV_HEADERS + expect(csv.length).to eq 2 end end end diff --git a/spec/services/reports/csv_reports_service_spec.rb b/spec/services/reports/csv_reports_service_spec.rb index 5d80e1f45..0cb2a4b81 100644 --- a/spec/services/reports/csv_reports_service_spec.rb +++ b/spec/services/reports/csv_reports_service_spec.rb @@ -13,15 +13,16 @@ GroupForm.create!(form_id: 1, group:) GroupForm.create!(form_id: 2, group:) GroupForm.create!(form_id: 3, group:) + GroupForm.create!(form_id: 4, group:) allow(Reports::FormDocumentsService).to receive(:live_form_documents).and_return(form_documents_response_json) end describe "#live_forms_csv" do - it "returns a CSV with 4 rows, including the header row" do + it "returns a CSV with a header row and a row for each form" do csv = csv_reports_service.live_forms_csv rows = CSV.parse(csv) - expect(rows.length).to eq 4 + expect(rows.length).to eq 5 end it "has expected values" do @@ -53,10 +54,10 @@ end describe "#live_forms_with_routes_csv" do - it "returns a CSV with 2 rows, including the header row" do + it "returns a CSV with a header row and a row for each form with routes" do csv = csv_reports_service.live_forms_with_routes_csv rows = CSV.parse(csv) - expect(rows.length).to eq 2 + expect(rows.length).to eq 3 end it "includes form with routes" do @@ -99,10 +100,10 @@ describe "#live_questions_csv" do context "when answer_type is nil" do - it "returns a CSV with 16 rows, including the header row" do + it "returns a CSV with a header row and a rows for each question" do csv = csv_reports_service.live_questions_csv rows = CSV.parse(csv) - expect(rows.length).to eq 16 + expect(rows.length).to eq 18 end it "has expected values for text question" do diff --git a/spec/services/reports/feature_report_service_spec.rb b/spec/services/reports/feature_report_service_spec.rb index 1c4859803..45e0ee696 100644 --- a/spec/services/reports/feature_report_service_spec.rb +++ b/spec/services/reports/feature_report_service_spec.rb @@ -8,6 +8,7 @@ GroupForm.create!(form_id: 1, group:) GroupForm.create!(form_id: 2, group:) GroupForm.create!(form_id: 3, group:) + GroupForm.create!(form_id: 4, group:) allow(Reports::FormDocumentsService).to receive(:live_form_documents).and_return(form_documents_response_json) end @@ -16,31 +17,31 @@ it "returns the feature report" do report = described_class.report expect(report).to eq({ - total_live_forms: 3, + total_live_forms: 4, live_forms_with_payment: 1, - live_forms_with_routing: 1, + live_forms_with_routing: 2, live_forms_with_add_another_answer: 1, live_forms_with_csv_submission_enabled: 1, live_forms_with_answer_type: { "address" => 1, "date" => 1, "email" => 2, - "name" => 1, + "name" => 2, "national_insurance_number" => 1, "number" => 1, "phone_number" => 1, - "selection" => 2, + "selection" => 3, "text" => 3, }, live_steps_with_answer_type: { "address" => 1, "date" => 1, "email" => 2, - "name" => 1, + "name" => 2, "national_insurance_number" => 1, "number" => 1, "phone_number" => 1, - "selection" => 2, + "selection" => 3, "text" => 5, }, }) @@ -92,12 +93,20 @@ describe "#live_forms_with_routes" do it "returns forms with routes" do forms = described_class.live_forms_with_routes - expect(forms.length).to eq 1 + expect(forms.length).to eq 2 expect(forms).to include( - form_name: "Branch route form", - form_id: 3, - organisation_name: group.organisation.name, - number_of_routes: 2, + { + form_name: "Branch route form", + form_id: 3, + organisation_name: group.organisation.name, + number_of_routes: 2, + }, + { + form_name: "Skip route form", + form_id: 4, + organisation_name: group.organisation.name, + number_of_routes: 1, + }, ) end end diff --git a/spec/services/reports/form_documents_service_spec.rb b/spec/services/reports/form_documents_service_spec.rb index bad1082e3..c4639a0fd 100644 --- a/spec/services/reports/form_documents_service_spec.rb +++ b/spec/services/reports/form_documents_service_spec.rb @@ -1,80 +1,83 @@ require "rails_helper" RSpec.describe Reports::FormDocumentsService do - let(:form_documents_url) { "#{Settings.forms_api.base_url}/api/v2/form-documents".freeze } - # This response JSON was generated by making a real API request to forms-api with the data from the database seeds. - # Once we have transitioned to using the V2 API in forms-admin, it might make more sense to use factories to generate - # the response. - let(:form_documents_response_json) { file_fixture("form_documents_response.json").read } + describe ".live_form_documents" do + let(:form_documents_url) { "#{Settings.forms_api.base_url}/api/v2/form-documents".freeze } + # This response JSON was generated by making a real API request to forms-api with the data from the database seeds. + # Once we have transitioned to using the V2 API in forms-admin, it might make more sense to use factories to generate + # the response. + let(:form_documents_response_json) { file_fixture("form_documents_response.json").read } - before do - stub_request(:get, form_documents_url) - .with(query: { page: "1", per_page: "3", tag: "live" }) - .to_return(body: form_documents_response_json, headers: response_headers(9, 0, 3)) - stub_request(:get, form_documents_url) - .with(query: { page: "2", per_page: "3", tag: "live" }) - .to_return(body: form_documents_response_json, headers: response_headers(9, 3, 3)) - stub_request(:get, form_documents_url) - .with(query: { page: "3", per_page: "3", tag: "live" }) - .to_return(body: form_documents_response_json, headers: response_headers(9, 6, 3)) - end - - it "makes request to forms-api for each page of results" do - form_documents = described_class.live_form_documents.to_a - expect(form_documents.size).to eq(9) - assert_requested(:get, form_documents_url, query: { page: "1", per_page: "3", tag: "live" }, headers:, times: 1) - assert_requested(:get, form_documents_url, query: { page: "2", per_page: "3", tag: "live" }, headers:, times: 1) - assert_requested(:get, form_documents_url, query: { page: "3", per_page: "3", tag: "live" }, headers:, times: 1) - end - - it "returns form documents" do - form_document = described_class.live_form_documents.first - expect(form_document).to be_a(Hash) - expect(form_document).to have_key("form_id") - end - - context "when forms-api responds with a non-success status code" do before do + allow(Settings.reports).to receive(:forms_api_forms_per_request_page).and_return 4 + + stub_request(:get, form_documents_url) + .with(query: { page: "1", per_page: "4", tag: "live" }) + .to_return(body: form_documents_response_json, headers: response_headers(12, 0, 4)) stub_request(:get, form_documents_url) - .with(query: { page: "1", per_page: "3", tag: "live" }) - .to_return(body: "There was an error", status: 400) + .with(query: { page: "2", per_page: "4", tag: "live" }) + .to_return(body: form_documents_response_json, headers: response_headers(12, 4, 4)) + stub_request(:get, form_documents_url) + .with(query: { page: "3", per_page: "4", tag: "live" }) + .to_return(body: form_documents_response_json, headers: response_headers(12, 8, 4)) end - it "raises a StandardError" do - expect { described_class.live_form_documents.first }.to raise_error( - StandardError, "Forms API responded with a non-success HTTP code when retrieving form documents: status 400" - ) + it "makes request to forms-api for each page of results" do + form_documents = described_class.live_form_documents.to_a + expect(form_documents.size).to eq(12) + assert_requested(:get, form_documents_url, query: { page: "1", per_page: "4", tag: "live" }, headers:, times: 1) + assert_requested(:get, form_documents_url, query: { page: "2", per_page: "4", tag: "live" }, headers:, times: 1) + assert_requested(:get, form_documents_url, query: { page: "3", per_page: "4", tag: "live" }, headers:, times: 1) end - end - context "when there are forms from internal organisations" do - let(:organisation) { build :organisation, id: 1, internal: false } - let(:internal_organisation) { build :organisation, id: 1, internal: true } - let(:group) { build :group, id: 1, organisation: } - let(:internal_group) { build :group, id: 1, organisation: internal_organisation } - let(:group_form) { GroupForm.new(group:) } - let(:internal_group_form) { GroupForm.new(group: internal_group) } + it "returns form documents" do + form_document = described_class.live_form_documents.first + expect(form_document).to be_a(Hash) + expect(form_document).to have_key("form_id") + end - before do - allow(GroupForm).to receive(:find_by_form_id).with(1).and_return(group_form) - allow(GroupForm).to receive(:find_by_form_id).with(2).and_return(group_form) - allow(GroupForm).to receive(:find_by_form_id).with(3).and_return(internal_group_form) + context "when forms-api responds with a non-success status code" do + before do + stub_request(:get, form_documents_url) + .with(query: { page: "1", per_page: "4", tag: "live" }) + .to_return(body: "There was an error", status: 400) + end + + it "raises a StandardError" do + expect { described_class.live_form_documents.first }.to raise_error( + StandardError, "Forms API responded with a non-success HTTP code when retrieving form documents: status 400" + ) + end end - it "does not include these forms in the live_form_documents output" do - form_documents = described_class.live_form_documents.to_a - expect(form_documents.size).to eq(6) + context "when there are forms from internal organisations" do + let(:organisation) { build :organisation, id: 1, internal: false } + let(:internal_organisation) { build :organisation, id: 1, internal: true } + let(:group) { build :group, id: 1, organisation: } + let(:internal_group) { build :group, id: 1, organisation: internal_organisation } + let(:group_form) { GroupForm.new(group:) } + let(:internal_group_form) { GroupForm.new(group: internal_group) } - form_documents_with_internal_id = form_documents.filter { it["id"] == 3 } - expect(form_documents_with_internal_id).to be_empty + before do + allow(GroupForm).to receive(:find_by_form_id).and_return(group_form) + allow(GroupForm).to receive(:find_by_form_id).with(3).and_return(internal_group_form) + end + + it "does not include these forms in the live_form_documents output" do + form_documents = described_class.live_form_documents.to_a + expect(form_documents.size).to eq(9) + + form_documents_with_internal_id = form_documents.filter { it["id"] == 3 } + expect(form_documents_with_internal_id).to be_empty + end end - end - def response_headers(total, offset, limit) - { - "pagination-total" => total.to_s, - "pagination-offset" => offset.to_s, - "pagination-limit" => limit.to_s, - } + def response_headers(total, offset, limit) + { + "pagination-total" => total.to_s, + "pagination-offset" => offset.to_s, + "pagination-limit" => limit.to_s, + } + end end end diff --git a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb index 05c7fa133..6c9a2e944 100644 --- a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb +++ b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -describe "reports/forms_with_csv_submission_enabled.html.erb" do +describe "reports/forms_with_csv_submission_enabled" do let(:forms) do [ { @@ -17,7 +17,7 @@ end before do - render template: "reports/forms_with_csv_submission_enabled", locals: { forms: } + render locals: { forms: } end describe "page title" do diff --git a/spec/views/reports/forms_with_payments.html.erb_spec.rb b/spec/views/reports/forms_with_payments.html.erb_spec.rb index e8fec4211..bdf4bb3bc 100644 --- a/spec/views/reports/forms_with_payments.html.erb_spec.rb +++ b/spec/views/reports/forms_with_payments.html.erb_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -describe "reports/forms_with_payments.html.erb" do +describe "reports/forms_with_payments" do let(:forms) do [ { @@ -17,7 +17,7 @@ end before do - render template: "reports/forms_with_payments", locals: { forms: } + render locals: { forms: } end describe "page title" do diff --git a/spec/views/reports/forms_with_routes.html.erb_spec.rb b/spec/views/reports/forms_with_routes.html.erb_spec.rb index 7e1947a4d..1047eb459 100644 --- a/spec/views/reports/forms_with_routes.html.erb_spec.rb +++ b/spec/views/reports/forms_with_routes.html.erb_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -describe "reports/forms_with_routes.html.erb" do +describe "reports/forms_with_routes" do let(:forms) do [ { @@ -19,7 +19,7 @@ end before do - render template: "reports/forms_with_routes", locals: { forms: } + render locals: { forms: } end describe "page title" do diff --git a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb index a19bfcd0e..726835f57 100644 --- a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb +++ b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -describe "reports/questions_with_add_another_answer.html.erb" do +describe "reports/questions_with_add_another_answer" do let(:questions) do [ { @@ -19,7 +19,7 @@ end before do - render template: "reports/questions_with_add_another_answer", locals: { answer_type: "email", questions: } + render locals: { questions: } end describe "page title" do diff --git a/spec/views/reports/questions_with_answer_type.html.erb_spec.rb b/spec/views/reports/questions_with_answer_type.html.erb_spec.rb index 09f2b7ca7..7b9c607c9 100644 --- a/spec/views/reports/questions_with_answer_type.html.erb_spec.rb +++ b/spec/views/reports/questions_with_answer_type.html.erb_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -describe "reports/questions_with_answer_type.html.erb" do +describe "reports/questions_with_answer_type" do let(:questions) do [ { @@ -19,7 +19,7 @@ end before do - render template: "reports/questions_with_answer_type", locals: { answer_type: "email", questions: } + render locals: { answer_type: "email", questions: } end describe "page title" do From 8a29b084ad7fb7d8e03416f727c37ce7050de9ba Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 9 May 2025 10:03:36 +0300 Subject: [PATCH 02/15] Add ReportHelper helpers to get data in the right shape for govuk_table Pass head and rows to govuk_table [[1]] as arrays of strings, rather than constructing the table in each view template. This reduces repetition and makes reuse easier. [1]: https://govuk-components.netlify.app/components/table --- app/helpers/report_helper.rb | 57 +++++++ ...forms_with_csv_submission_enabled.html.erb | 21 +-- .../reports/forms_with_payments.html.erb | 21 +-- app/views/reports/forms_with_routes.html.erb | 23 +-- ...questions_with_add_another_answer.html.erb | 23 +-- .../questions_with_answer_type.html.erb | 23 +-- spec/helpers/report_helpers_spec.rb | 159 ++++++++++++++++++ 7 files changed, 236 insertions(+), 91 deletions(-) create mode 100644 app/helpers/report_helper.rb create mode 100644 spec/helpers/report_helpers_spec.rb diff --git a/app/helpers/report_helper.rb b/app/helpers/report_helper.rb new file mode 100644 index 000000000..30d009407 --- /dev/null +++ b/app/helpers/report_helper.rb @@ -0,0 +1,57 @@ +module ReportHelper + def report_forms_table_head + [ + I18n.t("reports.form_or_questions_list_table.headings.form_name"), + I18n.t("reports.form_or_questions_list_table.headings.organisation"), + ] + end + + def report_forms_table_rows(forms) + forms.map { |form| report_forms_table_row(form) } + end + + def report_forms_with_routes_table_head + [ + *report_forms_table_head, + I18n.t("reports.form_or_questions_list_table.headings.number_of_routes"), + ] + end + + def report_forms_with_routes_table_rows(forms) + forms.map { |form| report_forms_with_routes_table_row(form) } + end + + def report_questions_table_head + [ + *report_forms_table_head, + I18n.t("reports.form_or_questions_list_table.headings.question_text"), + ] + end + + def report_questions_table_rows(questions) + questions.map { |question| report_questions_table_row(question) } + end + +private + + def report_forms_table_row(form) + [ + govuk_link_to(form[:form_name], live_form_pages_path(form_id: form[:form_id])), + form[:organisation_name], + ] + end + + def report_forms_with_routes_table_row(form) + [ + *report_forms_table_row(form), + form[:number_of_routes].to_s, + ] + end + + def report_questions_table_row(question) + [ + *report_forms_table_row(question), + question[:question_text], + ] + end +end diff --git a/app/views/reports/forms_with_csv_submission_enabled.html.erb b/app/views/reports/forms_with_csv_submission_enabled.html.erb index c1d5563fa..372c6c55a 100644 --- a/app/views/reports/forms_with_csv_submission_enabled.html.erb +++ b/app/views/reports/forms_with_csv_submission_enabled.html.erb @@ -8,22 +8,9 @@
- <%= govuk_table do |table| %> - - <%= table.with_head do |head| %> - <%= head.with_row do |row| %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.form_name")) %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.organisation")) %> - <% end %> - <% end %> - <%= table.with_body do |body| %> - <% forms.each do |form| %> - <%= body.with_row do |row| %> - <%= row.with_cell(text: govuk_link_to(form[:form_name], live_form_path(form_id: form[:form_id]))) %> - <%= row.with_cell(text: form[:organisation_name]) %> - <% end %> - <% end %> - <% end %> - <% end %> + <%= govuk_table( + head: report_forms_table_head, + rows: report_forms_table_rows(forms), + ) %>
diff --git a/app/views/reports/forms_with_payments.html.erb b/app/views/reports/forms_with_payments.html.erb index 229ea4b2f..9557920e6 100644 --- a/app/views/reports/forms_with_payments.html.erb +++ b/app/views/reports/forms_with_payments.html.erb @@ -8,22 +8,9 @@
- <%= govuk_table do |table| %> - - <%= table.with_head do |head| %> - <%= head.with_row do |row| %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.form_name")) %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.organisation")) %> - <% end %> - <% end %> - <%= table.with_body do |body| %> - <% forms.each do |form| %> - <%= body.with_row do |row| %> - <%= row.with_cell(text: govuk_link_to(form[:form_name], live_form_path(form_id: form[:form_id]))) %> - <%= row.with_cell(text: form[:organisation_name]) %> - <% end %> - <% end %> - <% end %> - <% end %> + <%= govuk_table( + head: report_forms_table_head, + rows: report_forms_table_rows(forms), + ) %>
diff --git a/app/views/reports/forms_with_routes.html.erb b/app/views/reports/forms_with_routes.html.erb index 35fd9dfa0..bb6d7c106 100644 --- a/app/views/reports/forms_with_routes.html.erb +++ b/app/views/reports/forms_with_routes.html.erb @@ -8,24 +8,9 @@
- <%= govuk_table do |table| %> - - <%= table.with_head do |head| %> - <%= head.with_row do |row| %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.form_name")) %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.organisation")) %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.number_of_routes")) %> - <% end %> - <% end %> - <%= table.with_body do |body| %> - <% forms.each do |form| %> - <%= body.with_row do |row| %> - <%= row.with_cell(text: govuk_link_to(form[:form_name], live_form_pages_path(form_id: form[:form_id]))) %> - <%= row.with_cell(text: form[:organisation_name]) %> - <%= row.with_cell(text: form[:number_of_routes]) %> - <% end %> - <% end %> - <% end %> - <% end %> + <%= govuk_table( + head: report_forms_with_routes_table_head, + rows: report_forms_with_routes_table_rows(forms), + ) %>
diff --git a/app/views/reports/questions_with_add_another_answer.html.erb b/app/views/reports/questions_with_add_another_answer.html.erb index 1eb305217..061221139 100644 --- a/app/views/reports/questions_with_add_another_answer.html.erb +++ b/app/views/reports/questions_with_add_another_answer.html.erb @@ -8,24 +8,9 @@
- <%= govuk_table do |table| %> - - <%= table.with_head do |head| %> - <%= head.with_row do |row| %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.form_name")) %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.organisation")) %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.question_text")) %> - <% end %> - <% end %> - <%= table.with_body do |body| %> - <% questions.each do |question| %> - <%= body.with_row do |row| %> - <%= row.with_cell(text: govuk_link_to(question[:form_name], live_form_pages_path(form_id: question[:form_id]))) %> - <%= row.with_cell(text: question[:organisation_name]) %> - <%= row.with_cell(text: question[:question_text]) %> - <% end %> - <% end %> - <% end %> - <% end %> + <%= govuk_table( + head: report_questions_table_head, + rows: report_questions_table_rows(questions), + ) %>
diff --git a/app/views/reports/questions_with_answer_type.html.erb b/app/views/reports/questions_with_answer_type.html.erb index 35e38ab02..caf3f82ce 100644 --- a/app/views/reports/questions_with_answer_type.html.erb +++ b/app/views/reports/questions_with_answer_type.html.erb @@ -8,24 +8,9 @@
- <%= govuk_table do |table| %> - - <%= table.with_head do |head| %> - <%= head.with_row do |row| %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.form_name")) %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.organisation")) %> - <%= row.with_cell(text: t("reports.form_or_questions_list_table.headings.question_text")) %> - <% end %> - <% end %> - <%= table.with_body do |body| %> - <% questions.each do |question| %> - <%= body.with_row do |row| %> - <%= row.with_cell(text: govuk_link_to(question[:form_name], live_form_pages_path(form_id: question[:form_id]))) %> - <%= row.with_cell(text: question[:organisation_name]) %> - <%= row.with_cell(text: question[:question_text]) %> - <% end %> - <% end %> - <% end %> - <% end %> + <%= govuk_table( + head: report_questions_table_head, + rows: report_questions_table_rows(questions), + ) %>
diff --git a/spec/helpers/report_helpers_spec.rb b/spec/helpers/report_helpers_spec.rb new file mode 100644 index 000000000..158ccdff6 --- /dev/null +++ b/spec/helpers/report_helpers_spec.rb @@ -0,0 +1,159 @@ +require "rails_helper" + +RSpec.describe ReportHelper, type: :helper do + describe "#report_forms_table_head" do + it "returns the column headings for a table of forms" do + expect(helper.report_forms_table_head).to eq [ + "Form name", + "Organisation", + ] + end + end + + describe "#report_forms_table_rows" do + let(:forms) do + [ + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } } }, + { "form_id" => 4, "content" => { "name" => "Skip route form" }, "group" => { "organisation" => { "name" => "Department for Testing" } } }, + ] + end + + it "returns an array of arrays of strings" do + expect(helper.report_forms_table_rows(forms)) + .to be_an(Array) + .and(all(be_an(Array))) + .and(all(all(be_a(String)))) + end + + it "returns a row for each form" do + expect(helper.report_forms_table_rows(forms).length) + .to eq forms.length + end + + it "has a column in each row for each column heading" do + expect(helper.report_forms_table_rows(forms).map(&:length)) + .to all eq helper.report_forms_table_head.length + end + + it "formats a link for each form for the first column of each row" do + expect(helper.report_forms_table_rows(forms).map(&:first)).to eq [ + "All question types form", + "Branch route form", + "Skip route form", + ] + end + + it "includes the organisation name for each form for the second column of each row" do + expect(helper.report_forms_table_rows(forms).map(&:second)).to eq [ + "Government Digital Service", + "Ministry of Tests", + "Department for Testing", + ] + end + end + + describe "#report_forms_with_routes_table_head" do + it "returns the column headings for a table of forms and details of their routes" do + expect(helper.report_forms_with_routes_table_head).to eq [ + "Form name", + "Organisation", + "Number of routes", + ] + end + end + + describe "#report_forms_with_routes_table_rows" do + let(:forms) do + [ + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } }, "metadata" => { "number_of_routes" => 2 } }, + { "form_id" => 4, "content" => { "name" => "Skip route form" }, "group" => { "organisation" => { "name" => "Department for Testing" } }, "metadata" => { "number_of_routes" => 1 } }, + ] + end + + it "returns an array of arrays of strings" do + expect(helper.report_forms_with_routes_table_rows(forms)) + .to be_an(Array) + .and(all(be_an(Array))) + .and(all(all(be_a(String)))) + end + + it "returns a row for each form" do + expect(helper.report_forms_with_routes_table_rows(forms).length) + .to eq forms.length + end + + it "has a column in each row for each column heading" do + expect(helper.report_forms_with_routes_table_rows(forms).map(&:length)) + .to all eq helper.report_forms_with_routes_table_head.length + end + + it "formats a link for each form for the first column of each row" do + expect(helper.report_forms_with_routes_table_rows(forms).map(&:first)).to eq [ + "Branch route form", + "Skip route form", + ] + end + + it "includes the organisation name for each form for the second column of each row" do + expect(helper.report_forms_with_routes_table_rows(forms).map(&:second)).to eq [ + "Ministry of Tests", + "Department for Testing", + ] + end + + it "includes the number of routes in the form" do + expect(helper.report_forms_with_routes_table_rows(forms).map(&:third)).to eq %w[ + 2 + 1 + ] + end + end + + describe "#report_questions_table_head" do + it "returns the column headings for a table of questions" do + expect(helper.report_questions_table_head).to eq [ + "Form name", + "Organisation", + "Question text", + ] + end + end + + describe "#report_questions_table_rows" do + let(:questions) do + [ + { "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, + { "data" => { "question_text" => "What’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } } } }, + ] + end + + it "returns an array of arrays of strings" do + expect(helper.report_questions_table_rows(questions)) + .to be_an(Array) + .and(all(be_an(Array))) + .and(all(all(be_a(String)))) + end + + it "formats a link for each form for the first column of each row" do + expect(helper.report_questions_table_rows(questions).map(&:first)).to eq [ + "All question types form", + "Branch route form", + ] + end + + it "includes the organisation name for each form for the second column of each row" do + expect(helper.report_questions_table_rows(questions).map(&:second)).to eq [ + "Government Digital Service", + "Ministry of Tests", + ] + end + + it "includes the question text for each question for the third column of each row" do + expect(helper.report_questions_table_rows(questions).map(&:third)).to eq [ + "Email address", + "What’s your email address?", + ] + end + end +end From 6c7e0d3b77d13bff5b9f4451868834827fa55e1b Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Mon, 12 May 2025 11:25:56 +0300 Subject: [PATCH 03/15] Change FeatureReportService to return complete records --- app/helpers/report_helper.rb | 10 +- .../reports/feature_report_service.rb | 38 +-- .../reports/feature_report_service_spec.rb | 271 ++++++++++++++---- ...th_csv_submission_enabled.html.erb_spec.rb | 12 +- .../forms_with_payments.html.erb_spec.rb | 12 +- .../forms_with_routes.html.erb_spec.rb | 14 +- ...s_with_add_another_answer.html.erb_spec.rb | 14 +- ...uestions_with_answer_type.html.erb_spec.rb | 14 +- 8 files changed, 253 insertions(+), 132 deletions(-) diff --git a/app/helpers/report_helper.rb b/app/helpers/report_helper.rb index 30d009407..b2b618bed 100644 --- a/app/helpers/report_helper.rb +++ b/app/helpers/report_helper.rb @@ -36,22 +36,22 @@ def report_questions_table_rows(questions) def report_forms_table_row(form) [ - govuk_link_to(form[:form_name], live_form_pages_path(form_id: form[:form_id])), - form[:organisation_name], + govuk_link_to(form["content"]["name"], live_form_pages_path(form_id: form["form_id"])), + form["group"]["organisation"]["name"], ] end def report_forms_with_routes_table_row(form) [ *report_forms_table_row(form), - form[:number_of_routes].to_s, + form["metadata"]["number_of_routes"].to_s, ] end def report_questions_table_row(question) [ - *report_forms_table_row(question), - question[:question_text], + *report_forms_table_row(question["form"]), + question["data"]["question_text"], ] end end diff --git a/app/services/reports/feature_report_service.rb b/app/services/reports/feature_report_service.rb index c1479f21d..2710bb640 100644 --- a/app/services/reports/feature_report_service.rb +++ b/app/services/reports/feature_report_service.rb @@ -36,15 +36,17 @@ def report def questions_with_answer_type(answer_type) Reports::FormDocumentsService.live_form_documents.flat_map do |form| - form["content"]["steps"].select { |step| step["data"]["answer_type"] == answer_type } - .map { |step| questions_details(form, step) } + form["content"]["steps"] + .select { |step| step["data"]["answer_type"] == answer_type } + .map { |step| questions_details(form, step) } end end def live_questions_with_add_another_answer Reports::FormDocumentsService.live_form_documents.flat_map do |form| - form["content"]["steps"].select { |step| step["data"]["is_repeatable"] } - .map { |step| questions_details(form, step) } + form["content"]["steps"] + .select { |step| step["data"]["is_repeatable"] } + .map { |step| questions_details(form, step) } end end @@ -69,28 +71,26 @@ def live_forms_with_csv_submission_enabled private def questions_details(form, step) - form_id = form["form_id"] - { - form_name: form["content"]["name"], - form_id: form_id, - organisation_name: organisation_name(form_id), - question_text: step["data"]["question_text"], - } + form = form_details(form) + step.dup.merge("form" => form) end def form_with_routes_details(form) - form_details = form_details(form) - form_details[:number_of_routes] = form["content"]["steps"].count { |step| step["routing_conditions"].present? } - form_details + form = form_details(form) + form["metadata"] = { + "number_of_routes" => form["content"]["steps"].count { |step| step["routing_conditions"].present? }, + } + form end def form_details(form) - form_id = form["form_id"] - { - form_name: form["content"]["name"], - form_id: form_id, - organisation_name: organisation_name(form_id), + form = form.dup + form["group"] = { + "organisation" => { + "name" => organisation_name(form["form_id"]), + }, } + form end def organisation_name(form_id) diff --git a/spec/services/reports/feature_report_service_spec.rb b/spec/services/reports/feature_report_service_spec.rb index 45e0ee696..958354504 100644 --- a/spec/services/reports/feature_report_service_spec.rb +++ b/spec/services/reports/feature_report_service_spec.rb @@ -49,64 +49,205 @@ end describe "#questions_with_answer_type" do - it "returns questions with the given answer type" do + it "returns details needed to render report" do questions = described_class.questions_with_answer_type("email") + expect(questions).to match [ + a_hash_including( + "form" => a_hash_including( + "form_id" => 1, + "content" => a_hash_including( + "name" => "All question types form", + ), + ), + "data" => a_hash_including( + "question_text" => "Email address", + ), + ), + a_hash_including( + "form" => a_hash_including( + "form_id" => 3, + "content" => a_hash_including( + "name" => "Branch route form", + ), + ), + "data" => a_hash_including( + "question_text" => "What’s your email address?", + ), + ), + ] + end + + it "returns questions with the given answer type" do + questions = described_class.questions_with_answer_type("name") expect(questions.length).to eq 2 - expect(questions).to include( - { - form_name: "All question types form", - form_id: 1, - organisation_name: group.organisation.name, - question_text: "Email address", - }, + expect(questions).to all match( + a_hash_including( + "data" => a_hash_including( + "answer_type" => "name", + ), + ), + ) + end + + it "includes a reference to the form document" do + questions = described_class.questions_with_answer_type("text") + expect(questions).to all include( + "form" => a_hash_including( + "form_id", + "content" => a_hash_including( + "name", + ), + ), ) - expect(questions).to include({ - form_name: "Branch route form", - form_id: 3, - organisation_name: group.organisation.name, - question_text: "What’s your email address?", - }) end end describe "#live_questions_with_add_another_answer" do + it "returns details needed to render report" do + questions = described_class.live_questions_with_add_another_answer + expect(questions).to match [ + a_hash_including( + "form" => a_hash_including( + "form_id" => 1, + "content" => a_hash_including( + "name" => "All question types form", + ), + "group" => a_hash_including( + "organisation" => a_hash_including( + "name" => group.organisation.name, + ), + ), + ), + "data" => a_hash_including( + "question_text" => "Single line of text", + ), + ), + a_hash_including( + "form" => a_hash_including( + "form_id" => 1, + "content" => a_hash_including( + "name" => "All question types form", + ), + "group" => a_hash_including( + "organisation" => a_hash_including( + "name" => group.organisation.name, + ), + ), + ), + "data" => a_hash_including( + "question_text" => "Number", + ), + ), + ] + end + it "returns questions with add another answer" do questions = described_class.live_questions_with_add_another_answer - expect(questions.length).to eq 2 - expect(questions).to include( - { - form_name: "All question types form", - form_id: 1, - organisation_name: group.organisation.name, - question_text: "Single line of text", - }, + expect(questions).to all match( + a_hash_including( + "data" => a_hash_including( + "is_repeatable" => true, + ), + ), + ) + end + + it "includes a reference to the form document" do + questions = described_class.questions_with_answer_type("text") + expect(questions).to all include( + "form" => a_hash_including( + "form_id", + "content" => a_hash_including( + "name", + ), + ), + ) + end + + it "includes a reference to the organisation record" do + questions = described_class.questions_with_answer_type("text") + expect(questions).to all include( + "form" => a_hash_including( + "group" => a_hash_including( + "organisation" => a_hash_including( + "name", + ), + ), + ), ) - expect(questions).to include({ - form_name: "All question types form", - form_id: 1, - organisation_name: group.organisation.name, - question_text: "Number", - }) end end describe "#live_forms_with_routes" do + it "returns details needed to render report" do + forms = described_class.live_forms_with_routes + expect(forms).to match [ + a_hash_including( + "form_id" => 3, + "content" => a_hash_including( + "name" => "Branch route form", + ), + "group" => a_hash_including( + "organisation" => a_hash_including( + "name" => group.organisation.name, + ), + ), + "metadata" => { + "number_of_routes" => 2, + }, + ), + a_hash_including( + "form_id" => 4, + "content" => a_hash_including( + "name" => "Skip route form", + ), + "group" => a_hash_including( + "organisation" => a_hash_including( + "name" => group.organisation.name, + ), + ), + "metadata" => { + "number_of_routes" => 1, + }, + ), + ] + end + it "returns forms with routes" do forms = described_class.live_forms_with_routes - expect(forms.length).to eq 2 - expect(forms).to include( - { - form_name: "Branch route form", - form_id: 3, - organisation_name: group.organisation.name, - number_of_routes: 2, - }, - { - form_name: "Skip route form", - form_id: 4, - organisation_name: group.organisation.name, - number_of_routes: 1, - }, + expect(forms).to match [ + a_hash_including( + "form_id" => 3, + "content" => a_hash_including( + "name" => "Branch route form", + ), + ), + a_hash_including( + "form_id" => 4, + "content" => a_hash_including( + "name" => "Skip route form", + ), + ), + ] + end + + it "includes counts of routes" do + forms = described_class.live_forms_with_routes + expect(forms).to all include( + "metadata" => a_hash_including( + "number_of_routes" => an_instance_of(Integer), + ), + ) + end + + it "includes a reference to the organisation record" do + forms = described_class.live_forms_with_routes + expect(forms).to all include( + "group" => a_hash_including( + "organisation" => a_hash_including( + "name", + ), + ), ) end end @@ -114,11 +255,24 @@ describe "#live_forms_with_payments" do it "returns live forms with payments" do forms = described_class.live_forms_with_payments - expect(forms.length).to eq 1 - expect(forms).to include( - form_name: "All question types form", - form_id: 1, - organisation_name: group.organisation.name, + expect(forms).to match [ + a_hash_including( + "form_id" => 1, + "content" => a_hash_including( + "name" => "All question types form", + ), + ), + ] + end + + it "includes a reference to the organisation record" do + forms = described_class.live_forms_with_routes + expect(forms).to all include( + "group" => a_hash_including( + "organisation" => a_hash_including( + "name", + ), + ), ) end end @@ -126,11 +280,24 @@ describe "#live_forms_with_csv_submission_enabled" do it "returns live forms with csv enabled" do forms = described_class.live_forms_with_csv_submission_enabled - expect(forms.length).to eq 1 - expect(forms).to include( - form_name: "All question types form", - form_id: 1, - organisation_name: group.organisation.name, + expect(forms).to match [ + a_hash_including( + "form_id" => 1, + "content" => a_hash_including( + "name" => "All question types form", + ), + ), + ] + end + + it "includes a reference to the organisation record" do + forms = described_class.live_forms_with_routes + expect(forms).to all include( + "group" => a_hash_including( + "organisation" => a_hash_including( + "name", + ), + ), ) end end diff --git a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb index 6c9a2e944..2777ad81e 100644 --- a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb +++ b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb @@ -3,16 +3,8 @@ describe "reports/forms_with_csv_submission_enabled" do let(:forms) do [ - { - form_name: "All question types form", - form_id: 1, - organisation_name: "Government Digital Service", - }, - { - form_name: "Branch route form", - form_id: 3, - organisation_name: "Government Digital Service", - }, + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, ] end diff --git a/spec/views/reports/forms_with_payments.html.erb_spec.rb b/spec/views/reports/forms_with_payments.html.erb_spec.rb index bdf4bb3bc..791382b18 100644 --- a/spec/views/reports/forms_with_payments.html.erb_spec.rb +++ b/spec/views/reports/forms_with_payments.html.erb_spec.rb @@ -3,16 +3,8 @@ describe "reports/forms_with_payments" do let(:forms) do [ - { - form_name: "All question types form", - form_id: 1, - organisation_name: "Government Digital Service", - }, - { - form_name: "Branch route form", - form_id: 3, - organisation_name: "Government Digital Service", - }, + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, ] end diff --git a/spec/views/reports/forms_with_routes.html.erb_spec.rb b/spec/views/reports/forms_with_routes.html.erb_spec.rb index 1047eb459..55f0ff121 100644 --- a/spec/views/reports/forms_with_routes.html.erb_spec.rb +++ b/spec/views/reports/forms_with_routes.html.erb_spec.rb @@ -3,18 +3,8 @@ describe "reports/forms_with_routes" do let(:forms) do [ - { - form_name: "All question types form", - form_id: 1, - organisation_name: "Government Digital Service", - number_of_routes: 1, - }, - { - form_name: "Branch route form", - form_id: 3, - organisation_name: "Government Digital Service", - number_of_routes: 2, - }, + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } }, "metadata" => { "number_of_routes" => 1 } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } }, "metadata" => { "number_of_routes" => 2 } }, ] end diff --git a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb index 726835f57..2590aa550 100644 --- a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb +++ b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb @@ -3,18 +3,8 @@ describe "reports/questions_with_add_another_answer" do let(:questions) do [ - { - form_name: "All question types form", - form_id: 1, - organisation_name: "Government Digital Service", - question_text: "Email address", - }, - { - form_name: "Branch route form", - form_id: 3, - organisation_name: "Government Digital Service", - question_text: "What’s your email address?", - }, + { "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, + { "data" => { "question_text" => "What’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, ] end diff --git a/spec/views/reports/questions_with_answer_type.html.erb_spec.rb b/spec/views/reports/questions_with_answer_type.html.erb_spec.rb index 7b9c607c9..1c72a4add 100644 --- a/spec/views/reports/questions_with_answer_type.html.erb_spec.rb +++ b/spec/views/reports/questions_with_answer_type.html.erb_spec.rb @@ -3,18 +3,8 @@ describe "reports/questions_with_answer_type" do let(:questions) do [ - { - form_name: "All question types form", - form_id: 1, - organisation_name: "Government Digital Service", - question_text: "Email address", - }, - { - form_name: "Branch route form", - form_id: 3, - organisation_name: "Government Digital Service", - question_text: "What’s your email address?", - }, + { "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, + { "data" => { "question_text" => "What’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, ] end From 7b85a0ea3841daa2e37c61360d3f36dbf15a75e9 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 13 May 2025 16:34:48 +0300 Subject: [PATCH 04/15] Change FeatureReportService keys in report hash Remove references to `live` from the report key names, so the code is more generic and reusable. --- .../reports/feature_report_service.rb | 32 ++++++------- app/views/reports/features.html.erb | 16 +++---- .../reports/feature_report_service_spec.rb | 14 +++--- spec/views/reports/features.html.erb_spec.rb | 46 +++++++++---------- 4 files changed, 54 insertions(+), 54 deletions(-) diff --git a/app/services/reports/feature_report_service.rb b/app/services/reports/feature_report_service.rb index 2710bb640..af5f072f6 100644 --- a/app/services/reports/feature_report_service.rb +++ b/app/services/reports/feature_report_service.rb @@ -2,32 +2,32 @@ class Reports::FeatureReportService class << self def report report = { - total_live_forms: 0, - live_forms_with_payment: 0, - live_forms_with_routing: 0, - live_forms_with_add_another_answer: 0, - live_forms_with_csv_submission_enabled: 0, - live_forms_with_answer_type: HashWithIndifferentAccess.new, - live_steps_with_answer_type: HashWithIndifferentAccess.new, + total_forms: 0, + forms_with_payment: 0, + forms_with_routing: 0, + forms_with_add_another_answer: 0, + forms_with_csv_submission_enabled: 0, + forms_with_answer_type: HashWithIndifferentAccess.new, + steps_with_answer_type: HashWithIndifferentAccess.new, } Reports::FormDocumentsService.live_form_documents.each do |form| - report[:total_live_forms] += 1 - report[:live_forms_with_payment] += 1 if Reports::FormDocumentsService.has_payments?(form) - report[:live_forms_with_routing] += 1 if Reports::FormDocumentsService.has_routes?(form) - report[:live_forms_with_add_another_answer] += 1 if form["content"]["steps"].any? { |step| step["data"]["is_repeatable"] } - report[:live_forms_with_csv_submission_enabled] += 1 if Reports::FormDocumentsService.has_csv_submission_enabled?(form) + report[:total_forms] += 1 + report[:forms_with_payment] += 1 if Reports::FormDocumentsService.has_payments?(form) + report[:forms_with_routing] += 1 if Reports::FormDocumentsService.has_routes?(form) + report[:forms_with_add_another_answer] += 1 if form["content"]["steps"].any? { |step| step["data"]["is_repeatable"] } + report[:forms_with_csv_submission_enabled] += 1 if Reports::FormDocumentsService.has_csv_submission_enabled?(form) answer_types_in_form = form["content"]["steps"].map { |step| step["data"]["answer_type"] } answer_types_in_form.uniq.each do |answer_type| - report[:live_forms_with_answer_type][answer_type] ||= 0 - report[:live_forms_with_answer_type][answer_type] += 1 + report[:forms_with_answer_type][answer_type] ||= 0 + report[:forms_with_answer_type][answer_type] += 1 end answer_types_in_form.each do |answer_type| - report[:live_steps_with_answer_type][answer_type] ||= 0 - report[:live_steps_with_answer_type][answer_type] += 1 + report[:steps_with_answer_type][answer_type] ||= 0 + report[:steps_with_answer_type][answer_type] += 1 end end diff --git a/app/views/reports/features.html.erb b/app/views/reports/features.html.erb index a2ee9f902..725646a7c 100644 --- a/app/views/reports/features.html.erb +++ b/app/views/reports/features.html.erb @@ -8,23 +8,23 @@ <%= govuk_summary_list do |summary_list| %> <%= summary_list.with_row do |row| %> <%= row.with_key(text: t(".features.total_live_forms")) %> - <%= row.with_value(text: data[:total_live_forms]) %> + <%= row.with_value(text: data[:total_forms]) %> <% end %> <%= summary_list.with_row do |row| %> <%= row.with_key(text: t(".features.live_forms_with_routes")) %> - <%= row.with_value(text: govuk_link_to(data[:live_forms_with_routing], report_forms_with_routes_path, no_visited_state: true)) %> + <%= row.with_value(text: govuk_link_to(data[:forms_with_routing], report_forms_with_routes_path, no_visited_state: true)) %> <% end %> <%= summary_list.with_row do |row| %> <%= row.with_key(text: t(".features.live_forms_with_payments")) %> - <%= row.with_value(text: govuk_link_to(data[:live_forms_with_payment], report_forms_with_payments_path, no_visited_state: true)) %> + <%= row.with_value(text: govuk_link_to(data[:forms_with_payment], report_forms_with_payments_path, no_visited_state: true)) %> <% end %> <%= summary_list.with_row do |row| %> <%= row.with_key(text: t(".features.live_forms_with_add_another_answer")) %> - <%= row.with_value(text: govuk_link_to(data[:live_forms_with_add_another_answer], report_questions_with_add_another_answer_path, no_visited_state: true)) %> + <%= row.with_value(text: govuk_link_to(data[:forms_with_add_another_answer], report_questions_with_add_another_answer_path, no_visited_state: true)) %> <% end %> <%= summary_list.with_row do |row| %> <%= row.with_key(text: t(".features.live_forms_with_csv_submission_enabled")) %> - <%= row.with_value(text: govuk_link_to(data[:live_forms_with_csv_submission_enabled], report_forms_with_csv_submission_enabled_path, no_visited_state: true)) %> + <%= row.with_value(text: govuk_link_to(data[:forms_with_csv_submission_enabled], report_forms_with_csv_submission_enabled_path, no_visited_state: true)) %> <% end %> <% end %> @@ -46,11 +46,11 @@ <%= body.with_row do |row| %> <%= row.with_cell(header: true, text: t("helpers.label.page.answer_type_options.names.#{answer_type}")) %> <% if answer_type_links[answer_type].present? %> - <%= row.with_cell(text: govuk_link_to(data[:live_forms_with_answer_type][answer_type] || 0, answer_type_links[answer_type], no_visited_state: true), numeric: true, html_attributes: { data: { "live-forms-with-answer-type-#{answer_type.to_s.dasherize}": true } }) %> + <%= row.with_cell(text: govuk_link_to(data[:forms_with_answer_type][answer_type] || 0, answer_type_links[answer_type], no_visited_state: true), numeric: true, html_attributes: { data: { "live-forms-with-answer-type-#{answer_type.to_s.dasherize}": true } }) %> <% else %> - <%= row.with_cell(text: data[:live_forms_with_answer_type][answer_type] || 0, numeric: true, html_attributes: { data: { "live-forms-with-answer-type-#{answer_type.to_s.dasherize}": true } }) %> + <%= row.with_cell(text: data[:forms_with_answer_type][answer_type] || 0, numeric: true, html_attributes: { data: { "live-forms-with-answer-type-#{answer_type.to_s.dasherize}": true } }) %> <% end %> - <%= row.with_cell(text: govuk_link_to(data[:live_steps_with_answer_type][answer_type] || 0, report_questions_with_answer_type_path(answer_type:), no_visited_state: true), numeric: true, html_attributes: { data: { "live-pages-with-answer-type-#{answer_type.to_s.dasherize}": true } }) %> + <%= row.with_cell(text: govuk_link_to(data[:steps_with_answer_type][answer_type] || 0, report_questions_with_answer_type_path(answer_type:), no_visited_state: true), numeric: true, html_attributes: { data: { "live-pages-with-answer-type-#{answer_type.to_s.dasherize}": true } }) %> <% end %> <% end %> <% end %> diff --git a/spec/services/reports/feature_report_service_spec.rb b/spec/services/reports/feature_report_service_spec.rb index 958354504..363fa292b 100644 --- a/spec/services/reports/feature_report_service_spec.rb +++ b/spec/services/reports/feature_report_service_spec.rb @@ -17,12 +17,12 @@ it "returns the feature report" do report = described_class.report expect(report).to eq({ - total_live_forms: 4, - live_forms_with_payment: 1, - live_forms_with_routing: 2, - live_forms_with_add_another_answer: 1, - live_forms_with_csv_submission_enabled: 1, - live_forms_with_answer_type: { + total_forms: 4, + forms_with_payment: 1, + forms_with_routing: 2, + forms_with_add_another_answer: 1, + forms_with_csv_submission_enabled: 1, + forms_with_answer_type: { "address" => 1, "date" => 1, "email" => 2, @@ -33,7 +33,7 @@ "selection" => 3, "text" => 3, }, - live_steps_with_answer_type: { + steps_with_answer_type: { "address" => 1, "date" => 1, "email" => 2, diff --git a/spec/views/reports/features.html.erb_spec.rb b/spec/views/reports/features.html.erb_spec.rb index 7942960d0..e1976b111 100644 --- a/spec/views/reports/features.html.erb_spec.rb +++ b/spec/views/reports/features.html.erb_spec.rb @@ -3,8 +3,8 @@ describe "reports/features.html.erb" do let(:report) do { - total_live_forms: 3, - live_forms_with_answer_type: { + total_forms: 3, + forms_with_answer_type: { address: 1, date: 1, email: 1, @@ -16,7 +16,7 @@ selection: 3, text: 3, }.with_indifferent_access, - live_steps_with_answer_type: { + steps_with_answer_type: { address: 1, date: 1, email: 1, @@ -28,10 +28,10 @@ selection: 4, text: 5, }.with_indifferent_access, - live_forms_with_payment: 1, - live_forms_with_routing: 2, - live_forms_with_add_another_answer: 3, - live_forms_with_csv_submission_enabled: 2, + forms_with_payment: 1, + forms_with_routing: 2, + forms_with_add_another_answer: 3, + forms_with_csv_submission_enabled: 2, } end @@ -54,7 +54,7 @@ end it "includes the number of total live forms" do - expect(rendered).to have_css(".govuk-summary-list__row", text: "Total live forms#{report[:total_live_forms]}") + expect(rendered).to have_css(".govuk-summary-list__row", text: "Total live forms#{report[:total_forms]}") end Page::ANSWER_TYPES.map(&:to_sym).each do |answer_type| @@ -63,49 +63,49 @@ end it "includes the number of live forms with #{answer_type}" do - expect(rendered).to have_css("[data-live-forms-with-answer-type-#{answer_type.to_s.dasherize}]", text: report[:live_forms_with_answer_type][answer_type].to_s) + expect(rendered).to have_css("[data-live-forms-with-answer-type-#{answer_type.to_s.dasherize}]", text: report[:forms_with_answer_type][answer_type].to_s) end it "includes the number of live pages with #{answer_type}" do - expect(rendered).to have_css("[data-live-pages-with-answer-type-#{answer_type.to_s.dasherize}]", text: report[:live_steps_with_answer_type][answer_type].to_s) + expect(rendered).to have_css("[data-live-pages-with-answer-type-#{answer_type.to_s.dasherize}]", text: report[:steps_with_answer_type][answer_type].to_s) end end context "when an answer type is missing from the data" do let(:report) do { - total_live_forms: 3, - live_forms_with_answer_type: { address: 1 }, - live_steps_with_answer_type: { address: 1 }, - live_forms_with_payment: 1, - live_forms_with_routing: 2, - live_forms_with_add_another_answer: 3, - live_forms_with_csv_submission_enabled: 2, + total_forms: 3, + forms_with_answer_type: { address: 1 }, + steps_with_answer_type: { address: 1 }, + forms_with_payment: 1, + forms_with_routing: 2, + forms_with_add_another_answer: 3, + forms_with_csv_submission_enabled: 2, } end - it "displays 0 for live_forms_with_answer_type" do + it "displays 0 for forms_with_answer_type" do expect(rendered).to have_css("[data-live-forms-with-answer-type-number]", text: "0") end - it "displays 0 for live_steps_with_answer_type" do + it "displays 0 for steps_with_answer_type" do expect(rendered).to have_css("[data-live-pages-with-answer-type-number]", text: "0") end end it "includes the number of live forms with routes" do - expect(rendered).to have_css(".govuk-summary-list__row", text: "Live forms with routes#{report[:live_forms_with_routing]}") + expect(rendered).to have_css(".govuk-summary-list__row", text: "Live forms with routes#{report[:forms_with_routing]}") end it "includes the number of live forms with payments" do - expect(rendered).to have_css(".govuk-summary-list__row", text: "Live forms with payments#{report[:live_forms_with_payment]}") + expect(rendered).to have_css(".govuk-summary-list__row", text: "Live forms with payments#{report[:forms_with_payment]}") end it "includes the number of live forms with add another answer" do - expect(rendered).to have_css(".govuk-summary-list__row", text: "Live forms with add another answer#{report[:live_forms_with_add_another_answer]}") + expect(rendered).to have_css(".govuk-summary-list__row", text: "Live forms with add another answer#{report[:forms_with_add_another_answer]}") end it "includes the number of live forms with CSV submission enabled" do - expect(rendered).to have_css(".govuk-summary-list__row", text: "Live forms with CSV submission enabled#{report[:live_forms_with_csv_submission_enabled]}") + expect(rendered).to have_css(".govuk-summary-list__row", text: "Live forms with CSV submission enabled#{report[:forms_with_csv_submission_enabled]}") end end From c3ea1eedba9764be836ad16b559e122fb3b0a263 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 14 May 2025 10:50:38 +0300 Subject: [PATCH 05/15] Change FeatureReportService to receive form_documents Rather than call FormDocumentsService inside FeatureReportService and couple those two classes together, change FeatureReportService to expect to be passed a list of form_documents. This also allows in future for the source of form documents (for instance, from live to draft) to be changed without needing to change FeatureReportService. --- app/controllers/reports_controller.rb | 18 +- .../reports/feature_report_service.rb | 166 +++++++++--------- .../reports/feature_report_service_spec.rb | 44 +++-- 3 files changed, 118 insertions(+), 110 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 7da1a7724..bba7db0c2 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -5,38 +5,44 @@ class ReportsController < ApplicationController def index; end def features - data = Reports::FeatureReportService.report + forms = Reports::FormDocumentsService.live_form_documents + data = Reports::FeatureReportService.new(forms).report render template: "reports/features", locals: { data: } end def questions_with_answer_type answer_type = params.require(:answer_type) - questions = Reports::FeatureReportService.questions_with_answer_type(answer_type) + forms = Reports::FormDocumentsService.live_form_documents + questions = Reports::FeatureReportService.new(forms).questions_with_answer_type(answer_type) render template: "reports/questions_with_answer_type", locals: { answer_type:, questions: } end def questions_with_add_another_answer - questions = Reports::FeatureReportService.live_questions_with_add_another_answer + forms = Reports::FormDocumentsService.live_form_documents + questions = Reports::FeatureReportService.new(forms).questions_with_add_another_answer render template: "reports/questions_with_add_another_answer", locals: { questions: } end def forms_with_routes - forms = Reports::FeatureReportService.live_forms_with_routes + forms = Reports::FormDocumentsService.live_form_documents + forms = Reports::FeatureReportService.new(forms).forms_with_routes render template: "reports/forms_with_routes", locals: { forms: forms } end def forms_with_payments - forms = Reports::FeatureReportService.live_forms_with_payments + forms = Reports::FormDocumentsService.live_form_documents + forms = Reports::FeatureReportService.new(forms).forms_with_payments render template: "reports/forms_with_payments", locals: { forms: forms } end def forms_with_csv_submission_enabled - forms = Reports::FeatureReportService.live_forms_with_csv_submission_enabled + forms = Reports::FormDocumentsService.live_form_documents + forms = Reports::FeatureReportService.new(forms).forms_with_csv_submission_enabled render template: "reports/forms_with_csv_submission_enabled", locals: { forms: forms } end diff --git a/app/services/reports/feature_report_service.rb b/app/services/reports/feature_report_service.rb index af5f072f6..337b4e3d5 100644 --- a/app/services/reports/feature_report_service.rb +++ b/app/services/reports/feature_report_service.rb @@ -1,100 +1,104 @@ class Reports::FeatureReportService - class << self - def report - report = { - total_forms: 0, - forms_with_payment: 0, - forms_with_routing: 0, - forms_with_add_another_answer: 0, - forms_with_csv_submission_enabled: 0, - forms_with_answer_type: HashWithIndifferentAccess.new, - steps_with_answer_type: HashWithIndifferentAccess.new, - } - - Reports::FormDocumentsService.live_form_documents.each do |form| - report[:total_forms] += 1 - report[:forms_with_payment] += 1 if Reports::FormDocumentsService.has_payments?(form) - report[:forms_with_routing] += 1 if Reports::FormDocumentsService.has_routes?(form) - report[:forms_with_add_another_answer] += 1 if form["content"]["steps"].any? { |step| step["data"]["is_repeatable"] } - report[:forms_with_csv_submission_enabled] += 1 if Reports::FormDocumentsService.has_csv_submission_enabled?(form) - - answer_types_in_form = form["content"]["steps"].map { |step| step["data"]["answer_type"] } - - answer_types_in_form.uniq.each do |answer_type| - report[:forms_with_answer_type][answer_type] ||= 0 - report[:forms_with_answer_type][answer_type] += 1 - end - - answer_types_in_form.each do |answer_type| - report[:steps_with_answer_type][answer_type] ||= 0 - report[:steps_with_answer_type][answer_type] += 1 - end - end + attr_reader :form_documents - report - end + def initialize(form_documents) + @form_documents = form_documents + end + + def report + report = { + total_forms: 0, + forms_with_payment: 0, + forms_with_routing: 0, + forms_with_add_another_answer: 0, + forms_with_csv_submission_enabled: 0, + forms_with_answer_type: HashWithIndifferentAccess.new, + steps_with_answer_type: HashWithIndifferentAccess.new, + } + + form_documents.each do |form| + report[:total_forms] += 1 + report[:forms_with_payment] += 1 if Reports::FormDocumentsService.has_payments?(form) + report[:forms_with_routing] += 1 if Reports::FormDocumentsService.has_routes?(form) + report[:forms_with_add_another_answer] += 1 if form["content"]["steps"].any? { |step| step["data"]["is_repeatable"] } + report[:forms_with_csv_submission_enabled] += 1 if Reports::FormDocumentsService.has_csv_submission_enabled?(form) + + answer_types_in_form = form["content"]["steps"].map { |step| step["data"]["answer_type"] } - def questions_with_answer_type(answer_type) - Reports::FormDocumentsService.live_form_documents.flat_map do |form| - form["content"]["steps"] - .select { |step| step["data"]["answer_type"] == answer_type } - .map { |step| questions_details(form, step) } + answer_types_in_form.uniq.each do |answer_type| + report[:forms_with_answer_type][answer_type] ||= 0 + report[:forms_with_answer_type][answer_type] += 1 end - end - def live_questions_with_add_another_answer - Reports::FormDocumentsService.live_form_documents.flat_map do |form| - form["content"]["steps"] - .select { |step| step["data"]["is_repeatable"] } - .map { |step| questions_details(form, step) } + answer_types_in_form.each do |answer_type| + report[:steps_with_answer_type][answer_type] ||= 0 + report[:steps_with_answer_type][answer_type] += 1 end end - def live_forms_with_routes - Reports::FormDocumentsService.live_form_documents - .select { |form| Reports::FormDocumentsService.has_routes?(form) } - .map { |form| form_with_routes_details(form) } - end + report + end - def live_forms_with_payments - Reports::FormDocumentsService.live_form_documents - .select { |form| Reports::FormDocumentsService.has_payments?(form) } - .map { |form| form_details(form) } + def questions_with_answer_type(answer_type) + form_documents.flat_map do |form| + form["content"]["steps"] + .select { |step| step["data"]["answer_type"] == answer_type } + .map { |step| questions_details(form, step) } end + end - def live_forms_with_csv_submission_enabled - Reports::FormDocumentsService.live_form_documents - .select { |form| Reports::FormDocumentsService.has_csv_submission_enabled?(form) } - .map { |form| form_details(form) } + def questions_with_add_another_answer + form_documents.flat_map do |form| + form["content"]["steps"] + .select { |step| step["data"]["is_repeatable"] } + .map { |step| questions_details(form, step) } end + end + + def forms_with_routes + form_documents + .select { |form| Reports::FormDocumentsService.has_routes?(form) } + .map { |form| form_with_routes_details(form) } + end - private + def forms_with_payments + form_documents + .select { |form| Reports::FormDocumentsService.has_payments?(form) } + .map { |form| form_details(form) } + end - def questions_details(form, step) - form = form_details(form) - step.dup.merge("form" => form) - end + def forms_with_csv_submission_enabled + form_documents + .select { |form| Reports::FormDocumentsService.has_csv_submission_enabled?(form) } + .map { |form| form_details(form) } + end - def form_with_routes_details(form) - form = form_details(form) - form["metadata"] = { - "number_of_routes" => form["content"]["steps"].count { |step| step["routing_conditions"].present? }, - } - form - end +private - def form_details(form) - form = form.dup - form["group"] = { - "organisation" => { - "name" => organisation_name(form["form_id"]), - }, - } - form - end + def questions_details(form, step) + form = form_details(form) + step.dup.merge("form" => form) + end - def organisation_name(form_id) - GroupForm.find_by_form_id(form_id)&.group&.organisation&.name - end + def form_with_routes_details(form) + form = form_details(form) + form["metadata"] = { + "number_of_routes" => form["content"]["steps"].count { |step| step["routing_conditions"].present? }, + } + form + end + + def form_details(form) + form = form.dup + form["group"] = { + "organisation" => { + "name" => organisation_name(form["form_id"]), + }, + } + form + end + + def organisation_name(form_id) + GroupForm.find_by_form_id(form_id)&.group&.organisation&.name end end diff --git a/spec/services/reports/feature_report_service_spec.rb b/spec/services/reports/feature_report_service_spec.rb index 363fa292b..eea3f5fe7 100644 --- a/spec/services/reports/feature_report_service_spec.rb +++ b/spec/services/reports/feature_report_service_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe Reports::FeatureReportService do - let(:form_documents_response_json) { JSON.parse(file_fixture("form_documents_response.json").read) } + let(:form_documents) { JSON.parse(file_fixture("form_documents_response.json").read) } let(:group) { create(:group) } before do @@ -9,13 +9,11 @@ GroupForm.create!(form_id: 2, group:) GroupForm.create!(form_id: 3, group:) GroupForm.create!(form_id: 4, group:) - - allow(Reports::FormDocumentsService).to receive(:live_form_documents).and_return(form_documents_response_json) end describe "#report" do it "returns the feature report" do - report = described_class.report + report = described_class.new(form_documents).report expect(report).to eq({ total_forms: 4, forms_with_payment: 1, @@ -50,7 +48,7 @@ describe "#questions_with_answer_type" do it "returns details needed to render report" do - questions = described_class.questions_with_answer_type("email") + questions = described_class.new(form_documents).questions_with_answer_type("email") expect(questions).to match [ a_hash_including( "form" => a_hash_including( @@ -78,7 +76,7 @@ end it "returns questions with the given answer type" do - questions = described_class.questions_with_answer_type("name") + questions = described_class.new(form_documents).questions_with_answer_type("name") expect(questions.length).to eq 2 expect(questions).to all match( a_hash_including( @@ -90,7 +88,7 @@ end it "includes a reference to the form document" do - questions = described_class.questions_with_answer_type("text") + questions = described_class.new(form_documents).questions_with_answer_type("text") expect(questions).to all include( "form" => a_hash_including( "form_id", @@ -102,9 +100,9 @@ end end - describe "#live_questions_with_add_another_answer" do + describe "#questions_with_add_another_answer" do it "returns details needed to render report" do - questions = described_class.live_questions_with_add_another_answer + questions = described_class.new(form_documents).questions_with_add_another_answer expect(questions).to match [ a_hash_including( "form" => a_hash_including( @@ -142,7 +140,7 @@ end it "returns questions with add another answer" do - questions = described_class.live_questions_with_add_another_answer + questions = described_class.new(form_documents).questions_with_add_another_answer expect(questions).to all match( a_hash_including( "data" => a_hash_including( @@ -153,7 +151,7 @@ end it "includes a reference to the form document" do - questions = described_class.questions_with_answer_type("text") + questions = described_class.new(form_documents).questions_with_answer_type("text") expect(questions).to all include( "form" => a_hash_including( "form_id", @@ -165,7 +163,7 @@ end it "includes a reference to the organisation record" do - questions = described_class.questions_with_answer_type("text") + questions = described_class.new(form_documents).questions_with_answer_type("text") expect(questions).to all include( "form" => a_hash_including( "group" => a_hash_including( @@ -178,9 +176,9 @@ end end - describe "#live_forms_with_routes" do + describe "#forms_with_routes" do it "returns details needed to render report" do - forms = described_class.live_forms_with_routes + forms = described_class.new(form_documents).forms_with_routes expect(forms).to match [ a_hash_including( "form_id" => 3, @@ -214,7 +212,7 @@ end it "returns forms with routes" do - forms = described_class.live_forms_with_routes + forms = described_class.new(form_documents).forms_with_routes expect(forms).to match [ a_hash_including( "form_id" => 3, @@ -232,7 +230,7 @@ end it "includes counts of routes" do - forms = described_class.live_forms_with_routes + forms = described_class.new(form_documents).forms_with_routes expect(forms).to all include( "metadata" => a_hash_including( "number_of_routes" => an_instance_of(Integer), @@ -241,7 +239,7 @@ end it "includes a reference to the organisation record" do - forms = described_class.live_forms_with_routes + forms = described_class.new(form_documents).forms_with_routes expect(forms).to all include( "group" => a_hash_including( "organisation" => a_hash_including( @@ -252,9 +250,9 @@ end end - describe "#live_forms_with_payments" do + describe "#forms_with_payments" do it "returns live forms with payments" do - forms = described_class.live_forms_with_payments + forms = described_class.new(form_documents).forms_with_payments expect(forms).to match [ a_hash_including( "form_id" => 1, @@ -266,7 +264,7 @@ end it "includes a reference to the organisation record" do - forms = described_class.live_forms_with_routes + forms = described_class.new(form_documents).forms_with_routes expect(forms).to all include( "group" => a_hash_including( "organisation" => a_hash_including( @@ -277,9 +275,9 @@ end end - describe "#live_forms_with_csv_submission_enabled" do + describe "#forms_with_csv_submission_enabled" do it "returns live forms with csv enabled" do - forms = described_class.live_forms_with_csv_submission_enabled + forms = described_class.new(form_documents).forms_with_csv_submission_enabled expect(forms).to match [ a_hash_including( "form_id" => 1, @@ -291,7 +289,7 @@ end it "includes a reference to the organisation record" do - forms = described_class.live_forms_with_routes + forms = described_class.new(form_documents).forms_with_routes expect(forms).to all include( "group" => a_hash_including( "organisation" => a_hash_including( From 5ca42cc9c5dcfe745434a3a82c58afdded21d6d2 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Thu, 15 May 2025 10:19:19 +0300 Subject: [PATCH 06/15] Add ReportHelper#report_table helper --- app/helpers/report_helper.rb | 35 ++++++ ...forms_with_csv_submission_enabled.html.erb | 5 +- .../reports/forms_with_payments.html.erb | 5 +- app/views/reports/forms_with_routes.html.erb | 5 +- ...questions_with_add_another_answer.html.erb | 5 +- spec/helpers/report_helpers_spec.rb | 115 ++++++++++++++---- ...s_with_add_another_answer.html.erb_spec.rb | 4 +- 7 files changed, 135 insertions(+), 39 deletions(-) diff --git a/app/helpers/report_helper.rb b/app/helpers/report_helper.rb index b2b618bed..b962e629a 100644 --- a/app/helpers/report_helper.rb +++ b/app/helpers/report_helper.rb @@ -1,4 +1,39 @@ module ReportHelper + def report_table(records) + type = records.first.fetch("type", "form") + + if type == "form" + with_routes = records.first.dig("metadata", "number_of_routes").present? + + with_routes ? report_forms_with_routes_table(records) : report_forms_table(records) + elsif type == "question_page" + report_questions_table(records) + else + raise "type of records '#{type}' is not one of 'forms', 'question_page'" + end + end + + def report_forms_table(forms) + { + head: report_forms_table_head, + rows: report_forms_table_rows(forms), + } + end + + def report_forms_with_routes_table(forms) + { + head: report_forms_with_routes_table_head, + rows: report_forms_with_routes_table_rows(forms), + } + end + + def report_questions_table(questions) + { + head: report_questions_table_head, + rows: report_questions_table_rows(questions), + } + end + def report_forms_table_head [ I18n.t("reports.form_or_questions_list_table.headings.form_name"), diff --git a/app/views/reports/forms_with_csv_submission_enabled.html.erb b/app/views/reports/forms_with_csv_submission_enabled.html.erb index 372c6c55a..9c1708d2b 100644 --- a/app/views/reports/forms_with_csv_submission_enabled.html.erb +++ b/app/views/reports/forms_with_csv_submission_enabled.html.erb @@ -8,9 +8,6 @@
- <%= govuk_table( - head: report_forms_table_head, - rows: report_forms_table_rows(forms), - ) %> + <%= govuk_table(**report_table(forms)) %>
diff --git a/app/views/reports/forms_with_payments.html.erb b/app/views/reports/forms_with_payments.html.erb index 9557920e6..f63563725 100644 --- a/app/views/reports/forms_with_payments.html.erb +++ b/app/views/reports/forms_with_payments.html.erb @@ -8,9 +8,6 @@
- <%= govuk_table( - head: report_forms_table_head, - rows: report_forms_table_rows(forms), - ) %> + <%= govuk_table(**report_table(forms)) %>
diff --git a/app/views/reports/forms_with_routes.html.erb b/app/views/reports/forms_with_routes.html.erb index bb6d7c106..09a48bed8 100644 --- a/app/views/reports/forms_with_routes.html.erb +++ b/app/views/reports/forms_with_routes.html.erb @@ -8,9 +8,6 @@
- <%= govuk_table( - head: report_forms_with_routes_table_head, - rows: report_forms_with_routes_table_rows(forms), - ) %> + <%= govuk_table(**report_table(forms)) %>
diff --git a/app/views/reports/questions_with_add_another_answer.html.erb b/app/views/reports/questions_with_add_another_answer.html.erb index 061221139..28a00a5ab 100644 --- a/app/views/reports/questions_with_add_another_answer.html.erb +++ b/app/views/reports/questions_with_add_another_answer.html.erb @@ -8,9 +8,6 @@
- <%= govuk_table( - head: report_questions_table_head, - rows: report_questions_table_rows(questions), - ) %> + <%= govuk_table(**report_table(questions)) %>
diff --git a/spec/helpers/report_helpers_spec.rb b/spec/helpers/report_helpers_spec.rb index 158ccdff6..8bba94cf9 100644 --- a/spec/helpers/report_helpers_spec.rb +++ b/spec/helpers/report_helpers_spec.rb @@ -1,6 +1,99 @@ require "rails_helper" RSpec.describe ReportHelper, type: :helper do + let(:forms) do + [ + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } } }, + { "form_id" => 4, "content" => { "name" => "Skip route form" }, "group" => { "organisation" => { "name" => "Department for Testing" } } }, + ] + end + + let(:forms_with_routes) do + [ + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } }, "metadata" => { "number_of_routes" => 2 } }, + { "form_id" => 4, "content" => { "name" => "Skip route form" }, "group" => { "organisation" => { "name" => "Department for Testing" } }, "metadata" => { "number_of_routes" => 1 } }, + ] + end + + let(:questions) do + [ + { "type" => "question_page", "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, + { "type" => "question_page", "data" => { "question_text" => "What’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } } } }, + ] + end + + describe "#report_table" do + before do + allow(helper).to receive(:report_forms_table).and_call_original + allow(helper).to receive(:report_forms_with_routes_table).and_call_original + allow(helper).to receive(:report_questions_table).and_call_original + end + + context "with list of forms" do + it "calls #report_forms_table" do + helper.report_table(forms) + expect(helper).to have_received(:report_forms_table).with(forms) + end + end + + context "with list of forms with routes" do + it "calls #report_forms_with_routes_table" do + helper.report_table(forms_with_routes) + expect(helper).to have_received(:report_forms_with_routes_table).with(forms_with_routes) + end + end + + context "with list of questions" do + it "calls #report_questions_table" do + helper.report_table(questions) + expect(helper).to have_received(:report_questions_table).with(questions) + end + end + end + + describe "#report_forms_table" do + it "has table head" do + expect(helper.report_forms_table(forms)).to include( + head: helper.report_forms_table_head, + ) + end + + it "has table rows" do + expect(helper.report_forms_table(forms)).to include( + rows: helper.report_forms_table_rows(forms), + ) + end + end + + describe "#report_forms_with_routes_table" do + it "has table head" do + expect(helper.report_forms_with_routes_table(forms_with_routes)).to include( + head: helper.report_forms_with_routes_table_head, + ) + end + + it "has table rows" do + expect(helper.report_forms_with_routes_table(forms_with_routes)).to include( + rows: helper.report_forms_with_routes_table_rows(forms_with_routes), + ) + end + end + + describe "#report_questions_table" do + it "has table head" do + expect(helper.report_questions_table(questions)).to include( + head: helper.report_questions_table_head, + ) + end + + it "has table rows" do + expect(helper.report_questions_table(questions)).to include( + rows: helper.report_questions_table_rows(questions), + ) + end + end + describe "#report_forms_table_head" do it "returns the column headings for a table of forms" do expect(helper.report_forms_table_head).to eq [ @@ -11,14 +104,6 @@ end describe "#report_forms_table_rows" do - let(:forms) do - [ - { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, - { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } } }, - { "form_id" => 4, "content" => { "name" => "Skip route form" }, "group" => { "organisation" => { "name" => "Department for Testing" } } }, - ] - end - it "returns an array of arrays of strings" do expect(helper.report_forms_table_rows(forms)) .to be_an(Array) @@ -64,12 +149,7 @@ end describe "#report_forms_with_routes_table_rows" do - let(:forms) do - [ - { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } }, "metadata" => { "number_of_routes" => 2 } }, - { "form_id" => 4, "content" => { "name" => "Skip route form" }, "group" => { "organisation" => { "name" => "Department for Testing" } }, "metadata" => { "number_of_routes" => 1 } }, - ] - end + let(:forms) { forms_with_routes } it "returns an array of arrays of strings" do expect(helper.report_forms_with_routes_table_rows(forms)) @@ -121,13 +201,6 @@ end describe "#report_questions_table_rows" do - let(:questions) do - [ - { "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, - { "data" => { "question_text" => "What’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } } } }, - ] - end - it "returns an array of arrays of strings" do expect(helper.report_questions_table_rows(questions)) .to be_an(Array) diff --git a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb index 2590aa550..f94038789 100644 --- a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb +++ b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb @@ -3,8 +3,8 @@ describe "reports/questions_with_add_another_answer" do let(:questions) do [ - { "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, - { "data" => { "question_text" => "What’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, + { "type" => "question_page", "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, + { "type" => "question_page", "data" => { "question_text" => "What’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, ] end From 790913f780e534f0a4c0420dbe7d573fad038504 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 13 May 2025 11:58:43 +0300 Subject: [PATCH 07/15] Change paths for feature report routes - Scope routes for feature reports - Make feature report CSV paths more similar to corresponding HTML page paths - Add routing specs Change the paths for routes related to feature reports to be more consistent and neatly scoped together. The purpose of this is to highlight similarities and eventually be more DRY. --- config/routes.rb | 26 ++++++----- spec/routing/reports_routing_spec.rb | 65 ++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 11 deletions(-) create mode 100644 spec/routing/reports_routing_spec.rb diff --git a/config/routes.rb b/config/routes.rb index 9463f85b4..be37ede29 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -192,14 +192,22 @@ resources :memberships, only: %i[destroy update] - scope :reports do + scope "/reports" do get "/", to: "reports#index", as: :reports - get "features", to: "reports#features", as: :report_features - get "questions-with-answer-type/:answer_type", to: "reports#questions_with_answer_type", as: :report_questions_with_answer_type - get "questions-with-add-another-answer", to: "reports#questions_with_add_another_answer", as: :report_questions_with_add_another_answer - get "forms-with-routes", to: "reports#forms_with_routes", as: :report_forms_with_routes - get "forms-with-payments", to: "reports#forms_with_payments", as: :report_forms_with_payments - get "forms-with-csv-submission-enabled", to: "reports#forms_with_csv_submission_enabled", as: :report_forms_with_csv_submission_enabled + + scope "/features" do + get "/", to: "reports#features", as: :report_features + get "questions-with-answer-type/:answer_type", to: "reports#questions_with_answer_type", as: :report_questions_with_answer_type + get "questions-with-add-another-answer", format: false, to: "reports#questions_with_add_another_answer", as: :report_questions_with_add_another_answer + get "questions-with-add-another-answer.csv", format: :csv, to: "reports#live_questions_with_add_another_answer_csv", as: :report_live_questions_with_add_another_answer_csv + get "forms-with-routes", format: false, to: "reports#forms_with_routes", as: :report_forms_with_routes + get "forms-with-routes.csv", format: :csv, to: "reports#live_forms_with_routes_csv", as: :report_live_forms_with_routes_csv + get "forms-with-payments", format: false, to: "reports#forms_with_payments", as: :report_forms_with_payments + get "forms-with-payments.csv", format: :csv, to: "reports#live_forms_with_payments_csv", as: :report_live_forms_with_payments_csv + get "forms-with-csv-submission-enabled", format: false, to: "reports#forms_with_csv_submission_enabled", as: :report_forms_with_csv_submission_enabled + get "forms-with-csv-submission-enabled.csv", format: :csv, to: "reports#live_forms_with_csv_submission_enabled_csv", as: :report_live_forms_with_csv_submission_enabled_csv + end + get "users", to: "reports#users", as: :report_users get "add_another_answer", to: "reports#add_another_answer", as: :report_add_another_answer get "last-signed-in-at", to: "reports#last_signed_in_at", as: :report_last_signed_in_at @@ -209,11 +217,7 @@ get "selection-questions-with-checkboxes", to: "reports#selection_questions_with_checkboxes", as: :report_selection_questions_with_checkboxes get "csv-downloads", to: "reports#csv_downloads", as: :report_csv_downloads get "live-forms-csv", to: "reports#live_forms_csv", as: :report_live_forms_csv - get "live-forms-with-routes-csv", to: "reports#live_forms_with_routes_csv", as: :report_live_forms_with_routes_csv - get "live-forms-with-payments-csv", to: "reports#live_forms_with_payments_csv", as: :report_live_forms_with_payments_csv - get "live-forms-with-csv-submission-enabled-csv", to: "reports#live_forms_with_csv_submission_enabled_csv", as: :report_live_forms_with_csv_submission_enabled_csv get "live-questions-csv", to: "reports#live_questions_csv", as: :report_live_questions_csv - get "live-questions-with-add-another-answer-csv", to: "reports#live_questions_with_add_another_answer_csv", as: :report_live_questions_with_add_another_answer_csv end get "/maintenance" => "errors#maintenance", as: :maintenance_page diff --git a/spec/routing/reports_routing_spec.rb b/spec/routing/reports_routing_spec.rb new file mode 100644 index 000000000..516e72356 --- /dev/null +++ b/spec/routing/reports_routing_spec.rb @@ -0,0 +1,65 @@ +require "rails_helper" + +RSpec.describe ReportsController, type: :routing do + describe "feature reports" do + describe "routing" do + it "routes to#features" do + expect(get: "/reports/features").to route_to("reports#features") + end + + it "routes to #questions_with_answer_type" do + expect(get: "/reports/features/questions-with-answer-type/text").to route_to("reports#questions_with_answer_type", answer_type: "text") + end + + it "routes to #questions_with_add_another_answer" do + expect(get: "/reports/features/questions-with-add-another-answer").to route_to("reports#questions_with_add_another_answer") + end + + it "routes to #live_questions_with_add_another_answer_csv" do + expect(get: "/reports/features/questions-with-add-another-answer.csv").to route_to("reports#live_questions_with_add_another_answer_csv") + end + + it "routes to #forms_with_routes" do + expect(get: "/reports/features/forms-with-routes").to route_to("reports#forms_with_routes") + end + + it "routes to #live_forms_with_routes_csv" do + expect(get: "/reports/features/forms-with-routes.csv").to route_to("reports#live_forms_with_routes_csv") + end + + it "routes to #forms_with_payments" do + expect(get: "/reports/features/forms-with-payments").to route_to("reports#forms_with_payments") + end + + it "routes to #live_forms_with_payments_csv" do + expect(get: "/reports/features/forms-with-payments.csv").to route_to("reports#live_forms_with_payments_csv") + end + + it "routes to #forms_with_csv_submission_enabled" do + expect(get: "/reports/features/forms-with-csv-submission-enabled").to route_to("reports#forms_with_csv_submission_enabled") + end + + it "routes to #live_forms_with_csv_submission_enabled_csv" do + expect(get: "/reports/features/forms-with-csv-submission-enabled.csv").to route_to("reports#live_forms_with_csv_submission_enabled_csv") + end + end + + describe "path helpers" do + it "routes to #live_questions_with_add_another_answer_csv" do + expect(get: report_live_questions_with_add_another_answer_csv_path).to route_to("reports#live_questions_with_add_another_answer_csv") + end + + it "routes to #live_forms_with_routes_csv" do + expect(get: report_live_forms_with_routes_csv_path).to route_to("reports#live_forms_with_routes_csv") + end + + it "routes to #live_forms_with_payments_csv" do + expect(get: report_live_forms_with_payments_csv_path).to route_to("reports#live_forms_with_payments_csv") + end + + it "routes to #live_forms_with_csv_submission_enabled_csv" do + expect(get: report_live_forms_with_csv_submission_enabled_csv_path).to route_to("reports#live_forms_with_csv_submission_enabled_csv") + end + end + end +end From f5f68fa92300405de8fbc664d715dfd14aac5bd7 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 16 May 2025 09:28:00 +0300 Subject: [PATCH 08/15] Change CsvReportsService to receive form_documents Rather than call FormDocumentsService inside CsvReportsService and couple those two classes together, change CsvReportService to expect to be passed a list of form_documents. This allows us to reuse the filtering logic in FeatureReportService instead of reimplementing it in CsvReportsService, lets us change the source of documents in future, and also lets us increases the commonality in the controller actions for rendering a report in HTML and CSV so they might be combined. --- app/controllers/reports_controller.rb | 27 +++++-- app/services/reports/csv_reports_service.rb | 44 +++-------- .../reports/csv_reports_service_spec.rb | 79 ++++--------------- 3 files changed, 47 insertions(+), 103 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index bba7db0c2..4bc52ba04 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -88,38 +88,53 @@ def selection_questions_with_checkboxes def csv_downloads; end def live_forms_csv - send_data Reports::CsvReportsService.new.live_forms_csv, + forms = Reports::FormDocumentsService.live_form_documents + + send_data Reports::CsvReportsService.new(forms).forms_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_report')}" end def live_forms_with_routes_csv - send_data Reports::CsvReportsService.new.live_forms_with_routes_csv, + forms = Reports::FormDocumentsService.live_form_documents + forms = Reports::FeatureReportService.new(forms).forms_with_routes + + send_data Reports::CsvReportsService.new(forms).forms_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_routes_report')}" end def live_forms_with_payments_csv - send_data Reports::CsvReportsService.new.live_forms_with_payments_csv, + forms = Reports::FormDocumentsService.live_form_documents + forms = Reports::FeatureReportService.new(forms).forms_with_payments + + send_data Reports::CsvReportsService.new(forms).forms_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_payments_report')}" end def live_forms_with_csv_submission_enabled_csv - send_data Reports::CsvReportsService.new.live_forms_with_csv_submission_enabled_csv, + forms = Reports::FormDocumentsService.live_form_documents + forms = Reports::FeatureReportService.new(forms).forms_with_csv_submission_enabled + + send_data Reports::CsvReportsService.new(forms).forms_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_csv_submission_enabled_report')}" end def live_questions_csv answer_type = params[:answer_type] - send_data Reports::CsvReportsService.new.live_questions_csv(answer_type:), + forms = Reports::FormDocumentsService.live_form_documents + + send_data Reports::CsvReportsService.new(forms).questions_csv(answer_type:), type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{questions_csv_filename(answer_type)}" end def live_questions_with_add_another_answer_csv - send_data Reports::CsvReportsService.new.live_questions_with_add_another_answer_csv, + forms = Reports::FormDocumentsService.live_form_documents + + send_data Reports::CsvReportsService.new(forms).questions_with_add_another_answer_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_questions_with_add_another_answer_report')}" end diff --git a/app/services/reports/csv_reports_service.rb b/app/services/reports/csv_reports_service.rb index d1afbad54..f2f076433 100644 --- a/app/services/reports/csv_reports_service.rb +++ b/app/services/reports/csv_reports_service.rb @@ -51,51 +51,27 @@ class Reports::CsvReportsService IS_REPEATABLE_COLUMN_INDEX = QUESTIONS_CSV_HEADERS.find_index(IS_REPEATABLE) - def live_forms_csv - CSV.generate do |csv| - csv << FORM_CSV_HEADERS - - Reports::FormDocumentsService.live_form_documents.each do |form_document| - csv << form_row(form_document) - end - end - end - - def live_forms_with_routes_csv - CSV.generate do |csv| - csv << FORM_CSV_HEADERS + attr_reader :form_documents - Reports::FormDocumentsService.live_form_documents.each do |form_document| - csv << form_row(form_document) if Reports::FormDocumentsService.has_routes?(form_document) - end - end - end - - def live_forms_with_payments_csv - CSV.generate do |csv| - csv << FORM_CSV_HEADERS - - Reports::FormDocumentsService.live_form_documents.each do |form_document| - csv << form_row(form_document) if Reports::FormDocumentsService.has_payments?(form_document) - end - end + def initialize(form_documents) + @form_documents = form_documents end - def live_forms_with_csv_submission_enabled_csv + def forms_csv CSV.generate do |csv| csv << FORM_CSV_HEADERS - Reports::FormDocumentsService.live_form_documents.each do |form_document| - csv << form_row(form_document) if Reports::FormDocumentsService.has_csv_submission_enabled?(form_document) + form_documents.each do |form_document| + csv << form_row(form_document) end end end - def live_questions_csv(answer_type: nil) + def questions_csv(answer_type: nil) CSV.generate do |csv| csv << QUESTIONS_CSV_HEADERS - Reports::FormDocumentsService.live_form_documents.each do |form_document| + form_documents.each do |form_document| question_rows = question_rows(form_document, answer_type).compact question_rows.each do |question| @@ -105,11 +81,11 @@ def live_questions_csv(answer_type: nil) end end - def live_questions_with_add_another_answer_csv + def questions_with_add_another_answer_csv CSV.generate do |csv| csv << QUESTIONS_CSV_HEADERS - Reports::FormDocumentsService.live_form_documents.each do |form_document| + form_documents.each do |form_document| question_rows = question_rows(form_document, nil).compact question_rows.each do |question| diff --git a/spec/services/reports/csv_reports_service_spec.rb b/spec/services/reports/csv_reports_service_spec.rb index 0cb2a4b81..76de75650 100644 --- a/spec/services/reports/csv_reports_service_spec.rb +++ b/spec/services/reports/csv_reports_service_spec.rb @@ -2,10 +2,10 @@ RSpec.describe Reports::CsvReportsService do subject(:csv_reports_service) do - described_class.new + described_class.new(form_documents) end - let(:form_documents_response_json) { JSON.parse(file_fixture("form_documents_response.json").read) } + let(:form_documents) { JSON.parse(file_fixture("form_documents_response.json").read) } let(:group) { create(:group) } @@ -14,19 +14,17 @@ GroupForm.create!(form_id: 2, group:) GroupForm.create!(form_id: 3, group:) GroupForm.create!(form_id: 4, group:) - - allow(Reports::FormDocumentsService).to receive(:live_form_documents).and_return(form_documents_response_json) end - describe "#live_forms_csv" do + describe "#forms_csv" do it "returns a CSV with a header row and a row for each form" do - csv = csv_reports_service.live_forms_csv + csv = csv_reports_service.forms_csv rows = CSV.parse(csv) expect(rows.length).to eq 5 end it "has expected values" do - csv = csv_reports_service.live_forms_csv + csv = csv_reports_service.forms_csv rows = CSV.parse(csv) expect(rows[1]).to eq([ "1", @@ -53,61 +51,16 @@ end end - describe "#live_forms_with_routes_csv" do - it "returns a CSV with a header row and a row for each form with routes" do - csv = csv_reports_service.live_forms_with_routes_csv - rows = CSV.parse(csv) - expect(rows.length).to eq 3 - end - - it "includes form with routes" do - csv = csv_reports_service.live_forms_with_routes_csv - rows = CSV.parse(csv) - has_routes_column_index = rows[0].find_index("Has routes") - expect(rows[1][has_routes_column_index]).to eq "true" - end - end - - describe "#live_forms_with_payments_csv" do - it "returns a CSV with 2 rows, including the header row" do - csv = csv_reports_service.live_forms_with_payments_csv - rows = CSV.parse(csv) - expect(rows.length).to eq 2 - end - - it "includes form with payments" do - csv = csv_reports_service.live_forms_with_payments_csv - rows = CSV.parse(csv) - payment_url_column_index = rows[0].find_index("Payment URL") - expect(rows[1][payment_url_column_index]).to eq "https://www.gov.uk/payments/your-payment-link" - end - end - - describe "#live_forms_with_csv_submission_enabled_csv" do - it "returns a CSV with 2 rows, including the header row" do - csv = csv_reports_service.live_forms_with_csv_submission_enabled_csv - rows = CSV.parse(csv) - expect(rows.length).to eq 2 - end - - it "includes form with submission type email_with_csv" do - csv = csv_reports_service.live_forms_with_csv_submission_enabled_csv - rows = CSV.parse(csv) - submission_type_column_index = rows[0].find_index("Submission type") - expect(rows[1][submission_type_column_index]).to eq "email_with_csv" - end - end - - describe "#live_questions_csv" do + describe "#questions_csv" do context "when answer_type is nil" do it "returns a CSV with a header row and a rows for each question" do - csv = csv_reports_service.live_questions_csv + csv = csv_reports_service.questions_csv rows = CSV.parse(csv) expect(rows.length).to eq 18 end it "has expected values for text question" do - csv = csv_reports_service.live_questions_csv + csv = csv_reports_service.questions_csv rows = CSV.parse(csv) text_question_row = rows.detect { |row| row.include? "Single line of text" } expect(text_question_row).to eq([ @@ -136,7 +89,7 @@ end it "has expected values for selection question" do - csv = csv_reports_service.live_questions_csv + csv = csv_reports_service.questions_csv rows = CSV.parse(csv) selection_question_row = rows.detect { |row| row.include? "Selection from a list of options" } expect(selection_question_row).to eq([ @@ -165,7 +118,7 @@ end it "has expected values for name question" do - csv = csv_reports_service.live_questions_csv + csv = csv_reports_service.questions_csv rows = CSV.parse(csv) name_question_row = rows.detect { |row| row.include? "What’s your name?" } expect(name_question_row).to eq([ @@ -194,7 +147,7 @@ end it "has expected values for question with routing conditions" do - csv = csv_reports_service.live_questions_csv + csv = csv_reports_service.questions_csv rows = CSV.parse(csv) routing_question_row = rows.detect { |row| row.include? "How many times have you filled out this form?" } expect(routing_question_row).to eq([ @@ -225,13 +178,13 @@ context "when answer_type is provided" do it "returns 3 rows, including the header row" do - csv = csv_reports_service.live_questions_csv(answer_type: "email") + csv = csv_reports_service.questions_csv(answer_type: "email") rows = CSV.parse(csv) expect(rows.length).to eq 3 end it "only includes the desired answer_type" do - csv = csv_reports_service.live_questions_csv(answer_type: "email") + csv = csv_reports_service.questions_csv(answer_type: "email") rows = CSV.parse(csv) answer_type_column_index = rows[0].find_index("Answer type") expect(rows[1][answer_type_column_index]).to eq "email" @@ -240,15 +193,15 @@ end end - describe "#live_questions_with_add_another_answer_csv" do + describe "#questions_with_add_another_answer_csv" do it "returns 3 rows, including the header row" do - csv = csv_reports_service.live_questions_with_add_another_answer_csv + csv = csv_reports_service.questions_with_add_another_answer_csv rows = CSV.parse(csv) expect(rows.length).to eq 3 end it "only includes questions with add another answer" do - csv = csv_reports_service.live_questions_with_add_another_answer_csv + csv = csv_reports_service.questions_with_add_another_answer_csv rows = CSV.parse(csv) answer_type_column_index = rows[0].find_index("Is repeatable?") expect(rows[1][answer_type_column_index]).to eq "true" From 7933ef547f89ab6bbdfc6f68b65cac96178f7373 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 16 May 2025 10:02:45 +0300 Subject: [PATCH 09/15] Split CsvReportsService into two Put code for generating CSV of questions into QuestionCsvReportService and code for generating CSV of forms into FormCsvReportService. --- app/controllers/reports_controller.rb | 12 ++-- .../reports/forms_csv_report_service.rb | 71 +++++++++++++++++++ ...ice.rb => questions_csv_report_service.rb} | 62 +--------------- spec/requests/reports_controller_spec.rb | 12 ++-- .../reports/forms_csv_report_service_spec.rb | 53 ++++++++++++++ ...b => questions_csv_report_service_spec.rb} | 37 +--------- 6 files changed, 138 insertions(+), 109 deletions(-) create mode 100644 app/services/reports/forms_csv_report_service.rb rename app/services/reports/{csv_reports_service.rb => questions_csv_report_service.rb} (63%) create mode 100644 spec/services/reports/forms_csv_report_service_spec.rb rename spec/services/reports/{csv_reports_service_spec.rb => questions_csv_report_service_spec.rb} (84%) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 4bc52ba04..1dca21b53 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -90,7 +90,7 @@ def csv_downloads; end def live_forms_csv forms = Reports::FormDocumentsService.live_form_documents - send_data Reports::CsvReportsService.new(forms).forms_csv, + send_data Reports::FormsCsvReportService.new(forms).forms_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_report')}" end @@ -99,7 +99,7 @@ def live_forms_with_routes_csv forms = Reports::FormDocumentsService.live_form_documents forms = Reports::FeatureReportService.new(forms).forms_with_routes - send_data Reports::CsvReportsService.new(forms).forms_csv, + send_data Reports::FormsCsvReportService.new(forms).forms_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_routes_report')}" end @@ -108,7 +108,7 @@ def live_forms_with_payments_csv forms = Reports::FormDocumentsService.live_form_documents forms = Reports::FeatureReportService.new(forms).forms_with_payments - send_data Reports::CsvReportsService.new(forms).forms_csv, + send_data Reports::FormsCsvReportService.new(forms).forms_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_payments_report')}" end @@ -117,7 +117,7 @@ def live_forms_with_csv_submission_enabled_csv forms = Reports::FormDocumentsService.live_form_documents forms = Reports::FeatureReportService.new(forms).forms_with_csv_submission_enabled - send_data Reports::CsvReportsService.new(forms).forms_csv, + send_data Reports::FormsCsvReportService.new(forms).forms_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_csv_submission_enabled_report')}" end @@ -126,7 +126,7 @@ def live_questions_csv answer_type = params[:answer_type] forms = Reports::FormDocumentsService.live_form_documents - send_data Reports::CsvReportsService.new(forms).questions_csv(answer_type:), + send_data Reports::QuestionsCsvReportService.new(forms).questions_csv(answer_type:), type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{questions_csv_filename(answer_type)}" end @@ -134,7 +134,7 @@ def live_questions_csv def live_questions_with_add_another_answer_csv forms = Reports::FormDocumentsService.live_form_documents - send_data Reports::CsvReportsService.new(forms).questions_with_add_another_answer_csv, + send_data Reports::QuestionsCsvReportService.new(forms).questions_with_add_another_answer_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_questions_with_add_another_answer_report')}" end diff --git a/app/services/reports/forms_csv_report_service.rb b/app/services/reports/forms_csv_report_service.rb new file mode 100644 index 000000000..bcec44a6a --- /dev/null +++ b/app/services/reports/forms_csv_report_service.rb @@ -0,0 +1,71 @@ +require "csv" + +class Reports::FormsCsvReportService + FORM_CSV_HEADERS = [ + "Form ID", + "Status", + "Form name", + "Slug", + "Organisation name", + "Organisation ID", + "Group name", + "Group ID", + "Created at", + "Updated at", + "Number of questions", + "Has routes", + "Payment URL", + "Support URL", + "Support URL text", + "Support email", + "Support phone", + "Privacy policy URL", + "What happens next markdown", + "Submission type", + ].freeze + + attr_reader :form_documents + + def initialize(form_documents) + @form_documents = form_documents + end + + def forms_csv + CSV.generate do |csv| + csv << FORM_CSV_HEADERS + + form_documents.each do |form_document| + csv << form_row(form_document) + end + end + end + +private + + def form_row(form) + form_id = form["form_id"] + group = GroupForm.find_by_form_id(form_id)&.group + [ + form_id, + form["tag"], + form["content"]["name"], + form["content"]["form_slug"], + group&.organisation&.name, + group&.organisation&.id, + group&.name, + group&.external_id, + form["content"]["created_at"], + form["content"]["updated_at"], + form["content"]["steps"].length, + form["content"]["steps"].any? { |step| step["routing_conditions"].present? }, + form["content"]["payment_url"], + form["content"]["support_url"], + form["content"]["support_url_text"], + form["content"]["support_email"], + form["content"]["support_phone"], + form["content"]["privacy_policy_url"], + form["content"]["what_happens_next_markdown"], + form["content"]["submission_type"], + ] + end +end diff --git a/app/services/reports/csv_reports_service.rb b/app/services/reports/questions_csv_report_service.rb similarity index 63% rename from app/services/reports/csv_reports_service.rb rename to app/services/reports/questions_csv_report_service.rb index f2f076433..3f43600fb 100644 --- a/app/services/reports/csv_reports_service.rb +++ b/app/services/reports/questions_csv_report_service.rb @@ -1,29 +1,6 @@ require "csv" -class Reports::CsvReportsService - FORM_CSV_HEADERS = [ - "Form ID", - "Status", - "Form name", - "Slug", - "Organisation name", - "Organisation ID", - "Group name", - "Group ID", - "Created at", - "Updated at", - "Number of questions", - "Has routes", - "Payment URL", - "Support URL", - "Support URL text", - "Support email", - "Support phone", - "Privacy policy URL", - "What happens next markdown", - "Submission type", - ].freeze - +class Reports::QuestionsCsvReportService IS_REPEATABLE = "Is repeatable?".freeze QUESTIONS_CSV_HEADERS = [ "Form ID", @@ -57,16 +34,6 @@ def initialize(form_documents) @form_documents = form_documents end - def forms_csv - CSV.generate do |csv| - csv << FORM_CSV_HEADERS - - form_documents.each do |form_document| - csv << form_row(form_document) - end - end - end - def questions_csv(answer_type: nil) CSV.generate do |csv| csv << QUESTIONS_CSV_HEADERS @@ -97,33 +64,6 @@ def questions_with_add_another_answer_csv private - def form_row(form) - form_id = form["form_id"] - group = GroupForm.find_by_form_id(form_id)&.group - [ - form_id, - form["tag"], - form["content"]["name"], - form["content"]["form_slug"], - group&.organisation&.name, - group&.organisation&.id, - group&.name, - group&.external_id, - form["content"]["created_at"], - form["content"]["updated_at"], - form["content"]["steps"].length, - form["content"]["steps"].any? { |step| step["routing_conditions"].present? }, - form["content"]["payment_url"], - form["content"]["support_url"], - form["content"]["support_url_text"], - form["content"]["support_email"], - form["content"]["support_phone"], - form["content"]["privacy_policy_url"], - form["content"]["what_happens_next_markdown"], - form["content"]["submission_type"], - ] - end - def question_rows(form, answer_type) form_id = form["form_id"] group = GroupForm.find_by_form_id(form_id)&.group diff --git a/spec/requests/reports_controller_spec.rb b/spec/requests/reports_controller_spec.rb index 5fc9dc1a8..78912417a 100644 --- a/spec/requests/reports_controller_spec.rb +++ b/spec/requests/reports_controller_spec.rb @@ -725,7 +725,7 @@ it "has expected response body" do csv = CSV.parse(response.body, headers: true) - expect(csv.headers).to eq Reports::CsvReportsService::FORM_CSV_HEADERS + expect(csv.headers).to eq Reports::FormsCsvReportService::FORM_CSV_HEADERS expect(csv.length).to eq 4 end end @@ -747,7 +747,7 @@ it "has expected response body" do csv = CSV.parse(response.body, headers: true) - expect(csv.headers).to eq Reports::CsvReportsService::FORM_CSV_HEADERS + expect(csv.headers).to eq Reports::FormsCsvReportService::FORM_CSV_HEADERS expect(csv.length).to eq 2 expect(csv.by_col["Form name"]).to eq [ "Branch route form", @@ -773,7 +773,7 @@ it "has expected response body" do csv = CSV.parse(response.body, headers: true) - expect(csv.headers).to eq Reports::CsvReportsService::FORM_CSV_HEADERS + expect(csv.headers).to eq Reports::FormsCsvReportService::FORM_CSV_HEADERS expect(csv.length).to eq 1 expect(csv.by_col["Form name"]).to eq [ "All question types form", @@ -798,7 +798,7 @@ it "has expected response body" do csv = CSV.parse(response.body, headers: true) - expect(csv.headers).to eq Reports::CsvReportsService::FORM_CSV_HEADERS + expect(csv.headers).to eq Reports::FormsCsvReportService::FORM_CSV_HEADERS expect(csv.length).to eq 1 expect(csv.by_col["Form name"]).to eq [ "All question types form", @@ -823,7 +823,7 @@ it "has expected response body" do csv = CSV.parse(response.body, headers: true) - expect(csv.headers).to eq Reports::CsvReportsService::QUESTIONS_CSV_HEADERS + expect(csv.headers).to eq Reports::QuestionsCsvReportService::QUESTIONS_CSV_HEADERS expect(csv.length).to eq 17 end end @@ -845,7 +845,7 @@ it "has expected response body" do csv = CSV.parse(response.body, headers: true) - expect(csv.headers).to eq Reports::CsvReportsService::QUESTIONS_CSV_HEADERS + expect(csv.headers).to eq Reports::QuestionsCsvReportService::QUESTIONS_CSV_HEADERS expect(csv.length).to eq 2 end end diff --git a/spec/services/reports/forms_csv_report_service_spec.rb b/spec/services/reports/forms_csv_report_service_spec.rb new file mode 100644 index 000000000..6c50f2d01 --- /dev/null +++ b/spec/services/reports/forms_csv_report_service_spec.rb @@ -0,0 +1,53 @@ +require "rails_helper" + +RSpec.describe Reports::FormsCsvReportService do + subject(:csv_reports_service) do + described_class.new(form_documents) + end + + let(:form_documents) { JSON.parse(file_fixture("form_documents_response.json").read) } + + let(:group) { create(:group) } + + before do + GroupForm.create!(form_id: 1, group:) + GroupForm.create!(form_id: 2, group:) + GroupForm.create!(form_id: 3, group:) + GroupForm.create!(form_id: 4, group:) + end + + describe "#forms_csv" do + it "returns a CSV with a header row and a row for each form" do + csv = csv_reports_service.forms_csv + rows = CSV.parse(csv) + expect(rows.length).to eq 5 + end + + it "has expected values" do + csv = csv_reports_service.forms_csv + rows = CSV.parse(csv) + expect(rows[1]).to eq([ + "1", + "live", + "All question types form", + "all-question-types-form", + group.organisation.name, + group.organisation.id.to_s, + group.name, + group.external_id, + "2025-01-02T16:24:31.203Z", + "2025-01-02T16:24:31.255Z", + "9", + "false", + "https://www.gov.uk/payments/your-payment-link", + nil, + nil, + "your.email+fakedata84701@gmail.com.gov.uk", + "08000800", + "https://www.gov.uk/help/privacy-notice", + "Test", + "email_with_csv", + ]) + end + end +end diff --git a/spec/services/reports/csv_reports_service_spec.rb b/spec/services/reports/questions_csv_report_service_spec.rb similarity index 84% rename from spec/services/reports/csv_reports_service_spec.rb rename to spec/services/reports/questions_csv_report_service_spec.rb index 76de75650..869f51bf5 100644 --- a/spec/services/reports/csv_reports_service_spec.rb +++ b/spec/services/reports/questions_csv_report_service_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe Reports::CsvReportsService do +RSpec.describe Reports::QuestionsCsvReportService do subject(:csv_reports_service) do described_class.new(form_documents) end @@ -16,41 +16,6 @@ GroupForm.create!(form_id: 4, group:) end - describe "#forms_csv" do - it "returns a CSV with a header row and a row for each form" do - csv = csv_reports_service.forms_csv - rows = CSV.parse(csv) - expect(rows.length).to eq 5 - end - - it "has expected values" do - csv = csv_reports_service.forms_csv - rows = CSV.parse(csv) - expect(rows[1]).to eq([ - "1", - "live", - "All question types form", - "all-question-types-form", - group.organisation.name, - group.organisation.id.to_s, - group.name, - group.external_id, - "2025-01-02T16:24:31.203Z", - "2025-01-02T16:24:31.255Z", - "9", - "false", - "https://www.gov.uk/payments/your-payment-link", - nil, - nil, - "your.email+fakedata84701@gmail.com.gov.uk", - "08000800", - "https://www.gov.uk/help/privacy-notice", - "Test", - "email_with_csv", - ]) - end - end - describe "#questions_csv" do context "when answer_type is nil" do it "returns a CSV with a header row and a rows for each question" do From a36df7b71f7f52d943e5091fcf373cb53e249c4c Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 16 May 2025 11:21:16 +0300 Subject: [PATCH 10/15] Change QuestionsCsvReportService to receive question_page documents Rather than have QuestionCsvReportService extract and filter questions from forms itself, make it expect to be passed a list of question_page documents, so that we can reuse the code in FeatureReportService and reduce duplication. --- app/controllers/reports_controller.rb | 10 +- .../reports/feature_report_service.rb | 7 + .../reports/questions_csv_report_service.rb | 83 +++--- .../reports/feature_report_service_spec.rb | 36 +++ .../questions_csv_report_service_spec.rb | 259 ++++++++---------- 5 files changed, 195 insertions(+), 200 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 1dca21b53..82869835c 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -125,16 +125,22 @@ def live_forms_with_csv_submission_enabled_csv def live_questions_csv answer_type = params[:answer_type] forms = Reports::FormDocumentsService.live_form_documents + questions = if answer_type + Reports::FeatureReportService.new(forms).questions_with_answer_type(answer_type) + else + Reports::FeatureReportService.new(forms).questions + end - send_data Reports::QuestionsCsvReportService.new(forms).questions_csv(answer_type:), + send_data Reports::QuestionsCsvReportService.new(questions).questions_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{questions_csv_filename(answer_type)}" end def live_questions_with_add_another_answer_csv forms = Reports::FormDocumentsService.live_form_documents + questions = Reports::FeatureReportService.new(forms).questions_with_add_another_answer - send_data Reports::QuestionsCsvReportService.new(forms).questions_with_add_another_answer_csv, + send_data Reports::QuestionsCsvReportService.new(questions).questions_csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_questions_with_add_another_answer_report')}" end diff --git a/app/services/reports/feature_report_service.rb b/app/services/reports/feature_report_service.rb index 337b4e3d5..f8ebbb1b1 100644 --- a/app/services/reports/feature_report_service.rb +++ b/app/services/reports/feature_report_service.rb @@ -39,6 +39,13 @@ def report report end + def questions + form_documents.flat_map do |form| + form["content"]["steps"] + .map { |step| questions_details(form, step) } + end + end + def questions_with_answer_type(answer_type) form_documents.flat_map do |form| form["content"]["steps"] diff --git a/app/services/reports/questions_csv_report_service.rb b/app/services/reports/questions_csv_report_service.rb index 3f43600fb..8d79e9df1 100644 --- a/app/services/reports/questions_csv_report_service.rb +++ b/app/services/reports/questions_csv_report_service.rb @@ -28,72 +28,51 @@ class Reports::QuestionsCsvReportService IS_REPEATABLE_COLUMN_INDEX = QUESTIONS_CSV_HEADERS.find_index(IS_REPEATABLE) - attr_reader :form_documents + attr_reader :question_page_documents - def initialize(form_documents) - @form_documents = form_documents + def initialize(question_page_documents) + @question_page_documents = question_page_documents end - def questions_csv(answer_type: nil) + def questions_csv CSV.generate do |csv| csv << QUESTIONS_CSV_HEADERS - form_documents.each do |form_document| - question_rows = question_rows(form_document, answer_type).compact - - question_rows.each do |question| - csv << question - end - end - end - end - - def questions_with_add_another_answer_csv - CSV.generate do |csv| - csv << QUESTIONS_CSV_HEADERS - - form_documents.each do |form_document| - question_rows = question_rows(form_document, nil).compact - - question_rows.each do |question| - csv << question if question[IS_REPEATABLE_COLUMN_INDEX] - end + question_page_documents.each do |question_page_document| + csv << question_row(question_page_document) end end end private - def question_rows(form, answer_type) + def question_row(step) + form = step["form"] form_id = form["form_id"] group = GroupForm.find_by_form_id(form_id)&.group - form["content"]["steps"].each_with_index.map do |step, index| - next if answer_type.present? && step["data"]["answer_type"] != answer_type - - [ - form_id, - form["tag"], - form["content"]["name"], - group&.organisation&.name, - group&.organisation&.id, - group&.name, - group&.external_id, - index + 1, - step["data"]["question_text"], - step["data"]["answer_type"], - step["data"]["hint_text"], - step["data"]["page_heading"], - step["data"]["guidance_markdown"], - step["data"]["is_optional"], - step["data"]["is_repeatable"], - step["routing_conditions"].present?, - step.dig("data", "answer_settings", "input_type"), - step.dig("data", "answer_settings", "only_one_option").presence.try { |o| o.to_s == "true" }, - step.dig("data", "answer_settings", "selection_options")&.length, - step.dig("data", "answer_settings", "title_needed"), - step["data"]["answer_settings"].as_json, - ] - end + [ + form_id, + form["tag"], + form["content"]["name"], + group&.organisation&.name, + group&.organisation&.id, + group&.name, + group&.external_id, + step["position"], + step["data"]["question_text"], + step["data"]["answer_type"], + step["data"]["hint_text"], + step["data"]["page_heading"], + step["data"]["guidance_markdown"], + step["data"]["is_optional"], + step["data"]["is_repeatable"], + step["routing_conditions"].present?, + step.dig("data", "answer_settings", "input_type"), + step.dig("data", "answer_settings", "only_one_option").presence.try { |o| o.to_s == "true" }, + step.dig("data", "answer_settings", "selection_options")&.length, + step.dig("data", "answer_settings", "title_needed"), + step["data"]["answer_settings"].as_json, + ] end end diff --git a/spec/services/reports/feature_report_service_spec.rb b/spec/services/reports/feature_report_service_spec.rb index eea3f5fe7..949228e2d 100644 --- a/spec/services/reports/feature_report_service_spec.rb +++ b/spec/services/reports/feature_report_service_spec.rb @@ -46,6 +46,42 @@ end end + describe "#questions" do + it "returns all questions in all forms given" do + questions = described_class.new(form_documents).questions + expect(questions.length).to eq 17 + end + + it "returns details needed to render report" do + questions = described_class.new(form_documents).questions + expect(questions).to all match( + a_hash_including( + "form" => a_hash_including( + "form_id" => an_instance_of(Integer), + "content" => a_hash_including( + "name" => a_kind_of(String), + ), + ), + "data" => a_hash_including( + "question_text" => a_kind_of(String), + ), + ), + ) + end + + it "includes a reference to the form document" do + questions = described_class.new(form_documents).questions_with_answer_type("text") + expect(questions).to all include( + "form" => a_hash_including( + "form_id", + "content" => a_hash_including( + "name", + ), + ), + ) + end + end + describe "#questions_with_answer_type" do it "returns details needed to render report" do questions = described_class.new(form_documents).questions_with_answer_type("email") diff --git a/spec/services/reports/questions_csv_report_service_spec.rb b/spec/services/reports/questions_csv_report_service_spec.rb index 869f51bf5..69aaa56fc 100644 --- a/spec/services/reports/questions_csv_report_service_spec.rb +++ b/spec/services/reports/questions_csv_report_service_spec.rb @@ -2,9 +2,10 @@ RSpec.describe Reports::QuestionsCsvReportService do subject(:csv_reports_service) do - described_class.new(form_documents) + described_class.new(question_page_documents) end + let(:question_page_documents) { Reports::FeatureReportService.new(form_documents).questions } let(:form_documents) { JSON.parse(file_fixture("form_documents_response.json").read) } let(:group) { create(:group) } @@ -17,160 +18,126 @@ end describe "#questions_csv" do - context "when answer_type is nil" do - it "returns a CSV with a header row and a rows for each question" do - csv = csv_reports_service.questions_csv - rows = CSV.parse(csv) - expect(rows.length).to eq 18 - end - - it "has expected values for text question" do - csv = csv_reports_service.questions_csv - rows = CSV.parse(csv) - text_question_row = rows.detect { |row| row.include? "Single line of text" } - expect(text_question_row).to eq([ - "1", - "live", - "All question types form", - group.organisation.name, - group.organisation.id.to_s, - group.name, - group.external_id, - "1", - "Single line of text", - "text", - nil, - nil, - nil, - "false", - "true", - "false", - "single_line", - nil, - nil, - nil, - "{\"input_type\" => \"single_line\"}", - ]) - end - - it "has expected values for selection question" do - csv = csv_reports_service.questions_csv - rows = CSV.parse(csv) - selection_question_row = rows.detect { |row| row.include? "Selection from a list of options" } - expect(selection_question_row).to eq([ - "1", - "live", - "All question types form", - group.organisation.name, - group.organisation.id.to_s, - group.name, - group.external_id, - "8", - "Selection from a list of options", - "selection", - nil, - nil, - nil, - "true", - "false", - "false", - nil, - "false", - "3", - nil, - "{\"only_one_option\" => \"0\", \"selection_options\" => [{\"name\" => \"Option 1\"}, {\"name\" => \"Option 2\"}, {\"name\" => \"Option 3\"}]}", - ]) - end - - it "has expected values for name question" do - csv = csv_reports_service.questions_csv - rows = CSV.parse(csv) - name_question_row = rows.detect { |row| row.include? "What’s your name?" } - expect(name_question_row).to eq([ - "3", - "live", - "Branch route form", - group.organisation.name, - group.organisation.id.to_s, - group.name, - group.external_id, - "2", - "What’s your name?", - "name", - nil, - nil, - nil, - "false", - "false", - "false", - "full_name", - nil, - nil, - "false", - "{\"input_type\" => \"full_name\", \"title_needed\" => false}", - ]) - end - - it "has expected values for question with routing conditions" do - csv = csv_reports_service.questions_csv - rows = CSV.parse(csv) - routing_question_row = rows.detect { |row| row.include? "How many times have you filled out this form?" } - expect(routing_question_row).to eq([ - "3", - "live", - "Branch route form", - group.organisation.name, - group.organisation.id.to_s, - group.name, - group.external_id, - "1", - "How many times have you filled out this form?", - "selection", - nil, - nil, - nil, - "false", - "false", - "true", - nil, - "true", - "2", - nil, - "{\"only_one_option\" => \"true\", \"selection_options\" => [{\"name\" => \"Once\"}, {\"name\" => \"More than once\"}]}", - ]) - end + it "returns a CSV with a header row and a rows for each question" do + csv = csv_reports_service.questions_csv + rows = CSV.parse(csv) + expect(rows.length).to eq 18 end - context "when answer_type is provided" do - it "returns 3 rows, including the header row" do - csv = csv_reports_service.questions_csv(answer_type: "email") - rows = CSV.parse(csv) - expect(rows.length).to eq 3 - end + it "has expected values for text question" do + csv = csv_reports_service.questions_csv + rows = CSV.parse(csv) + text_question_row = rows.detect { |row| row.include? "Single line of text" } + expect(text_question_row).to eq([ + "1", + "live", + "All question types form", + group.organisation.name, + group.organisation.id.to_s, + group.name, + group.external_id, + "1", + "Single line of text", + "text", + nil, + nil, + nil, + "false", + "true", + "false", + "single_line", + nil, + nil, + nil, + "{\"input_type\" => \"single_line\"}", + ]) + end - it "only includes the desired answer_type" do - csv = csv_reports_service.questions_csv(answer_type: "email") - rows = CSV.parse(csv) - answer_type_column_index = rows[0].find_index("Answer type") - expect(rows[1][answer_type_column_index]).to eq "email" - expect(rows[2][answer_type_column_index]).to eq "email" - end + it "has expected values for selection question" do + csv = csv_reports_service.questions_csv + rows = CSV.parse(csv) + selection_question_row = rows.detect { |row| row.include? "Selection from a list of options" } + expect(selection_question_row).to eq([ + "1", + "live", + "All question types form", + group.organisation.name, + group.organisation.id.to_s, + group.name, + group.external_id, + "8", + "Selection from a list of options", + "selection", + nil, + nil, + nil, + "true", + "false", + "false", + nil, + "false", + "3", + nil, + "{\"only_one_option\" => \"0\", \"selection_options\" => [{\"name\" => \"Option 1\"}, {\"name\" => \"Option 2\"}, {\"name\" => \"Option 3\"}]}", + ]) end - end - describe "#questions_with_add_another_answer_csv" do - it "returns 3 rows, including the header row" do - csv = csv_reports_service.questions_with_add_another_answer_csv + it "has expected values for name question" do + csv = csv_reports_service.questions_csv rows = CSV.parse(csv) - expect(rows.length).to eq 3 + name_question_row = rows.detect { |row| row.include? "What’s your name?" } + expect(name_question_row).to eq([ + "3", + "live", + "Branch route form", + group.organisation.name, + group.organisation.id.to_s, + group.name, + group.external_id, + "2", + "What’s your name?", + "name", + nil, + nil, + nil, + "false", + "false", + "false", + "full_name", + nil, + nil, + "false", + "{\"input_type\" => \"full_name\", \"title_needed\" => false}", + ]) end - it "only includes questions with add another answer" do - csv = csv_reports_service.questions_with_add_another_answer_csv + it "has expected values for question with routing conditions" do + csv = csv_reports_service.questions_csv rows = CSV.parse(csv) - answer_type_column_index = rows[0].find_index("Is repeatable?") - expect(rows[1][answer_type_column_index]).to eq "true" - expect(rows[2][answer_type_column_index]).to eq "true" + routing_question_row = rows.detect { |row| row.include? "How many times have you filled out this form?" } + expect(routing_question_row).to eq([ + "3", + "live", + "Branch route form", + group.organisation.name, + group.organisation.id.to_s, + group.name, + group.external_id, + "1", + "How many times have you filled out this form?", + "selection", + nil, + nil, + nil, + "false", + "false", + "true", + nil, + "true", + "2", + nil, + "{\"only_one_option\" => \"true\", \"selection_options\" => [{\"name\" => \"Once\"}, {\"name\" => \"More than once\"}]}", + ]) end end end From 799081137b33c489203db376ad90e437dfc5b2a2 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 16 May 2025 13:54:04 +0300 Subject: [PATCH 11/15] Combine routes for HTML and CSV feature reports Rather than having two different controller actions for the same data in two different formats, we can (now that we have made the code similar enough) combine the pairs into one each, differentiating by the format suffix in the request path [[1]]. The main advantage of this is that when adding a new feature report in future there is less to do. [1]: https://guides.rubyonrails.org/routing.html#format-segments --- app/controllers/reports_controller.rb | 68 ++++++++----------- ...forms_with_csv_submission_enabled.html.erb | 2 +- .../reports/forms_with_payments.html.erb | 2 +- app/views/reports/forms_with_routes.html.erb | 2 +- ...questions_with_add_another_answer.html.erb | 2 +- config/routes.rb | 12 ++-- spec/requests/reports_controller_spec.rb | 16 ++--- spec/routing/reports_routing_spec.rb | 32 ++++----- ...th_csv_submission_enabled.html.erb_spec.rb | 2 +- .../forms_with_payments.html.erb_spec.rb | 2 +- .../forms_with_routes.html.erb_spec.rb | 2 +- ...s_with_add_another_answer.html.erb_spec.rb | 2 +- 12 files changed, 64 insertions(+), 80 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 82869835c..d04047bb8 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -23,28 +23,52 @@ def questions_with_add_another_answer forms = Reports::FormDocumentsService.live_form_documents questions = Reports::FeatureReportService.new(forms).questions_with_add_another_answer - render template: "reports/questions_with_add_another_answer", locals: { questions: } + if params[:format] == "csv" + send_data Reports::QuestionsCsvReportService.new(questions).questions_csv, + type: "text/csv; charset=iso-8859-1", + disposition: "attachment; filename=#{csv_filename('live_questions_with_add_another_answer_report')}" + else + render template: "reports/questions_with_add_another_answer", locals: { questions: } + end end def forms_with_routes forms = Reports::FormDocumentsService.live_form_documents forms = Reports::FeatureReportService.new(forms).forms_with_routes - render template: "reports/forms_with_routes", locals: { forms: forms } + if params[:format] == "csv" + send_data Reports::FormsCsvReportService.new(forms).forms_csv, + type: "text/csv; charset=iso-8859-1", + disposition: "attachment; filename=#{csv_filename('live_forms_with_routes_report')}" + else + render template: "reports/forms_with_routes", locals: { forms: forms } + end end def forms_with_payments forms = Reports::FormDocumentsService.live_form_documents forms = Reports::FeatureReportService.new(forms).forms_with_payments - render template: "reports/forms_with_payments", locals: { forms: forms } + if params[:format] == "csv" + send_data Reports::FormsCsvReportService.new(forms).forms_csv, + type: "text/csv; charset=iso-8859-1", + disposition: "attachment; filename=#{csv_filename('live_forms_with_payments_report')}" + else + render template: "reports/forms_with_payments", locals: { forms: forms } + end end def forms_with_csv_submission_enabled forms = Reports::FormDocumentsService.live_form_documents forms = Reports::FeatureReportService.new(forms).forms_with_csv_submission_enabled - render template: "reports/forms_with_csv_submission_enabled", locals: { forms: forms } + if params[:format] == "csv" + send_data Reports::FormsCsvReportService.new(forms).forms_csv, + type: "text/csv; charset=iso-8859-1", + disposition: "attachment; filename=#{csv_filename('live_forms_with_csv_submission_enabled_report')}" + else + render template: "reports/forms_with_csv_submission_enabled", locals: { forms: forms } + end end def users @@ -95,33 +119,6 @@ def live_forms_csv disposition: "attachment; filename=#{csv_filename('live_forms_report')}" end - def live_forms_with_routes_csv - forms = Reports::FormDocumentsService.live_form_documents - forms = Reports::FeatureReportService.new(forms).forms_with_routes - - send_data Reports::FormsCsvReportService.new(forms).forms_csv, - type: "text/csv; charset=iso-8859-1", - disposition: "attachment; filename=#{csv_filename('live_forms_with_routes_report')}" - end - - def live_forms_with_payments_csv - forms = Reports::FormDocumentsService.live_form_documents - forms = Reports::FeatureReportService.new(forms).forms_with_payments - - send_data Reports::FormsCsvReportService.new(forms).forms_csv, - type: "text/csv; charset=iso-8859-1", - disposition: "attachment; filename=#{csv_filename('live_forms_with_payments_report')}" - end - - def live_forms_with_csv_submission_enabled_csv - forms = Reports::FormDocumentsService.live_form_documents - forms = Reports::FeatureReportService.new(forms).forms_with_csv_submission_enabled - - send_data Reports::FormsCsvReportService.new(forms).forms_csv, - type: "text/csv; charset=iso-8859-1", - disposition: "attachment; filename=#{csv_filename('live_forms_with_csv_submission_enabled_report')}" - end - def live_questions_csv answer_type = params[:answer_type] forms = Reports::FormDocumentsService.live_form_documents @@ -136,15 +133,6 @@ def live_questions_csv disposition: "attachment; filename=#{questions_csv_filename(answer_type)}" end - def live_questions_with_add_another_answer_csv - forms = Reports::FormDocumentsService.live_form_documents - questions = Reports::FeatureReportService.new(forms).questions_with_add_another_answer - - send_data Reports::QuestionsCsvReportService.new(questions).questions_csv, - type: "text/csv; charset=iso-8859-1", - disposition: "attachment; filename=#{csv_filename('live_questions_with_add_another_answer_report')}" - end - private def check_user_has_permission diff --git a/app/views/reports/forms_with_csv_submission_enabled.html.erb b/app/views/reports/forms_with_csv_submission_enabled.html.erb index 9c1708d2b..16aed1959 100644 --- a/app/views/reports/forms_with_csv_submission_enabled.html.erb +++ b/app/views/reports/forms_with_csv_submission_enabled.html.erb @@ -4,7 +4,7 @@

<%= t(".heading")%>

-

<%=govuk_link_to(t(".download_csv"), report_live_forms_with_csv_submission_enabled_csv_path)%>

+

<%=govuk_link_to(t(".download_csv"), report_forms_with_csv_submission_enabled_path(format: :csv))%>

diff --git a/app/views/reports/forms_with_payments.html.erb b/app/views/reports/forms_with_payments.html.erb index f63563725..0fa3ade0b 100644 --- a/app/views/reports/forms_with_payments.html.erb +++ b/app/views/reports/forms_with_payments.html.erb @@ -4,7 +4,7 @@

<%= t(".heading")%>

-

<%=govuk_link_to(t(".download_csv"), report_live_forms_with_payments_csv_path)%>

+

<%=govuk_link_to(t(".download_csv"), report_forms_with_payments_path(format: :csv))%>

diff --git a/app/views/reports/forms_with_routes.html.erb b/app/views/reports/forms_with_routes.html.erb index 09a48bed8..2bf031ff7 100644 --- a/app/views/reports/forms_with_routes.html.erb +++ b/app/views/reports/forms_with_routes.html.erb @@ -4,7 +4,7 @@

<%= t(".heading")%>

-

<%=govuk_link_to(t(".download_csv"), report_live_forms_with_routes_csv_path)%>

+

<%=govuk_link_to(t(".download_csv"), report_forms_with_routes_path(format: :csv))%>

diff --git a/app/views/reports/questions_with_add_another_answer.html.erb b/app/views/reports/questions_with_add_another_answer.html.erb index 28a00a5ab..f09ec58f0 100644 --- a/app/views/reports/questions_with_add_another_answer.html.erb +++ b/app/views/reports/questions_with_add_another_answer.html.erb @@ -4,7 +4,7 @@

<%= t(".heading")%>

-

<%=govuk_link_to(t(".download_csv"), report_live_questions_with_add_another_answer_csv_path)%>

+

<%=govuk_link_to(t(".download_csv"), report_questions_with_add_another_answer_path(format: :csv))%>

diff --git a/config/routes.rb b/config/routes.rb index be37ede29..f5733054a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -198,14 +198,10 @@ scope "/features" do get "/", to: "reports#features", as: :report_features get "questions-with-answer-type/:answer_type", to: "reports#questions_with_answer_type", as: :report_questions_with_answer_type - get "questions-with-add-another-answer", format: false, to: "reports#questions_with_add_another_answer", as: :report_questions_with_add_another_answer - get "questions-with-add-another-answer.csv", format: :csv, to: "reports#live_questions_with_add_another_answer_csv", as: :report_live_questions_with_add_another_answer_csv - get "forms-with-routes", format: false, to: "reports#forms_with_routes", as: :report_forms_with_routes - get "forms-with-routes.csv", format: :csv, to: "reports#live_forms_with_routes_csv", as: :report_live_forms_with_routes_csv - get "forms-with-payments", format: false, to: "reports#forms_with_payments", as: :report_forms_with_payments - get "forms-with-payments.csv", format: :csv, to: "reports#live_forms_with_payments_csv", as: :report_live_forms_with_payments_csv - get "forms-with-csv-submission-enabled", format: false, to: "reports#forms_with_csv_submission_enabled", as: :report_forms_with_csv_submission_enabled - get "forms-with-csv-submission-enabled.csv", format: :csv, to: "reports#live_forms_with_csv_submission_enabled_csv", as: :report_live_forms_with_csv_submission_enabled_csv + get "questions-with-add-another-answer", to: "reports#questions_with_add_another_answer", as: :report_questions_with_add_another_answer + get "forms-with-routes", to: "reports#forms_with_routes", as: :report_forms_with_routes + get "forms-with-payments", to: "reports#forms_with_payments", as: :report_forms_with_payments + get "forms-with-csv-submission-enabled", to: "reports#forms_with_csv_submission_enabled", as: :report_forms_with_csv_submission_enabled end get "users", to: "reports#users", as: :report_users diff --git a/spec/requests/reports_controller_spec.rb b/spec/requests/reports_controller_spec.rb index 78912417a..06b205c7a 100644 --- a/spec/requests/reports_controller_spec.rb +++ b/spec/requests/reports_controller_spec.rb @@ -730,13 +730,13 @@ end end - describe "#live_forms_with_routes_csv" do + describe "#forms_with_routes as csv" do before do login_as_super_admin_user travel_to Time.utc(2025, 5, 15, 15, 31, 57) - get report_live_forms_with_routes_csv_path + get report_forms_with_routes_path(format: :csv) end it_behaves_like "csv response" @@ -756,13 +756,13 @@ end end - describe "#live_forms_with_payments_csv" do + describe "#forms_with_payments as csv" do before do login_as_super_admin_user travel_to Time.utc(2025, 5, 15, 15, 31, 57) - get report_live_forms_with_payments_csv_path + get report_forms_with_payments_path(format: :csv) end it_behaves_like "csv response" @@ -781,13 +781,13 @@ end end - describe "#live_forms_with_csv_submission_enabled_csv" do + describe "#forms_with_csv_submission_enabled as csv" do before do login_as_super_admin_user travel_to Time.utc(2025, 5, 15, 15, 31, 57) - get report_live_forms_with_csv_submission_enabled_csv_path + get report_forms_with_csv_submission_enabled_path(format: :csv) end it_behaves_like "csv response" @@ -828,13 +828,13 @@ end end - describe "#live_questions_with_add_another_answer_csv" do + describe "#questions_with_add_another_answer as csv" do before do login_as_super_admin_user travel_to Time.utc(2025, 5, 15, 15, 31, 57) - get report_live_questions_with_add_another_answer_csv_path + get report_questions_with_add_another_answer_path(format: :csv) end it_behaves_like "csv response" diff --git a/spec/routing/reports_routing_spec.rb b/spec/routing/reports_routing_spec.rb index 516e72356..c2aad0ddd 100644 --- a/spec/routing/reports_routing_spec.rb +++ b/spec/routing/reports_routing_spec.rb @@ -15,50 +15,50 @@ expect(get: "/reports/features/questions-with-add-another-answer").to route_to("reports#questions_with_add_another_answer") end - it "routes to #live_questions_with_add_another_answer_csv" do - expect(get: "/reports/features/questions-with-add-another-answer.csv").to route_to("reports#live_questions_with_add_another_answer_csv") + it "routes to #questions_with_add_another_answer with csv format" do + expect(get: "/reports/features/questions-with-add-another-answer.csv").to route_to("reports#questions_with_add_another_answer", format: "csv") end it "routes to #forms_with_routes" do expect(get: "/reports/features/forms-with-routes").to route_to("reports#forms_with_routes") end - it "routes to #live_forms_with_routes_csv" do - expect(get: "/reports/features/forms-with-routes.csv").to route_to("reports#live_forms_with_routes_csv") + it "routes to #forms_with_routes with csv format" do + expect(get: "/reports/features/forms-with-routes.csv").to route_to("reports#forms_with_routes", format: "csv") end it "routes to #forms_with_payments" do expect(get: "/reports/features/forms-with-payments").to route_to("reports#forms_with_payments") end - it "routes to #live_forms_with_payments_csv" do - expect(get: "/reports/features/forms-with-payments.csv").to route_to("reports#live_forms_with_payments_csv") + it "routes to #forms_with_payments with csv format" do + expect(get: "/reports/features/forms-with-payments.csv").to route_to("reports#forms_with_payments", format: "csv") end it "routes to #forms_with_csv_submission_enabled" do expect(get: "/reports/features/forms-with-csv-submission-enabled").to route_to("reports#forms_with_csv_submission_enabled") end - it "routes to #live_forms_with_csv_submission_enabled_csv" do - expect(get: "/reports/features/forms-with-csv-submission-enabled.csv").to route_to("reports#live_forms_with_csv_submission_enabled_csv") + it "routes to #forms_with_csv_submission_enabled with csv format" do + expect(get: "/reports/features/forms-with-csv-submission-enabled.csv").to route_to("reports#forms_with_csv_submission_enabled", format: "csv") end end describe "path helpers" do - it "routes to #live_questions_with_add_another_answer_csv" do - expect(get: report_live_questions_with_add_another_answer_csv_path).to route_to("reports#live_questions_with_add_another_answer_csv") + it "routes to #questions_with_add_another_answer as csv" do + expect(get: report_questions_with_add_another_answer_path(format: :csv)).to route_to("reports#questions_with_add_another_answer", format: "csv") end - it "routes to #live_forms_with_routes_csv" do - expect(get: report_live_forms_with_routes_csv_path).to route_to("reports#live_forms_with_routes_csv") + it "routes to #forms_with_routes as csv" do + expect(get: report_forms_with_routes_path(format: :csv)).to route_to("reports#forms_with_routes", format: "csv") end - it "routes to #live_forms_with_payments_csv" do - expect(get: report_live_forms_with_payments_csv_path).to route_to("reports#live_forms_with_payments_csv") + it "routes to #forms_with_payments as csv" do + expect(get: report_forms_with_payments_path(format: :csv)).to route_to("reports#forms_with_payments", format: "csv") end - it "routes to #live_forms_with_csv_submission_enabled_csv" do - expect(get: report_live_forms_with_csv_submission_enabled_csv_path).to route_to("reports#live_forms_with_csv_submission_enabled_csv") + it "routes to #forms_with_csv_submission_enabled as csv" do + expect(get: report_forms_with_csv_submission_enabled_path(format: :csv)).to route_to("reports#forms_with_csv_submission_enabled", format: "csv") end end end diff --git a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb index 2777ad81e..8fd0b3605 100644 --- a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb +++ b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb @@ -23,7 +23,7 @@ end it "has a link to download the CSV" do - expect(rendered).to have_link("Download data about all live forms with CSV submission enabled as a CSV file", href: report_live_forms_with_csv_submission_enabled_csv_path) + expect(rendered).to have_link("Download data about all live forms with CSV submission enabled as a CSV file", href: report_forms_with_csv_submission_enabled_path(format: :csv)) end describe "questions table" do diff --git a/spec/views/reports/forms_with_payments.html.erb_spec.rb b/spec/views/reports/forms_with_payments.html.erb_spec.rb index 791382b18..2837eb240 100644 --- a/spec/views/reports/forms_with_payments.html.erb_spec.rb +++ b/spec/views/reports/forms_with_payments.html.erb_spec.rb @@ -23,7 +23,7 @@ end it "has a link to download the CSV" do - expect(rendered).to have_link("Download data about all live forms with payments as a CSV file", href: report_live_forms_with_payments_csv_path) + expect(rendered).to have_link("Download data about all live forms with payments as a CSV file", href: report_forms_with_payments_path(format: :csv)) end describe "questions table" do diff --git a/spec/views/reports/forms_with_routes.html.erb_spec.rb b/spec/views/reports/forms_with_routes.html.erb_spec.rb index 55f0ff121..0bc1174ec 100644 --- a/spec/views/reports/forms_with_routes.html.erb_spec.rb +++ b/spec/views/reports/forms_with_routes.html.erb_spec.rb @@ -23,7 +23,7 @@ end it "has a link to download the CSV" do - expect(rendered).to have_link("Download data about all live forms with routes as a CSV file", href: report_live_forms_with_routes_csv_path) + expect(rendered).to have_link("Download data about all live forms with routes as a CSV file", href: report_forms_with_routes_path(format: :csv)) end describe "questions table" do diff --git a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb index f94038789..1e9fabf74 100644 --- a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb +++ b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb @@ -23,7 +23,7 @@ end it "has a link to download the CSV" do - expect(rendered).to have_link("Download all questions with add another answer in live forms as a CSV file", href: report_live_questions_with_add_another_answer_csv_path) + expect(rendered).to have_link("Download all questions with add another answer in live forms as a CSV file", href: report_questions_with_add_another_answer_path(format: :csv)) end describe "questions table" do From 105b68ed384618cd9e916b9d589eb4ba52ff5a2f Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 16 May 2025 16:06:09 +0300 Subject: [PATCH 12/15] Refactor templates for feature reports views Change the template for each feature report to get the translation keys using a local, rather than the conventional Rails shorthand. The advantage of this is that the templates all start to look the same... --- app/controllers/reports_controller.rb | 8 ++++---- .../reports/forms_with_csv_submission_enabled.html.erb | 6 +++--- app/views/reports/forms_with_payments.html.erb | 6 +++--- app/views/reports/forms_with_routes.html.erb | 6 +++--- .../reports/questions_with_add_another_answer.html.erb | 6 +++--- config/i18n-tasks.yml | 2 ++ .../forms_with_csv_submission_enabled.html.erb_spec.rb | 4 +++- spec/views/reports/forms_with_payments.html.erb_spec.rb | 4 +++- spec/views/reports/forms_with_routes.html.erb_spec.rb | 4 +++- .../questions_with_add_another_answer.html.erb_spec.rb | 4 +++- 10 files changed, 30 insertions(+), 20 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index d04047bb8..65f719dc1 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -28,7 +28,7 @@ def questions_with_add_another_answer type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_questions_with_add_another_answer_report')}" else - render template: "reports/questions_with_add_another_answer", locals: { questions: } + render template: "reports/questions_with_add_another_answer", locals: { report: params[:action], questions: } end end @@ -41,7 +41,7 @@ def forms_with_routes type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_routes_report')}" else - render template: "reports/forms_with_routes", locals: { forms: forms } + render template: "reports/forms_with_routes", locals: { report: params[:action], forms: } end end @@ -54,7 +54,7 @@ def forms_with_payments type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_payments_report')}" else - render template: "reports/forms_with_payments", locals: { forms: forms } + render template: "reports/forms_with_payments", locals: { report: params[:action], forms: } end end @@ -67,7 +67,7 @@ def forms_with_csv_submission_enabled type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_csv_submission_enabled_report')}" else - render template: "reports/forms_with_csv_submission_enabled", locals: { forms: forms } + render template: "reports/forms_with_csv_submission_enabled", locals: { report: params[:action], forms: } end end diff --git a/app/views/reports/forms_with_csv_submission_enabled.html.erb b/app/views/reports/forms_with_csv_submission_enabled.html.erb index 16aed1959..450490e3e 100644 --- a/app/views/reports/forms_with_csv_submission_enabled.html.erb +++ b/app/views/reports/forms_with_csv_submission_enabled.html.erb @@ -1,10 +1,10 @@ -<% set_page_title(t(".heading"))%> +<% set_page_title(t("reports.#{report}.heading"))%> <% content_for :back_link, govuk_back_link_to(report_features_path, t("reports.back_to_feature_usage")) %>
-

<%= t(".heading")%>

+

<%= t("reports.#{report}.heading")%>

-

<%=govuk_link_to(t(".download_csv"), report_forms_with_csv_submission_enabled_path(format: :csv))%>

+

<%=govuk_link_to(t("reports.#{report}.download_csv"), url_for(format: :csv))%>

diff --git a/app/views/reports/forms_with_payments.html.erb b/app/views/reports/forms_with_payments.html.erb index 0fa3ade0b..450490e3e 100644 --- a/app/views/reports/forms_with_payments.html.erb +++ b/app/views/reports/forms_with_payments.html.erb @@ -1,10 +1,10 @@ -<% set_page_title(t(".heading"))%> +<% set_page_title(t("reports.#{report}.heading"))%> <% content_for :back_link, govuk_back_link_to(report_features_path, t("reports.back_to_feature_usage")) %>
-

<%= t(".heading")%>

+

<%= t("reports.#{report}.heading")%>

-

<%=govuk_link_to(t(".download_csv"), report_forms_with_payments_path(format: :csv))%>

+

<%=govuk_link_to(t("reports.#{report}.download_csv"), url_for(format: :csv))%>

diff --git a/app/views/reports/forms_with_routes.html.erb b/app/views/reports/forms_with_routes.html.erb index 2bf031ff7..450490e3e 100644 --- a/app/views/reports/forms_with_routes.html.erb +++ b/app/views/reports/forms_with_routes.html.erb @@ -1,10 +1,10 @@ -<% set_page_title(t(".heading"))%> +<% set_page_title(t("reports.#{report}.heading"))%> <% content_for :back_link, govuk_back_link_to(report_features_path, t("reports.back_to_feature_usage")) %>
-

<%= t(".heading")%>

+

<%= t("reports.#{report}.heading")%>

-

<%=govuk_link_to(t(".download_csv"), report_forms_with_routes_path(format: :csv))%>

+

<%=govuk_link_to(t("reports.#{report}.download_csv"), url_for(format: :csv))%>

diff --git a/app/views/reports/questions_with_add_another_answer.html.erb b/app/views/reports/questions_with_add_another_answer.html.erb index f09ec58f0..e12c65cae 100644 --- a/app/views/reports/questions_with_add_another_answer.html.erb +++ b/app/views/reports/questions_with_add_another_answer.html.erb @@ -1,10 +1,10 @@ -<% set_page_title(t(".heading"))%> +<% set_page_title(t("reports.#{report}.heading"))%> <% content_for :back_link, govuk_back_link_to(report_features_path, t("reports.back_to_feature_usage")) %>
-

<%= t(".heading")%>

+

<%= t("reports.#{report}.heading")%>

-

<%=govuk_link_to(t(".download_csv"), report_questions_with_add_another_answer_path(format: :csv))%>

+

<%=govuk_link_to(t("reports.#{report}.download_csv"), url_for(format: :csv))%>

diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index 6ba0ecc64..6e04a5680 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -132,6 +132,8 @@ ignore_unused: - 'errors.*' - 'date.formats.*' - 'helpers.*' +- 'reports.*.heading' +- 'reports.*.download_csv' - 'routing_page.{branch_routing,exit_pages}.*' ## Exclude these keys from the `i18n-tasks eq-base' report: diff --git a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb index 8fd0b3605..f83e33a00 100644 --- a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb +++ b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" describe "reports/forms_with_csv_submission_enabled" do + let(:report) { controller.request.path_parameters[:action] } + let(:forms) do [ { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, @@ -9,7 +11,7 @@ end before do - render locals: { forms: } + render locals: { report:, forms: } end describe "page title" do diff --git a/spec/views/reports/forms_with_payments.html.erb_spec.rb b/spec/views/reports/forms_with_payments.html.erb_spec.rb index 2837eb240..8310c01e6 100644 --- a/spec/views/reports/forms_with_payments.html.erb_spec.rb +++ b/spec/views/reports/forms_with_payments.html.erb_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" describe "reports/forms_with_payments" do + let(:report) { controller.request.path_parameters[:action] } + let(:forms) do [ { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, @@ -9,7 +11,7 @@ end before do - render locals: { forms: } + render locals: { report:, forms: } end describe "page title" do diff --git a/spec/views/reports/forms_with_routes.html.erb_spec.rb b/spec/views/reports/forms_with_routes.html.erb_spec.rb index 0bc1174ec..917ba4197 100644 --- a/spec/views/reports/forms_with_routes.html.erb_spec.rb +++ b/spec/views/reports/forms_with_routes.html.erb_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" describe "reports/forms_with_routes" do + let(:report) { controller.request.path_parameters[:action] } + let(:forms) do [ { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } }, "metadata" => { "number_of_routes" => 1 } }, @@ -9,7 +11,7 @@ end before do - render locals: { forms: } + render locals: { report:, forms: } end describe "page title" do diff --git a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb index 1e9fabf74..f318a431e 100644 --- a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb +++ b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" describe "reports/questions_with_add_another_answer" do + let(:report) { controller.request.path_parameters[:action] } + let(:questions) do [ { "type" => "question_page", "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, @@ -9,7 +11,7 @@ end before do - render locals: { questions: } + render locals: { report:, questions: } end describe "page title" do From 1407ca98fb06742722ac1082ed724bfac62dbb8a Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Mon, 19 May 2025 10:18:18 +0300 Subject: [PATCH 13/15] Combine templates for feature reports views Now that the templates all look the same for routes for feature reports, we can use the same one template for all routes. This means a lot less work needed for adding new feature reports in future. --- app/controllers/reports_controller.rb | 8 +- ...abled.html.erb => feature_report.html.erb} | 2 +- .../reports/forms_with_payments.html.erb | 13 -- app/views/reports/forms_with_routes.html.erb | 13 -- ...questions_with_add_another_answer.html.erb | 13 -- spec/requests/reports_controller_spec.rb | 8 +- .../reports/feature_report.html.erb_spec.rb | 202 ++++++++++++++++++ ...th_csv_submission_enabled.html.erb_spec.rb | 52 ----- .../forms_with_payments.html.erb_spec.rb | 52 ----- .../forms_with_routes.html.erb_spec.rb | 55 ----- ...s_with_add_another_answer.html.erb_spec.rb | 55 ----- 11 files changed, 211 insertions(+), 262 deletions(-) rename app/views/reports/{forms_with_csv_submission_enabled.html.erb => feature_report.html.erb} (90%) delete mode 100644 app/views/reports/forms_with_payments.html.erb delete mode 100644 app/views/reports/forms_with_routes.html.erb delete mode 100644 app/views/reports/questions_with_add_another_answer.html.erb create mode 100644 spec/views/reports/feature_report.html.erb_spec.rb delete mode 100644 spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb delete mode 100644 spec/views/reports/forms_with_payments.html.erb_spec.rb delete mode 100644 spec/views/reports/forms_with_routes.html.erb_spec.rb delete mode 100644 spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 65f719dc1..af2432fda 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -28,7 +28,7 @@ def questions_with_add_another_answer type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_questions_with_add_another_answer_report')}" else - render template: "reports/questions_with_add_another_answer", locals: { report: params[:action], questions: } + render template: "reports/feature_report", locals: { report: params[:action], records: questions } end end @@ -41,7 +41,7 @@ def forms_with_routes type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_routes_report')}" else - render template: "reports/forms_with_routes", locals: { report: params[:action], forms: } + render template: "reports/feature_report", locals: { report: params[:action], records: forms } end end @@ -54,7 +54,7 @@ def forms_with_payments type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_payments_report')}" else - render template: "reports/forms_with_payments", locals: { report: params[:action], forms: } + render template: "reports/feature_report", locals: { report: params[:action], records: forms } end end @@ -67,7 +67,7 @@ def forms_with_csv_submission_enabled type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_csv_submission_enabled_report')}" else - render template: "reports/forms_with_csv_submission_enabled", locals: { report: params[:action], forms: } + render template: "reports/feature_report", locals: { report: params[:action], records: forms } end end diff --git a/app/views/reports/forms_with_csv_submission_enabled.html.erb b/app/views/reports/feature_report.html.erb similarity index 90% rename from app/views/reports/forms_with_csv_submission_enabled.html.erb rename to app/views/reports/feature_report.html.erb index 450490e3e..f71186f24 100644 --- a/app/views/reports/forms_with_csv_submission_enabled.html.erb +++ b/app/views/reports/feature_report.html.erb @@ -8,6 +8,6 @@
- <%= govuk_table(**report_table(forms)) %> + <%= govuk_table(**report_table(records)) %>
diff --git a/app/views/reports/forms_with_payments.html.erb b/app/views/reports/forms_with_payments.html.erb deleted file mode 100644 index 450490e3e..000000000 --- a/app/views/reports/forms_with_payments.html.erb +++ /dev/null @@ -1,13 +0,0 @@ -<% set_page_title(t("reports.#{report}.heading"))%> -<% content_for :back_link, govuk_back_link_to(report_features_path, t("reports.back_to_feature_usage")) %> -
-
-

<%= t("reports.#{report}.heading")%>

- -

<%=govuk_link_to(t("reports.#{report}.download_csv"), url_for(format: :csv))%>

-
- -
- <%= govuk_table(**report_table(forms)) %> -
-
diff --git a/app/views/reports/forms_with_routes.html.erb b/app/views/reports/forms_with_routes.html.erb deleted file mode 100644 index 450490e3e..000000000 --- a/app/views/reports/forms_with_routes.html.erb +++ /dev/null @@ -1,13 +0,0 @@ -<% set_page_title(t("reports.#{report}.heading"))%> -<% content_for :back_link, govuk_back_link_to(report_features_path, t("reports.back_to_feature_usage")) %> -
-
-

<%= t("reports.#{report}.heading")%>

- -

<%=govuk_link_to(t("reports.#{report}.download_csv"), url_for(format: :csv))%>

-
- -
- <%= govuk_table(**report_table(forms)) %> -
-
diff --git a/app/views/reports/questions_with_add_another_answer.html.erb b/app/views/reports/questions_with_add_another_answer.html.erb deleted file mode 100644 index e12c65cae..000000000 --- a/app/views/reports/questions_with_add_another_answer.html.erb +++ /dev/null @@ -1,13 +0,0 @@ -<% set_page_title(t("reports.#{report}.heading"))%> -<% content_for :back_link, govuk_back_link_to(report_features_path, t("reports.back_to_feature_usage")) %> -
-
-

<%= t("reports.#{report}.heading")%>

- -

<%=govuk_link_to(t("reports.#{report}.download_csv"), url_for(format: :csv))%>

-
- -
- <%= govuk_table(**report_table(questions)) %> -
-
diff --git a/spec/requests/reports_controller_spec.rb b/spec/requests/reports_controller_spec.rb index 06b205c7a..68321ff9e 100644 --- a/spec/requests/reports_controller_spec.rb +++ b/spec/requests/reports_controller_spec.rb @@ -233,7 +233,7 @@ end it "renders the features report view" do - expect(response).to render_template("reports/questions_with_add_another_answer") + expect(response).to render_template("reports/feature_report") end it "includes the report data" do @@ -291,7 +291,7 @@ end it "renders the features report view" do - expect(response).to render_template("reports/forms_with_routes") + expect(response).to render_template("reports/feature_report") end it "includes the report data" do @@ -349,7 +349,7 @@ end it "renders the features report view" do - expect(response).to render_template("reports/forms_with_payments") + expect(response).to render_template("reports/feature_report") end it "includes the report data" do @@ -407,7 +407,7 @@ end it "renders the features report view" do - expect(response).to render_template("reports/forms_with_csv_submission_enabled") + expect(response).to render_template("reports/feature_report") end it "includes the report data" do diff --git a/spec/views/reports/feature_report.html.erb_spec.rb b/spec/views/reports/feature_report.html.erb_spec.rb new file mode 100644 index 000000000..9d8cd48ca --- /dev/null +++ b/spec/views/reports/feature_report.html.erb_spec.rb @@ -0,0 +1,202 @@ +require "rails_helper" + +describe "reports/feature_report" do + let(:report) {} + let(:records) { [] } + + before do + controller.request.path_parameters[:action] = report + + render locals: { report:, records: } + end + + context "with forms_with_csv_submission_enabled report" do + let(:report) { "forms_with_csv_submission_enabled" } + let(:records) do + [ + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, + ] + end + + describe "page title" do + it "matches the heading" do + expect(view.content_for(:title)).to eq "Live forms with CSV submission enabled" + end + end + + it "has a back link to feature usage report" do + expect(view.content_for(:back_link)).to have_link("Back to feature and answer type usage", href: report_features_path) + end + + it "has a link to download the CSV" do + expect(rendered).to have_link("Download data about all live forms with CSV submission enabled as a CSV file", href: report_forms_with_csv_submission_enabled_path(format: :csv)) + end + + describe "questions table" do + it "has the correct headers" do + page = Capybara.string(rendered.html) + within(page.find(".govuk-table__head")) do + expect(page.find_all(".govuk-table__header"[0])).to have_text "Form name" + expect(page.find_all(".govuk-table__header"[1])).to have_text "Organisation" + end + end + + it "has rows for each question" do + page = Capybara.string(rendered.html) + within(page.find_all(".govuk-table__row")[1]) do + expect(page.find_all(".govuk-table__cell"[0])).to have_text "All question types form" + expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" + end + within(page.find_all(".govuk-table__row")[2]) do + expect(page.find_all(".govuk-table__cell"[0])).to have_text "Branch route form" + expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" + end + end + end + end + + context "with forms_with_payments report" do + let(:report) { "forms_with_payments" } + let(:records) do + [ + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, + ] + end + + describe "page title" do + it "matches the heading" do + expect(view.content_for(:title)).to eq "Live forms with payments" + end + end + + it "has a back link to feature usage report" do + expect(view.content_for(:back_link)).to have_link("Back to feature and answer type usage", href: report_features_path) + end + + it "has a link to download the CSV" do + expect(rendered).to have_link("Download data about all live forms with payments as a CSV file", href: report_forms_with_payments_path(format: :csv)) + end + + describe "questions table" do + it "has the correct headers" do + page = Capybara.string(rendered.html) + within(page.find(".govuk-table__head")) do + expect(page.find_all(".govuk-table__header"[0])).to have_text "Form name" + expect(page.find_all(".govuk-table__header"[1])).to have_text "Organisation" + end + end + + it "has rows for each question" do + page = Capybara.string(rendered.html) + within(page.find_all(".govuk-table__row")[1]) do + expect(page.find_all(".govuk-table__cell"[0])).to have_text "All question types form" + expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" + end + within(page.find_all(".govuk-table__row")[2]) do + expect(page.find_all(".govuk-table__cell"[0])).to have_text "Branch route form" + expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" + end + end + end + end + + context "with forms_with_routes report" do + let(:report) { "forms_with_routes" } + let(:records) do + [ + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } }, "metadata" => { "number_of_routes" => 1 } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } }, "metadata" => { "number_of_routes" => 2 } }, + ] + end + + describe "page title" do + it "matches the heading" do + expect(view.content_for(:title)).to eq "Live forms with routes" + end + end + + it "has a back link to feature usage report" do + expect(view.content_for(:back_link)).to have_link("Back to feature and answer type usage", href: report_features_path) + end + + it "has a link to download the CSV" do + expect(rendered).to have_link("Download data about all live forms with routes as a CSV file", href: report_forms_with_routes_path(format: :csv)) + end + + describe "questions table" do + it "has the correct headers" do + page = Capybara.string(rendered.html) + within(page.find(".govuk-table__head")) do + expect(page.find_all(".govuk-table__header"[0])).to have_text "Form name" + expect(page.find_all(".govuk-table__header"[1])).to have_text "Organisation" + expect(page.find_all(".govuk-table__header"[2])).to have_text "Number of routes" + end + end + + it "has rows for each question" do + page = Capybara.string(rendered.html) + within(page.find_all(".govuk-table__row")[1]) do + expect(page.find_all(".govuk-table__cell"[0])).to have_text "All question types form" + expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" + expect(page.find_all(".govuk-table__cell"[2])).to have_text "1" + end + within(page.find_all(".govuk-table__row")[2]) do + expect(page.find_all(".govuk-table__cell"[0])).to have_text "Branch route form" + expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" + expect(page.find_all(".govuk-table__cell"[2])).to have_text "2" + end + end + end + end + + context "with questions_with_add_another_answer report" do + let(:report) { "questions_with_add_another_answer" } + let(:records) do + [ + { "type" => "question_page", "data" => { "question_text" => "email address" }, "form" => { "form_id" => 1, "content" => { "name" => "all question types form" }, "group" => { "organisation" => { "name" => "government digital service" } } } }, + { "type" => "question_page", "data" => { "question_text" => "what’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "branch route form" }, "group" => { "organisation" => { "name" => "government digital service" } } } }, + ] + end + + describe "page title" do + it "matches the heading" do + expect(view.content_for(:title)).to eq "Questions with add another answer in live forms" + end + end + + it "has a back link to feature usage report" do + expect(view.content_for(:back_link)).to have_link("Back to feature and answer type usage", href: report_features_path) + end + + it "has a link to download the CSV" do + expect(rendered).to have_link("Download all questions with add another answer in live forms as a CSV file", href: report_questions_with_add_another_answer_path(format: :csv)) + end + + describe "questions table" do + it "has the correct headers" do + page = Capybara.string(rendered.html) + within(page.find(".govuk-table__head")) do + expect(page.find_all(".govuk-table__header"[0])).to have_text "Form name" + expect(page.find_all(".govuk-table__header"[1])).to have_text "Organisation" + expect(page.find_all(".govuk-table__header"[2])).to have_text "Question text" + end + end + + it "has rows for each question" do + page = Capybara.string(rendered.html) + within(page.find_all(".govuk-table__row")[1]) do + expect(page.find_all(".govuk-table__cell"[0])).to have_text "All question types form" + expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" + expect(page.find_all(".govuk-table__cell"[2])).to have_text "Email address" + end + within(page.find_all(".govuk-table__row")[2]) do + expect(page.find_all(".govuk-table__cell"[0])).to have_text "Branch route form" + expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" + expect(page.find_all(".govuk-table__cell"[2])).to have_text "What's your email address?" + end + end + end + end +end diff --git a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb b/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb deleted file mode 100644 index f83e33a00..000000000 --- a/spec/views/reports/forms_with_csv_submission_enabled.html.erb_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -require "rails_helper" - -describe "reports/forms_with_csv_submission_enabled" do - let(:report) { controller.request.path_parameters[:action] } - - let(:forms) do - [ - { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, - { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, - ] - end - - before do - render locals: { report:, forms: } - end - - describe "page title" do - it "matches the heading" do - expect(view.content_for(:title)).to eq "Live forms with CSV submission enabled" - end - end - - it "has a back link to feature usage report" do - expect(view.content_for(:back_link)).to have_link("Back to feature and answer type usage", href: report_features_path) - end - - it "has a link to download the CSV" do - expect(rendered).to have_link("Download data about all live forms with CSV submission enabled as a CSV file", href: report_forms_with_csv_submission_enabled_path(format: :csv)) - end - - describe "questions table" do - it "has the correct headers" do - page = Capybara.string(rendered.html) - within(page.find(".govuk-table__head")) do - expect(page.find_all(".govuk-table__header"[0])).to have_text "Form name" - expect(page.find_all(".govuk-table__header"[1])).to have_text "Organisation" - end - end - - it "has rows for each question" do - page = Capybara.string(rendered.html) - within(page.find_all(".govuk-table__row")[1]) do - expect(page.find_all(".govuk-table__cell"[0])).to have_text "All question types form" - expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" - end - within(page.find_all(".govuk-table__row")[2]) do - expect(page.find_all(".govuk-table__cell"[0])).to have_text "Branch route form" - expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" - end - end - end -end diff --git a/spec/views/reports/forms_with_payments.html.erb_spec.rb b/spec/views/reports/forms_with_payments.html.erb_spec.rb deleted file mode 100644 index 8310c01e6..000000000 --- a/spec/views/reports/forms_with_payments.html.erb_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -require "rails_helper" - -describe "reports/forms_with_payments" do - let(:report) { controller.request.path_parameters[:action] } - - let(:forms) do - [ - { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, - { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } }, - ] - end - - before do - render locals: { report:, forms: } - end - - describe "page title" do - it "matches the heading" do - expect(view.content_for(:title)).to eq "Live forms with payments" - end - end - - it "has a back link to feature usage report" do - expect(view.content_for(:back_link)).to have_link("Back to feature and answer type usage", href: report_features_path) - end - - it "has a link to download the CSV" do - expect(rendered).to have_link("Download data about all live forms with payments as a CSV file", href: report_forms_with_payments_path(format: :csv)) - end - - describe "questions table" do - it "has the correct headers" do - page = Capybara.string(rendered.html) - within(page.find(".govuk-table__head")) do - expect(page.find_all(".govuk-table__header"[0])).to have_text "Form name" - expect(page.find_all(".govuk-table__header"[1])).to have_text "Organisation" - end - end - - it "has rows for each question" do - page = Capybara.string(rendered.html) - within(page.find_all(".govuk-table__row")[1]) do - expect(page.find_all(".govuk-table__cell"[0])).to have_text "All question types form" - expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" - end - within(page.find_all(".govuk-table__row")[2]) do - expect(page.find_all(".govuk-table__cell"[0])).to have_text "Branch route form" - expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" - end - end - end -end diff --git a/spec/views/reports/forms_with_routes.html.erb_spec.rb b/spec/views/reports/forms_with_routes.html.erb_spec.rb deleted file mode 100644 index 917ba4197..000000000 --- a/spec/views/reports/forms_with_routes.html.erb_spec.rb +++ /dev/null @@ -1,55 +0,0 @@ -require "rails_helper" - -describe "reports/forms_with_routes" do - let(:report) { controller.request.path_parameters[:action] } - - let(:forms) do - [ - { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } }, "metadata" => { "number_of_routes" => 1 } }, - { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } }, "metadata" => { "number_of_routes" => 2 } }, - ] - end - - before do - render locals: { report:, forms: } - end - - describe "page title" do - it "matches the heading" do - expect(view.content_for(:title)).to eq "Live forms with routes" - end - end - - it "has a back link to feature usage report" do - expect(view.content_for(:back_link)).to have_link("Back to feature and answer type usage", href: report_features_path) - end - - it "has a link to download the CSV" do - expect(rendered).to have_link("Download data about all live forms with routes as a CSV file", href: report_forms_with_routes_path(format: :csv)) - end - - describe "questions table" do - it "has the correct headers" do - page = Capybara.string(rendered.html) - within(page.find(".govuk-table__head")) do - expect(page.find_all(".govuk-table__header"[0])).to have_text "Form name" - expect(page.find_all(".govuk-table__header"[1])).to have_text "Organisation" - expect(page.find_all(".govuk-table__header"[2])).to have_text "Number of routes" - end - end - - it "has rows for each question" do - page = Capybara.string(rendered.html) - within(page.find_all(".govuk-table__row")[1]) do - expect(page.find_all(".govuk-table__cell"[0])).to have_text "All question types form" - expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" - expect(page.find_all(".govuk-table__cell"[2])).to have_text "1" - end - within(page.find_all(".govuk-table__row")[2]) do - expect(page.find_all(".govuk-table__cell"[0])).to have_text "Branch route form" - expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" - expect(page.find_all(".govuk-table__cell"[2])).to have_text "2" - end - end - end -end diff --git a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb b/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb deleted file mode 100644 index f318a431e..000000000 --- a/spec/views/reports/questions_with_add_another_answer.html.erb_spec.rb +++ /dev/null @@ -1,55 +0,0 @@ -require "rails_helper" - -describe "reports/questions_with_add_another_answer" do - let(:report) { controller.request.path_parameters[:action] } - - let(:questions) do - [ - { "type" => "question_page", "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, - { "type" => "question_page", "data" => { "question_text" => "What’s your email address?" }, "form" => { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Government Digital Service" } } } }, - ] - end - - before do - render locals: { report:, questions: } - end - - describe "page title" do - it "matches the heading" do - expect(view.content_for(:title)).to eq "Questions with add another answer in live forms" - end - end - - it "has a back link to feature usage report" do - expect(view.content_for(:back_link)).to have_link("Back to feature and answer type usage", href: report_features_path) - end - - it "has a link to download the CSV" do - expect(rendered).to have_link("Download all questions with add another answer in live forms as a CSV file", href: report_questions_with_add_another_answer_path(format: :csv)) - end - - describe "questions table" do - it "has the correct headers" do - page = Capybara.string(rendered.html) - within(page.find(".govuk-table__head")) do - expect(page.find_all(".govuk-table__header"[0])).to have_text "Form name" - expect(page.find_all(".govuk-table__header"[1])).to have_text "Organisation" - expect(page.find_all(".govuk-table__header"[2])).to have_text "Question text" - end - end - - it "has rows for each question" do - page = Capybara.string(rendered.html) - within(page.find_all(".govuk-table__row")[1]) do - expect(page.find_all(".govuk-table__cell"[0])).to have_text "All question types form" - expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" - expect(page.find_all(".govuk-table__cell"[2])).to have_text "Email address" - end - within(page.find_all(".govuk-table__row")[2]) do - expect(page.find_all(".govuk-table__cell"[0])).to have_text "Branch route form" - expect(page.find_all(".govuk-table__cell"[1])).to have_text "Government Digital Service" - expect(page.find_all(".govuk-table__cell"[2])).to have_text "What's your email address?" - end - end - end -end From a726c5dc911899f180911af8fbd66683d2e81914 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Mon, 19 May 2025 10:53:35 +0300 Subject: [PATCH 14/15] Rename csv methods --- app/controllers/reports_controller.rb | 12 ++++++------ app/services/reports/forms_csv_report_service.rb | 2 +- app/services/reports/questions_csv_report_service.rb | 2 +- .../reports/forms_csv_report_service_spec.rb | 6 +++--- .../reports/questions_csv_report_service_spec.rb | 12 ++++++------ 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index af2432fda..f083a4ce7 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -24,7 +24,7 @@ def questions_with_add_another_answer questions = Reports::FeatureReportService.new(forms).questions_with_add_another_answer if params[:format] == "csv" - send_data Reports::QuestionsCsvReportService.new(questions).questions_csv, + send_data Reports::QuestionsCsvReportService.new(questions).csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_questions_with_add_another_answer_report')}" else @@ -37,7 +37,7 @@ def forms_with_routes forms = Reports::FeatureReportService.new(forms).forms_with_routes if params[:format] == "csv" - send_data Reports::FormsCsvReportService.new(forms).forms_csv, + send_data Reports::FormsCsvReportService.new(forms).csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_routes_report')}" else @@ -50,7 +50,7 @@ def forms_with_payments forms = Reports::FeatureReportService.new(forms).forms_with_payments if params[:format] == "csv" - send_data Reports::FormsCsvReportService.new(forms).forms_csv, + send_data Reports::FormsCsvReportService.new(forms).csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_payments_report')}" else @@ -63,7 +63,7 @@ def forms_with_csv_submission_enabled forms = Reports::FeatureReportService.new(forms).forms_with_csv_submission_enabled if params[:format] == "csv" - send_data Reports::FormsCsvReportService.new(forms).forms_csv, + send_data Reports::FormsCsvReportService.new(forms).csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_with_csv_submission_enabled_report')}" else @@ -114,7 +114,7 @@ def csv_downloads; end def live_forms_csv forms = Reports::FormDocumentsService.live_form_documents - send_data Reports::FormsCsvReportService.new(forms).forms_csv, + send_data Reports::FormsCsvReportService.new(forms).csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{csv_filename('live_forms_report')}" end @@ -128,7 +128,7 @@ def live_questions_csv Reports::FeatureReportService.new(forms).questions end - send_data Reports::QuestionsCsvReportService.new(questions).questions_csv, + send_data Reports::QuestionsCsvReportService.new(questions).csv, type: "text/csv; charset=iso-8859-1", disposition: "attachment; filename=#{questions_csv_filename(answer_type)}" end diff --git a/app/services/reports/forms_csv_report_service.rb b/app/services/reports/forms_csv_report_service.rb index bcec44a6a..ec75c74b7 100644 --- a/app/services/reports/forms_csv_report_service.rb +++ b/app/services/reports/forms_csv_report_service.rb @@ -30,7 +30,7 @@ def initialize(form_documents) @form_documents = form_documents end - def forms_csv + def csv CSV.generate do |csv| csv << FORM_CSV_HEADERS diff --git a/app/services/reports/questions_csv_report_service.rb b/app/services/reports/questions_csv_report_service.rb index 8d79e9df1..c1e64e609 100644 --- a/app/services/reports/questions_csv_report_service.rb +++ b/app/services/reports/questions_csv_report_service.rb @@ -34,7 +34,7 @@ def initialize(question_page_documents) @question_page_documents = question_page_documents end - def questions_csv + def csv CSV.generate do |csv| csv << QUESTIONS_CSV_HEADERS diff --git a/spec/services/reports/forms_csv_report_service_spec.rb b/spec/services/reports/forms_csv_report_service_spec.rb index 6c50f2d01..c6eff3072 100644 --- a/spec/services/reports/forms_csv_report_service_spec.rb +++ b/spec/services/reports/forms_csv_report_service_spec.rb @@ -16,15 +16,15 @@ GroupForm.create!(form_id: 4, group:) end - describe "#forms_csv" do + describe "#csv" do it "returns a CSV with a header row and a row for each form" do - csv = csv_reports_service.forms_csv + csv = csv_reports_service.csv rows = CSV.parse(csv) expect(rows.length).to eq 5 end it "has expected values" do - csv = csv_reports_service.forms_csv + csv = csv_reports_service.csv rows = CSV.parse(csv) expect(rows[1]).to eq([ "1", diff --git a/spec/services/reports/questions_csv_report_service_spec.rb b/spec/services/reports/questions_csv_report_service_spec.rb index 69aaa56fc..f6ccf96ab 100644 --- a/spec/services/reports/questions_csv_report_service_spec.rb +++ b/spec/services/reports/questions_csv_report_service_spec.rb @@ -17,15 +17,15 @@ GroupForm.create!(form_id: 4, group:) end - describe "#questions_csv" do + describe "#csv" do it "returns a CSV with a header row and a rows for each question" do - csv = csv_reports_service.questions_csv + csv = csv_reports_service.csv rows = CSV.parse(csv) expect(rows.length).to eq 18 end it "has expected values for text question" do - csv = csv_reports_service.questions_csv + csv = csv_reports_service.csv rows = CSV.parse(csv) text_question_row = rows.detect { |row| row.include? "Single line of text" } expect(text_question_row).to eq([ @@ -54,7 +54,7 @@ end it "has expected values for selection question" do - csv = csv_reports_service.questions_csv + csv = csv_reports_service.csv rows = CSV.parse(csv) selection_question_row = rows.detect { |row| row.include? "Selection from a list of options" } expect(selection_question_row).to eq([ @@ -83,7 +83,7 @@ end it "has expected values for name question" do - csv = csv_reports_service.questions_csv + csv = csv_reports_service.csv rows = CSV.parse(csv) name_question_row = rows.detect { |row| row.include? "What’s your name?" } expect(name_question_row).to eq([ @@ -112,7 +112,7 @@ end it "has expected values for question with routing conditions" do - csv = csv_reports_service.questions_csv + csv = csv_reports_service.csv rows = CSV.parse(csv) routing_question_row = rows.detect { |row| row.include? "How many times have you filled out this form?" } expect(routing_question_row).to eq([ From fdbd38e87fc13e03c415b7b551e6f7fedb0dab23 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Fri, 23 May 2025 10:08:34 +0300 Subject: [PATCH 15/15] Fix organisation name when form is not in group --- app/helpers/report_helper.rb | 2 +- spec/helpers/report_helpers_spec.rb | 42 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/app/helpers/report_helper.rb b/app/helpers/report_helper.rb index b962e629a..d2ddd4abb 100644 --- a/app/helpers/report_helper.rb +++ b/app/helpers/report_helper.rb @@ -72,7 +72,7 @@ def report_questions_table_rows(questions) def report_forms_table_row(form) [ govuk_link_to(form["content"]["name"], live_form_pages_path(form_id: form["form_id"])), - form["group"]["organisation"]["name"], + form.dig("group", "organisation", "name") || "", ] end diff --git a/spec/helpers/report_helpers_spec.rb b/spec/helpers/report_helpers_spec.rb index 8bba94cf9..e5ddc041a 100644 --- a/spec/helpers/report_helpers_spec.rb +++ b/spec/helpers/report_helpers_spec.rb @@ -136,6 +136,20 @@ "Department for Testing", ] end + + context "when form is not in a group" do + let(:forms) do + [ + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => nil }, + ] + end + + it "returns the empty string for the organisation name" do + expect(helper.report_forms_table_rows(forms).map(&:second)).to eq [ + "", + ] + end + end end describe "#report_forms_with_routes_table_head" do @@ -188,6 +202,20 @@ 1 ] end + + context "when form is not in a group" do + let(:forms) do + [ + { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => nil }, + ] + end + + it "returns the empty string for the organisation name" do + expect(helper.report_forms_table_rows(forms).map(&:second)).to eq [ + "", + ] + end + end end describe "#report_questions_table_head" do @@ -228,5 +256,19 @@ "What’s your email address?", ] end + + context "when form is not in a group" do + let(:questions) do + [ + { "type" => "question_page", "data" => { "question_text" => "Email address" }, "form" => { "form_id" => 1, "content" => { "name" => "All question types form" }, "group" => nil } }, + ] + end + + it "returns the empty string for the organisation name" do + expect(helper.report_questions_table_rows(questions).map(&:second)).to eq [ + "", + ] + end + end end end