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] [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