Remove permission check from controller

It's already being checked in the service. Also added
a new MergeRequestRuleDestroyService for deleting an MR
level rule for consistency.

ee/app/services/approval_rules/base_service.rb

@@ -3,21 +3,17 @@
 module ApprovalRules
   class BaseService < ::BaseService
     def execute
-      return error(['Prohibited']) unless can_edit?
+      return error(['Prohibited'], 403) unless can_edit?
 
-      filter_eligible_users!
-      filter_eligible_groups!
-
-      if rule.update(params)
-        rule.reset
-        success
-      else
-        error(rule.errors.messages)
-      end
+      action
     end
 
     private
 
+    def action
+      raise 'Not implemented'
+    end
+
     attr_reader :rule
 
     def can_edit?
@@ -27,17 +23,5 @@ module ApprovalRules
     def success(*args, &blk)
       super.tap { |hsh| hsh[:rule] = rule }
     end
-
-    def filter_eligible_users!
-      return unless params.key?(:user_ids)
-
-      params[:users] = project.members_among(User.id_in(params.delete(:user_ids)))
-    end
-
-    def filter_eligible_groups!
-      return unless params.key?(:group_ids)
-
-      params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
-    end
   end
 end

ee/app/services/approval_rules/create_service.rb

@@ -2,6 +2,8 @@
 
 module ApprovalRules
   class CreateService < ::ApprovalRules::BaseService
+    include ::ApprovalRules::Updater
+
     # @param target [Project, MergeRequest]
     def initialize(target, user, params)
       @rule = target.approval_rules.build

ee/app/services/approval_rules/merge_request_rule_destroy_service.rb

+# frozen_string_literal: true
+
+module ApprovalRules
+  class MergeRequestRuleDestroyService < ::ApprovalRules::BaseService
+    def initialize(approval_rule, user)
+      @rule = approval_rule
+
+      super(@rule.project, user, params)
+    end
+
+    def action
+      @rule.destroy
+
+      if @rule.destroyed?
+        success
+      else
+        error(rule.errors.messages)
+      end
+    end
+  end
+end

ee/app/services/approval_rules/update_service.rb

@@ -2,6 +2,8 @@
 
 module ApprovalRules
   class UpdateService < ::ApprovalRules::BaseService
+    include ::ApprovalRules::Updater
+
     attr_reader :rule, :keep_existing_hidden_groups
 
     def initialize(approval_rule, user, params)

ee/app/services/concerns/approval_rules/updater.rb

+# frozen_string_literal: true
+
+module ApprovalRules
+  module Updater
+    def action
+      filter_eligible_users!
+      filter_eligible_groups!
+
+      if rule.update(params)
+        rule.reset
+        success
+      else
+        error(rule.errors.messages)
+      end
+    end
+
+    private
+
+    def filter_eligible_users!
+      return unless params.key?(:user_ids)
+
+      params[:users] = project.members_among(User.id_in(params.delete(:user_ids)))
+    end
+
+    def filter_eligible_groups!
+      return unless params.key?(:group_ids)
+
+      params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
+    end
+  end
+end

ee/lib/api/helpers/project_approval_rules_helpers.rb

@@ -40,7 +40,7 @@ module API
         if result[:status] == :success
           present result[:rule], with: present_with, current_user: current_user
         else
-          render_api_error!(result[:message], 400)
+          render_api_error!(result[:message], result[:http_status] || 400)
         end
       end
 
@@ -54,7 +54,7 @@ module API
         if result[:status] == :success
           present result[:rule], with: present_with, current_user: current_user
         else
-          render_api_error!(result[:message], 400)
+          render_api_error!(result[:message], result[:http_status] || 400)
         end
       end
 

ee/lib/api/merge_request_approval_rules.rb

@@ -7,10 +7,8 @@ module API
     ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
 
     helpers do
-      def find_merge_request_approval_rule_with_access(merge_request, id, access_level = :edit_approval_rule)
-        approval_rule = merge_request.approval_rules.find_by_id!(id)
-        authorize! access_level, approval_rule
-        approval_rule
+      def find_merge_request_approval_rule(merge_request, id)
+        merge_request.approval_rules.find_by_id!(id)
       end
     end
 
@@ -46,7 +44,7 @@ module API
           if result[:status] == :success
             present result[:rule], with: EE::API::Entities::MergeRequestApprovalRule, current_user: current_user
           else
-            render_api_error!(result[:message], 400)
+            render_api_error!(result[:message], result[:http_status] || 400)
           end
         end
 
@@ -65,13 +63,13 @@ module API
           put do
             merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
             params = declared_params(include_missing: false)
-            approval_rule = find_merge_request_approval_rule_with_access(merge_request, params.delete(:approval_rule_id))
+            approval_rule = find_merge_request_approval_rule(merge_request, params.delete(:approval_rule_id))
             result = ::ApprovalRules::UpdateService.new(approval_rule, current_user, params).execute
 
             if result[:status] == :success
               present result[:rule], with: EE::API::Entities::MergeRequestApprovalRule, current_user: current_user
             else
-              render_api_error!(result[:message], 400)
+              render_api_error!(result[:message], result[:http_status] || 400)
             end
           end
 
@@ -81,10 +79,14 @@ module API
           end
           delete do
             merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
-            approval_rule = find_merge_request_approval_rule_with_access(merge_request, params[:approval_rule_id])
+            approval_rule = find_merge_request_approval_rule(merge_request, params[:approval_rule_id])
 
             destroy_conditionally!(approval_rule) do |rule|
-              approval_rule.destroy
+              result = ::ApprovalRules::MergeRequestRuleDestroyService.new(rule, current_user).execute
+
+              if result[:status] == :error
+                render_api_error!(result[:message], result[:http_status] || 400)
+              end
             end
           end
         end

ee/spec/services/approval_rules/merge_request_rule_destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApprovalRules::MergeRequestRuleDestroyService do
+  let(:rule) { create(:approval_merge_request_rule) }
+  let(:user) { create(:user) }
+
+  subject(:result) { described_class.new(rule, user).execute }
+
+  before do
+    allow(Ability).to receive(:allowed?).and_call_original
+    allow(Ability)
+      .to receive(:allowed?)
+      .with(user, :edit_approval_rule, rule)
+      .at_least(:once)
+      .and_return(can_edit?)
+  end
+
+  context 'user cannot edit approval rule' do
+    let(:can_edit?) { false }
+
+    it 'returns error status' do
+      expect(result[:status]).to eq(:error)
+    end
+  end
+
+  context 'user can edit approval rule' do
+    let(:can_edit?) { true }
+
+    context 'when rule successfully deleted' do
+      it 'returns successful status' do
+        expect(result[:status]).to eq(:success)
+      end
+    end
+
+    context 'when rule not successfully deleted' do
+      before do
+        allow(rule).to receive(:destroyed?).and_return(false)
+      end
+
+      it 'returns error status' do
+        expect(result[:status]).to eq(:error)
+      end
+    end
+  end
+end