From 766c61954a31c842e3b81dd5b996b11e6f0dac13 Mon Sep 17 00:00:00 2001 From: Jack S Date: Tue, 16 May 2023 11:09:00 +0100 Subject: [PATCH] Add scoping for data coordinators --- app/helpers/log_actions_helper.rb | 6 +- app/policies/lettings_log_policy.rb | 27 ++++- app/policies/log_policy.rb | 24 ---- app/policies/sales_log_policy.rb | 27 ++++- ...cy_spec.rb => lettings_log_policy_spec.rb} | 15 ++- spec/policies/sales_log_policy_spec.rb | 114 ++++++++++++++++++ 6 files changed, 181 insertions(+), 32 deletions(-) delete mode 100644 app/policies/log_policy.rb rename spec/policies/{log_policy_spec.rb => lettings_log_policy_spec.rb} (86%) create mode 100644 spec/policies/sales_log_policy_spec.rb diff --git a/app/helpers/log_actions_helper.rb b/app/helpers/log_actions_helper.rb index daca9dd30..760a5523b 100644 --- a/app/helpers/log_actions_helper.rb +++ b/app/helpers/log_actions_helper.rb @@ -30,8 +30,12 @@ private end end + def policy_class_for(log) + log.lettings? ? LettingsLogPolicy : SalesLogPolicy + end + def delete_button_for_log(log) - if LogPolicy.new(current_user, log).destroy? + if policy_class_for(log).new(current_user, log).destroy? govuk_button_link_to( "Delete log", lettings_log_delete_confirmation_path(log), diff --git a/app/policies/lettings_log_policy.rb b/app/policies/lettings_log_policy.rb index f340b56ce..1d92cd72f 100644 --- a/app/policies/lettings_log_policy.rb +++ b/app/policies/lettings_log_policy.rb @@ -1,2 +1,27 @@ -class LettingsLogPolicy < LogPolicy +class LettingsLogPolicy + attr_reader :user, :log + + def initialize(user, log) + @user = user + @log = log + end + + def destroy? + return false unless log && user + + # Can only delete editable logs + return false unless log.collection_period_open? + + # Only delete logs with answered questions + return false unless log.in_progress? || log.completed? + + # Support users can delete any log + return true if user.support? + + # Data coordinators can delete any log visible to them + return true if user.data_coordinator? && user.lettings_logs.visible.include?(log) + + # Data providers can only delete the log if it is assigned to them + log.created_by == user + end end diff --git a/app/policies/log_policy.rb b/app/policies/log_policy.rb deleted file mode 100644 index 4ade0ad03..000000000 --- a/app/policies/log_policy.rb +++ /dev/null @@ -1,24 +0,0 @@ -class LogPolicy - attr_reader :user, :log - - def initialize(user, log) - @user = user - @log = log - end - - def destroy? - return false unless log && user - - # Can only delete editable logs - return false unless log.collection_period_open? - - # Only delete logs with answered questions - return false unless log.in_progress? || log.completed? - - # Data coordinators and support users can delete any log - return true if user.data_coordinator? || user.support? - - # Data providers can only delete the log if it is assigned to them - log.created_by == user - end -end diff --git a/app/policies/sales_log_policy.rb b/app/policies/sales_log_policy.rb index 61de31f31..671690831 100644 --- a/app/policies/sales_log_policy.rb +++ b/app/policies/sales_log_policy.rb @@ -1,2 +1,27 @@ -class SalesLogPolicy < LogPolicy +class SalesLogPolicy + attr_reader :user, :log + + def initialize(user, log) + @user = user + @log = log + end + + def destroy? + return false unless log && user + + # Can only delete editable logs + return false unless log.collection_period_open? + + # Only delete logs with answered questions + return false unless log.in_progress? || log.completed? + + # Support users can delete any log + return true if user.support? + + # Data coordinators can delete any log visible to them + return true if user.data_coordinator? && user.sales_logs.visible.include?(log) + + # Data providers can only delete the log if it is assigned to them + log.created_by == user + end end diff --git a/spec/policies/log_policy_spec.rb b/spec/policies/lettings_log_policy_spec.rb similarity index 86% rename from spec/policies/log_policy_spec.rb rename to spec/policies/lettings_log_policy_spec.rb index 1bedfa300..a89280212 100644 --- a/spec/policies/log_policy_spec.rb +++ b/spec/policies/lettings_log_policy_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe LogPolicy do +RSpec.describe LettingsLogPolicy do subject(:policy) { described_class } permissions :destroy? do @@ -60,18 +60,23 @@ RSpec.describe LogPolicy do [ %i[lettings_log in_progress], %i[lettings_log completed], - %i[sales_log in_progress], - %i[sales_log completed], ].each do |type, status| let(:log) { create(type, status) } context "when #{type} status: #{status}" do context "when user is data coordinator" do let(:user) { create(:user, :data_coordinator) } + let(:user_of_managing_org) { create(:user, :data_coordinator, organisation: log.managing_organisation) } - it "does allow deletion of log" do + it "does not allow deletion of log" do expect(log).to receive(:collection_period_open?) - expect(policy).to permit(user, log) + expect(policy).not_to permit(user, log) + end + + it "allows deletion of log" do + expect(log).to receive(:collection_period_open?) + + expect(policy).to permit(user_of_managing_org, log) end end diff --git a/spec/policies/sales_log_policy_spec.rb b/spec/policies/sales_log_policy_spec.rb new file mode 100644 index 000000000..38a8cc7b8 --- /dev/null +++ b/spec/policies/sales_log_policy_spec.rb @@ -0,0 +1,114 @@ +require "rails_helper" + +RSpec.describe SalesLogPolicy do + subject(:policy) { described_class } + + permissions :destroy? do + let(:log) { create(:sales_log, :in_progress) } + + context "when log nil" do + before do + allow(log).to receive(:collection_period_open?).and_return(false) + end + + it "does not allow deletion of log" do + expect(policy).not_to permit(build(:user, :support), nil) + end + end + + context "when user nil" do + before do + allow(log).to receive(:collection_period_open?).and_return(false) + end + + it "does not allow deletion of log" do + expect(policy).not_to permit(nil, build(:sales_log, :in_progress)) + end + end + + context "when collection period closed" do + before do + allow(log).to receive(:collection_period_open?).and_return(false) + end + + it "does not allow deletion of log" do + expect(log).to receive(:collection_period_open?) + + expect(policy).not_to permit(build(:user, :support), log) + end + end + + context "when collection period open" do + before do + allow(log).to receive(:collection_period_open?).and_return(true) + end + + context "when not started" do + before do + allow(log).to receive(:in_progress?).and_return(false) + allow(log).to receive(:completed?).and_return(false) + end + + it "does not allow deletion of log" do + expect(log).to receive(:in_progress?) + expect(log).to receive(:collection_period_open?) + + expect(policy).not_to permit(build(:user, :support), log) + end + end + + [ + %i[sales_log in_progress], + %i[sales_log completed], + ].each do |type, status| + let(:log) { create(type, status) } + context "when #{type} status: #{status}" do + context "when user is data coordinator" do + let(:user) { create(:user, :data_coordinator) } + let(:user_of_owning_org) { create(:user, :data_coordinator, organisation: log.owning_organisation) } + + it "does not allow deletion of log" do + expect(log).to receive(:collection_period_open?) + + expect(policy).not_to permit(user, log) + end + + it "allows deletion of log" do + expect(log).to receive(:collection_period_open?) + + expect(policy).to permit(user_of_owning_org, log) + end + end + + context "when user is support" do + let(:user) { create(:user, :support) } + + it "does allow deletion of log" do + expect(log).to receive(:collection_period_open?) + + expect(policy).to permit(user, log) + end + end + + context "when user is data provider" do + let(:user) { create(:user) } + + it "does not allow deletion of log" do + expect(log).to receive(:collection_period_open?) + + expect(policy).not_to permit(user, log) + end + + context "when the log is assigned to the user" do + let(:log) { create(:sales_log, :in_progress, created_by: user) } + + it "does allow deletion of log" do + expect(policy).to permit(user, log) + end + end + end + end + end + end + end +end