From f9fe1711509a6d4b708acf074fc84327b533ef0b Mon Sep 17 00:00:00 2001 From: mao chao Date: Thu, 10 Mar 2022 18:03:00 +0800 Subject: [PATCH 1/3] Refactor: Introduce BaseThirdPartyWiki We extract BaseThirdPartyWiki from Shimo and Confluence, reduce duplicate code. Also add post migration to add `third_party_wiki` category for Shimo and Confluence. Changelog: other --- app/models/integration.rb | 3 ++ .../integrations/base_third_party_wiki.rb | 38 +++++++++++++++++ app/models/integrations/confluence.rb | 15 +------ app/models/integrations/shimo.rb | 17 +------- ...grate_shimo_confluence_service_category.rb | 21 ++++++++++ db/schema_migrations/20220324032250 | 1 + ...e_shimo_confluence_integration_category.rb | 27 ++++++++++++ locale/gitlab.pot | 3 ++ ...mo_confluence_integration_category_spec.rb | 27 ++++++++++++ spec/lib/sidebars/projects/panel_spec.rb | 36 +++++++++++++--- ..._shimo_confluence_service_category_spec.rb | 33 +++++++++++++++ spec/models/integration_spec.rb | 11 +++++ .../base_third_party_wiki_spec.rb | 42 +++++++++++++++++++ 13 files changed, 238 insertions(+), 36 deletions(-) create mode 100644 app/models/integrations/base_third_party_wiki.rb create mode 100644 db/post_migrate/20220324032250_migrate_shimo_confluence_service_category.rb create mode 100644 db/schema_migrations/20220324032250 create mode 100644 lib/gitlab/background_migration/migrate_shimo_confluence_integration_category.rb create mode 100644 spec/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category_spec.rb create mode 100644 spec/migrations/20220324032250_migrate_shimo_confluence_service_category_spec.rb create mode 100644 spec/models/integrations/base_third_party_wiki_spec.rb diff --git a/app/models/integration.rb b/app/models/integration.rb index 32aaee4b49ea37..72a995e9450490 100644 --- a/app/models/integration.rb +++ b/app/models/integration.rb @@ -96,6 +96,9 @@ class Integration < ApplicationRecord validate :validate_belongs_to_project_or_group scope :external_issue_trackers, -> { where(category: 'issue_tracker').active } + # TODO: Will be modified in 14.10 + # Details: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74501#note_744393645 + scope :third_party_wikis, -> { where(type: %w[Integrations::Confluence Integrations::Shimo]).active } scope :by_name, ->(name) { by_type(integration_name_to_type(name)) } scope :external_wikis, -> { by_name(:external_wiki).active } scope :active, -> { where(active: true) } diff --git a/app/models/integrations/base_third_party_wiki.rb b/app/models/integrations/base_third_party_wiki.rb new file mode 100644 index 00000000000000..33d8fb45161ede --- /dev/null +++ b/app/models/integrations/base_third_party_wiki.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Integrations + class BaseThirdPartyWiki < Integration + default_value_for :category, 'third_party_wiki' + + validate :only_one_third_party_wiki, if: :activated?, on: :manual_change + + after_commit :cache_project_has_integration + + def self.supported_events + %w() + end + + private + + def only_one_third_party_wiki + return unless project_level? + + if project.integrations.third_party_wikis.id_not_in(id).any? + errors.add(:base, _('Another third-party wiki is already in use. Only one third-party wiki integration can be active at a time')) + end + end + + def cache_project_has_integration + return unless project && !project.destroyed? + + project_setting = project.project_setting + + project_setting.public_send("#{project_settings_cache_key}=", active?) # rubocop:disable GitlabSecurity/PublicSend + project_setting.save! + end + + def project_settings_cache_key + "has_#{self.class.to_param}" + end + end +end diff --git a/app/models/integrations/confluence.rb b/app/models/integrations/confluence.rb index 65adce7a8d6980..4e1d1993d026e4 100644 --- a/app/models/integrations/confluence.rb +++ b/app/models/integrations/confluence.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Integrations - class Confluence < Integration + class Confluence < BaseThirdPartyWiki VALID_SCHEME_MATCH = %r{\Ahttps?\Z}.freeze VALID_HOST_MATCH = %r{\A.+\.atlassian\.net\Z}.freeze VALID_PATH_MATCH = %r{\A/wiki(/|\Z)}.freeze @@ -11,16 +11,10 @@ class Confluence < Integration validates :confluence_url, presence: true, if: :activated? validate :validate_confluence_url_is_cloud, if: :activated? - after_commit :cache_project_has_confluence - def self.to_param 'confluence' end - def self.supported_events - %w() - end - def title s_('ConfluenceService|Confluence Workspace') end @@ -80,12 +74,5 @@ def confluence_uri_valid? rescue URI::InvalidURIError false end - - def cache_project_has_confluence - return unless project && !project.destroyed? - - project.project_setting.save! unless project.project_setting.persisted? - project.project_setting.update_column(:has_confluence, active?) - end end end diff --git a/app/models/integrations/shimo.rb b/app/models/integrations/shimo.rb index 0e1023bb7a7b80..dd25a0bc558ac5 100644 --- a/app/models/integrations/shimo.rb +++ b/app/models/integrations/shimo.rb @@ -1,12 +1,10 @@ # frozen_string_literal: true module Integrations - class Shimo < Integration + class Shimo < BaseThirdPartyWiki prop_accessor :external_wiki_url validates :external_wiki_url, presence: true, public_url: true, if: :activated? - after_commit :cache_project_has_shimo - def render? return false unless Feature.enabled?(:shimo_integration, project) @@ -33,10 +31,6 @@ def execute(_data) nil end - def self.supported_events - %w() - end - def fields [ { @@ -47,14 +41,5 @@ def fields } ] end - - private - - def cache_project_has_shimo - return unless project && !project.destroyed? - - project.project_setting.save! unless project.project_setting.persisted? - project.project_setting.update_column(:has_shimo, activated?) - end end end diff --git a/db/post_migrate/20220324032250_migrate_shimo_confluence_service_category.rb b/db/post_migrate/20220324032250_migrate_shimo_confluence_service_category.rb new file mode 100644 index 00000000000000..d341cc508744c7 --- /dev/null +++ b/db/post_migrate/20220324032250_migrate_shimo_confluence_service_category.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class MigrateShimoConfluenceServiceCategory < Gitlab::Database::Migration[1.0] + MIGRATION = 'MigrateShimoConfluenceIntegrationCategory' + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 10_000 + + disable_ddl_transaction! + + def up + queue_background_migration_jobs_by_range_at_intervals( + define_batchable_model('integrations').where(type_new: %w[Integrations::Confluence Integrations::Shimo]), + MIGRATION, + DELAY_INTERVAL, + batch_size: BATCH_SIZE, + track_jobs: true) + end + + def down + end +end diff --git a/db/schema_migrations/20220324032250 b/db/schema_migrations/20220324032250 new file mode 100644 index 00000000000000..ef37a8ff2234f2 --- /dev/null +++ b/db/schema_migrations/20220324032250 @@ -0,0 +1 @@ +fcfead40cfa1d75303bd0d392c09b5b0399ce5163e7ad6f8650360fe3adbe85d \ No newline at end of file diff --git a/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category.rb b/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category.rb new file mode 100644 index 00000000000000..ec4631d1e34c44 --- /dev/null +++ b/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # The class to migrate category of integrations to third_party_wiki for confluence and shimo + class MigrateShimoConfluenceIntegrationCategory + include Gitlab::Database::DynamicModelHelpers + + def perform(start_id, end_id) + define_batchable_model('integrations', connection: ::ActiveRecord::Base.connection) + .where(id: start_id..end_id, type_new: %w[Integrations::Confluence Integrations::Shimo]) + .update_all(category: 'third_party_wiki') + + mark_job_as_succeeded(start_id, end_id) + end + + private + + def mark_job_as_succeeded(*arguments) + Gitlab::Database::BackgroundMigrationJob.mark_all_as_succeeded( + self.class.name.demodulize, + arguments + ) + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 906cf5654f4953..b2ffce3aeeeddb 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4259,6 +4259,9 @@ msgstr "" msgid "Another issue tracker is already in use. Only one issue tracker service can be active at a time" msgstr "" +msgid "Another third-party wiki is already in use. Only one third-party wiki integration can be active at a time" +msgstr "" + msgid "Anti-spam verification" msgstr "" diff --git a/spec/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category_spec.rb b/spec/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category_spec.rb new file mode 100644 index 00000000000000..ff38fd1d0c793e --- /dev/null +++ b/spec/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::MigrateShimoConfluenceIntegrationCategory do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:integrations) { table(:integrations) } + let(:perform) { described_class.new.perform(1, 5) } + + before do + namespace = namespaces.create!(name: 'test', path: 'test') + projects.create!(id: 1, namespace_id: namespace.id, name: 'gitlab', path: 'gitlab') + integrations.create!(id: 1, active: true, type_new: "Integrations::SlackSlashCommands", category: 'chat', project_id: 1) + integrations.create!(id: 3, active: true, type_new: "Integrations::Confluence", category: 'common', project_id: 1) + integrations.create!(id: 5, active: true, type_new: "Integrations::Shimo", category: 'common', project_id: 1) + end + + describe '#up' do + it 'updates category to third_party_wiki for Shimo and Confluence' do + perform + + expect(integrations.where(category: 'third_party_wiki').count).to eq(2) + expect(integrations.where(category: 'chat').count).to eq(1) + end + end +end diff --git a/spec/lib/sidebars/projects/panel_spec.rb b/spec/lib/sidebars/projects/panel_spec.rb index 7e69a2dfe52eb0..ff253eedd0818c 100644 --- a/spec/lib/sidebars/projects/panel_spec.rb +++ b/spec/lib/sidebars/projects/panel_spec.rb @@ -17,16 +17,40 @@ subject { described_class.new(context).instance_variable_get(:@menus) } context 'when integration is present and active' do - let_it_be(:confluence) { create(:confluence_integration, active: true) } + context 'confluence only' do + let_it_be(:confluence) { create(:confluence_integration, active: true) } - let(:project) { confluence.project } + let(:project) { confluence.project } - it 'contains Confluence menu item' do - expect(subject.index { |i| i.is_a?(Sidebars::Projects::Menus::ConfluenceMenu) }).not_to be_nil + it 'contains Confluence menu item' do + expect(subject.index { |i| i.is_a?(Sidebars::Projects::Menus::ConfluenceMenu) }).not_to be_nil + end + + it 'does not contain Wiki menu item' do + expect(subject.index { |i| i.is_a?(Sidebars::Projects::Menus::WikiMenu) }).to be_nil + end end - it 'does not contain Wiki menu item' do - expect(subject.index { |i| i.is_a?(Sidebars::Projects::Menus::WikiMenu) }).to be_nil + context 'shimo only' do + let_it_be(:shimo) { create(:shimo_integration, active: true) } + + let(:project) { shimo.project } + + it 'contains Shimo menu item' do + expect(subject.index { |i| i.is_a?(Sidebars::Projects::Menus::ShimoMenu) }).not_to be_nil + end + end + + context 'confluence & shimo' do + let_it_be(:confluence) { create(:confluence_integration, active: true) } + let_it_be(:shimo) { create(:shimo_integration, active: true) } + + let(:project) { confluence.project } + + it 'contains Confluence menu item, not Shimo' do + expect(subject.index { |i| i.is_a?(Sidebars::Projects::Menus::ConfluenceMenu) }).not_to be_nil + expect(subject.index { |i| i.is_a?(Sidebars::Projects::Menus::ShimoMenu) }).to be_nil + end end end diff --git a/spec/migrations/20220324032250_migrate_shimo_confluence_service_category_spec.rb b/spec/migrations/20220324032250_migrate_shimo_confluence_service_category_spec.rb new file mode 100644 index 00000000000000..7100634f8ea365 --- /dev/null +++ b/spec/migrations/20220324032250_migrate_shimo_confluence_service_category_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe MigrateShimoConfluenceServiceCategory, :migration do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:integrations) { table(:integrations) } + + before do + namespace = namespaces.create!(name: 'test', path: 'test') + projects.create!(id: 1, namespace_id: namespace.id, name: 'gitlab', path: 'gitlab') + integrations.create!(id: 1, active: true, type_new: "Integrations::SlackSlashCommands", category: 'chat', project_id: 1) + integrations.create!(id: 3, active: true, type_new: "Integrations::Confluence", category: 'common', project_id: 1) + integrations.create!(id: 5, active: true, type_new: "Integrations::Shimo", category: 'common', project_id: 1) + end + + describe '#up' do + it 'correctly schedules background migrations', :aggregate_failures do + stub_const("#{described_class.name}::BATCH_SIZE", 2) + + Sidekiq::Testing.fake! do + freeze_time do + migrate! + + expect(described_class::MIGRATION).to be_scheduled_migration(3, 5) + expect(BackgroundMigrationWorker.jobs.size).to eq(1) + end + end + end + end +end diff --git a/spec/models/integration_spec.rb b/spec/models/integration_spec.rb index a1638cdc6afb39..21f6d320d71a26 100644 --- a/spec/models/integration_spec.rb +++ b/spec/models/integration_spec.rb @@ -60,6 +60,17 @@ end describe 'Scopes' do + describe '.third_party_wikis' do + let!(:integration1) { create(:jira_integration) } + let!(:integration2) { create(:redmine_integration) } + let!(:integration3) { create(:confluence_integration) } + let!(:integration4) { create(:shimo_integration) } + + it 'returns the right group integration' do + expect(described_class.third_party_wikis).to contain_exactly(integration3, integration4) + end + end + describe '.with_default_settings' do it 'returns the correct integrations' do instance_integration = create(:integration, :instance) diff --git a/spec/models/integrations/base_third_party_wiki_spec.rb b/spec/models/integrations/base_third_party_wiki_spec.rb new file mode 100644 index 00000000000000..1c75b1da516754 --- /dev/null +++ b/spec/models/integrations/base_third_party_wiki_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Integrations::BaseThirdPartyWiki do + describe 'Validations' do + let_it_be_with_reload(:project) { create(:project) } + + describe 'only one third party wiki per project' do + subject(:integration) { create(:shimo_integration, project: project, active: true) } + + before_all do + create(:confluence_integration, project: project, active: true) + end + + context 'when integration is changed manually by user' do + it 'executes the validation' do + valid = integration.valid?(:manual_change) + + expect(valid).to be_falsey + expect(integration.errors[:base]).to include( + _('Another third-party wiki is already in use. Only one third-party wiki integration can be active at a time') + ) + end + end + + context 'when integration is changed internally' do + it 'does not execute the validation' do + expect(integration.valid?).to be_truthy + end + end + + context 'when integration is not on the project level' do + subject(:integration) { create(:shimo_integration, :instance, active: true) } + + it 'executes the validation' do + expect(integration.valid?(:manual_change)).to be_truthy + end + end + end + end +end -- GitLab From 0c2306b5b09d4bdc1fe46c0c752a617614528b22 Mon Sep 17 00:00:00 2001 From: Andy Soiron Date: Mon, 4 Apr 2022 14:35:19 +0000 Subject: [PATCH 2/3] Update milestone in comment --- app/models/integration.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/integration.rb b/app/models/integration.rb index 72a995e9450490..3101d257d9efc1 100644 --- a/app/models/integration.rb +++ b/app/models/integration.rb @@ -96,7 +96,7 @@ class Integration < ApplicationRecord validate :validate_belongs_to_project_or_group scope :external_issue_trackers, -> { where(category: 'issue_tracker').active } - # TODO: Will be modified in 14.10 + # TODO: Will be modified in 15.0 # Details: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74501#note_744393645 scope :third_party_wikis, -> { where(type: %w[Integrations::Confluence Integrations::Shimo]).active } scope :by_name, ->(name) { by_type(integration_name_to_type(name)) } -- GitLab From 5001048ee469c6cbde27f35e8b10c6228a670e1a Mon Sep 17 00:00:00 2001 From: mao chao Date: Wed, 6 Apr 2022 11:56:56 +0800 Subject: [PATCH 3/3] Fix rubocop offenses --- app/models/integrations/base_third_party_wiki.rb | 3 ++- .../migrate_shimo_confluence_integration_category_spec.rb | 3 ++- ...032250_migrate_shimo_confluence_service_category_spec.rb | 3 ++- spec/models/integrations/base_third_party_wiki_spec.rb | 6 +++--- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/models/integrations/base_third_party_wiki.rb b/app/models/integrations/base_third_party_wiki.rb index 33d8fb45161ede..24f5bec93cfe11 100644 --- a/app/models/integrations/base_third_party_wiki.rb +++ b/app/models/integrations/base_third_party_wiki.rb @@ -18,7 +18,8 @@ def only_one_third_party_wiki return unless project_level? if project.integrations.third_party_wikis.id_not_in(id).any? - errors.add(:base, _('Another third-party wiki is already in use. Only one third-party wiki integration can be active at a time')) + errors.add(:base, _('Another third-party wiki is already in use. '\ + 'Only one third-party wiki integration can be active at a time')) end end diff --git a/spec/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category_spec.rb b/spec/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category_spec.rb index ff38fd1d0c793e..c7f0607fc884dc 100644 --- a/spec/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category_spec.rb +++ b/spec/lib/gitlab/background_migration/migrate_shimo_confluence_integration_category_spec.rb @@ -11,7 +11,8 @@ before do namespace = namespaces.create!(name: 'test', path: 'test') projects.create!(id: 1, namespace_id: namespace.id, name: 'gitlab', path: 'gitlab') - integrations.create!(id: 1, active: true, type_new: "Integrations::SlackSlashCommands", category: 'chat', project_id: 1) + integrations.create!(id: 1, active: true, type_new: "Integrations::SlackSlashCommands", + category: 'chat', project_id: 1) integrations.create!(id: 3, active: true, type_new: "Integrations::Confluence", category: 'common', project_id: 1) integrations.create!(id: 5, active: true, type_new: "Integrations::Shimo", category: 'common', project_id: 1) end diff --git a/spec/migrations/20220324032250_migrate_shimo_confluence_service_category_spec.rb b/spec/migrations/20220324032250_migrate_shimo_confluence_service_category_spec.rb index 7100634f8ea365..38db6d51e7e396 100644 --- a/spec/migrations/20220324032250_migrate_shimo_confluence_service_category_spec.rb +++ b/spec/migrations/20220324032250_migrate_shimo_confluence_service_category_spec.rb @@ -11,7 +11,8 @@ before do namespace = namespaces.create!(name: 'test', path: 'test') projects.create!(id: 1, namespace_id: namespace.id, name: 'gitlab', path: 'gitlab') - integrations.create!(id: 1, active: true, type_new: "Integrations::SlackSlashCommands", category: 'chat', project_id: 1) + integrations.create!(id: 1, active: true, type_new: "Integrations::SlackSlashCommands", + category: 'chat', project_id: 1) integrations.create!(id: 3, active: true, type_new: "Integrations::Confluence", category: 'common', project_id: 1) integrations.create!(id: 5, active: true, type_new: "Integrations::Shimo", category: 'common', project_id: 1) end diff --git a/spec/models/integrations/base_third_party_wiki_spec.rb b/spec/models/integrations/base_third_party_wiki_spec.rb index 1c75b1da516754..11e044c2a18b71 100644 --- a/spec/models/integrations/base_third_party_wiki_spec.rb +++ b/spec/models/integrations/base_third_party_wiki_spec.rb @@ -18,9 +18,9 @@ valid = integration.valid?(:manual_change) expect(valid).to be_falsey - expect(integration.errors[:base]).to include( - _('Another third-party wiki is already in use. Only one third-party wiki integration can be active at a time') - ) + error_message = 'Another third-party wiki is already in use. '\ + 'Only one third-party wiki integration can be active at a time' + expect(integration.errors[:base]).to include _(error_message) end end -- GitLab