From f930551de5e67b6f5c76f490808a212f9a195446 Mon Sep 17 00:00:00 2001 From: Samuel Date: Mon, 12 May 2025 15:52:25 +0100 Subject: [PATCH 1/4] move organisation questions to be first --- app/models/form/sales/questions/managing_organisation.rb | 4 +--- app/models/form/sales/questions/owning_organisation_id.rb | 2 +- app/models/form/sales/questions/sale_date.rb | 2 +- app/models/form/sales/subsections/setup.rb | 2 +- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/models/form/sales/questions/managing_organisation.rb b/app/models/form/sales/questions/managing_organisation.rb index ffe143ab5..b68a359d7 100644 --- a/app/models/form/sales/questions/managing_organisation.rb +++ b/app/models/form/sales/questions/managing_organisation.rb @@ -4,7 +4,7 @@ class Form::Sales::Questions::ManagingOrganisation < ::Form::Question @id = "managing_organisation_id" @derived = true @type = "select" - @question_number = QUESTION_NUMBER_FROM_YEAR[form.start_date.year] || QUESTION_NUMBER_FROM_YEAR[QUESTION_NUMBER_FROM_YEAR.keys.max] + @question_number = 2 end def answer_options(log = nil, user = nil) @@ -71,6 +71,4 @@ private def selected_answer_option_is_derived?(_log) true end - - QUESTION_NUMBER_FROM_YEAR = { 2024 => 2, 2025 => 3 }.freeze end diff --git a/app/models/form/sales/questions/owning_organisation_id.rb b/app/models/form/sales/questions/owning_organisation_id.rb index d4ece8554..faf72e59e 100644 --- a/app/models/form/sales/questions/owning_organisation_id.rb +++ b/app/models/form/sales/questions/owning_organisation_id.rb @@ -93,5 +93,5 @@ private "#{name} (inactive as of #{merge_date.to_fs(:govuk_date)})" end - QUESTION_NUMBER_FROM_YEAR = { 2023 => nil, 2024 => 1, 2025 => 2 }.freeze + QUESTION_NUMBER_FROM_YEAR = { 2023 => nil, 2024 => 1 }.freeze end diff --git a/app/models/form/sales/questions/sale_date.rb b/app/models/form/sales/questions/sale_date.rb index 8adefd87c..00f0c86dd 100644 --- a/app/models/form/sales/questions/sale_date.rb +++ b/app/models/form/sales/questions/sale_date.rb @@ -6,5 +6,5 @@ class Form::Sales::Questions::SaleDate < ::Form::Question @question_number = QUESTION_NUMBER_FROM_YEAR[form.start_date.year] || QUESTION_NUMBER_FROM_YEAR[QUESTION_NUMBER_FROM_YEAR.keys.max] end - QUESTION_NUMBER_FROM_YEAR = { 2023 => 1, 2024 => 3, 2025 => 1 }.freeze + QUESTION_NUMBER_FROM_YEAR = { 2023 => 1, 2024 => 3 }.freeze end diff --git a/app/models/form/sales/subsections/setup.rb b/app/models/form/sales/subsections/setup.rb index be6c20400..42eea0026 100644 --- a/app/models/form/sales/subsections/setup.rb +++ b/app/models/form/sales/subsections/setup.rb @@ -7,9 +7,9 @@ class Form::Sales::Subsections::Setup < ::Form::Subsection def pages @pages ||= [ - Form::Sales::Pages::SaleDate.new(nil, nil, self), Form::Sales::Pages::OwningOrganisation.new(nil, nil, self), Form::Sales::Pages::ManagingOrganisation.new(nil, nil, self), + Form::Sales::Pages::SaleDate.new(nil, nil, self), Form::Sales::Pages::CreatedBy.new(nil, nil, self), Form::Sales::Pages::PurchaserCode.new(nil, nil, self), Form::Sales::Pages::OwnershipScheme.new(nil, nil, self), From 9bf0fdd14794eb0016ffc4cea880bb6d39760692 Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 14 May 2025 11:37:09 +0100 Subject: [PATCH 2/4] fix tests --- app/services/csv/sales_log_csv_service.rb | 9 +++++++++ spec/models/form/sales/subsections/setup_spec.rb | 6 +++--- spec/requests/form_controller_spec.rb | 6 +++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/app/services/csv/sales_log_csv_service.rb b/app/services/csv/sales_log_csv_service.rb index 9f16a3f8b..51e0f9602 100644 --- a/app/services/csv/sales_log_csv_service.rb +++ b/app/services/csv/sales_log_csv_service.rb @@ -262,6 +262,7 @@ module Csv attributes = insert_checkbox_options(ordered_questions) final_attributes = insert_derived_and_related_attributes(non_question_fields + attributes) order_address_fields_for_support(final_attributes) + order_organisation_fields_before_sales_date_field(final_attributes) @user.support? ? final_attributes : final_attributes - SUPPORT_ONLY_ATTRIBUTES end @@ -321,6 +322,14 @@ module Csv end end + def order_organisation_fields_before_sales_date_field(attributes) + sales_date_index = attributes.find_index { |q| q == "saledate" } + if sales_date_index + attributes.reject! { |q| q == "owning_organisation_name" || q == "managing_organisation_name" } + attributes.insert(sales_date_index, "owning_organisation_name", "managing_organisation_name") + end + end + def non_question_fields case @year when 2022 diff --git a/spec/models/form/sales/subsections/setup_spec.rb b/spec/models/form/sales/subsections/setup_spec.rb index f93e7ef5e..87f848c91 100644 --- a/spec/models/form/sales/subsections/setup_spec.rb +++ b/spec/models/form/sales/subsections/setup_spec.rb @@ -29,9 +29,9 @@ RSpec.describe Form::Sales::Subsections::Setup, type: :model do it "has correct pages" do expect(setup.pages.map(&:id)).to eq( %w[ - completion_date owning_organisation managing_organisation + completion_date assigned_to purchaser_code ownership_scheme @@ -56,9 +56,9 @@ RSpec.describe Form::Sales::Subsections::Setup, type: :model do it "has correct pages" do expect(setup.pages.map(&:id)).to eq( %w[ - completion_date owning_organisation managing_organisation + completion_date assigned_to purchaser_code ownership_scheme @@ -87,9 +87,9 @@ RSpec.describe Form::Sales::Subsections::Setup, type: :model do it "has correct pages" do expect(setup.pages.map(&:id)).to eq( %w[ - completion_date owning_organisation managing_organisation + completion_date assigned_to purchaser_code ownership_scheme diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb index a6c80abc6..c5c158a92 100644 --- a/spec/requests/form_controller_spec.rb +++ b/spec/requests/form_controller_spec.rb @@ -182,7 +182,7 @@ RSpec.describe FormController, type: :request do it "correctly sets owning organisation" do post "/sales-logs/#{sales_log.id}/owning-organisation", params: params - expect(response).to redirect_to("/sales-logs/#{sales_log.id}/assigned-to") + expect(response).to redirect_to("/sales-logs/#{sales_log.id}/completion-date") follow_redirect! sales_log.reload expect(sales_log.owning_organisation).to eq(managing_organisation) @@ -209,7 +209,7 @@ RSpec.describe FormController, type: :request do it "does not reset assigned to" do post "/sales-logs/#{sales_log.id}/owning-organisation", params: params - expect(response).to redirect_to("/sales-logs/#{sales_log.id}/assigned-to") + expect(response).to redirect_to("/sales-logs/#{sales_log.id}/completion-date") follow_redirect! sales_log.reload expect(sales_log.assigned_to).to eq(assigned_to) @@ -238,7 +238,7 @@ RSpec.describe FormController, type: :request do it "does not reset assigned to" do post "/sales-logs/#{sales_log.id}/owning-organisation", params: params - expect(response).to redirect_to("/sales-logs/#{sales_log.id}/assigned-to") + expect(response).to redirect_to("/sales-logs/#{sales_log.id}/completion-date") follow_redirect! sales_log.reload expect(sales_log.assigned_to).to eq(assigned_to) From 08e13f72b415a9a652764d4b092cc25e4138fb28 Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 14 May 2025 14:25:14 +0100 Subject: [PATCH 3/4] fix tests after merge --- app/services/csv/sales_log_csv_service.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app/services/csv/sales_log_csv_service.rb b/app/services/csv/sales_log_csv_service.rb index 51e0f9602..5ca263b43 100644 --- a/app/services/csv/sales_log_csv_service.rb +++ b/app/services/csv/sales_log_csv_service.rb @@ -259,10 +259,10 @@ module Csv ordered_questions = FormHandler.instance.ordered_questions_for_year(@year, "sales") ordered_questions.reject! { |q| q.id.match?(/((?= 2025 + order_saledate_question_before_owning_organisation_question(ordered_questions) attributes = insert_checkbox_options(ordered_questions) final_attributes = insert_derived_and_related_attributes(non_question_fields + attributes) order_address_fields_for_support(final_attributes) - order_organisation_fields_before_sales_date_field(final_attributes) @user.support? ? final_attributes : final_attributes - SUPPORT_ONLY_ATTRIBUTES end @@ -322,11 +322,12 @@ module Csv end end - def order_organisation_fields_before_sales_date_field(attributes) - sales_date_index = attributes.find_index { |q| q == "saledate" } - if sales_date_index - attributes.reject! { |q| q == "owning_organisation_name" || q == "managing_organisation_name" } - attributes.insert(sales_date_index, "owning_organisation_name", "managing_organisation_name") + def order_saledate_question_before_owning_organisation_question(ordered_questions) + saledate_question_index = ordered_questions.find_index { |q| q.id == "saledate" } + owning_organisation_index = ordered_questions.find_index { |q| q.id == "owning_organisation_id" } + if saledate_question_index && owning_organisation_index + saledate_question = ordered_questions.delete_at(saledate_question_index) + ordered_questions.insert(owning_organisation_index, saledate_question) end end From 7a3e4498d702078d6168341673e3e9871ff33436 Mon Sep 17 00:00:00 2001 From: Samuel Date: Thu, 15 May 2025 15:54:05 +0100 Subject: [PATCH 4/4] document this function --- app/services/csv/sales_log_csv_service.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/services/csv/sales_log_csv_service.rb b/app/services/csv/sales_log_csv_service.rb index 5ca263b43..0b6612a19 100644 --- a/app/services/csv/sales_log_csv_service.rb +++ b/app/services/csv/sales_log_csv_service.rb @@ -322,6 +322,12 @@ module Csv end end + # as part of CLDC-3719 it was decided to move the saledate question to be first in the form + # this caused issues reported in CLDC-4025 where the user only enter saledates for their active organisation + # we decided to move the organisation question back to being first + # however, we did not want to reorder the CSV export as this would disrupt existing users' data pipelines + # so, this function reorders questions back when exporting CSVs + # next year, we can remove this function as we will be reordering the csv fields anyway def order_saledate_question_before_owning_organisation_question(ordered_questions) saledate_question_index = ordered_questions.find_index { |q| q.id == "saledate" } owning_organisation_index = ordered_questions.find_index { |q| q.id == "owning_organisation_id" }