From 89318bed6af44eb7b35e73cb36387f9eb9365f80 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Fri, 28 Apr 2023 10:34:11 +0100 Subject: [PATCH 1/4] [CLDC-2202] Allow coordinators to set created_by (#1533) * [CLDC-2202] Allow coordinators to set created_by * Scope user selection when data coordinator * Remove managing_for_other_user_enabled * Move sales created_by page and question out of common * Address comments - only select required users - remove not needed CYA checks --- app/models/form/common/pages/created_by.rb | 16 ---- .../form/common/questions/created_by_id.rb | 46 ---------- app/models/form/lettings/pages/created_by.rb | 5 +- .../form/lettings/questions/created_by_id.rb | 43 +++++---- app/models/form/lettings/subsections/setup.rb | 35 +------- app/models/form/sales/pages/created_by.rb | 19 ++++ .../form/sales/questions/created_by_id.rb | 54 ++++++++++++ app/models/form/sales/subsections/setup.rb | 2 +- app/services/feature_toggle.rb | 4 - spec/features/lettings_log_spec.rb | 4 +- .../common/questions/created_by_id_spec.rb | 82 ----------------- .../form/lettings/pages/created_by_spec.rb | 10 ++- .../lettings/questions/created_by_id_spec.rb | 79 +++++++++-------- .../pages/created_by_spec.rb | 16 +++- .../sales/questions/created_by_id_spec.rb | 88 +++++++++++++++++++ 15 files changed, 256 insertions(+), 247 deletions(-) delete mode 100644 app/models/form/common/pages/created_by.rb delete mode 100644 app/models/form/common/questions/created_by_id.rb create mode 100644 app/models/form/sales/pages/created_by.rb create mode 100644 app/models/form/sales/questions/created_by_id.rb delete mode 100644 spec/models/form/common/questions/created_by_id_spec.rb rename spec/models/form/{common => sales}/pages/created_by_spec.rb (71%) create mode 100644 spec/models/form/sales/questions/created_by_id_spec.rb diff --git a/app/models/form/common/pages/created_by.rb b/app/models/form/common/pages/created_by.rb deleted file mode 100644 index 94c7dc587..000000000 --- a/app/models/form/common/pages/created_by.rb +++ /dev/null @@ -1,16 +0,0 @@ -class Form::Common::Pages::CreatedBy < ::Form::Page - def initialize(id, hsh, subsection) - super - @id = "created_by" - end - - def questions - @questions ||= [ - Form::Common::Questions::CreatedById.new(nil, nil, self), - ] - end - - def routed_to?(_log, current_user) - !!current_user&.support? - end -end diff --git a/app/models/form/common/questions/created_by_id.rb b/app/models/form/common/questions/created_by_id.rb deleted file mode 100644 index ee2767a4f..000000000 --- a/app/models/form/common/questions/created_by_id.rb +++ /dev/null @@ -1,46 +0,0 @@ -class Form::Common::Questions::CreatedById < ::Form::Question - def initialize(id, hsh, page) - super - @id = "created_by_id" - @check_answer_label = "User" - @header = "Which user are you creating this log for?" - @type = "select" - end - - def answer_options - answer_opts = { "" => "Select an option" } - return answer_opts unless ActiveRecord::Base.connected? - - User.select(:id, :name).each_with_object(answer_opts) do |user, hsh| - hsh[user.id] = user.name - hsh - end - end - - def displayed_answer_options(log, _user = nil) - return answer_options unless log.owning_organisation - - user_ids = log.owning_organisation.users.pluck(:id) + [""] - answer_options.select { |k, _v| user_ids.include?(k) } - end - - def label_from_value(value, _log = nil, _user = nil) - return unless value - - answer_options[value] - end - - def hidden_in_check_answers?(_log, current_user) - !current_user.support? - end - - def derived? - true - end - -private - - def selected_answer_option_is_derived?(_log) - false - end -end diff --git a/app/models/form/lettings/pages/created_by.rb b/app/models/form/lettings/pages/created_by.rb index 8a30428e0..02eb44207 100644 --- a/app/models/form/lettings/pages/created_by.rb +++ b/app/models/form/lettings/pages/created_by.rb @@ -11,6 +11,9 @@ class Form::Lettings::Pages::CreatedBy < ::Form::Page end def routed_to?(_log, current_user) - !!current_user&.support? + return true if current_user&.support? + return true if current_user&.data_coordinator? + + false end end diff --git a/app/models/form/lettings/questions/created_by_id.rb b/app/models/form/lettings/questions/created_by_id.rb index 6402f14e0..4ed1efdd5 100644 --- a/app/models/form/lettings/questions/created_by_id.rb +++ b/app/models/form/lettings/questions/created_by_id.rb @@ -1,4 +1,6 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question + ANSWER_OPTS = { "" => "Select an option" }.freeze + def initialize(id, hsh, page) super @id = "created_by_id" @@ -8,33 +10,32 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question end def answer_options - answer_opts = { "" => "Select an option" } - return answer_opts unless ActiveRecord::Base.connected? + ANSWER_OPTS + end - User.select(:id, :name, :email).each_with_object(answer_opts) do |user, hsh| - hsh[user.id] = "#{user.name} (#{user.email})" + def displayed_answer_options(log, current_user = nil) + return ANSWER_OPTS unless current_user + + users = [] + users += if current_user.support? + [ + (log.owning_organisation&.users if log.owning_organisation), + (log.managing_organisation&.users if log.managing_organisation), + ].flatten + else + current_user.organisation.users + end.uniq.compact + + users.each_with_object(ANSWER_OPTS.dup) do |user, hsh| + hsh[user.id] = present_user(user) hsh end end - def displayed_answer_options(log, _user = nil) - return answer_options unless log.owning_organisation && log.managing_organisation - - user_ids = [""] - user_ids += log.owning_organisation.users.pluck(:id) - user_ids += log.managing_organisation.users.pluck(:id) - - answer_options.select { |k, _v| user_ids.include?(k) } - end - def label_from_value(value, _log = nil, _user = nil) return unless value - answer_options[value] - end - - def hidden_in_check_answers?(_log, current_user) - !current_user.support? + present_user(User.find(value)) end def derived? @@ -43,6 +44,10 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question private + def present_user(user) + "#{user.name} (#{user.email})" + end + def selected_answer_option_is_derived?(_log) true end diff --git a/app/models/form/lettings/subsections/setup.rb b/app/models/form/lettings/subsections/setup.rb index 312ed5255..f8514007c 100644 --- a/app/models/form/lettings/subsections/setup.rb +++ b/app/models/form/lettings/subsections/setup.rb @@ -8,12 +8,11 @@ class Form::Lettings::Subsections::Setup < ::Form::Subsection def pages @pages ||= [ - organisation_page, - stock_owner_page, + Form::Lettings::Pages::StockOwner.new(nil, nil, self), Form::Lettings::Pages::MinRentValueCheck.new("stock_owner_min_rent_value_check", nil, self), Form::Lettings::Pages::MaxRentValueCheck.new("stock_owner_max_rent_value_check", nil, self), - managing_organisation_page, - created_by_page, + Form::Lettings::Pages::ManagingOrganisation.new(nil, nil, self), + Form::Lettings::Pages::CreatedBy.new(nil, nil, self), Form::Lettings::Pages::NeedsType.new(nil, nil, self), Form::Lettings::Pages::Scheme.new(nil, nil, self), Form::Lettings::Pages::Location.new(nil, nil, self), @@ -34,32 +33,4 @@ class Form::Lettings::Subsections::Setup < ::Form::Subsection def enabled?(_lettings_log) true end - -private - - def organisation_page - return if FeatureToggle.managing_for_other_user_enabled? - - Form::Common::Pages::Organisation.new(nil, nil, self) - end - - def stock_owner_page - return unless FeatureToggle.managing_for_other_user_enabled? - - Form::Lettings::Pages::StockOwner.new(nil, nil, self) - end - - def managing_organisation_page - return unless FeatureToggle.managing_for_other_user_enabled? - - Form::Lettings::Pages::ManagingOrganisation.new(nil, nil, self) - end - - def created_by_page - if FeatureToggle.managing_for_other_user_enabled? - Form::Lettings::Pages::CreatedBy.new(nil, nil, self) - else - Form::Common::Pages::CreatedBy.new(nil, nil, self) - end - end end diff --git a/app/models/form/sales/pages/created_by.rb b/app/models/form/sales/pages/created_by.rb new file mode 100644 index 000000000..3b8fcb56c --- /dev/null +++ b/app/models/form/sales/pages/created_by.rb @@ -0,0 +1,19 @@ +class Form::Sales::Pages::CreatedBy < ::Form::Page + def initialize(id, hsh, subsection) + super + @id = "created_by" + end + + def questions + @questions ||= [ + Form::Sales::Questions::CreatedById.new(nil, nil, self), + ] + end + + def routed_to?(_log, current_user) + return true if current_user&.support? + return true if current_user&.data_coordinator? + + false + end +end diff --git a/app/models/form/sales/questions/created_by_id.rb b/app/models/form/sales/questions/created_by_id.rb new file mode 100644 index 000000000..44b64184e --- /dev/null +++ b/app/models/form/sales/questions/created_by_id.rb @@ -0,0 +1,54 @@ +class Form::Sales::Questions::CreatedById < ::Form::Question + ANSWER_OPTS = { "" => "Select an option" }.freeze + + def initialize(id, hsh, page) + super + @id = "created_by_id" + @check_answer_label = "User" + @header = "Which user are you creating this log for?" + @type = "select" + end + + def answer_options + ANSWER_OPTS + end + + def displayed_answer_options(log, current_user = nil) + return ANSWER_OPTS unless log.owning_organisation + return ANSWER_OPTS unless current_user + + users = current_user.support? ? log.owning_organisation.users : current_user.organisation.users + + users.each_with_object(ANSWER_OPTS.dup) do |user, hsh| + hsh[user.id] = present_user(user) + hsh + end + end + + def label_from_value(value, _log = nil, _user = nil) + return unless value + + present_user(User.find(value)) + end + + def hidden_in_check_answers?(_log, current_user) + return false if current_user.support? + return false if current_user.data_coordinator? + + true + end + + def derived? + true + end + +private + + def present_user(user) + "#{user.name} (#{user.email})" + end + + def selected_answer_option_is_derived?(_log) + false + end +end diff --git a/app/models/form/sales/subsections/setup.rb b/app/models/form/sales/subsections/setup.rb index c723b732c..4413575d6 100644 --- a/app/models/form/sales/subsections/setup.rb +++ b/app/models/form/sales/subsections/setup.rb @@ -8,7 +8,7 @@ class Form::Sales::Subsections::Setup < ::Form::Subsection def pages @pages ||= [ Form::Common::Pages::Organisation.new(nil, nil, self), - Form::Common::Pages::CreatedBy.new(nil, nil, self), + Form::Sales::Pages::CreatedBy.new(nil, nil, self), Form::Sales::Pages::SaleDate.new(nil, nil, self), Form::Sales::Pages::SaleDateCheck.new(nil, nil, self), Form::Sales::Pages::PurchaserCode.new(nil, nil, self), diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index de706585a..25c37b6bb 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -28,10 +28,6 @@ class FeatureToggle true end - def self.managing_for_other_user_enabled? - true - end - def self.bulk_upload_lettings_logs? true end diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb index 29f3324af..464f67d49 100644 --- a/spec/features/lettings_log_spec.rb +++ b/spec/features/lettings_log_spec.rb @@ -240,10 +240,12 @@ RSpec.describe "Lettings Log Features" do context "when completing the setup log section" do context "and there is at most 1 potential stock owner" do - it "does not include the owning organisation and created by questions" do + it "does not include the owning organisation and includes the created by questions" do visit("/lettings-logs") click_button("Create a new lettings log") click_link("Set up this lettings log") + select(user.name, from: "lettings-log-created-by-id-field") + click_button("Save and continue") log_id = page.current_path.scan(/\d/).join expect(page).to have_current_path("/lettings-logs/#{log_id}/needs-type") visit("lettings-logs/#{log_id}/setup/check-answers") diff --git a/spec/models/form/common/questions/created_by_id_spec.rb b/spec/models/form/common/questions/created_by_id_spec.rb deleted file mode 100644 index dde117f43..000000000 --- a/spec/models/form/common/questions/created_by_id_spec.rb +++ /dev/null @@ -1,82 +0,0 @@ -require "rails_helper" - -RSpec.describe Form::Common::Questions::CreatedById, type: :model do - subject(:question) { described_class.new(question_id, question_definition, page) } - - let(:question_id) { nil } - let(:question_definition) { nil } - let(:page) { instance_double(Form::Page) } - let(:subsection) { instance_double(Form::Subsection) } - let(:form) { instance_double(Form) } - let(:user_1) { FactoryBot.create(:user, name: "first user") } - let(:user_2) { FactoryBot.create(:user, name: "second user") } - let!(:expected_answer_options) do - { - "" => "Select an option", - user_1.id => user_1.name, - user_2.id => user_2.name, - } - end - - it "has correct page" do - expect(question.page).to eq(page) - end - - it "has the correct id" do - expect(question.id).to eq("created_by_id") - end - - it "has the correct header" do - expect(question.header).to eq("Which user are you creating this log for?") - end - - it "has the correct check_answer_label" do - expect(question.check_answer_label).to eq("User") - end - - it "has the correct type" do - expect(question.type).to eq("select") - end - - it "has the correct hint_text" do - expect(question.hint_text).to be_nil - end - - it "has the correct answer options" do - expect(question.answer_options).to eq(expected_answer_options) - end - - it "is marked as derived" do - expect(question.derived?).to be true - end - - context "when the current user is support" do - let(:support_user) { FactoryBot.build(:user, :support) } - - it "is shown in check answers" do - expect(question.hidden_in_check_answers?(nil, support_user)).to be false - end - end - - context "when the current user is not support" do - let(:user) { FactoryBot.build(:user) } - - it "is not shown in check answers" do - expect(question.hidden_in_check_answers?(nil, user)).to be true - end - end - - context "when the owning organisation is already set" do - let(:lettings_log) { FactoryBot.create(:lettings_log, owning_organisation: user_2.organisation) } - let(:expected_answer_options) do - { - "" => "Select an option", - user_2.id => user_2.name, - } - end - - it "only displays users that belong to that organisation" do - expect(question.displayed_answer_options(lettings_log)).to eq(expected_answer_options) - end - end -end diff --git a/spec/models/form/lettings/pages/created_by_spec.rb b/spec/models/form/lettings/pages/created_by_spec.rb index b1ac0853e..24b7b90d5 100644 --- a/spec/models/form/lettings/pages/created_by_spec.rb +++ b/spec/models/form/lettings/pages/created_by_spec.rb @@ -22,9 +22,15 @@ RSpec.describe Form::Lettings::Pages::CreatedBy, type: :model do end end - context "when not support" do + context "when data coordinator" do + it "is shown" do + expect(page.routed_to?(nil, create(:user, :data_coordinator))).to eq(true) + end + end + + context "when data provider" do it "is not shown" do - expect(page.routed_to?(nil, create(:user, :data_coordinator))).to eq(false) + expect(page.routed_to?(nil, create(:user))).to eq(false) end end end diff --git a/spec/models/form/lettings/questions/created_by_id_spec.rb b/spec/models/form/lettings/questions/created_by_id_spec.rb index c82ec13e1..0610e5b2a 100644 --- a/spec/models/form/lettings/questions/created_by_id_spec.rb +++ b/spec/models/form/lettings/questions/created_by_id_spec.rb @@ -8,16 +8,6 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do let(:page) { instance_double(Form::Page) } let(:subsection) { instance_double(Form::Subsection) } let(:form) { instance_double(Form) } - let(:user_1) { create(:user, name: "first user") } - let(:user_2) { create(:user, name: "second user") } - let(:user_3) { create(:user, name: "third user") } - let!(:expected_answer_options) do - { - "" => "Select an option", - user_1.id => "#{user_1.name} (#{user_1.email})", - user_2.id => "#{user_2.name} (#{user_2.email})", - } - end it "has correct page" do expect(question.page).to eq(page) @@ -44,7 +34,7 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do end it "has the correct answer options" do - expect(question.answer_options).to eq(expected_answer_options) + expect(question.answer_options).to eq({ "" => "Select an option" }) end it "is marked as derived" do @@ -52,40 +42,51 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do end context "when the current user is support" do - let(:support_user) { FactoryBot.build(:user, :support) } - - it "is shown in check answers" do - expect(question.hidden_in_check_answers?(nil, support_user)).to be false + let(:owning_org_user) { create(:user) } + let(:managing_org_user) { create(:user) } + let(:support_user) { create(:user, :support, organisation: owning_org_user.organisation) } + + describe "#displayed_answer_options" do + let(:lettings_log) do + create(:lettings_log, created_by: support_user, owning_organisation: owning_org_user.organisation, managing_organisation: managing_org_user.organisation) + end + + let(:expected_answer_options) do + { + "" => "Select an option", + managing_org_user.id => "#{managing_org_user.name} (#{managing_org_user.email})", + owning_org_user.id => "#{owning_org_user.name} (#{owning_org_user.email})", + support_user.id => "#{support_user.name} (#{support_user.email})", + } + end + + it "only displays users that belong to owning and managing organisations" do + expect(question.displayed_answer_options(lettings_log, support_user)).to eq(expected_answer_options) + end end end - context "when the current user is not support" do - let(:user) { FactoryBot.build(:user) } + context "when the current user is data_coordinator" do + let(:managing_org_user) { create(:user) } + let(:data_coordinator) { create(:user, :data_coordinator) } - it "is not shown in check answers" do - expect(question.hidden_in_check_answers?(nil, user)).to be true - end - end + describe "#displayed_answer_options" do + let(:lettings_log) do + create(:lettings_log, created_by: data_coordinator, owning_organisation: data_coordinator.organisation, managing_organisation: managing_org_user.organisation) + end - context "when the owning organisation is already set" do - let(:lettings_log) do - create( - :lettings_log, - created_by: user_2, - owning_organisation: user_2.organisation, - managing_organisation: user_3.organisation, - ) - end - let(:expected_answer_options) do - { - "" => "Select an option", - user_2.id => "#{user_2.name} (#{user_2.email})", - user_3.id => "#{user_3.name} (#{user_3.email})", - } - end + let(:user_in_same_org) { create(:user, organisation: data_coordinator.organisation) } + + let(:expected_answer_options) do + { + "" => "Select an option", + data_coordinator.id => "#{data_coordinator.name} (#{data_coordinator.email})", + } + end - it "only displays users that belong to that organisation" do - expect(question.displayed_answer_options(lettings_log)).to eq(expected_answer_options) + it "only displays users that belong user's org" do + expect(question.displayed_answer_options(lettings_log, data_coordinator)).to eq(expected_answer_options) + end end end end diff --git a/spec/models/form/common/pages/created_by_spec.rb b/spec/models/form/sales/pages/created_by_spec.rb similarity index 71% rename from spec/models/form/common/pages/created_by_spec.rb rename to spec/models/form/sales/pages/created_by_spec.rb index db9561ac2..2d3d7b7d7 100644 --- a/spec/models/form/common/pages/created_by_spec.rb +++ b/spec/models/form/sales/pages/created_by_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe Form::Common::Pages::CreatedBy, type: :model do +RSpec.describe Form::Sales::Pages::CreatedBy, type: :model do subject(:page) { described_class.new(page_id, page_definition, subsection) } let(:page_id) { nil } @@ -34,15 +34,23 @@ RSpec.describe Form::Common::Pages::CreatedBy, type: :model do end context "when the current user is a support user" do - let(:support_user) { FactoryBot.build(:user, :support) } + let(:support_user) { build(:user, :support) } it "is shown" do expect(page.routed_to?(lettings_log, support_user)).to be true end end - context "when the current user is not a support user" do - let(:user) { FactoryBot.build(:user) } + context "when the current user is a data coordinator" do + let(:support_user) { build(:user, :data_coordinator) } + + it "is shown" do + expect(page.routed_to?(lettings_log, support_user)).to be true + end + end + + context "when the current user is a data provider" do + let(:user) { build(:user) } it "is not shown" do expect(page.routed_to?(lettings_log, user)).to be false diff --git a/spec/models/form/sales/questions/created_by_id_spec.rb b/spec/models/form/sales/questions/created_by_id_spec.rb new file mode 100644 index 000000000..fd0122cb8 --- /dev/null +++ b/spec/models/form/sales/questions/created_by_id_spec.rb @@ -0,0 +1,88 @@ +require "rails_helper" + +RSpec.describe Form::Sales::Questions::CreatedById, type: :model do + subject(:question) { described_class.new(question_id, question_definition, page) } + + let(:question_id) { nil } + let(:question_definition) { nil } + let(:page) { instance_double(Form::Page) } + let(:subsection) { instance_double(Form::Subsection) } + let(:form) { instance_double(Form) } + + it "has correct page" do + expect(question.page).to eq(page) + end + + it "has the correct id" do + expect(question.id).to eq("created_by_id") + end + + it "has the correct header" do + expect(question.header).to eq("Which user are you creating this log for?") + end + + it "has the correct check_answer_label" do + expect(question.check_answer_label).to eq("User") + end + + it "has the correct type" do + expect(question.type).to eq("select") + end + + it "has the correct hint_text" do + expect(question.hint_text).to be_nil + end + + it "is marked as derived" do + expect(question.derived?).to be true + end + + context "when the current user is support" do + let(:support_user) { build(:user, :support) } + + it "is shown in check answers" do + expect(question.hidden_in_check_answers?(nil, support_user)).to be false + end + + describe "#displayed_answer_options" do + let(:owning_org_user) { create(:user) } + let(:sales_log) { create(:sales_log, owning_organisation: owning_org_user.organisation) } + let(:expected_answer_options) do + { + "" => "Select an option", + owning_org_user.id => "#{owning_org_user.name} (#{owning_org_user.email})", + } + end + + it "only displays users that belong to the owning organisation" do + expect(question.displayed_answer_options(sales_log, support_user)).to eq(expected_answer_options) + end + end + end + + context "when the current user is data_coordinator" do + let(:data_coordinator) { create(:user, :data_coordinator) } + + it "is shown in check answers" do + expect(question.hidden_in_check_answers?(nil, data_coordinator)).to be false + end + + describe "#displayed_answer_options" do + let(:owning_org_user) { create(:user) } + let(:sales_log) { create(:sales_log, owning_organisation: owning_org_user.organisation) } + let!(:user_in_same_org) { create(:user, organisation: data_coordinator.organisation) } + + let(:expected_answer_options) do + { + "" => "Select an option", + user_in_same_org.id => "#{user_in_same_org.name} (#{user_in_same_org.email})", + data_coordinator.id => "#{data_coordinator.name} (#{data_coordinator.email})", + } + end + + it "only displays users that belong user's org" do + expect(question.displayed_answer_options(sales_log, data_coordinator)).to eq(expected_answer_options) + end + end + end +end From 8ef0d1f2cd568b6787d5d26885f7ce61b0f8f356 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Fri, 28 Apr 2023 12:56:48 +0100 Subject: [PATCH 2/4] CLDC-2301 Update bulk upload journey copy (#1590) # Context - https://digital.dclg.gov.uk/jira/browse/CLDC-2301 - Content for 2022 and 2023 bulk upload journey need to differ to give users the correct info they need to complete the journey # Changes - Update 2022 and 2023 `Prepare your file` and guidance pages --- .../bulk_upload_lettings/prepare_your_file.rb | 19 ++++---- ...ml.erb => prepare_your_file_2022.html.erb} | 12 +++-- .../forms/prepare_your_file_2023.html.erb | 45 +++++++++++++++++++ .../bulk_upload_shared/guidance.html.erb | 15 +++++-- 4 files changed, 74 insertions(+), 17 deletions(-) rename app/views/bulk_upload_lettings_logs/forms/{prepare_your_file.html.erb => prepare_your_file_2022.html.erb} (77%) create mode 100644 app/views/bulk_upload_lettings_logs/forms/prepare_your_file_2023.html.erb diff --git a/app/models/forms/bulk_upload_lettings/prepare_your_file.rb b/app/models/forms/bulk_upload_lettings/prepare_your_file.rb index b5d28a7a2..26032e614 100644 --- a/app/models/forms/bulk_upload_lettings/prepare_your_file.rb +++ b/app/models/forms/bulk_upload_lettings/prepare_your_file.rb @@ -9,7 +9,12 @@ module Forms attribute :needstype, :integer def view_path - "bulk_upload_lettings_logs/forms/prepare_your_file" + case year + when 2022 + "bulk_upload_lettings_logs/forms/prepare_your_file_2022" + else + "bulk_upload_lettings_logs/forms/prepare_your_file_2023" + end end def back_path @@ -29,7 +34,7 @@ module Forms case year when 2022 "/files/bulk-upload-lettings-template-2022-23.xlsx" - when 2023 + else "/files/bulk-upload-lettings-legacy-template-2023-24.xlsx" end end @@ -38,7 +43,7 @@ module Forms case year when 2022 "/files/bulk-upload-lettings-template-2022-23.xlsx" - when 2023 + else "/files/bulk-upload-lettings-template-2023-24.xlsx" end end @@ -47,7 +52,7 @@ module Forms case year when 2022 "/files/bulk-upload-lettings-specification-2022-23.xlsx" - when 2023 + else "/files/bulk-upload-lettings-specification-2023-24.xlsx" end end @@ -56,12 +61,6 @@ module Forms "#{year}/#{year + 1 - 2000}" end - def inset_text - if year == 2022 - '

For 2022/23 data, you cannot have a CSV file with both general needs logs and supported housing logs. These must be in separate files.

'.html_safe - end - end - def save! true end diff --git a/app/views/bulk_upload_lettings_logs/forms/prepare_your_file.html.erb b/app/views/bulk_upload_lettings_logs/forms/prepare_your_file_2022.html.erb similarity index 77% rename from app/views/bulk_upload_lettings_logs/forms/prepare_your_file.html.erb rename to app/views/bulk_upload_lettings_logs/forms/prepare_your_file_2022.html.erb index 45c2be562..632f320a2 100644 --- a/app/views/bulk_upload_lettings_logs/forms/prepare_your_file.html.erb +++ b/app/views/bulk_upload_lettings_logs/forms/prepare_your_file_2022.html.erb @@ -11,27 +11,31 @@

Prepare your file

Download template

+

Create your file

+ - <%= @form.inset_text %> + <%= govuk_inset_text(text: "Upload separate files for general needs and supported housing logs for 2022/23 data.") %>

Save your file

+ - <%= f.govuk_submit %> + <%= f.govuk_submit class: "govuk-!-margin-top-7" %> <% end %> diff --git a/app/views/bulk_upload_lettings_logs/forms/prepare_your_file_2023.html.erb b/app/views/bulk_upload_lettings_logs/forms/prepare_your_file_2023.html.erb new file mode 100644 index 000000000..a1d25397b --- /dev/null +++ b/app/views/bulk_upload_lettings_logs/forms/prepare_your_file_2023.html.erb @@ -0,0 +1,45 @@ +<% content_for :before_content do %> + <%= govuk_back_link href: @form.back_path %> +<% end %> + +
+
+ <%= form_with model: @form, scope: :form, url: bulk_upload_lettings_log_path(id: "prepare-your-file"), method: :patch do |f| %> + <%= f.hidden_field :year %> + + Upload lettings logs in bulk (<%= @form.year_combo %>) +

Prepare your file

+ +

Download template

+ +
    +
  • + If your organisation is new to using bulk upload or you have updated your HMS export, use <%= govuk_link_to "this template (improved question ordering)", @form.template_path %>. +
    • + +
  • + If your organisation was using bulk upload on the previous CORE website and hasn't changed its HMS to be compatible with the new CORE service, use <%= govuk_link_to "this template", @form.old_template_path %>. +
    • +
+ +

Create your file

+ +
    +
  • Fill in the template with CORE data from your housing management system according to the <%= govuk_link_to "Lettings #{@form.year_combo} Bulk Upload Specification", @form.specification_path %>
    • +
  • Username field: To assign a log to someone else, enter the email address they use to log into CORE
    • +
  • If you have to manually enter large volumes of data into the bulk upload template, we recommend creating logs directly in the service instead. <%= govuk_link_to "Find out more about exporting your data", bulk_upload_lettings_log_path(id: "guidance", form: { year: @form.year }) %>
    • +
+ + <%= govuk_inset_text(text: "You can upload both general needs and supported housing logs in the same file for 2023/24 data.") %> + +

Save your file

+ +
    +
  • Save your file as a CSV
    • +
  • Your file should now be ready to upload
    • +
+ + <%= f.govuk_submit class: "govuk-!-margin-top-7" %> + <% end %> +
+
diff --git a/app/views/bulk_upload_shared/guidance.html.erb b/app/views/bulk_upload_shared/guidance.html.erb index 7b4048469..9d2147105 100644 --- a/app/views/bulk_upload_shared/guidance.html.erb +++ b/app/views/bulk_upload_shared/guidance.html.erb @@ -26,20 +26,29 @@

Exporting CSV data

+

Export CSV data directly from your current systems, or export then adjust it to CSV.

You can then upload it via a button at the top of the lettings and sales logs pages.

- <%= govuk_details(summary_text: "My organisation has a CMS") do %> + + <%= govuk_details(summary_text: "My organisation has a HMS") do %>

Some HMS providers sell an add-on "eCORE" module, which exports CSV data for you.

It can take HMS providers a while to update these per new collection year, so you may have to wait for updates to export, or adjust your data manually post-export.

<% end %> - <%= govuk_details(summary_text: "My organisation does not have a CMS") do %> + + <%= govuk_details(summary_text: "My organisation does not have a HMS") do %>

Your organisation’s IT team may be able to export CSV data for you - <%= govuk_link_to "find out more about data specification", @form.specification_path, target: "_blank" %>. This document outlines:

+ -

Fields can appear in any order, as long as you include the <%= govuk_link_to "template document", @form.template_path %> headers, to easily identify what each column represents. You can rearrange data columns to match your system exports, copy-pasting multiple columns at once. For data stored in multiple systems, you can copy-paste all columns for one system next to each other, repeating this for subsequent system exports.

+ + <% if @form.year == 2022 %> +

Fields must be in the same order as <%= govuk_link_to "the template", @form.template_path %>. Copy each column of your exported data one at a time and paste into the correct template column.

+ <% else %> +

Fields can appear in any order, as long as you include the <%= govuk_link_to "template document", @form.template_path %> headers, to easily identify what each column represents. You can rearrange data columns to match your system exports, copy-pasting multiple columns at once. For data stored in multiple systems, you can copy-paste all columns for one system next to each other, repeating this for subsequent system exports.

+ <% end %> <% end %>
From 45128b7250225fa815a2a3e5cb963eabb210050d Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Fri, 28 Apr 2023 15:35:40 +0100 Subject: [PATCH 3/4] Refactor with split of log to CSV by log type (#1593) # Context - Partially related to https://digital.dclg.gov.uk/jira/browse/CLDC-2316 - Comprehending sales or lettings bulk upload CSV is enough, comprehending both simultaneously is rather challenging # Changes - Split out test helper class by log type ie lettings/sales --- .../bulk_upload/lettings/log_creator_spec.rb | 8 +- .../bulk_upload/lettings/validator_spec.rb | 30 +-- .../lettings/year2022/csv_parser_spec.rb | 30 +-- .../lettings/year2023/csv_parser_spec.rb | 30 +-- spec/services/bulk_upload/processor_spec.rb | 6 +- .../bulk_upload/sales/log_creator_spec.rb | 8 +- .../bulk_upload/sales/validator_spec.rb | 34 ++-- .../sales/year2022/csv_parser_spec.rb | 8 +- .../{log_to_csv.rb => lettings_log_to_csv.rb} | 153 +------------- spec/support/bulk_upload/sales_log_to_csv.rb | 187 ++++++++++++++++++ 10 files changed, 265 insertions(+), 229 deletions(-) rename spec/support/bulk_upload/{log_to_csv.rb => lettings_log_to_csv.rb} (68%) create mode 100644 spec/support/bulk_upload/sales_log_to_csv.rb diff --git a/spec/services/bulk_upload/lettings/log_creator_spec.rb b/spec/services/bulk_upload/lettings/log_creator_spec.rb index 2f05a0ba5..adf4d3845 100644 --- a/spec/services/bulk_upload/lettings/log_creator_spec.rb +++ b/spec/services/bulk_upload/lettings/log_creator_spec.rb @@ -35,9 +35,9 @@ RSpec.describe BulkUpload::Lettings::LogCreator do let(:log) { LettingsLog.new } before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end @@ -66,7 +66,7 @@ RSpec.describe BulkUpload::Lettings::LogCreator do end before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index 679602ec0..10adcb86a 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/spec/services/bulk_upload/lettings/validator_spec.rb @@ -50,7 +50,7 @@ RSpec.describe BulkUpload::Lettings::Validator do context "when file has headers" do context "and file has extra invalid headers" do let(:seed) { rand } - let(:log_to_csv) { BulkUpload::LogToCsv.new(log:) } + let(:log_to_csv) { BulkUpload::LettingsLogToCsv.new(log:) } let(:field_numbers) { log_to_csv.default_2022_field_numbers + %w[invalid_field_number] } let(:field_values) { log_to_csv.to_2022_row + %w[value_for_invalid_field_number] } @@ -67,7 +67,7 @@ RSpec.describe BulkUpload::Lettings::Validator do context "and file has too few valid headers" do let(:seed) { rand } - let(:log_to_csv) { BulkUpload::LogToCsv.new(log:) } + let(:log_to_csv) { BulkUpload::LettingsLogToCsv.new(log:) } let(:field_numbers) { log_to_csv.default_2022_field_numbers } let(:field_values) { log_to_csv.to_2022_row } @@ -87,7 +87,7 @@ RSpec.describe BulkUpload::Lettings::Validator do context "and file has too many valid headers" do let(:seed) { rand } - let(:log_to_csv) { BulkUpload::LogToCsv.new(log:) } + let(:log_to_csv) { BulkUpload::LettingsLogToCsv.new(log:) } let(:field_numbers) { log_to_csv.default_2022_field_numbers + %w[23] } let(:field_values) { log_to_csv.to_2022_row + %w[value] } @@ -138,7 +138,7 @@ RSpec.describe BulkUpload::Lettings::Validator do context "when file has headers" do context "and file has extra invalid headers" do let(:seed) { rand } - let(:log_to_csv) { BulkUpload::LogToCsv.new(log:) } + let(:log_to_csv) { BulkUpload::LettingsLogToCsv.new(log:) } let(:field_numbers) { log_to_csv.default_2023_field_numbers + %w[invalid_field_number] } let(:field_values) { log_to_csv.to_2023_row + %w[value_for_invalid_field_number] } @@ -155,7 +155,7 @@ RSpec.describe BulkUpload::Lettings::Validator do context "and file has too few valid headers" do let(:seed) { rand } - let(:log_to_csv) { BulkUpload::LogToCsv.new(log:) } + let(:log_to_csv) { BulkUpload::LettingsLogToCsv.new(log:) } let(:field_numbers) { log_to_csv.default_2023_field_numbers } let(:field_values) { log_to_csv.to_2023_row } @@ -175,7 +175,7 @@ RSpec.describe BulkUpload::Lettings::Validator do context "and file has too many valid headers" do let(:seed) { rand } - let(:log_to_csv) { BulkUpload::LogToCsv.new(log:) } + let(:log_to_csv) { BulkUpload::LettingsLogToCsv.new(log:) } let(:field_numbers) { log_to_csv.default_2023_field_numbers + %w[23] } let(:field_values) { log_to_csv.to_2023_row + %w[value] } @@ -233,8 +233,8 @@ RSpec.describe BulkUpload::Lettings::Validator do end before do - file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n").default_2023_field_numbers_row(seed:)) - file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n").to_2023_csv_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:, line_ending: "\r\n").default_2023_field_numbers_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:, line_ending: "\r\n").to_2023_csv_row(seed:)) file.close end @@ -280,7 +280,7 @@ RSpec.describe BulkUpload::Lettings::Validator do let(:path) { file.path } before do - file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end @@ -322,8 +322,8 @@ RSpec.describe BulkUpload::Lettings::Validator do let(:log_2) { build(:lettings_log, :completed, created_by: user) } before do - file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0, overrides: { illness: 100 }).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0, overrides: { illness: 100 }).to_2022_csv_row) file.close end @@ -338,8 +338,8 @@ RSpec.describe BulkUpload::Lettings::Validator do let(:log_2) { build(:lettings_log, :completed, renttype: 1, created_by: user) } before do - file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end @@ -355,7 +355,7 @@ RSpec.describe BulkUpload::Lettings::Validator do let(:log_1) { build(:lettings_log, :completed, renttype: 1, created_by: user, owning_organisation: unaffiliated_org) } before do - file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end @@ -369,7 +369,7 @@ RSpec.describe BulkUpload::Lettings::Validator do let(:log) { build(:lettings_log, :in_progress, created_by: user, startdate: Time.zone.local(2022, 5, 1)) } before do - file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end diff --git a/spec/services/bulk_upload/lettings/year2022/csv_parser_spec.rb b/spec/services/bulk_upload/lettings/year2022/csv_parser_spec.rb index 10e722e9a..cae0285bf 100644 --- a/spec/services/bulk_upload/lettings/year2022/csv_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2022/csv_parser_spec.rb @@ -9,8 +9,8 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do context "when parsing csv with headers" do before do - file.write(BulkUpload::LogToCsv.new(log:).default_2022_field_numbers_row) - file.write(BulkUpload::LogToCsv.new(log:).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2022_field_numbers_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2022_csv_row) file.rewind end @@ -27,8 +27,8 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do context "when parsing csv with headers with extra rows" do before do file.write("Extra row\n") - file.write(BulkUpload::LogToCsv.new(log:).default_2022_field_numbers_row) - file.write(BulkUpload::LogToCsv.new(log:).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2022_field_numbers_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2022_csv_row) file.rewind end @@ -46,8 +46,8 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do let(:seed) { rand } before do - file.write(BulkUpload::LogToCsv.new(log:).default_2022_field_numbers_row(seed:)) - file.write(BulkUpload::LogToCsv.new(log:).to_2022_csv_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2022_field_numbers_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2022_csv_row(seed:)) file.rewind end @@ -63,7 +63,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do context "when parsing csv with extra invalid headers" do let(:seed) { rand } - let(:log_to_csv) { BulkUpload::LogToCsv.new(log:) } + let(:log_to_csv) { BulkUpload::LettingsLogToCsv.new(log:) } let(:field_numbers) { log_to_csv.default_2022_field_numbers + %w[invalid_field_number] } let(:field_values) { log_to_csv.to_2022_row + %w[value_for_invalid_field_number] } @@ -84,7 +84,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do context "when parsing csv without headers" do before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end @@ -103,7 +103,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do before do file.write(bom) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end @@ -117,7 +117,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do before do file.write(invalid_sequence) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end @@ -129,8 +129,8 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do describe "#column_for_field", aggregate_failures: true do context "when with headers using default ordering" do before do - file.write(BulkUpload::LogToCsv.new(log:).default_2022_field_numbers_row) - file.write(BulkUpload::LogToCsv.new(log:).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2022_field_numbers_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2022_csv_row) file.rewind end @@ -142,7 +142,7 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do context "when without headers using default ordering" do before do - file.write(BulkUpload::LogToCsv.new(log:).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2022_csv_row) file.rewind end @@ -156,8 +156,8 @@ RSpec.describe BulkUpload::Lettings::Year2022::CsvParser do let(:seed) { 123 } before do - file.write(BulkUpload::LogToCsv.new(log:).default_2022_field_numbers_row(seed:)) - file.write(BulkUpload::LogToCsv.new(log:).to_2022_csv_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2022_field_numbers_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2022_csv_row(seed:)) file.rewind end diff --git a/spec/services/bulk_upload/lettings/year2023/csv_parser_spec.rb b/spec/services/bulk_upload/lettings/year2023/csv_parser_spec.rb index fc82faa8d..c94bdae35 100644 --- a/spec/services/bulk_upload/lettings/year2023/csv_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2023/csv_parser_spec.rb @@ -15,8 +15,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do file.write("Can be empty?\n") file.write("Type of letting the question applies to\n") file.write("Duplicate check field?\n") - file.write(BulkUpload::LogToCsv.new(log:).default_2023_field_numbers_row) - file.write(BulkUpload::LogToCsv.new(log:).to_2023_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2023_field_numbers_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2023_csv_row) file.rewind end @@ -39,8 +39,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do file.write("Can be empty?\n") file.write("Type of letting the question applies to\n") file.write("Duplicate check field?\n") - file.write(BulkUpload::LogToCsv.new(log:).default_2023_field_numbers_row) - file.write(BulkUpload::LogToCsv.new(log:).to_2023_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2023_field_numbers_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2023_csv_row) file.rewind end @@ -64,8 +64,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do file.write("Can be empty?\n") file.write("Type of letting the question applies to\n") file.write("Duplicate check field?\n") - file.write(BulkUpload::LogToCsv.new(log:).default_2023_field_numbers_row(seed:)) - file.write(BulkUpload::LogToCsv.new(log:).to_2023_csv_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2023_field_numbers_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2023_csv_row(seed:)) file.rewind end @@ -81,7 +81,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do context "when parsing csv with extra invalid headers" do let(:seed) { rand } - let(:log_to_csv) { BulkUpload::LogToCsv.new(log:) } + let(:log_to_csv) { BulkUpload::LettingsLogToCsv.new(log:) } let(:field_numbers) { log_to_csv.default_2023_field_numbers + %w[invalid_field_number] } let(:field_values) { log_to_csv.to_2023_row + %w[value_for_invalid_field_number] } @@ -108,7 +108,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do context "when parsing csv without headers" do before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2023_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2023_csv_row) file.rewind end @@ -127,7 +127,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do before do file.write(bom) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2023_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2023_csv_row) file.rewind end @@ -141,7 +141,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do before do file.write(invalid_sequence) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2023_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2023_csv_row) file.rewind end @@ -159,8 +159,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do file.write("Can be empty?\n") file.write("Type of letting the question applies to\n") file.write("Duplicate check field?\n") - file.write(BulkUpload::LogToCsv.new(log:).default_2023_field_numbers_row) - file.write(BulkUpload::LogToCsv.new(log:).to_2023_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2023_field_numbers_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2023_csv_row) file.rewind end @@ -172,7 +172,7 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do context "when without headers using default ordering" do before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2023_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2023_csv_row) file.rewind end @@ -192,8 +192,8 @@ RSpec.describe BulkUpload::Lettings::Year2023::CsvParser do file.write("Can be empty?\n") file.write("Type of letting the question applies to\n") file.write("Duplicate check field?\n") - file.write(BulkUpload::LogToCsv.new(log:).default_2023_field_numbers_row(seed:)) - file.write(BulkUpload::LogToCsv.new(log:).to_2023_csv_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:).default_2023_field_numbers_row(seed:)) + file.write(BulkUpload::LettingsLogToCsv.new(log:).to_2023_csv_row(seed:)) file.rewind end diff --git a/spec/services/bulk_upload/processor_spec.rb b/spec/services/bulk_upload/processor_spec.rb index 64ee0e197..0c9d50be8 100644 --- a/spec/services/bulk_upload/processor_spec.rb +++ b/spec/services/bulk_upload/processor_spec.rb @@ -165,7 +165,7 @@ RSpec.describe BulkUpload::Processor do end before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) @@ -213,7 +213,7 @@ RSpec.describe BulkUpload::Processor do end before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) @@ -265,7 +265,7 @@ RSpec.describe BulkUpload::Processor do end before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::LettingsLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) diff --git a/spec/services/bulk_upload/sales/log_creator_spec.rb b/spec/services/bulk_upload/sales/log_creator_spec.rb index 4ec1a07fd..440fcca16 100644 --- a/spec/services/bulk_upload/sales/log_creator_spec.rb +++ b/spec/services/bulk_upload/sales/log_creator_spec.rb @@ -35,9 +35,9 @@ RSpec.describe BulkUpload::Sales::LogCreator do let(:log) { SalesLog.new } before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end @@ -59,7 +59,7 @@ RSpec.describe BulkUpload::Sales::LogCreator do end before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end diff --git a/spec/services/bulk_upload/sales/validator_spec.rb b/spec/services/bulk_upload/sales/validator_spec.rb index c2236f38d..f56c8a95b 100644 --- a/spec/services/bulk_upload/sales/validator_spec.rb +++ b/spec/services/bulk_upload/sales/validator_spec.rb @@ -87,7 +87,7 @@ RSpec.describe BulkUpload::Sales::Validator do let(:path) { file.path } before do - file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end @@ -129,8 +129,8 @@ RSpec.describe BulkUpload::Sales::Validator do let(:log_2) { build(:sales_log, :completed, created_by: user) } before do - file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0, overrides: { organisation_id: "random" }).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0, overrides: { organisation_id: "random" }).to_2022_csv_row) file.close end @@ -145,8 +145,8 @@ RSpec.describe BulkUpload::Sales::Validator do let(:log_2) { build(:sales_log, :completed, created_by: user) } before do - file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end @@ -162,7 +162,7 @@ RSpec.describe BulkUpload::Sales::Validator do let(:log_1) { build(:sales_log, :completed, created_by: user, owning_organisation: unaffiliated_org) } before do - file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end @@ -176,7 +176,7 @@ RSpec.describe BulkUpload::Sales::Validator do let(:log) { build(:sales_log, created_by: user, saledate: Time.zone.local(2022, 5, 1)) } before do - file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end @@ -195,11 +195,11 @@ RSpec.describe BulkUpload::Sales::Validator do let(:log_5) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) } before do - file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_3, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_4, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_5, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_3, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_4, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_5, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end @@ -217,11 +217,11 @@ RSpec.describe BulkUpload::Sales::Validator do let(:log_5) { build(:sales_log, :in_progress, created_by: user, saledate: Time.zone.local(2022, 5, 1)) } before do - file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_3, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_4, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) - file.write(BulkUpload::LogToCsv.new(log: log_5, line_ending: "\r\n", col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_2, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_3, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_4, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log: log_5, line_ending: "\r\n", col_offset: 0).to_2022_csv_row) file.close end diff --git a/spec/services/bulk_upload/sales/year2022/csv_parser_spec.rb b/spec/services/bulk_upload/sales/year2022/csv_parser_spec.rb index cabcefc1a..2d65075b5 100644 --- a/spec/services/bulk_upload/sales/year2022/csv_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2022/csv_parser_spec.rb @@ -22,7 +22,7 @@ RSpec.describe BulkUpload::Sales::Year2022::CsvParser do let(:log) { build(:sales_log, :completed) } before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end @@ -44,7 +44,7 @@ RSpec.describe BulkUpload::Sales::Year2022::CsvParser do before do file.write(bom) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.close end @@ -61,7 +61,7 @@ RSpec.describe BulkUpload::Sales::Year2022::CsvParser do before do file.write(invalid_sequence) - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.close end @@ -84,7 +84,7 @@ RSpec.describe BulkUpload::Sales::Year2022::CsvParser do let(:log) { build(:sales_log, :completed) } before do - file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_2022_sales_csv_row) + file.write(BulkUpload::SalesLogToCsv.new(log:, col_offset: 0).to_2022_csv_row) file.rewind end diff --git a/spec/support/bulk_upload/log_to_csv.rb b/spec/support/bulk_upload/lettings_log_to_csv.rb similarity index 68% rename from spec/support/bulk_upload/log_to_csv.rb rename to spec/support/bulk_upload/lettings_log_to_csv.rb index 6064e57e3..6d3110b2a 100644 --- a/spec/support/bulk_upload/log_to_csv.rb +++ b/spec/support/bulk_upload/lettings_log_to_csv.rb @@ -1,4 +1,4 @@ -class BulkUpload::LogToCsv +class BulkUpload::LettingsLogToCsv attr_reader :log, :line_ending, :col_offset, :overrides def initialize(log:, line_ending: "\n", col_offset: 1, overrides: {}) @@ -21,10 +21,6 @@ class BulkUpload::LogToCsv end end - def to_2022_sales_csv_row - (row_prefix + to_2022_sales_row).flatten.join(",") + line_ending - end - def default_2022_field_numbers (1..134).to_a end @@ -234,153 +230,6 @@ class BulkUpload::LogToCsv (row_prefix + row).flatten.join(",") + line_ending end - def to_2022_sales_row - [ - log.purchid, # 1 - log.saledate&.day, - log.saledate&.month, - log.saledate&.strftime("%y"), - nil, - log.noint, - log.age1, - log.age2, - log.age3, - log.age4, - log.age5, - log.age6, - - log.sex1, - log.sex2, - log.sex3, - log.sex4, - log.sex5, - log.sex6, - - log.relat2, - log.relat3, # 20 - log.relat4, - log.relat5, - log.relat6, - - log.ecstat1, - log.ecstat2, - log.ecstat3, - log.ecstat4, - log.ecstat5, - log.ecstat6, - - log.ethnic, # 30 - log.national, - log.income1, - log.income2, - log.inc1mort, - log.inc2mort, - log.savings, - log.prevown, - nil, - - log.prevten, - log.prevloc, # 40 - ((log.ppostcode_full || "").split(" ") || [""]).first, - ((log.ppostcode_full || "").split(" ") || [""]).last, - log.ppcodenk == 0 ? 1 : nil, - - log.pregyrha, - log.pregla, - log.pregghb, - log.pregother, - - log.disabled, - log.wheel, - log.beds, # 50 - log.proptype, - log.builtype, - log.la, - ((log.postcode_full || "").split(" ") || [""]).first, - ((log.postcode_full || "").split(" ") || [""]).last, - log.wchair, - - log.type, # shared ownership - log.resale, - log.hodate&.day, - log.hodate&.month, # 60 - log.hodate&.strftime("%y"), - log.exdate&.day, - log.exdate&.month, - log.exdate&.strftime("%y"), - log.lanomagr, - - log.frombeds, - log.fromprop, - - log.value, - log.equity, - log.mortgage, # 70 - log.extrabor, - log.deposit, - log.cashdis, - - log.mrent, - log.mscharge, - - log.type, # discounted ownership - log.value, - log.grant, - log.discount, - log.mortgage, # 80 - log.extrabor, - log.deposit, - log.mscharge, - - log.type, # outright sale - log.othtype, - nil, - - log.value, - log.mortgage, - log.extrabor, - log.deposit, # 90 - log.mscharge, - - overrides[:organisation_id] || log.owning_organisation&.old_visible_id, - log.created_by&.email, - nil, - hhregres, - nil, - log.armedforcesspouse, - log.mortgagelender, # shared ownership - log.mortgagelenderother, - log.mortgagelender, # discounted ownership 100 - log.mortgagelenderother, - log.mortgagelender, # outright ownership - log.mortgagelenderother, - - log.hb, - log.mortlen, # shared ownership - log.mortlen, # discounted ownership - log.mortlen, # outright ownership - - log.proplen, # discounted ownership - log.jointmore, - log.proplen, # shared ownership 110 - log.staircase, - log.privacynotice, - log.ownershipsch, - log.companybuy, # outright sale - log.buylivein, - log.jointpur, - log.buy1livein, - log.buy2livein, - log.hholdcount, - log.stairbought, # 120 - log.stairowned, - log.socprevten, - log.mortgageused, # shared ownership - log.mortgageused, # discounted ownership - log.mortgageused, # outright ownership - ] - end - private def renewal diff --git a/spec/support/bulk_upload/sales_log_to_csv.rb b/spec/support/bulk_upload/sales_log_to_csv.rb new file mode 100644 index 000000000..650eaad65 --- /dev/null +++ b/spec/support/bulk_upload/sales_log_to_csv.rb @@ -0,0 +1,187 @@ +class BulkUpload::SalesLogToCsv + attr_reader :log, :line_ending, :col_offset, :overrides + + def initialize(log:, line_ending: "\n", col_offset: 1, overrides: {}) + @log = log + @line_ending = line_ending + @col_offset = col_offset + @overrides = overrides + end + + def row_prefix + [nil] * col_offset + end + + def to_2022_csv_row + (row_prefix + to_2022_row).flatten.join(",") + line_ending + end + + def default_2022_field_numbers + (1..125).to_a + end + + def default_2022_field_numbers_row(seed: nil) + if seed + ["Bulk upload field number"] + default_2022_field_numbers.shuffle(random: Random.new(seed)) + else + ["Bulk upload field number"] + default_2022_field_numbers + end.flatten.join(",") + line_ending + end + + def to_2022_row + [ + log.purchid, # 1 + log.saledate&.day, + log.saledate&.month, + log.saledate&.strftime("%y"), + nil, + log.noint, + log.age1, + log.age2, + log.age3, + log.age4, + log.age5, + log.age6, + + log.sex1, + log.sex2, + log.sex3, + log.sex4, + log.sex5, + log.sex6, + + log.relat2, + log.relat3, # 20 + log.relat4, + log.relat5, + log.relat6, + + log.ecstat1, + log.ecstat2, + log.ecstat3, + log.ecstat4, + log.ecstat5, + log.ecstat6, + + log.ethnic, # 30 + log.national, + log.income1, + log.income2, + log.inc1mort, + log.inc2mort, + log.savings, + log.prevown, + nil, + + log.prevten, + log.prevloc, # 40 + ((log.ppostcode_full || "").split(" ") || [""]).first, + ((log.ppostcode_full || "").split(" ") || [""]).last, + log.ppcodenk == 0 ? 1 : nil, + + log.pregyrha, + log.pregla, + log.pregghb, + log.pregother, + + log.disabled, + log.wheel, + log.beds, # 50 + log.proptype, + log.builtype, + log.la, + ((log.postcode_full || "").split(" ") || [""]).first, + ((log.postcode_full || "").split(" ") || [""]).last, + log.wchair, + + log.type, # shared ownership + log.resale, + log.hodate&.day, + log.hodate&.month, # 60 + log.hodate&.strftime("%y"), + log.exdate&.day, + log.exdate&.month, + log.exdate&.strftime("%y"), + log.lanomagr, + + log.frombeds, + log.fromprop, + + log.value, + log.equity, + log.mortgage, # 70 + log.extrabor, + log.deposit, + log.cashdis, + + log.mrent, + log.mscharge, + + log.type, # discounted ownership + log.value, + log.grant, + log.discount, + log.mortgage, # 80 + log.extrabor, + log.deposit, + log.mscharge, + + log.type, # outright sale + log.othtype, + nil, + + log.value, + log.mortgage, + log.extrabor, + log.deposit, # 90 + log.mscharge, + + overrides[:organisation_id] || log.owning_organisation&.old_visible_id, + log.created_by&.email, + nil, + hhregres, + nil, + log.armedforcesspouse, + log.mortgagelender, # shared ownership + log.mortgagelenderother, + log.mortgagelender, # discounted ownership 100 + log.mortgagelenderother, + log.mortgagelender, # outright ownership + log.mortgagelenderother, + + log.hb, + log.mortlen, # shared ownership + log.mortlen, # discounted ownership + log.mortlen, # outright ownership + + log.proplen, # discounted ownership + log.jointmore, + log.proplen, # shared ownership 110 + log.staircase, + log.privacynotice, + log.ownershipsch, + log.companybuy, # outright sale + log.buylivein, + log.jointpur, + log.buy1livein, + log.buy2livein, + log.hholdcount, + log.stairbought, # 120 + log.stairowned, + log.socprevten, + log.mortgageused, # shared ownership + log.mortgageused, # discounted ownership + log.mortgageused, # outright ownership + ] + end + +private + + def hhregres + if log.hhregres == 1 + log.hhregresstill + else + log.hhregres + end + end +end From 751f8661db69276d4b12810fb99d1b6910755169 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Fri, 28 Apr 2023 15:53:11 +0100 Subject: [PATCH 4/4] CLDC-2057 Telephone number confirmation (#1595) * CLDC-2057 Merge request: ask to confirm number * address comments --- app/controllers/merge_requests_controller.rb | 26 +- .../absorbing_organisation.html.erb | 13 +- .../confirm_telephone_number.html.erb | 31 ++- app/views/merge_requests/merge_date.html.erb | 8 + .../merge_requests/organisations.html.erb | 16 +- config/locales/en.yml | 2 + config/routes.rb | 1 + ...30334_add_phone_number_to_merge_request.rb | 6 + db/schema.rb | 4 +- .../merge_requests_controller_spec.rb | 236 ++++++++++++++---- 10 files changed, 276 insertions(+), 67 deletions(-) create mode 100644 app/views/merge_requests/merge_date.html.erb create mode 100644 db/migrate/20230427130334_add_phone_number_to_merge_request.rb diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index 047925722..930a79b04 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -7,6 +7,7 @@ class MergeRequestsController < ApplicationController absorbing_organisation confirm_telephone_number new_org_name + merge_date ] before_action :authenticate_user! before_action :authenticate_scope!, except: [:create] @@ -15,6 +16,7 @@ class MergeRequestsController < ApplicationController def absorbing_organisation; end def confirm_telephone_number; end def new_org_name; end + def merge_date; end def create ActiveRecord::Base.transaction do @@ -70,6 +72,8 @@ private end when "organisations" absorbing_organisation_merge_request_path(@merge_request) + when "confirm_telephone_number" + merge_date_merge_request_path(@merge_request) end end @@ -96,6 +100,8 @@ private :other_merging_organisations, :status, :absorbing_organisation_id, + :telephone_number_correct, + :new_telephone_number, ) if merge_params[:requesting_organisation_id].present? && (current_user.data_coordinator? || current_user.data_provider?) @@ -111,13 +117,31 @@ private end end + if merge_params[:telephone_number_correct] == "1" + merge_params[:new_telephone_number] = nil + end + merge_params end def validate_response if page == "absorbing_organisation" && merge_request_params[:absorbing_organisation_id].blank? && merge_request_params[:new_absorbing_organisation].blank? @merge_request.errors.add(:absorbing_organisation_id, I18n.t("validations.merge_request.absorbing_organisation_blank")) - render previous_template + render previous_template, status: :unprocessable_entity + end + + if page == "confirm_telephone_number" + if merge_request_params[:telephone_number_correct].blank? && merge_request_params[:new_telephone_number].blank? + if @merge_request.absorbing_organisation.phone.present? + @merge_request.errors.add(:telephone_number_correct, I18n.t("validations.merge_request.telephone_number_correct_blank")) + else + @merge_request.errors.add(:telephone_number_correct, I18n.t("validations.merge_request.new_telephone_number_blank")) + end + render previous_template, status: :unprocessable_entity + elsif merge_request_params[:telephone_number_correct] == "0" && merge_request_params[:new_telephone_number].blank? + @merge_request.errors.add(:new_telephone_number, I18n.t("validations.merge_request.new_telephone_number_blank")) + render previous_template, status: :unprocessable_entity + end end end diff --git a/app/views/merge_requests/absorbing_organisation.html.erb b/app/views/merge_requests/absorbing_organisation.html.erb index cd2441d2b..e63154bac 100644 --- a/app/views/merge_requests/absorbing_organisation.html.erb +++ b/app/views/merge_requests/absorbing_organisation.html.erb @@ -4,14 +4,13 @@ <%= govuk_back_link href: organisations_merge_request_path(id: @merge_request) %> <% end %> -

Which organisation is absorbing the others?

+<%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> + <%= f.govuk_error_summary %> -
-
-

Select the organisation that the other organisations are merging into.

- - <%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> - <%= f.govuk_error_summary %> +

Which organisation is absorbing the others?

+
+
+

Select the organisation that the other organisations are merging into.

<%= f.govuk_radio_buttons_fieldset( :absorbing_organisation_id, diff --git a/app/views/merge_requests/confirm_telephone_number.html.erb b/app/views/merge_requests/confirm_telephone_number.html.erb index 2a585bc1f..9a05ee54e 100644 --- a/app/views/merge_requests/confirm_telephone_number.html.erb +++ b/app/views/merge_requests/confirm_telephone_number.html.erb @@ -4,10 +4,33 @@ <%= govuk_back_link href: absorbing_organisation_merge_request_path(id: @merge_request) %> <% end %> -

What is <%= @merge_request.absorbing_organisation.name %>'s telephone number?

+<%= form_with model: @merge_request, url: merge_request_path, method: :patch do |f| %> + <%= f.govuk_error_summary %> -
-
-

Confirm the telephone number on file, or enter a new one.

+

What is <%= @merge_request.absorbing_organisation.name %>'s telephone number?

+ +
+
+ <% if @merge_request.absorbing_organisation.phone.present? %> +

Confirm the telephone number on file, or enter a new one.

+
+ <%= @merge_request.absorbing_organisation.phone %> +
+ <% end %> + + <% if @merge_request.absorbing_organisation.phone.present? %> + <%= f.govuk_radio_buttons_fieldset(:telephone_number_correct, legend: nil) do %> + <%= f.govuk_radio_button :telephone_number_correct, 1, checked: @merge_request.telephone_number_correct?, label: { text: "This telephone number is correct" }, link_errors: true %> + <%= f.govuk_radio_button :telephone_number_correct, 0, checked: @merge_request.new_telephone_number.present?, label: { text: "Enter a new phone number" } do %> + <%= f.govuk_text_field :new_telephone_number, label: { text: "Telephone number" }, width: "two-thirds" %> + <% end %> + <%= f.hidden_field :page, value: "confirm_telephone_number" %> + <% end %> + <% else %> + <%= f.govuk_text_field :new_telephone_number, label: nil, width: "two-thirds" %> + <%= f.hidden_field :page, value: "confirm_telephone_number" %> + <% end %> + <%= f.govuk_submit %> + <% end %>
diff --git a/app/views/merge_requests/merge_date.html.erb b/app/views/merge_requests/merge_date.html.erb new file mode 100644 index 000000000..35d9ef36d --- /dev/null +++ b/app/views/merge_requests/merge_date.html.erb @@ -0,0 +1,8 @@ +<% content_for :before_content do %> + <% title = "Tell us if your organisation is merging" %> + <% content_for :title, title %> + <%# TODO: Update this backlink to also work with the create org flow %> + <%= govuk_back_link href: confirm_telephone_number_merge_request_path(@merge_request) %> +<% end %> + +

What is the merge date?

diff --git a/app/views/merge_requests/organisations.html.erb b/app/views/merge_requests/organisations.html.erb index 221fbe87e..2a3c7a444 100644 --- a/app/views/merge_requests/organisations.html.erb +++ b/app/views/merge_requests/organisations.html.erb @@ -4,16 +4,16 @@ <%= govuk_back_link href: merge_request_organisation_path(id: @merge_request.requesting_organisation_id) %> <% end %> -

Which organisations are merging?

+<%= form_with model: @merge_request, url: organisations_merge_request_path, method: :patch do |f| %> + <%= f.govuk_error_summary %> +

Which organisations are merging?

-
-
-

- Add all organisations to be merged - we have already added your own. -

+
+
+

+ Add all organisations to be merged - we have already added your own. +

- <%= form_with model: @merge_request, url: organisations_merge_request_path, method: :patch do |f| %> - <%= f.govuk_error_summary %>

Start typing to search

<%= render partial: "organisation_relationships/related_organisation_select_question", locals: { field: :merging_organisation, diff --git a/config/locales/en.yml b/config/locales/en.yml index ff409f2e0..a798a258f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -513,6 +513,8 @@ en: organisation_part_of_another_merge: "This organisation is part of another merge - select a different one" organisation_not_selected: "Select an organisation from the search list" absorbing_organisation_blank: Select the organisation absorbing the others + telephone_number_correct_blank: Select to confirm or enter a new telephone number + new_telephone_number_blank: Enter a valid telephone number soft_validations: net_income: diff --git a/config/routes.rb b/config/routes.rb index cdbfc4616..bec7aed6b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -139,6 +139,7 @@ Rails.application.routes.draw do get "absorbing-organisation" get "confirm-telephone-number" get "new-org-name" + get "merge-date" end end diff --git a/db/migrate/20230427130334_add_phone_number_to_merge_request.rb b/db/migrate/20230427130334_add_phone_number_to_merge_request.rb new file mode 100644 index 000000000..dec3d596f --- /dev/null +++ b/db/migrate/20230427130334_add_phone_number_to_merge_request.rb @@ -0,0 +1,6 @@ +class AddPhoneNumberToMergeRequest < ActiveRecord::Migration[7.0] + change_table :merge_requests, bulk: true do |t| + t.column :telephone_number_correct, :boolean + t.column :new_telephone_number, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index f891d1024..73956bd24 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_04_21_124536) do +ActiveRecord::Schema[7.0].define(version: 2023_04_27_130334) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -369,6 +369,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_04_21_124536) do t.integer "status" t.integer "absorbing_organisation_id" t.boolean "new_absorbing_organisation" + t.boolean "telephone_number_correct" + t.string "new_telephone_number" end create_table "organisation_relationships", force: :cascade do |t| diff --git a/spec/requests/merge_requests_controller_spec.rb b/spec/requests/merge_requests_controller_spec.rb index 5e2479189..943be6374 100644 --- a/spec/requests/merge_requests_controller_spec.rb +++ b/spec/requests/merge_requests_controller_spec.rb @@ -233,6 +233,38 @@ RSpec.describe MergeRequestsController, type: :request do end end + describe "#confirm_telephone_number" do + let(:merge_request) do + MergeRequest.create!( + absorbing_organisation: create(:organisation, phone: phone_number), + requesting_organisation: organisation, + ) + end + + before { get "/merge-request/#{merge_request.id}/confirm-telephone-number", headers: } + + context "when org has phone number" do + let(:phone_number) { 123 } + + it "asks to confirm or provide new number" do + expect(page).to have_content("This telephone number is correct") + expect(page).to have_content("Confirm the telephone number on file, or enter a new one.") + expect(page).to have_content(phone_number) + expect(page).to have_content("What is #{merge_request.absorbing_organisation.name}'s telephone number?") + end + end + + context "when org does not have a phone number set" do + let(:phone_number) { nil } + + it "asks provide new number" do + expect(page).not_to have_content("This telephone number is correct") + expect(page).not_to have_content("Confirm the telephone number on file, or enter a new one.") + expect(page).to have_content("What is #{merge_request.absorbing_organisation.name}'s telephone number?") + end + end + end + describe "#update" do before { sign_in user } @@ -260,72 +292,184 @@ RSpec.describe MergeRequestsController, type: :request do end end - context "when not answering the question" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } - let(:params) do - { merge_request: { page: "absorbing_organisation" } } - end - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: + describe "from absorbing_organisation page" do + context "when not answering the question" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { page: "absorbing_organisation" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "renders the error" do + request + + expect(page).to have_content("Select the organisation absorbing the others") + end + + it "does not update the request" do + expect { request }.not_to(change { merge_request.reload.attributes }) + end end - it "renders the error" do - request + context "when absorbing_organisation_id set to other" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { absorbing_organisation_id: "other", page: "absorbing_organisation" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "redirects to new org path" do + request - expect(page).to have_content("Select the organisation absorbing the others") + expect(response).to redirect_to(new_org_name_merge_request_path(merge_request)) + end + + it "resets absorbing_organisation and sets new_absorbing_organisation to true" do + expect { request }.to change { + merge_request.reload.absorbing_organisation + }.from(other_organisation).to(nil).and change { + merge_request.reload.new_absorbing_organisation + }.from(nil).to(true) + end end - it "does not update the request" do - expect { request }.not_to(change { merge_request.reload.attributes }) + context "when absorbing_organisation_id set to id" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } + let(:params) do + { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } } + end + + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "redirects telephone number path" do + request + + expect(response).to redirect_to(confirm_telephone_number_merge_request_path(merge_request)) + end + + it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do + expect { request }.to change { + merge_request.reload.absorbing_organisation + }.from(nil).to(other_organisation).and change { + merge_request.reload.new_absorbing_organisation + }.from(true).to(false) + end end end - context "when absorbing_organisation_id set to other" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } - let(:params) do - { merge_request: { absorbing_organisation_id: "other", page: "absorbing_organisation" } } - end - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: - end + describe "from confirm_telephone_number page" do + context "when confirming the number" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true, new_telephone_number: "123") } + let(:params) do + { merge_request: { telephone_number_correct: "1", page: "confirm_telephone_number" } } + end - it "redirects to new org path" do - request + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end - expect(response).to redirect_to(new_org_name_merge_request_path(merge_request)) - end + it "redirects telephone number path" do + request - it "resets absorbing_organisation and sets new_absorbing_organisation to true" do - expect { request }.to change { - merge_request.reload.absorbing_organisation - }.from(other_organisation).to(nil).and change { - merge_request.reload.new_absorbing_organisation - }.from(nil).to(true) + expect(response).to redirect_to(merge_date_merge_request_path(merge_request)) + end + + it "updates telephone_number_correct and sets new_telephone_number to nil" do + expect { request }.to change { + merge_request.reload.telephone_number_correct + }.from(nil).to(true).and change { + merge_request.reload.new_telephone_number + }.from("123").to(nil) + end end - end - context "when absorbing_organisation_id set to id" do - let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } - let(:params) do - { merge_request: { absorbing_organisation_id: other_organisation.id, page: "absorbing_organisation" } } + context "when setting new number" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, new_absorbing_organisation: true) } + let(:params) do + { merge_request: { telephone_number_correct: "0", new_telephone_number: "123", page: "confirm_telephone_number" } } + end + + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "redirects telephone number path" do + request + + expect(response).to redirect_to(merge_date_merge_request_path(merge_request)) + end + + it "updates telephone_number_correct and sets new_telephone_number to nil" do + expect { request }.to change { + merge_request.reload.new_telephone_number + }.from(nil).to("123") + end end - let(:request) do - patch "/merge-request/#{merge_request.id}", headers:, params: + context "when not answering the question and the org has phone number" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: create(:organisation, phone: "123")) } + let(:params) do + { merge_request: { page: "confirm_telephone_number" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "renders the error" do + request + + expect(page).to have_content("Select to confirm or enter a new telephone number") + end + + it "does not update the request" do + expect { request }.not_to(change { merge_request.reload.attributes }) + end end - it "redirects telephone number path" do - request + context "when not answering the question and the org does not have a phone number" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { page: "confirm_telephone_number" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "renders the error" do + request + + expect(page).to have_content("Enter a valid telephone number") + end - expect(response).to redirect_to(confirm_telephone_number_merge_request_path(merge_request)) + it "does not update the request" do + expect { request }.not_to(change { merge_request.reload.attributes }) + end end - it "updates absorbing_organisation_id and sets new_absorbing_organisation to false" do - expect { request }.to change { - merge_request.reload.absorbing_organisation - }.from(nil).to(other_organisation).and change { - merge_request.reload.new_absorbing_organisation - }.from(true).to(false) + context "when not answering the phone number" do + let(:merge_request) { MergeRequest.create!(requesting_organisation: organisation, absorbing_organisation: other_organisation) } + let(:params) do + { merge_request: { page: "confirm_telephone_number", telephone_number_correct: "0" } } + end + let(:request) do + patch "/merge-request/#{merge_request.id}", headers:, params: + end + + it "renders the error" do + request + + expect(page).to have_content("Enter a valid telephone number") + end + + it "does not update the request" do + expect { request }.not_to(change { merge_request.reload.attributes }) + end end end end