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

Extract group push rule update service

See merge request gitlab-org/gitlab!53127

ee/app/controllers/groups/push_rules_controller.rb

 # frozen_string_literal: true
+
 class Groups::PushRulesController < Groups::ApplicationController
   include Gitlab::Utils::StrongMemoize
   include PushRulesHelper
@@ -16,10 +17,14 @@ class Groups::PushRulesController < Groups::ApplicationController
   end
 
   def update
-    if @push_rule.update(push_rule_params)
+    service_response = PushRules::CreateOrUpdateService
+      .new(container: group, current_user: current_user, params: push_rule_params)
+      .execute
+
+    if service_response.success?
       flash[:notice] = _('Push Rule updated successfully.')
     else
-      flash[:alert] = @push_rule.errors.full_messages.join(', ').html_safe
+      flash[:alert] = service_response.message
     end
 
     redirect_to edit_group_push_rules_path(group)
@@ -45,8 +50,7 @@ class Groups::PushRulesController < Groups::ApplicationController
 
   def push_rule
     strong_memoize(:push_rule) do
-      group.update(push_rule: PushRule.create!) unless group.push_rule
-      group.push_rule
+      group.push_rule || group.build_push_rule
     end
   end
 

ee/app/models/ee/group.rb

@@ -57,7 +57,7 @@ module EE
 
       belongs_to :file_template_project, class_name: "Project"
 
-      belongs_to :push_rule
+      belongs_to :push_rule, inverse_of: :group
 
       # Use +checked_file_template_project+ instead, which implements important
       # visibility checks

ee/app/models/push_rule.rb

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

ee/lib/api/group_push_rule.rb

@@ -18,6 +18,22 @@ module API
           not_found! unless can?(current_user, :change_push_rules, user_group)
         end
 
+        def create_or_update_push_rule
+          service_response = PushRules::CreateOrUpdateService.new(
+            container: user_group,
+            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::GroupPushRule, user: current_user)
+          else
+            render_validation_error!(push_rule)
+          end
+        end
+
         params :push_rule_params do
           optional :deny_delete_tag, type: Boolean, desc: 'Deny deleting a tag'
           optional :member_check, type: Boolean, desc: 'Restrict commits by author (email) to existing GitLab users'
@@ -59,11 +75,8 @@ module API
         use :push_rule_params
       end
       post ":id/push_rule" do
-        render_api_error!(_('Group push rule exists, try updating'), 422) if user_group.push_rule
-
-        allowed_params = declared_params(include_missing: false)
-        user_group.update!(push_rule: PushRule.create!(allowed_params))
-        present user_group.push_rule, with: EE::API::Entities::GroupPushRule, user: current_user
+        unprocessable_entity!('Group push rule exists, try updating') if user_group.push_rule
+        create_or_update_push_rule
       end
 
       desc 'Edit push rule of a group' do
@@ -74,14 +87,8 @@ module API
         use :push_rule_params
       end
       put ":id/push_rule" do
-        push_rule = user_group.push_rule
-        not_found! unless push_rule
-
-        if push_rule.update(declared_params(include_missing: false))
-          present push_rule, with: EE::API::Entities::GroupPushRule, user: current_user
-        else
-          render_validation_error!(push_rule)
-        end
+        not_found!('Push Rule') unless user_group.push_rule
+        create_or_update_push_rule
       end
 
       desc 'Deletes group push rule' do

ee/spec/models/group_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe Group do
     it { is_expected.to have_many(:compliance_management_frameworks) }
     it { is_expected.to have_one(:deletion_schedule) }
     it { is_expected.to have_one(:group_wiki_repository) }
-    it { is_expected.to belong_to(:push_rule) }
+    it { is_expected.to belong_to(:push_rule).inverse_of(:group) }
     it { is_expected.to have_many(:saml_group_links) }
     it { is_expected.to have_many(:epics) }
     it { is_expected.to have_many(:epic_boards).inverse_of(:group) }

ee/spec/models/push_rule_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe PushRule do
 
   describe "Associations" do
     it { is_expected.to belong_to(:project).inverse_of(:push_rule) }
+    it { is_expected.to have_one(:group).inverse_of(:push_rule).autosave(true) }
   end
 
   describe "Validation" do

ee/spec/requests/api/group_push_rule_spec.rb

@@ -303,7 +303,7 @@ RSpec.describe API::GroupPushRule, 'GroupPushRule', api: true do
             put api("/groups/#{group_without_push_rule.id}/push_rule", admin), params: attributes_for_update
 
             expect(response).to have_gitlab_http_status(:not_found)
-            expect(json_response['message']).to include('Not Found')
+            expect(json_response['message']).to include('Push Rule Not Found')
           end
         end
 

locale/gitlab.pot

@@ -14704,9 +14704,6 @@ msgstr ""
 msgid "Group project URLs are prefixed with the group namespace"
 msgstr ""
 
-msgid "Group push rule exists, try updating"
-msgstr ""
-
 msgid "Group requires separate account"
 msgstr ""
 

spec/lib/gitlab/import_export/all_models.yml

@@ -740,3 +740,5 @@ status_page_published_incident:
 - issue
 issuable_sla:
   - issue
+push_rule:
+  - group