Merge branch '271274-extract-project-push-rule-update-service' into 'master'

Extract project push rule to service class

See merge request gitlab-org/gitlab!53733

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