From c424e9a8026a0a2b7e5bdee3eeb47e00eb44ab4d Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 20 May 2025 14:18:23 +0300 Subject: [PATCH 1/2] Add feature report for forms with branch routes --- app/helpers/report_helper.rb | 2 + .../reports/feature_report_service.rb | 9 ++ .../reports/form_documents_service.rb | 16 ++++ .../reports/forms_csv_report_service.rb | 2 + app/views/reports/features.html.erb | 4 + config/locales/en.yml | 5 ++ spec/helpers/report_helpers_spec.rb | 11 ++- .../reports/feature_report_service_spec.rb | 60 +++++++++++++ .../reports/form_documents_service_spec.rb | 88 ++++++++++++++++++- .../reports/forms_csv_report_service_spec.rb | 1 + 10 files changed, 193 insertions(+), 5 deletions(-) diff --git a/app/helpers/report_helper.rb b/app/helpers/report_helper.rb index d2ddd4abb..02c3f9718 100644 --- a/app/helpers/report_helper.rb +++ b/app/helpers/report_helper.rb @@ -49,6 +49,7 @@ def report_forms_with_routes_table_head [ *report_forms_table_head, I18n.t("reports.form_or_questions_list_table.headings.number_of_routes"), + I18n.t("reports.form_or_questions_list_table.headings.number_of_branch_routes"), ] end @@ -80,6 +81,7 @@ def report_forms_with_routes_table_row(form) [ *report_forms_table_row(form), form["metadata"]["number_of_routes"].to_s, + form["metadata"]["number_of_branch_routes"].to_s, ] end diff --git a/app/services/reports/feature_report_service.rb b/app/services/reports/feature_report_service.rb index 49f1346b0..4c148fe46 100644 --- a/app/services/reports/feature_report_service.rb +++ b/app/services/reports/feature_report_service.rb @@ -36,6 +36,7 @@ def report(name = nil) total_forms: 0, forms_with_payment: 0, forms_with_routing: 0, + forms_with_branch_routing: 0, forms_with_add_another_answer: 0, forms_with_csv_submission_enabled: 0, forms_with_answer_type: HashWithIndifferentAccess.new, @@ -46,6 +47,7 @@ def report(name = nil) 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_branch_routing] += 1 if Reports::FormDocumentsService.has_secondary_skip_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) @@ -94,6 +96,12 @@ def questions_with_answer_type(answer_type) .map { |form| form_with_routes_details(form) } end + define_report :forms_with_branch_routes do + form_documents + .select { |form| Reports::FormDocumentsService.has_secondary_skip_routes?(form) } + .map { |form| form_with_routes_details(form) } + end + define_report :forms_with_payments do form_documents .select { |form| Reports::FormDocumentsService.has_payments?(form) } @@ -117,6 +125,7 @@ def form_with_routes_details(form) form = form_details(form) form["metadata"] = { "number_of_routes" => form["content"]["steps"].count { |step| step["routing_conditions"].present? }, + "number_of_branch_routes" => Reports::FormDocumentsService.count_secondary_skip_routes(form), } form end diff --git a/app/services/reports/form_documents_service.rb b/app/services/reports/form_documents_service.rb index 557c47531..4370be3aa 100644 --- a/app/services/reports/form_documents_service.rb +++ b/app/services/reports/form_documents_service.rb @@ -26,6 +26,14 @@ def has_routes?(form_document) form_document["content"]["steps"].any? { |step| step["routing_conditions"].present? } end + def has_secondary_skip_routes?(form_document) + secondary_skip_conditions(form_document).any? + end + + def count_secondary_skip_routes(form_document) + secondary_skip_conditions(form_document).count + end + def has_payments?(form_document) form_document["content"]["payment_url"].present? end @@ -67,5 +75,13 @@ def is_in_internal_organisation?(form) group_form.group.organisation.internal? end + + def secondary_skip_conditions(form_document) + form_document["content"]["steps"].lazy.flat_map do |step| + (step["routing_conditions"]&.lazy || []).reject do |condition| + condition["check_page_id"] == condition["routing_page_id"] + end + end + end end end diff --git a/app/services/reports/forms_csv_report_service.rb b/app/services/reports/forms_csv_report_service.rb index ec75c74b7..f8827d8e0 100644 --- a/app/services/reports/forms_csv_report_service.rb +++ b/app/services/reports/forms_csv_report_service.rb @@ -14,6 +14,7 @@ class Reports::FormsCsvReportService "Updated at", "Number of questions", "Has routes", + "Has branch routes", "Payment URL", "Support URL", "Support URL text", @@ -58,6 +59,7 @@ def form_row(form) form["content"]["updated_at"], form["content"]["steps"].length, form["content"]["steps"].any? { |step| step["routing_conditions"].present? }, + Reports::FormDocumentsService.has_secondary_skip_routes?(form), form["content"]["payment_url"], form["content"]["support_url"], form["content"]["support_url_text"], diff --git a/app/views/reports/features.html.erb b/app/views/reports/features.html.erb index ee7368fda..463dac139 100644 --- a/app/views/reports/features.html.erb +++ b/app/views/reports/features.html.erb @@ -14,6 +14,10 @@ <%= row.with_key(text: t(".features.live_forms_with_routes")) %> <%= row.with_value(text: govuk_link_to(data[:forms_with_routing], feature_report_path(report: "forms-with-routes"), no_visited_state: true)) %> <% end %> + <%= summary_list.with_row do |row| %> + <%= row.with_key(text: t(".features.live_forms_with_branch_routes")) %> + <%= row.with_value(text: govuk_link_to(data[:forms_with_branch_routing], feature_report_path(report: "forms-with-branch-routes"), 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[:forms_with_payment], feature_report_path(report: "forms-with-payments"), no_visited_state: true)) %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 61178485a..afc09c6df 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1382,6 +1382,7 @@ en: features: heading: Feature usage live_forms_with_add_another_answer: Live forms with add another answer + live_forms_with_branch_routes: Live forms with branch routes live_forms_with_csv_submission_enabled: Live forms with CSV submission enabled live_forms_with_payments: Live forms with payments live_forms_with_routes: Live forms with routes @@ -1392,9 +1393,13 @@ en: form_or_questions_list_table: headings: form_name: Form name + number_of_branch_routes: Number of branch routes number_of_routes: Number of routes organisation: Organisation question_text: Question text + forms_with_branch_routes: + download_csv: Download data about all live forms with branch routes as a CSV file + heading: Live forms with branch routes forms_with_csv_submission_enabled: download_csv: Download data about all live forms with CSV submission enabled as a CSV file heading: Live forms with CSV submission enabled diff --git a/spec/helpers/report_helpers_spec.rb b/spec/helpers/report_helpers_spec.rb index e5ddc041a..950194b2c 100644 --- a/spec/helpers/report_helpers_spec.rb +++ b/spec/helpers/report_helpers_spec.rb @@ -11,8 +11,8 @@ 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 } }, + { "form_id" => 3, "content" => { "name" => "Branch route form" }, "group" => { "organisation" => { "name" => "Ministry of Tests" } }, "metadata" => { "number_of_routes" => 2, "number_of_branch_routes" => 1 } }, + { "form_id" => 4, "content" => { "name" => "Skip route form" }, "group" => { "organisation" => { "name" => "Department for Testing" } }, "metadata" => { "number_of_routes" => 1, "number_of_branch_routes" => 0 } }, ] end @@ -158,6 +158,7 @@ "Form name", "Organisation", "Number of routes", + "Number of branch routes", ] end end @@ -203,6 +204,12 @@ ] end + it "includes the number of branch routes in the form" do + expect(helper.report_forms_with_routes_table_rows(forms).map(&:fourth)).to eq %w[ + 1 + 0 + ] + context "when form is not in a group" do let(:forms) do [ diff --git a/spec/services/reports/feature_report_service_spec.rb b/spec/services/reports/feature_report_service_spec.rb index caec6b18a..55246320c 100644 --- a/spec/services/reports/feature_report_service_spec.rb +++ b/spec/services/reports/feature_report_service_spec.rb @@ -44,6 +44,7 @@ total_forms: 4, forms_with_payment: 1, forms_with_routing: 2, + forms_with_branch_routing: 1, forms_with_add_another_answer: 1, forms_with_csv_submission_enabled: 1, forms_with_answer_type: { @@ -277,6 +278,7 @@ ), "metadata" => { "number_of_routes" => 2, + "number_of_branch_routes" => 1, }, ), a_hash_including( @@ -291,6 +293,7 @@ ), "metadata" => { "number_of_routes" => 1, + "number_of_branch_routes" => 0, }, ), ] @@ -319,6 +322,7 @@ expect(forms).to all include( "metadata" => a_hash_including( "number_of_routes" => an_instance_of(Integer), + "number_of_branch_routes" => an_instance_of(Integer), ), ) end @@ -335,6 +339,62 @@ end end + describe "#forms_with_branch_routes" do + it "returns details needed to render report" do + forms = described_class.new(form_documents).forms_with_branch_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, + "number_of_branch_routes" => 1, + }, + ), + ] + end + + it "returns forms with branch routes" do + forms = described_class.new(form_documents).forms_with_branch_routes + expect(forms).to match [ + a_hash_including( + "form_id" => 3, + "content" => a_hash_including( + "name" => "Branch route form", + ), + ), + ] + end + + it "includes counts of routes" do + forms = described_class.new(form_documents).forms_with_branch_routes + expect(forms).to all include( + "metadata" => a_hash_including( + "number_of_routes" => an_instance_of(Integer), + "number_of_branch_routes" => an_instance_of(Integer), + ), + ) + end + + it "includes a reference to the organisation record" do + forms = described_class.new(form_documents).forms_with_branch_routes + expect(forms).to all include( + "group" => a_hash_including( + "organisation" => a_hash_including( + "name", + ), + ), + ) + end + end + describe "#forms_with_payments" do it "returns live forms with payments" do forms = described_class.new(form_documents).forms_with_payments diff --git a/spec/services/reports/form_documents_service_spec.rb b/spec/services/reports/form_documents_service_spec.rb index c4639a0fd..c19471795 100644 --- a/spec/services/reports/form_documents_service_spec.rb +++ b/spec/services/reports/form_documents_service_spec.rb @@ -1,11 +1,43 @@ require "rails_helper" RSpec.describe Reports::FormDocumentsService do + # 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) { JSON.load_file file_fixture("form_documents_response.json") } + + let(:branch_route_form) { form_documents[2] } + let(:skip_route_form) { form_documents[3] } + + let(:branch_route_form_2x1) do + form_document = branch_route_form.deep_dup + form_document["content"]["steps"] = [ + *form_document["content"]["steps"].deep_dup.each do |step| + step["id"] += 100 + step["next_step_id"] += 100 if step["next_step_id"] + step["routing_conditions"].each do |condition| + condition["id"] += 100 + condition["goto_page_id"] += 100 if condition["goto_page_id"] + condition["check_page_id"] += 100 if condition["check_page_id"] + condition["routing_page_id"] += 100 if condition["routing_page_id"] + end + end, + *form_document["content"]["steps"].deep_dup.each do |step| + step["id"] += 200 + step["next_step_id"] += 200 if step["next_step_id"] + step["routing_conditions"].each do |condition| + condition["id"] += 200 + condition["goto_page_id"] += 200 if condition["goto_page_id"] + condition["check_page_id"] += 200 if condition["check_page_id"] + condition["routing_page_id"] += 200 if condition["routing_page_id"] + end + end, + ] + form_document + end + 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 @@ -80,4 +112,54 @@ def response_headers(total, offset, limit) } end end + + describe ".has_secondary_skip_routes?" do + subject(:count_secondary_skip_routes) do + described_class.has_secondary_skip_routes?(form_document) + end + + context "when form has one step with one secondary skip condition" do + let(:form_document) { branch_route_form } + + it { is_expected.to be true } + end + + context "when form has two steps each with one secondary skip condition" do + let(:form_document) { branch_route_form_2x1 } + + it { is_expected.to be true } + end + + context "when form has no secondary skip conditions" do + let(:form_document) { form_documents[3] } + + it { is_expected.to be false } + end + end + + describe ".count_secondary_skip_routes" do + subject(:count_secondary_skip_routes) do + described_class.count_secondary_skip_routes(form_document) + end + + let(:form_documents) { JSON.load_file file_fixture("form_documents_response.json") } + + context "when form has one step with one secondary skip condition" do + let(:form_document) { branch_route_form } + + it { is_expected.to eq 1 } + end + + context "when form has two steps each with one secondary skip condition" do + let(:form_document) { branch_route_form_2x1 } + + it { is_expected.to eq 2 } + end + + context "when form has no secondary skip conditions" do + let(:form_document) { skip_route_form } + + it { is_expected.to eq 0 } + end + end end diff --git a/spec/services/reports/forms_csv_report_service_spec.rb b/spec/services/reports/forms_csv_report_service_spec.rb index c6eff3072..7410cac6a 100644 --- a/spec/services/reports/forms_csv_report_service_spec.rb +++ b/spec/services/reports/forms_csv_report_service_spec.rb @@ -39,6 +39,7 @@ "2025-01-02T16:24:31.255Z", "9", "false", + "false", "https://www.gov.uk/payments/your-payment-link", nil, nil, From 4ed85da61510ab83d933abbd47e3eca4deae0825 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Tue, 20 May 2025 15:05:09 +0300 Subject: [PATCH 2/2] Add whether a question has branch routes to the questions CSV report --- .../reports/form_documents_service.rb | 6 ++++ .../reports/questions_csv_report_service.rb | 2 ++ spec/helpers/report_helpers_spec.rb | 1 + .../reports/form_documents_service_spec.rb | 31 +++++++++++++++++ .../questions_csv_report_service_spec.rb | 34 +++++++++++++++++++ 5 files changed, 74 insertions(+) diff --git a/app/services/reports/form_documents_service.rb b/app/services/reports/form_documents_service.rb index 4370be3aa..53eb9fb1f 100644 --- a/app/services/reports/form_documents_service.rb +++ b/app/services/reports/form_documents_service.rb @@ -34,6 +34,12 @@ def count_secondary_skip_routes(form_document) secondary_skip_conditions(form_document).count end + def step_has_secondary_skip_route?(form_document, step) + secondary_skip_conditions(form_document).any? do |condition| + condition["check_page_id"] == step["id"] + end + end + def has_payments?(form_document) form_document["content"]["payment_url"].present? end diff --git a/app/services/reports/questions_csv_report_service.rb b/app/services/reports/questions_csv_report_service.rb index c1e64e609..cec215273 100644 --- a/app/services/reports/questions_csv_report_service.rb +++ b/app/services/reports/questions_csv_report_service.rb @@ -19,6 +19,7 @@ class Reports::QuestionsCsvReportService "Is optional?", IS_REPEATABLE, "Has routes?", + "Has branch routes?", "Answer settings - Input type", "Selection settings - Only one option?", "Selection settings - Number of options", @@ -68,6 +69,7 @@ def question_row(step) step["data"]["is_optional"], step["data"]["is_repeatable"], step["routing_conditions"].present?, + Reports::FormDocumentsService.step_has_secondary_skip_route?(form, step), 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, diff --git a/spec/helpers/report_helpers_spec.rb b/spec/helpers/report_helpers_spec.rb index 950194b2c..7cf73a805 100644 --- a/spec/helpers/report_helpers_spec.rb +++ b/spec/helpers/report_helpers_spec.rb @@ -209,6 +209,7 @@ 1 0 ] + end context "when form is not in a group" do let(:forms) do diff --git a/spec/services/reports/form_documents_service_spec.rb b/spec/services/reports/form_documents_service_spec.rb index c19471795..e91f979ff 100644 --- a/spec/services/reports/form_documents_service_spec.rb +++ b/spec/services/reports/form_documents_service_spec.rb @@ -162,4 +162,35 @@ def response_headers(total, offset, limit) it { is_expected.to eq 0 } end end + + describe ".step_has_secondary_skip_route?" do + let(:form_documents) { JSON.load_file file_fixture("form_documents_response.json") } + + context "when step is check page for secondary skip condition" do + let(:form_document) { branch_route_form } + let(:step) { form_document["content"]["steps"][0] } + + it "returns true" do + expect(described_class.step_has_secondary_skip_route?(form_document, step)).to be true + end + end + + context "when step is not check page for secondary skip condition" do + let(:form_document) { branch_route_form } + let(:step) { form_document["content"]["steps"][3] } + + it "returns false" do + expect(described_class.step_has_secondary_skip_route?(form_document, step)).to be false + end + end + + context "when form has no secondary skip conditions" do + let(:form_document) { skip_route_form } + let(:step) { form_document["content"]["steps"][0] } + + it "returns false" do + expect(described_class.step_has_secondary_skip_route?(form_document, step)).to be false + end + end + end end diff --git a/spec/services/reports/questions_csv_report_service_spec.rb b/spec/services/reports/questions_csv_report_service_spec.rb index f6ccf96ab..bd50552d6 100644 --- a/spec/services/reports/questions_csv_report_service_spec.rb +++ b/spec/services/reports/questions_csv_report_service_spec.rb @@ -45,6 +45,7 @@ "false", "true", "false", + "false", "single_line", nil, nil, @@ -74,6 +75,7 @@ "true", "false", "false", + "false", nil, "false", "3", @@ -103,6 +105,7 @@ "false", "false", "false", + "false", "full_name", nil, nil, @@ -112,6 +115,36 @@ end it "has expected values for question with routing conditions" do + csv = csv_reports_service.csv + rows = CSV.parse(csv) + routing_question_row = rows.detect { |row| row.include? "Would you like to submit anonymously?" } + expect(routing_question_row).to eq([ + "4", + "live", + "Skip route form", + group.organisation.name, + group.organisation.id.to_s, + group.name, + group.external_id, + "1", + "Would you like to submit anonymously?", + "selection", + nil, + nil, + nil, + "false", + "false", + "true", + "false", + nil, + "true", + "2", + nil, + "{\"only_one_option\" => \"true\", \"selection_options\" => [{\"name\" => \"Yes\"}, {\"name\" => \"No\"}]}", + ]) + end + + it "has expected values for question with branch routing conditions" do 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?" } @@ -132,6 +165,7 @@ "false", "false", "true", + "true", nil, "true", "2",