DEV: Refactor DiscourseAutomation::DestroyAutomation a bit

Small followup to 932bd6b.

plugins/automation/app/controllers/discourse_automation/admin_automations_controller.rb

@@ -81,7 +81,7 @@ module DiscourseAutomation
     end
 
     def destroy
-      DiscourseAutomation::DestroyAutomation.call(service_params) do
+      DiscourseAutomation::Destroy.call(service_params) do
         on_success { render(json: success_json) }
         on_model_not_found(:automation) { raise Discourse::NotFound }
         on_failed_policy(:can_destroy_automation) { raise Discourse::InvalidAccess }

plugins/automation/app/services/discourse_automation/destroy.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+class DiscourseAutomation::Destroy
+  include Service::Base
+
+  # @!method self.call(guardian:, params:)
+  #   @param [Guardian] guardian
+  #   @param [Hash] params
+  #   @option params [Integer] :automation_id
+  #   @return [Service::Base::Context]
+
+  policy :can_destroy_automation
+
+  params do
+    attribute :automation_id, :integer
+    validates :automation_id, presence: true
+  end
+
+  model :automation
+
+  transaction do
+    step :log_action
+    step :destroy_automation
+  end
+
+  private
+
+  def can_destroy_automation(guardian:)
+    guardian.is_admin?
+  end
+
+  def fetch_automation(params:)
+    DiscourseAutomation::Automation.find_by(id: params.automation_id)
+  end
+
+  def log_action(automation:, guardian:)
+    StaffActionLogger.new(guardian.user).log_custom(
+      "delete_automation",
+      **automation.slice(:id, :name, :script, :trigger),
+    )
+  end
+
+  def destroy_automation(automation:)
+    automation.destroy!
+  end
+end

plugins/automation/app/services/discourse_automation/destroy_automation.rb

@@ -1,48 +0,0 @@
-# frozen_string_literal: true
-
-module DiscourseAutomation
-  class DestroyAutomation
-    include ::Service::Base
-
-    # @!method self.call(guardian:, params:)
-    #   @param [Guardian] guardian
-    #   @param [Hash] params
-    #   @option params [Integer] :automation_id
-    #   @return [Service::Base::Context]
-    params do
-      attribute :automation_id, :integer
-      validates :automation_id, presence: true
-    end
-
-    model :automation
-    policy :can_destroy_automation
-    transaction do
-      step :log_action
-      step :destroy_automation
-    end
-
-    private
-
-    def fetch_automation(params:)
-      DiscourseAutomation::Automation.find_by(id: params.automation_id)
-    end
-
-    def can_destroy_automation(guardian:)
-      guardian.is_admin?
-    end
-
-    def log_action(automation:, guardian:)
-      StaffActionLogger.new(guardian.user).log_custom(
-        "delete_automation",
-        id: automation.id,
-        name: automation.name,
-        script: automation.script,
-        trigger: automation.trigger,
-      )
-    end
-
-    def destroy_automation(automation:)
-      automation.destroy!
-    end
-  end
-end

plugins/automation/spec/services/discourse_automation/destroy_spec.rb

@@ -1,9 +1,7 @@
 # frozen_string_literal: true
 
-RSpec.describe DiscourseAutomation::DestroyAutomation do
+RSpec.describe DiscourseAutomation::Destroy do
   describe described_class::Contract, type: :model do
-    subject(:contract) { described_class.new }
-
     it { is_expected.to validate_presence_of :automation_id }
   end
 
@@ -17,6 +15,25 @@ RSpec.describe DiscourseAutomation::DestroyAutomation do
     let(:params) { { automation_id: automation.id } }
     let(:dependencies) { { guardian: } }
 
+    context "when user can't destroy the automation" do
+      fab!(:user) { Fabricate(:user) }
+
+      it { is_expected.to fail_a_policy(:can_destroy_automation) }
+    end
+
+    context "when data is invalid" do
+      before { params[:automation_id] = nil }
+
+      it { is_expected.to fail_a_contract }
+    end
+
+    context "when the automation is not found" do
+      before { params[:automation_id] = 999 }
+
+      it { is_expected.to fail_to_find_a_model(:automation) }
+    end
+
+    context "when everything's ok" do
       it "logs the action" do
         expect { result }.to change { UserHistory.count }.by(1)
         expect(UserHistory.last.details).to eq(
@@ -27,17 +44,6 @@ RSpec.describe DiscourseAutomation::DestroyAutomation do
       it "destroys the automation" do
         expect { result }.to change { DiscourseAutomation::Automation.count }.by(-1)
       end
-
-    context "when the automation is not found" do
-      before { params[:automation_id] = 999 }
-
-      it { is_expected.to fail_to_find_a_model(:automation) }
-    end
-
-    context "when user can't destroy the automation" do
-      fab!(:user) { Fabricate(:user) }
-
-      it { is_expected.to fail_a_policy(:can_destroy_automation) }
     end
   end
 end