Extract project push rule to service class

Push rules can be treated as a type of settings, such as
application_settings, project_settings, etc. There must be only one for
a given project. Therefore, it would be better to just expose the update
interface and let the creation handled behind the scene.

This commit extracts project push rule create and update logic to
a dedicated service class. This change allows to centralize auditing
and logging of actions from Project Push Rule controller and API.

The POST and PUT API can handle both create and update without raising
`unprocessable_entity` and `not_found` error respectively. Since the
user can only manage push rule via project id and they can only ever
create one, this change reduces coordinating POST request before PUT
request.

ee/app/controllers/projects/push_rules_controller.rb

@@ -14,16 +14,19 @@ class Projects::PushRulesController < Projects::ApplicationController
   feature_category :source_code_management
 
   def update
-    @push_rule = project.push_rule
-    @push_rule.update(push_rule_params)
+    service_response = PushRules::CreateOrUpdateService.new(
+      container: project,
+      current_user: current_user,
+      params: push_rule_params
+    ).execute
 
-    if @push_rule.valid?
+    if service_response.success?
       flash[:notice] = _('Push Rules updated successfully.')
     else
-      flash[:alert] = @push_rule.errors.full_messages.join(', ').html_safe
+      flash[:alert] = service_response.message
     end
 
-    redirect_to_repository_settings(@project, anchor: 'js-push-rules')
+    redirect_to_repository_settings(project, anchor: 'js-push-rules')
   end
 
   private

ee/app/models/ee/project.rb

@@ -38,7 +38,7 @@ module EE
 
       has_one :repository_state, class_name: 'ProjectRepositoryState', inverse_of: :project
       has_one :project_registry, class_name: 'Geo::ProjectRegistry', inverse_of: :project
-      has_one :push_rule, ->(project) { project&.feature_available?(:push_rules) ? all : none }
+      has_one :push_rule, ->(project) { project&.feature_available?(:push_rules) ? all : none }, inverse_of: :project
       has_one :index_status
 
       has_one :github_service

ee/app/models/push_rule.rb

@@ -19,7 +19,7 @@ class PushRule < ApplicationRecord
     branch_name_regex
   ].freeze
 
-  belongs_to :project
+  belongs_to :project, inverse_of: :push_rule
 
   validates :max_file_size, numericality: { greater_than_or_equal_to: 0, only_integer: true }
   validates(*REGEX_COLUMNS, untrusted_regexp: true)

ee/app/services/push_rules/create_or_update_service.rb

+# frozen_string_literal: true
+
+module PushRules
+  class CreateOrUpdateService < BaseContainerService
+    def execute
+      push_rule = container.push_rule || container.build_push_rule
+
+      if push_rule.update(params)
+        ServiceResponse.success(payload: { push_rule: push_rule })
+      else
+        error_message = push_rule.errors.full_messages.to_sentence
+        ServiceResponse.error(message: error_message, payload: { push_rule: push_rule })
+      end
+    end
+  end
+end

ee/lib/api/project_push_rule.rb

@@ -8,6 +8,24 @@ module API
     before { check_project_feature_available!(:push_rules) }
     before { authorize_change_param(user_project, :commit_committer_check, :reject_unsigned_commits) }
 
+    helpers do
+      def create_or_update_push_rule
+        service_response = PushRules::CreateOrUpdateService.new(
+          container: user_project,
+          current_user: current_user,
+          params: declared_params(include_missing: false)
+        ).execute
+
+        push_rule = service_response.payload[:push_rule]
+
+        if service_response.success?
+          present(push_rule, with: EE::API::Entities::ProjectPushRule, user: current_user)
+        else
+          render_validation_error!(push_rule)
+        end
+      end
+    end
+
     params do
       requires :id, type: String, desc: 'The ID of a project'
     end
@@ -48,12 +66,8 @@ module API
         use :push_rule_params
       end
       post ":id/push_rule" do
