From bb2fa87e451b93cc867553279b8cd5bd1b2114dd Mon Sep 17 00:00:00 2001 From: Kat Date: Tue, 14 May 2024 10:56:16 +0100 Subject: [PATCH] Refactor --- app/models/form_handler.rb | 22 -- spec/jobs/email_csv_job_spec.rb | 4 +- spec/models/form_handler_spec.rb | 228 +++++------------- .../csv/lettings_log_csv_service_spec.rb | 5 +- .../csv/sales_log_csv_service_spec.rb | 5 +- 5 files changed, 69 insertions(+), 195 deletions(-) diff --git a/app/models/form_handler.rb b/app/models/form_handler.rb index 88895334e..e4774ccf1 100644 --- a/app/models/form_handler.rb +++ b/app/models/form_handler.rb @@ -61,28 +61,6 @@ class FormHandler @sales_forms end - def ordered_sales_questions_for_all_years - ordered_questions = current_sales_form.questions.uniq(&:id) - all_sales_forms = forms.filter { |name, _form| name.end_with? "sales" }.values - all_questions_from_available_sales_forms = all_sales_forms.flat_map(&:questions) - deprecated_questions_by_preceding_question_id(ordered_questions, all_questions_from_available_sales_forms).each do |preceding_question_id, deprecated_question| - index_of_preceding_question = ordered_questions.index { |q| q.id == preceding_question_id } - ordered_questions.insert(index_of_preceding_question + 1, deprecated_question) - end - ordered_questions - end - - def ordered_lettings_questions_for_all_years - ordered_questions = current_lettings_form.questions.uniq(&:id) - all_lettings_forms = forms.filter { |name, _form| name.end_with? "lettings" }.values - all_questions_from_available_lettings_forms = all_lettings_forms.flat_map(&:questions) - deprecated_questions_by_preceding_question_id(ordered_questions, all_questions_from_available_lettings_forms).each do |preceding_question_id, deprecated_question| - index_of_preceding_question = ordered_questions.index { |q| q.id == preceding_question_id } - ordered_questions.insert(index_of_preceding_question + 1, deprecated_question) - end - ordered_questions - end - def ordered_questions_for_year(year, type) return [] unless year diff --git a/spec/jobs/email_csv_job_spec.rb b/spec/jobs/email_csv_job_spec.rb index 3684c6bba..8e8a027ea 100644 --- a/spec/jobs/email_csv_job_spec.rb +++ b/spec/jobs/email_csv_job_spec.rb @@ -54,7 +54,7 @@ describe EmailCsvJob do job.perform(user, search_term, filters, all_orgs, organisation, codes_only_export) end - it "creates a LettingsLogCsvService with the correct export type" do + it "creates a LettingsLogCsvService with the correct export type and year" do expect(Csv::LettingsLogCsvService).to receive(:new).with(user:, export_type: "labels", year: 2023) codes_only = false job.perform(user, nil, {}, nil, nil, codes_only, "lettings", 2023) @@ -89,7 +89,7 @@ describe EmailCsvJob do job.perform(user, search_term, filters, all_orgs, organisation, codes_only_export, "sales") end - it "creates a SalesLogCsvService with the correct export type" do + it "creates a SalesLogCsvService with the correct export type and year" do expect(Csv::SalesLogCsvService).to receive(:new).with(user:, export_type: "labels", year: 2022) codes_only = false job.perform(user, nil, {}, nil, nil, codes_only, "sales", 2022) diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index 257c41b43..b642a9056 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -262,179 +262,73 @@ RSpec.describe FormHandler do end end - describe "#ordered_sales_questions_for_all_years" do - let(:result) { described_class.instance.ordered_sales_questions_for_all_years } - let(:now) { Time.zone.now } - - it "returns an array of questions" do - section = build(:section, :with_questions, question_ids: %w[1 2 3]) - sales_form = FormFactory.new(year: 1936, type: "sales") - .with_sections([section]) - .build - described_class.instance.use_fake_forms!({ "current_sales" => sales_form }) - expect(result).to(satisfy { |result| result.all? { |element| element.is_a?(Form::Question) } }) - end - - it "does not return multiple questions with the same id" do - first_section = build(:section, :with_questions, question_ids: %w[1 2 3]) - second_section = build(:section, :with_questions, question_ids: %w[2 3 4 5]) - sales_form = FormFactory.new(year: 1936, type: "sales") - .with_sections([first_section, second_section]) - .build - described_class.instance.use_fake_forms!({ "current_sales" => sales_form }) - expect(result.map(&:id)).to eq %w[1 2 3 4 5] - end - - it "returns the questions in the same order as the form" do - first_section = build(:section, :with_questions, question_ids: %w[1 2 3]) - second_section = build(:section, :with_questions, question_ids: %w[4 5 6]) - sales_form = FormFactory.new(year: 1945, type: "sales") - .with_sections([first_section, second_section]) - .build - described_class.instance.use_fake_forms!({ "current_sales" => sales_form }) - expect(result.map(&:id)).to eq %w[1 2 3 4 5 6] - end - - it "inserts questions from all years in their correct positions" do - original_section = build(:section, :with_questions, question_ids: %w[1 1a_deprecated 2 3]) - new_section = build(:section, :with_questions, question_ids: %w[1 2 2a_new 3]) - original_form = FormFactory.new(year: 1066, type: "sales") - .with_sections([original_section]) - .build - new_form = FormFactory.new(year: 1485, type: "sales") - .with_sections([new_section]) - .build - fake_forms = { - "previous_sales" => original_form, - "current_sales" => new_form, - } - described_class.instance.use_fake_forms!(fake_forms) - expect(result.map(&:id)).to eq %w[1 1a_deprecated 2 2a_new 3] - end - - it "inserts questions from previous years that would start a section after new questions in the previous section" do - original_first_section = build(:section) - original_first_section.subsections = [build(:subsection, :with_questions, question_ids: %w[1 2], id: "1", section: original_first_section)] - new_first_section = build(:section) - new_first_section.subsections = [build(:subsection, :with_questions, question_ids: %w[1 2 extra], id: "1", section: new_first_section)] - original_second_section = build(:section) - original_second_section.subsections = [build(:subsection, :with_questions, question_ids: %w[3 4], id: "2")] - new_second_section = build(:section) - new_second_section.subsections = [build(:subsection, :with_questions, question_ids: %w[3_new 4], id: "2")] - - original_form = FormFactory.new(year: 2022, type: "sales").with_sections([original_first_section, original_second_section]).build - new_form = FormFactory.new(year: 2023, type: "sales").with_sections([new_first_section, new_second_section]).build - - fake_forms = { - "current_sales" => new_form, - "previous_sales" => original_form, - } - described_class.instance.use_fake_forms!(fake_forms) - expect(result.map(&:id)).to eq %w[1 2 extra 3 3_new 4] - end - - it "builds the ordering based on the current form" do - archived_section = build(:section, :with_questions, question_ids: %w[0 1 2 3]) - previous_section = build(:section, :with_questions, question_ids: %w[0 1 3 2]) - current_section = build(:section, :with_questions, question_ids: %w[3 2 0 1]) - next_section = build(:section, :with_questions, question_ids: %w[3 2 1 0]) - - fake_forms = { - "current_sales" => FormFactory.new(year: 2023, type: "sales").with_sections([current_section]).build, - "previous_sales" => FormFactory.new(year: 2022, type: "sales").with_sections([previous_section]).build, - "next_sales" => FormFactory.new(year: 2021, type: "sales").with_sections([next_section]).build, - "archived_sales" => FormFactory.new(year: 2020, type: "sales").with_sections([archived_section]).build, - } - described_class.instance.use_fake_forms!(fake_forms) - expect(result.map(&:id)).to eq %w[3 2 0 1] - end - end - - describe "#ordered_lettings_questions_for_all_years" do - let(:result) { described_class.instance.ordered_lettings_questions_for_all_years } - let(:now) { Time.zone.now } - - it "returns an array of questions" do - section = build(:section, :with_questions, question_ids: %w[1 2 3]) - lettings_form = FormFactory.new(year: 2936, type: "lettings") - .with_sections([section]) - .build - described_class.instance.use_fake_forms!({ "current_lettings" => lettings_form }) - expect(result).to(satisfy { |result| result.all? { |element| element.is_a?(Form::Question) } }) - end + describe "#ordered_questions_for_year" do + context "with lettings" do + let(:result) { described_class.instance.ordered_questions_for_year(2936, "lettings") } + let(:now) { Time.zone.local(2936, 5, 1) } + + it "returns an array of questions" do + section = build(:section, :with_questions, question_ids: %w[1 2 3]) + lettings_form = FormFactory.new(year: 2936, type: "lettings") + .with_sections([section]) + .build + described_class.instance.use_fake_forms!({ "current_lettings" => lettings_form }) + expect(result).to(satisfy { |result| result.all? { |element| element.is_a?(Form::Question) } }) + end - it "does not return multiple questions with the same id" do - first_section = build(:section, :with_questions, question_ids: %w[1 2 3]) - second_section = build(:section, :with_questions, question_ids: %w[2 3 4 5]) - lettings_form = FormFactory.new(year: 2936, type: "lettings") - .with_sections([first_section, second_section]) - .build - described_class.instance.use_fake_forms!({ "current_lettings" => lettings_form }) - expect(result.map(&:id)).to eq %w[1 2 3 4 5] - end + it "does not return multiple questions with the same id" do + first_section = build(:section, :with_questions, question_ids: %w[1 2 3]) + second_section = build(:section, :with_questions, question_ids: %w[2 3 4 5]) + lettings_form = FormFactory.new(year: 2936, type: "lettings") + .with_sections([first_section, second_section]) + .build + described_class.instance.use_fake_forms!({ "current_lettings" => lettings_form }) + expect(result.map(&:id)).to eq %w[1 2 3 4 5] + end - it "returns the questions in the same order as the form" do - first_section = build(:section, :with_questions, question_ids: %w[1 2 3]) - second_section = build(:section, :with_questions, question_ids: %w[4 5 6]) - lettings_form = FormFactory.new(year: 2945, type: "lettings") - .with_sections([first_section, second_section]) - .build - described_class.instance.use_fake_forms!({ "current_lettings" => lettings_form }) - expect(result.map(&:id)).to eq %w[1 2 3 4 5 6] + it "returns the questions in the same order as the form" do + first_section = build(:section, :with_questions, question_ids: %w[1 2 3]) + second_section = build(:section, :with_questions, question_ids: %w[4 5 6]) + lettings_form = FormFactory.new(year: 2936, type: "lettings") + .with_sections([first_section, second_section]) + .build + described_class.instance.use_fake_forms!({ "current_lettings" => lettings_form }) + expect(result.map(&:id)).to eq %w[1 2 3 4 5 6] + end end - it "inserts questions from all years in their correct positions" do - original_section = build(:section, :with_questions, question_ids: %w[1 1a_deprecated 2 3]) - new_section = build(:section, :with_questions, question_ids: %w[1 2 2a_new 3]) - original_form = FormFactory.new(year: 2066, type: "lettings") - .with_sections([original_section]) - .build - new_form = FormFactory.new(year: 2485, type: "lettings") - .with_sections([new_section]) - .build - fake_forms = { - "previous_lettings" => original_form, - "current_lettings" => new_form, - } - described_class.instance.use_fake_forms!(fake_forms) - expect(result.map(&:id)).to eq %w[1 1a_deprecated 2 2a_new 3] - end + context "with sales" do + let(:result) { described_class.instance.ordered_questions_for_year(2936, "sales") } + let(:now) { Time.zone.local(2936, 5, 1) } + + it "returns an array of questions" do + section = build(:section, :with_questions, question_ids: %w[1 2 3]) + sales_form = FormFactory.new(year: 2936, type: "sales") + .with_sections([section]) + .build + described_class.instance.use_fake_forms!({ "current_sales" => sales_form }) + expect(result).to(satisfy { |result| result.all? { |element| element.is_a?(Form::Question) } }) + end - it "inserts questions from previous years that would start a section after new questions in the previous section" do - original_first_section = build(:section) - original_first_section.subsections = [build(:subsection, :with_questions, question_ids: %w[1 2], id: "1", section: original_first_section)] - new_first_section = build(:section) - new_first_section.subsections = [build(:subsection, :with_questions, question_ids: %w[1 2 extra], id: "1", section: new_first_section)] - original_second_section = build(:section) - original_second_section.subsections = [build(:subsection, :with_questions, question_ids: %w[3 4], id: "2")] - new_second_section = build(:section) - new_second_section.subsections = [build(:subsection, :with_questions, question_ids: %w[3_new 4], id: "2")] - - original_form = FormFactory.new(year: 2023, type: "lettings").with_sections([original_first_section, original_second_section]).build - new_form = FormFactory.new(year: 2024, type: "lettings").with_sections([new_first_section, new_second_section]).build - - fake_forms = { - "current_lettings" => new_form, - "previous_lettings" => original_form, - } - described_class.instance.use_fake_forms!(fake_forms) - expect(result.map(&:id)).to eq %w[1 2 extra 3 3_new 4] - end + it "does not return multiple questions with the same id" do + first_section = build(:section, :with_questions, question_ids: %w[1 2 3]) + second_section = build(:section, :with_questions, question_ids: %w[2 3 4 5]) + sales_form = FormFactory.new(year: 2936, type: "sales") + .with_sections([first_section, second_section]) + .build + described_class.instance.use_fake_forms!({ "current_sales" => sales_form }) + expect(result.map(&:id)).to eq %w[1 2 3 4 5] + end - it "builds the ordering based on the current form" do - archived_section = build(:section, :with_questions, question_ids: %w[0 1 2 3]) - previous_section = build(:section, :with_questions, question_ids: %w[0 1 3 2]) - current_section = build(:section, :with_questions, question_ids: %w[3 2 0 1]) - next_section = build(:section, :with_questions, question_ids: %w[3 2 1 0]) - - fake_forms = { - "current_lettings" => FormFactory.new(year: 2023, type: "sales").with_sections([current_section]).build, - "previous_lettings" => FormFactory.new(year: 2022, type: "sales").with_sections([previous_section]).build, - "next_lettings" => FormFactory.new(year: 2021, type: "sales").with_sections([next_section]).build, - "archived_lettings" => FormFactory.new(year: 2020, type: "sales").with_sections([archived_section]).build, - } - described_class.instance.use_fake_forms!(fake_forms) - expect(result.map(&:id)).to eq %w[3 2 0 1] + it "returns the questions in the same order as the form" do + first_section = build(:section, :with_questions, question_ids: %w[1 2 3]) + second_section = build(:section, :with_questions, question_ids: %w[4 5 6]) + sales_form = FormFactory.new(year: 2936, type: "sales") + .with_sections([first_section, second_section]) + .build + described_class.instance.use_fake_forms!({ "current_sales" => sales_form }) + expect(result.map(&:id)).to eq %w[1 2 3 4 5 6] + end end end # rubocop:enable RSpec/PredicateMatcher diff --git a/spec/services/csv/lettings_log_csv_service_spec.rb b/spec/services/csv/lettings_log_csv_service_spec.rb index fc32ee9b9..8bf33a21a 100644 --- a/spec/services/csv/lettings_log_csv_service_spec.rb +++ b/spec/services/csv/lettings_log_csv_service_spec.rb @@ -66,7 +66,7 @@ RSpec.describe Csv::LettingsLogCsvService do expect(csv.first.first).to eq "id" end - context "when stubbing :ordered_lettings_questions_for_all_years" do + context "when stubbing :ordered_questions_for_year" do let(:lettings_form) do FormFactory.new(year: 2050, type: "lettings") .with_sections([build(:section, :with_questions, question_ids:, questions:)]) @@ -74,6 +74,7 @@ RSpec.describe Csv::LettingsLogCsvService do end let(:question_ids) { [] } let(:questions) { nil } + let(:year) { 2050 } before do allow(FormHandler).to receive(:instance).and_return(form_handler_mock) @@ -86,7 +87,7 @@ RSpec.describe Csv::LettingsLogCsvService do allow(FormHandler).to receive(:instance).and_return(form_handler_mock) allow(form_handler_mock).to receive(:ordered_questions_for_year).and_return([]) service.prepare_csv(LettingsLog.all) - expect(form_handler_mock).to have_received(:ordered_questions_for_year).with(2024, "lettings") + expect(form_handler_mock).to have_received(:ordered_questions_for_year).with(2050, "lettings") end context "when it returns questions with particular ids" do diff --git a/spec/services/csv/sales_log_csv_service_spec.rb b/spec/services/csv/sales_log_csv_service_spec.rb index 2747ef434..4cd056fe9 100644 --- a/spec/services/csv/sales_log_csv_service_spec.rb +++ b/spec/services/csv/sales_log_csv_service_spec.rb @@ -55,7 +55,7 @@ RSpec.describe Csv::SalesLogCsvService do expect(csv.first.first).to eq "id" end - context "when stubbing :ordered_sales_questions_for_all_years" do + context "when stubbing :ordered_questions_for_year" do let(:sales_form) do FormFactory.new(year: 1936, type: "sales") .with_sections([build(:section, :with_questions, question_ids:, questions:)]) @@ -63,6 +63,7 @@ RSpec.describe Csv::SalesLogCsvService do end let(:question_ids) { [] } let(:questions) { nil } + let(:year) { 1936 } before do allow(FormHandler).to receive(:instance).and_return(form_handler_mock) @@ -75,7 +76,7 @@ RSpec.describe Csv::SalesLogCsvService do allow(FormHandler).to receive(:instance).and_return(form_handler_mock) allow(form_handler_mock).to receive(:ordered_questions_for_year).and_return([]) service - expect(form_handler_mock).to have_received(:ordered_questions_for_year).with(2024, "sales") + expect(form_handler_mock).to have_received(:ordered_questions_for_year).with(1936, "sales") end context "when it returns questions with particular ids" do