Add new association for push rules

Add new association between project settings or application settings
and push rules

Add background migration

With specs

Refactor push rules controller

To respect new settings

Add push rules to creating new project

Add to admin push rules controller new association

Add small changes

Fix migrations

Add changelog entry

Add changelog entry

Add db structure changes

Remove obsolete part

Fix problems with migrations

Fix migration specs

Fix last line of schema

Add cr remarks

Add cr remarks

Add cr remarks

Add cr remarks

Add cr remarks

Add cr remarks

Fix typo

Add cr remarks

Add cr remarks

Add cr remarks

Add cr remarks

Update structure file

app/models/application_setting.rb

@@ -14,6 +14,7 @@ class ApplicationSetting < ApplicationRecord
   add_authentication_token_field :static_objects_external_storage_auth_token
 
   belongs_to :self_monitoring_project, class_name: "Project", foreign_key: 'instance_administration_project_id'
+  belongs_to :push_rule
   alias_attribute :self_monitoring_project_id, :instance_administration_project_id
 
   belongs_to :instance_administrators_group, class_name: "Group"

app/models/project_setting.rb

@@ -9,3 +9,5 @@ class ProjectSetting < ApplicationRecord
     where(primary_key => safe_find_or_create_by(attrs))
   end
 end
+
+ProjectSetting.prepend_if_ee('EE::ProjectSetting')

changelogs/unreleased/refactor_push_rules.yml

+---
+title: Refactor push rules and add push_rule_id columns in project settings and application settings
+merge_request: 28286
+author:
+type: added

db/migrate/20200325104755_add_push_rules_id_to_project_settings.rb

+# frozen_string_literal: true
+
+class AddPushRulesIdToProjectSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :project_settings, :push_rule_id, :bigint
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :project_settings, :push_rule_id
+    end
+  end
+end

db/migrate/20200325104756_add_push_rules_foreign_key_to_project_settings.rb

+# frozen_string_literal: true
+
+class AddPushRulesForeignKeyToProjectSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :project_settings, :push_rule_id, unique: true
+    add_concurrent_foreign_key :project_settings, :push_rules, column: :push_rule_id, on_delete: :cascade
+  end
+
+  def down
+    remove_foreign_key_if_exists :project_settings, column: :push_rule_id
+    remove_concurrent_index :project_settings, :push_rule_id
+  end
+end

db/migrate/20200325104833_add_push_rules_id_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddPushRulesIdToApplicationSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :application_settings, :push_rule_id, :bigint
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :application_settings, :push_rule_id
+    end
+  end
+end

db/migrate/20200325104834_add_push_rules_foreign_key_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddPushRulesForeignKeyToApplicationSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :application_settings, :push_rule_id, unique: true
+    add_concurrent_foreign_key :application_settings, :push_rules, column: :push_rule_id, on_delete: :nullify
+  end
+
+  def down
+    remove_concurrent_index :application_settings, :push_rule_id
+    remove_foreign_key_if_exists :application_settings, column: :push_rule_id
+  end
+end

db/post_migrate/20200325162730_schedule_backfill_push_rules_id_in_projects.rb

+# frozen_string_literal: true
+
+class ScheduleBackfillPushRulesIdInProjects < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  MIGRATION = 'BackfillPushRulesIdInProjects'.freeze
+  BATCH_SIZE = 1_000
+
+  class PushRules < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'push_rules'
+  end
+
+  def up
+    # Update one record that is connected to the instance
+    value_to_be_updated_to = ScheduleBackfillPushRulesIdInProjects::PushRules.find_by(is_sample: true)&.id
+
+    execute "UPDATE application_settings SET push_rule_id = #{value_to_be_updated_to}" if value_to_be_updated_to
+
+    ApplicationSetting.expire
+
+    queue_background_migration_jobs_by_range_at_intervals(ScheduleBackfillPushRulesIdInProjects::PushRules,
+                                                          MIGRATION,
+                                                          5.minutes,
+                                                          batch_size: BATCH_SIZE)
+  end
+
+  def down
+    execute "UPDATE application_settings SET push_rule_id = NULL"
+
+    ApplicationSetting.expire
+  end
+end