-        if user_project.push_rule
-          error!("Project push rule exists", 422)
-        else
-          push_rule = user_project.create_push_rule(declared_params(include_missing: false))
-          present push_rule, with: EE::API::Entities::ProjectPushRule, user: current_user
-        end
+        unprocessable_entity!('Project push rule exists') if user_project.push_rule
+        create_or_update_push_rule
       end
 
       desc 'Update an existing project push rule' do
@@ -63,14 +77,8 @@ module API
         use :push_rule_params
       end
       put ":id/push_rule" do
-        push_rule = user_project.push_rule
-        not_found!('Push Rule') unless push_rule
-
-        if push_rule.update(declared_params(include_missing: false))
-          present push_rule, with: EE::API::Entities::ProjectPushRule, user: current_user
-        else
-          render_validation_error!(push_rule)
-        end
+        not_found!('Push Rule') unless user_project.push_rule
+        create_or_update_push_rule
       end
 
       desc 'Deletes project push rule'

ee/spec/models/project_spec.rb

@@ -25,6 +25,7 @@ RSpec.describe Project do
 
     it { is_expected.to have_one(:import_state).class_name('ProjectImportState') }
     it { is_expected.to have_one(:repository_state).class_name('ProjectRepositoryState').inverse_of(:project) }
+    it { is_expected.to have_one(:push_rule).inverse_of(:project) }
     it { is_expected.to have_one(:status_page_setting).class_name('StatusPage::ProjectSetting') }
     it { is_expected.to have_one(:compliance_framework_setting).class_name('ComplianceManagement::ComplianceFramework::ProjectSettings') }
     it { is_expected.to have_one(:compliance_management_framework).class_name('ComplianceManagement::Framework') }

ee/spec/models/push_rule_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe PushRule do
   let(:project) { Projects::CreateService.new(user, { name: 'test', namespace: user.namespace }).execute }
 
   describe "Associations" do
-    it { is_expected.to belong_to(:project) }
+    it { is_expected.to belong_to(:project).inverse_of(:push_rule) }
   end
 
   describe "Validation" do

ee/spec/requests/api/project_push_rule_spec.rb

@@ -4,9 +4,10 @@ require 'spec_helper'
 
 RSpec.describe API::ProjectPushRule, 'ProjectPushRule', api: true do
   include ApiHelpers
