Add scoping for data coordinators

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),

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

app/policies/log_policy.rb

@@ -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

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

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
 

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