From c315f6f17c9c8887c37f11386fe58290c352e389 Mon Sep 17 00:00:00 2001 From: Jack S Date: Mon, 24 Apr 2023 09:04:52 +0100 Subject: [PATCH] Address comments - only select required users - remove not needed CYA checks --- .../form/lettings/questions/created_by_id.rb | 50 ++++++++----------- .../form/sales/questions/created_by_id.rb | 27 +++++----- .../lettings/questions/created_by_id_spec.rb | 46 +---------------- .../sales/questions/created_by_id_spec.rb | 41 +-------------- 4 files changed, 37 insertions(+), 127 deletions(-) diff --git a/app/models/form/lettings/questions/created_by_id.rb b/app/models/form/lettings/questions/created_by_id.rb index 72b08d3fc..4ed1efdd5 100644 --- a/app/models/form/lettings/questions/created_by_id.rb +++ b/app/models/form/lettings/questions/created_by_id.rb @@ -10,42 +10,32 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question end def answer_options - return ANSWER_OPTS unless ActiveRecord::Base.connected? + ANSWER_OPTS + end - User.select(:id, :name, :email).each_with_object(ANSWER_OPTS.dup) 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_OPTS unless user - return ANSWER_OPTS unless user.support? || user.data_coordinator? - - user_ids = [""] - user_ids += if user.support? - [ - (log.owning_organisation&.users&.pluck(:id) if log.owning_organisation), - (log.managing_organisation&.users&.pluck(:id) if log.managing_organisation), - ] - else - user.organisation.users.pluck(:id) - end.flatten.uniq - - 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) - return false if current_user.support? - return false if current_user.data_coordinator? - - true + present_user(User.find(value)) end def derived? @@ -54,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/sales/questions/created_by_id.rb b/app/models/form/sales/questions/created_by_id.rb index 2563478a1..44b64184e 100644 --- a/app/models/form/sales/questions/created_by_id.rb +++ b/app/models/form/sales/questions/created_by_id.rb @@ -10,30 +10,25 @@ class Form::Sales::Questions::CreatedById < ::Form::Question end def answer_options - return ANSWER_OPTS unless ActiveRecord::Base.connected? - - User.select(:id, :name, :email).each_with_object(ANSWER_OPTS.dup) do |user, hsh| - hsh[user.id] = "#{user.name} (#{user.email})" - hsh - end + ANSWER_OPTS end - def displayed_answer_options(log, user = nil) + def displayed_answer_options(log, current_user = nil) return ANSWER_OPTS unless log.owning_organisation - return ANSWER_OPTS unless user - return ANSWER_OPTS unless user.support? || user.data_coordinator? - - users = user.support? ? log.owning_organisation.users : user.organisation.users + return ANSWER_OPTS unless current_user - user_ids = users.pluck(:id) + [""] + users = current_user.support? ? log.owning_organisation.users : current_user.organisation.users - answer_options.select { |k, _v| user_ids.include?(k) } + 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 - answer_options[value] + present_user(User.find(value)) end def hidden_in_check_answers?(_log, current_user) @@ -49,6 +44,10 @@ class Form::Sales::Questions::CreatedById < ::Form::Question private + def present_user(user) + "#{user.name} (#{user.email})" + end + def selected_answer_option_is_derived?(_log) false 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 c7b843a89..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 @@ -56,10 +46,6 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do let(:managing_org_user) { create(:user) } let(:support_user) { create(:user, :support, organisation: owning_org_user.organisation) } - 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(:lettings_log) do create(:lettings_log, created_by: support_user, owning_organisation: owning_org_user.organisation, managing_organisation: managing_org_user.organisation) @@ -84,10 +70,6 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do let(:managing_org_user) { create(:user) } 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(:lettings_log) do create(:lettings_log, created_by: data_coordinator, owning_organisation: data_coordinator.organisation, managing_organisation: managing_org_user.organisation) @@ -107,30 +89,4 @@ RSpec.describe Form::Lettings::Questions::CreatedById, type: :model do end end end - - context "when the current user is data_provider" do - let(:data_provider) { create(:user, :data_provider) } - - it "is not shown in check answers" do - expect(question.hidden_in_check_answers?(nil, data_provider)).to be true - end - - describe "#displayed_answer_options" do - let(:owning_org_user) { create(:user) } - let(:lettings_log) { create(:lettings_log, owning_organisation: owning_org_user.organisation) } - let(:user_in_same_org) { create(:user, organisation: data_provider.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_provider.id => "#{data_provider.name} (#{data_provider.email})", - } - end - - it "only displays users that belong user's org" do - expect(question.displayed_answer_options(lettings_log, data_provider)).to eq(Form::Lettings::Questions::CreatedById::ANSWER_OPTS) - end - end - end end diff --git a/spec/models/form/sales/questions/created_by_id_spec.rb b/spec/models/form/sales/questions/created_by_id_spec.rb index 1bbebe0ac..fd0122cb8 100644 --- a/spec/models/form/sales/questions/created_by_id_spec.rb +++ b/spec/models/form/sales/questions/created_by_id_spec.rb @@ -8,15 +8,6 @@ RSpec.describe Form::Sales::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!(: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) @@ -42,10 +33,6 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model 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 @@ -83,7 +70,7 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do 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!(:user_in_same_org) { create(:user, organisation: data_coordinator.organisation) } let(:expected_answer_options) do { @@ -98,30 +85,4 @@ RSpec.describe Form::Sales::Questions::CreatedById, type: :model do end end end - - context "when the current user is data_provider" do - let(:data_provider) { create(:user, :data_provider) } - - it "is not shown in check answers" do - expect(question.hidden_in_check_answers?(nil, data_provider)).to be true - 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_provider.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_provider.id => "#{data_provider.name} (#{data_provider.email})", - } - end - - it "only displays users that belong user's org" do - expect(question.displayed_answer_options(sales_log, data_provider)).to eq(Form::Sales::Questions::CreatedById::ANSWER_OPTS) - end - end - end end