db/structure.sql

@@ -399,7 +399,8 @@ CREATE TABLE public.application_settings (
     namespace_storage_size_limit bigint DEFAULT 0 NOT NULL,
     seat_link_enabled boolean DEFAULT true NOT NULL,
     container_expiration_policies_enable_historic_entries boolean DEFAULT false NOT NULL,
-    issues_create_limit integer DEFAULT 300 NOT NULL
+    issues_create_limit integer DEFAULT 300 NOT NULL,
+    push_rule_id bigint
 );
 
 CREATE SEQUENCE public.application_settings_id_seq
@@ -5025,7 +5026,8 @@ ALTER SEQUENCE public.project_repository_states_id_seq OWNED BY public.project_r
 CREATE TABLE public.project_settings (
     project_id integer NOT NULL,
     created_at timestamp with time zone NOT NULL,
-    updated_at timestamp with time zone NOT NULL
+    updated_at timestamp with time zone NOT NULL,
+    push_rule_id bigint
 );
 
 CREATE TABLE public.project_statistics (
@@ -8677,6 +8679,8 @@ CREATE INDEX index_application_settings_on_file_template_project_id ON public.ap
 
 CREATE INDEX index_application_settings_on_instance_administrators_group_id ON public.application_settings USING btree (instance_administrators_group_id);
 
+CREATE UNIQUE INDEX index_application_settings_on_push_rule_id ON public.application_settings USING btree (push_rule_id);
+
 CREATE INDEX index_application_settings_on_usage_stats_set_by_user_id ON public.application_settings USING btree (usage_stats_set_by_user_id);
 
 CREATE INDEX index_applicationsettings_on_instance_administration_project_id ON public.application_settings USING btree (instance_administration_project_id);
@@ -9891,6 +9895,8 @@ CREATE INDEX index_project_repositories_on_shard_id ON public.project_repositori
 
 CREATE UNIQUE INDEX index_project_repository_states_on_project_id ON public.project_repository_states USING btree (project_id);
 
+CREATE UNIQUE INDEX index_project_settings_on_push_rule_id ON public.project_settings USING btree (push_rule_id);
+
 CREATE INDEX index_project_statistics_on_namespace_id ON public.project_statistics USING btree (namespace_id);
 
 CREATE UNIQUE INDEX index_project_statistics_on_project_id ON public.project_statistics USING btree (project_id);
@@ -10636,6 +10642,9 @@ ALTER TABLE ONLY public.epics
 ALTER TABLE ONLY public.ci_pipelines
     ADD CONSTRAINT fk_3d34ab2e06 FOREIGN KEY (pipeline_schedule_id) REFERENCES public.ci_pipeline_schedules(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY public.project_settings
+    ADD CONSTRAINT fk_413a953e20 FOREIGN KEY (push_rule_id) REFERENCES public.push_rules(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.ci_pipeline_schedule_variables
     ADD CONSTRAINT fk_41c35fda51 FOREIGN KEY (pipeline_schedule_id) REFERENCES public.ci_pipeline_schedules(id) ON DELETE CASCADE;
 
@@ -10687,6 +10696,9 @@ ALTER TABLE ONLY public.merge_requests
 ALTER TABLE ONLY public.ci_builds
     ADD CONSTRAINT fk_6661f4f0e8 FOREIGN KEY (resource_group_id) REFERENCES public.ci_resource_groups(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY public.application_settings
+    ADD CONSTRAINT fk_693b8795e4 FOREIGN KEY (push_rule_id) REFERENCES public.push_rules(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY public.merge_requests
     ADD CONSTRAINT fk_6a5165a692 FOREIGN KEY (milestone_id) REFERENCES public.milestones(id) ON DELETE SET NULL;
 
@@ -13113,9 +13125,14 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200323134519
 20200324093258
 20200324115359
+20200325104755
+20200325104756
+20200325104833
+20200325104834
 20200325111432
 20200325152327
 20200325160952
+20200325162730
 20200325183636
 20200326114443
 20200326122700

ee/app/controllers/admin/push_rules_controller.rb

@@ -14,6 +14,7 @@ class Admin::PushRulesController < Admin::ApplicationController
     @push_rule.update(push_rule_params)
 
     if @push_rule.valid?
+      link_push_rule_to_application_settings
       redirect_to admin_push_rule_path, notice: _('Push Rule updated successfully.')
     else
       render :show
@@ -51,4 +52,10 @@ class Admin::PushRulesController < Admin::ApplicationController
   def set_application_setting
     @application_setting = ApplicationSetting.current_without_cache
   end
+
+  def link_push_rule_to_application_settings
+    return if @application_setting.push_rule_id
+
+    @application_setting.update(push_rule_id: @push_rule.id)
+  end
 end

ee/app/controllers/ee/projects/settings/repository_controller.rb

@@ -17,7 +17,11 @@ module EE
         def push_rule
           return unless project.feature_available?(:push_rules)
 
-          project.create_push_rule unless project.push_rule
+          unless project.push_rule
+            push_rule = project.create_push_rule
+            project.project_setting.update(push_rule_id: push_rule.id)
+          end
+
           @push_rule = project.push_rule # rubocop:disable Gitlab/ModuleWithInstanceVariables
         end
 

ee/app/models/ee/project_setting.rb

+# frozen_string_literal: true
+
+module EE
+  module ProjectSetting
+    extend ActiveSupport::Concern
+
+    prepended do
+      belongs_to :push_rule
+    end
+  end
+end

ee/app/services/ee/projects/create_service.rb

@@ -59,6 +59,7 @@ module EE
         if predefined_push_rule
           push_rule = predefined_push_rule.dup.tap { |gh| gh.is_sample = false }
           project.push_rule = push_rule
+          project.project_setting.update(push_rule: push_rule)
         end
       end
 

ee/spec/controllers/admin/push_rules_controller_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Admin::PushRulesController do
+  include StubENV
+
   let(:admin) { create(:admin) }
 
   before do
@@ -18,6 +20,10 @@ describe Admin::PushRulesController do
       }
     end
 
+    before do
+      stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+    end
+
     it 'updates sample push rule' do
       expect_next_instance_of(PushRule) do |instance|
         expect(instance).to receive(:update).with(ActionController::Parameters.new(params).permit!)
@@ -28,6 +34,12 @@ describe Admin::PushRulesController do
       expect(response).to redirect_to(admin_push_rule_path)
     end
 
+    it 'links push rule with application settings' do
+      patch :update, params: { push_rule: params }
+
+      expect(ApplicationSetting.current.push_rule_id).not_to be_nil
+    end
+
     context 'push rules unlicensed' do
       before do
         stub_licensed_features(push_rules: false)

ee/spec/controllers/projects/settings/repository_controller_spec.rb

@@ -21,6 +21,12 @@ describe Projects::Settings::RepositoryController do
         is_expected.to be_persisted
       end
 
+      it 'is connected to project_settings' do
+        get :show, params: { namespace_id: project.namespace, project_id: project }
+
+        expect(project.project_setting.push_rule).to eq(subject)
+      end
+
       context 'unlicensed' do
         before do
           stub_licensed_features(push_rules: false)

ee/spec/features/admin/admin_interacts_with_push_rules_spec.rb

@@ -3,9 +3,12 @@
 require "spec_helper"
 
 describe "Admin interacts with push rules" do
+  include StubENV
+
   let_it_be(:user) { create(:admin) }
 
   before do
+    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
     sign_in(user)
   end
 

ee/spec/services/projects/create_service_spec.rb

@@ -186,12 +186,12 @@ describe Projects::CreateService, '#execute' do
     end
   end
 
-  context 'git hook sample' do
+  context 'push rules sample' do
     let!(:sample) { create(:push_rule_sample) }
 
     subject(:push_rule) { create_project(user, opts).push_rule }
 
-    it 'creates git hook from sample' do
+    it 'creates push rule from sample' do
       is_expected.to have_attributes(
         force_push_regex: sample.force_push_regex,
         deny_delete_tag: sample.deny_delete_tag,
@@ -200,6 +200,12 @@ describe Projects::CreateService, '#execute' do
       )
     end
 
+    it 'creates association between project settings and push rule' do
+      project_setting = subject.project.project_setting
+
+      expect(project_setting.push_rule_id).to eq(subject.id)
+    end
+
     context 'push rules unlicensed' do
       before do
         stub_licensed_features(push_rules: false)

lib/gitlab/background_migration/backfill_push_rules_id_in_projects.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # Class that will insert record into project_push_rules
+    # for each existing push_rule
+    class BackfillPushRulesIdInProjects
+      # Temporary AR table for push rules
+      class ProjectSetting < ActiveRecord::Base
+        self.table_name = 'project_settings'
+      end
+
+      def perform(start_id, stop_id)
+        ProjectSetting.connection.execute(<<~SQL)
+          UPDATE project_settings ps1
+          SET push_rule_id = pr.id
+          FROM project_settings ps2
+          INNER JOIN push_rules pr
+          ON ps2.project_id = pr.project_id
+          WHERE pr.is_sample = false
+          AND pr.id BETWEEN #{start_id} AND #{stop_id}
+          AND ps1.project_id = ps2.project_id
+        SQL
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/backfill_push_rules_id_in_projects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::BackfillPushRulesIdInProjects, :migration, schema: 2020_03_25_162730 do
+  let(:push_rules) { table(:push_rules) }
+  let(:projects) { table(:projects) }
+  let(:project_settings) { table(:project_settings) }
+  let(:namespace) { table(:namespaces).create(name: 'user', path: 'user') }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    it 'creates new project push_rules for all push rules in the range' do
+      project_1 = projects.create(id: 1, namespace_id: namespace.id)
+      project_2 = projects.create(id: 2, namespace_id: namespace.id)
+      project_3 = projects.create(id: 3, namespace_id: namespace.id)
+      project_settings_1 = project_settings.create(project_id: project_1.id)
+      project_settings_2 = project_settings.create(project_id: project_2.id)
+      project_settings_3 = project_settings.create(project_id: project_3.id)
+      push_rule_1 = push_rules.create(id: 5, is_sample: false, project_id: project_1.id)
+      push_rule_2 = push_rules.create(id: 6, is_sample: false, project_id: project_2.id)
+      push_rules.create(id: 8, is_sample: false, project_id: 3)
+
+      subject.perform(5, 7)
+
+      expect(project_settings_1.reload.push_rule_id).to eq(push_rule_1.id)
+      expect(project_settings_2.reload.push_rule_id).to eq(push_rule_2.id)
+      expect(project_settings_3.reload.push_rule_id).to be_nil
+    end
+  end
+end

spec/migrations/schedule_backfill_push_rules_id_in_projects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'post_migrate', '20200325162730_schedule_backfill_push_rules_id_in_projects.rb')
+
+describe ScheduleBackfillPushRulesIdInProjects do
+  let(:push_rules) { table(:push_rules) }
+
+  it 'adds global rule association to application settings' do
+    application_settings = table(:application_settings)
+    setting = application_settings.create!
+    sample_rule = push_rules.create!(is_sample: true)
+
+    Sidekiq::Testing.fake! do
+      disable_migrations_output { migrate! }
+    end
+
+    setting.reload
+    expect(setting.push_rule_id).to eq(sample_rule.id)
+  end
+
+  it 'schedules worker to migrate project push rules' do
+    rule_1 = push_rules.create!
+    rule_2 = push_rules.create!
+
+    Sidekiq::Testing.fake! do
+      disable_migrations_output { migrate! }
+
+      expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+      expect(described_class::MIGRATION)
+        .to be_scheduled_delayed_migration(5.minutes, rule_1.id, rule_2.id)
+    end
+  end
+end