-  let(:user) { create(:user) }
-  let(:user3) { create(:user) }
-  let!(:project) { create(:project, :repository, creator_id: user.id, namespace: user.namespace) }
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:user3) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, creator_id: user.id, namespace: user.namespace) }
 
   before do
     stub_licensed_features(push_rules: push_rules_enabled,
@@ -192,6 +193,15 @@ RSpec.describe API::ProjectPushRule, 'ProjectPushRule', api: true do
           end
         end
       end
+
+      context 'invalid params', :aggregate_failures do
+        let(:rules_params) { { max_file_size: -10 } }
+
+        it 'returns an error' do
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']).to match('max_file_size' => ['must be greater than or equal to 0'])
+        end
+      end
     end
 
     it 'adds push rule to project with no file size' do
@@ -217,16 +227,14 @@ RSpec.describe API::ProjectPushRule, 'ProjectPushRule', api: true do
         expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
-  end
-
-  describe "POST /projects/:id/push_rule" do
-    before do
-      create(:push_rule, project: project)
-    end
 
     context "with existing push rule" do
-      it "does not add push rule to project" do
-        post api("/projects/#{project.id}/push_rule", user), params: { deny_delete_tag: true }
+      before do
+        create(:push_rule, project: project)
+      end
+
+      it 'returns an error response' do
+        post api("/projects/#{project.id}/push_rule", user), params: rules_params
 
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
       end
@@ -234,114 +242,144 @@ RSpec.describe API::ProjectPushRule, 'ProjectPushRule', api: true do
   end
 
   describe "PUT /projects/:id/push_rule" do
-    before do
-      create(:push_rule, project: project,
-             deny_delete_tag: true, commit_message_regex: 'Mended')
+    subject(:request) do
       put api("/projects/#{project.id}/push_rule", user), params: new_settings
     end
 
-    context "setting deny_delete_tag and commit_message_regex" do
-      let(:new_settings) do
-        { deny_delete_tag: false, commit_message_regex: 'Fixes \d+\..*' }
-      end
+    context 'with existing push rule' do
+      let_it_be(:push_rule) { create(:push_rule, project: project, deny_delete_tag: true, commit_message_regex: 'Mended') }
 
-      it "is successful" do
-        expect(response).to have_gitlab_http_status(:ok)
-      end
+      context "setting deny_delete_tag and commit_message_regex" do
+        let(:new_settings) do
+          { deny_delete_tag: false, commit_message_regex: 'Fixes \d+\..*' }
+        end
 
-      it 'includes the expected settings' do
-        subset = new_settings.transform_keys(&:to_s)
-        expect(json_response).to include(subset)
-      end
-    end
+        it "is successful" do
+          request
 
-    context "setting commit_committer_check" do
-      let(:new_settings) { { commit_committer_check: true } }
+          expect(response).to have_gitlab_http_status(:ok)
+        end
 
-      it "is successful" do
-        expect(response).to have_gitlab_http_status(:ok)
-      end
+        it 'includes the expected settings' do
+          request
 
-      it "sets the commit_committer_check" do
-        expect(json_response).to include('commit_committer_check' => true)
+          subset = new_settings.transform_keys(&:to_s)
+          expect(json_response).to include(subset)
+        end
       end
 
-      context 'the commit_committer_check feature is not enabled' do
-        let(:ccc_enabled) { false }
+      context "setting commit_committer_check" do
+        let(:new_settings) { { commit_committer_check: true } }
 
-        it "is an error to provide this parameter" do
-          expect(response).to have_gitlab_http_status(:forbidden)
+        it "is successful" do
+          request
+
+          expect(response).to have_gitlab_http_status(:ok)
         end
-      end
-    end
 
-    context "setting reject_unsigned_commits" do
-      let(:new_settings) { { reject_unsigned_commits: true } }
+        it "sets the commit_committer_check" do
+          request
 
-      it "is successful" do
-        expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response).to include('commit_committer_check' => true)
+        end
+
+        context 'the commit_committer_check feature is not enabled' do
+          let(:ccc_enabled) { false }
+
+          it "is an error to provide this parameter" do
+            request
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
+        end
       end
 
-      it "sets the reject_unsigned_commits" do
-        expect(json_response).to include('reject_unsigned_commits' => true)
+      context "setting reject_unsigned_commits" do
+        let(:new_settings) { { reject_unsigned_commits: true } }
+
+        it "is successful" do
+          request
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+
+        it "sets the reject_unsigned_commits" do
+          request
+
+          expect(json_response).to include('reject_unsigned_commits' => true)
+        end
+
+        context 'the reject_unsigned_commits feature is not enabled' do
+          let(:ruc_enabled) { false }
+
+          it "is an error to provide the this parameter" do
+            request
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
+        end
       end
 
-      context 'the reject_unsigned_commits feature is not enabled' do
-        let(:ruc_enabled) { false }
+      context "not providing parameters" do
+        let(:new_settings) { {} }
 
-        it "is an error to provide the this parameter" do
-          expect(response).to have_gitlab_http_status(:forbidden)
+        it "is an error" do
+          request
+
+          expect(response).to have_gitlab_http_status(:bad_request)
         end
       end
-    end
 
-    context "not providing parameters" do
-      let(:new_settings) { {} }
+      context 'invalid params', :aggregate_failures do
+        let(:new_settings) { { max_file_size: -10 } }
 
-      it "is an error" do
-        expect(response).to have_gitlab_http_status(:bad_request)
+        it 'returns an error' do
+          request
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']).to match('max_file_size' => ['must be greater than or equal to 0'])
+        end
       end
     end
-  end
 
-  describe "PUT /projects/:id/push_rule" do
-    it "gets error on non existing project push rule" do
-      put api("/projects/#{project.id}/push_rule", user),
-        params: { deny_delete_tag: false, commit_message_regex: 'Fixes \d+\..*' }
+    context 'without existing push rule' do
+      let(:new_settings) { { commit_committer_check: true } }
 
-      expect(response).to have_gitlab_http_status(:not_found)
+      it 'returns an error response', :aggregate_failures do
+        expect { request }.not_to change { PushRule.count }
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
     end
 
     it "does not update push rule for unauthorized user" do
-      post api("/projects/#{project.id}/push_rule", user3), params: { deny_delete_tag: true }
+      put api("/projects/#{project.id}/push_rule", user3), params: { deny_delete_tag: true }
 
       expect(response).to have_gitlab_http_status(:forbidden)
     end
   end
 
   describe "DELETE /projects/:id/push_rule" do
-    before do
-      create(:push_rule, project: project)
-    end
+    context 'for existing push rule' do
+      let_it_be(:push_rule) { create(:push_rule, project: project) }
 
-    context "maintainer" do
-      it "deletes push rule from project" do
-        delete api("/projects/#{project.id}/push_rule", user)
+      context "maintainer" do
+        it "deletes push rule from project" do
+          delete api("/projects/#{project.id}/push_rule", user)
 
-        expect(response).to have_gitlab_http_status(:no_content)
+          expect(response).to have_gitlab_http_status(:no_content)
+        end
       end
-    end
 
-    context "user with developer_access" do
-      it "returns a 403 error" do
-        delete api("/projects/#{project.id}/push_rule", user3)
+      context "user with developer_access" do
+        it "returns a 403 error" do
+          delete api("/projects/#{project.id}/push_rule", user3)
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
       end
     end
-  end
 
-  describe "DELETE /projects/:id/push_rule" do
     context "for non existing push rule" do
       it "deletes push rule from project" do
         delete api("/projects/#{project.id}/push_rule", user)

ee/spec/services/push_rules/create_or_update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe PushRules::CreateOrUpdateService, '#execute' do
+  let_it_be_with_reload(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+  let(:params) { { max_file_size: 28 } }
+
+  subject { described_class.new(container: project, current_user: user, params: params) }
+
+  shared_examples 'a failed update' do
+    let(:params) { { max_file_size: -28 } }
+
+    it 'responds with an error service response', :aggregate_failures do
+      response = subject.execute
+
+      expect(response).to be_error
+      expect(response.message).to eq('Max file size must be greater than or equal to 0')
+      expect(response.payload).to match(push_rule: project.push_rule)
+    end
+  end
+
+  context 'with existing push rule' do
+    let_it_be(:push_rule) { create(:push_rule, project: project) }
+
+    it 'updates existing push rule' do
+      expect { subject.execute }
+        .to change { PushRule.count }.by(0)
+        .and change { push_rule.reload.max_file_size }.to(28)
+    end
+
+    it 'responds with a successful service response', :aggregate_failures do
+      response = subject.execute
+
+      expect(response).to be_success
+      expect(response.payload).to match(push_rule: push_rule)
+    end
+
+    it_behaves_like 'a failed update'
+  end
+
+  context 'without existing push rule' do
+    it 'creates a new push rule', :aggregate_failures do
+      expect { subject.execute }.to change { PushRule.count }.by(1)
+
+      expect(project.push_rule.max_file_size).to eq(28)
+    end
+
+    it 'responds with a successful service response', :aggregate_failures do
+      response = subject.execute
+
+      expect(response).to be_success
+      expect(response.payload).to match(push_rule: project.push_rule)
+    end
+
+    it_behaves_like 'a failed update'
+  end
+end