From c1f4644accc22c8281905903ad6e282918424d1b Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 18 Jan 2021 13:30:59 +1300 Subject: [PATCH 1/3] Add PG trigger for has_external_issue_tracker We cache whether a project has an External Issue Tracker integration enabled in the `has_external_issue_tracker` column on `projects`. It will be `TRUE` if the project has any integration (a record in `services`) with a category of "issue_tracker" enabled. This was previously maintained with application code. This cache is easy to fall out of consistency when we fail to maintain it during bulk operations (including PostgreSQL cascading deletes https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48163/) as mentioned in: - https://gitlab.com/gitlab-org/gitlab/-/issues/273574#note_457006686 - https://gitlab.com/gitlab-org/gitlab/-/issues/289798 As Ecosystem is increasingly changing integration data using bulk operations this has been changed to be maintained through a PostgreSQL trigger. https://gitlab.com/gitlab-org/gitlab/-/issues/290715 https://gitlab.com/gitlab-org/gitlab/-/issues/289798 --- app/models/project.rb | 20 +-- app/models/service.rb | 11 -- .../bulk_create_integration_service.rb | 10 -- ...715-has_external_issue_tracker_trigger.yml | 5 + ..._add_has_external_issue_tracker_trigger.rb | 61 +++++++ db/schema_migrations/20210117210226 | 1 + db/structure.sql | 26 +++ ...has_external_issue_tracker_trigger_spec.rb | 146 +++++++++++++++++ spec/models/merge_request_spec.rb | 13 +- spec/models/project_spec.rb | 150 ++++++++++-------- spec/models/service_spec.rb | 46 ------ .../bulk_create_integration_service_spec.rb | 51 ------ spec/services/issues/close_service_spec.rb | 3 + .../merge_requests/build_service_spec.rb | 2 + .../system_notes/issuables_service_spec.rb | 2 + 15 files changed, 346 insertions(+), 201 deletions(-) create mode 100644 changelogs/unreleased/290715-has_external_issue_tracker_trigger.yml create mode 100644 db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb create mode 100644 db/schema_migrations/20210117210226 create mode 100644 spec/migrations/add_has_external_issue_tracker_trigger_spec.rb diff --git a/app/models/project.rb b/app/models/project.rb index ec7907988062d6..bfa85fc9d99b00 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1314,21 +1314,11 @@ def default_issues_tracker? end def external_issue_tracker - if has_external_issue_tracker.nil? - cache_has_external_issue_tracker - end + cache_has_external_issue_tracker if has_external_issue_tracker.nil? - if has_external_issue_tracker? - strong_memoize(:external_issue_tracker) do - services.external_issue_trackers.first - end - else - nil - end - end + return unless has_external_issue_tracker? - def cache_has_external_issue_tracker - update_column(:has_external_issue_tracker, services.external_issue_trackers.any?) if Gitlab::Database.read_write? + @external_issue_tracker ||= services.external_issue_trackers.first end def external_references_supported? @@ -2690,6 +2680,10 @@ def oids(objects, oids: []) def cache_has_external_wiki update_column(:has_external_wiki, services.external_wikis.any?) if Gitlab::Database.read_write? end + + def cache_has_external_issue_tracker + update_column(:has_external_issue_tracker, services.external_issue_trackers.any?) if Gitlab::Database.read_write? + end end Project.prepend_if_ee('EE::Project') diff --git a/app/models/service.rb b/app/models/service.rb index e5626462dd3957..2b92b2cd7d85eb 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -46,7 +46,6 @@ class Service < ApplicationRecord after_initialize :initialize_properties after_commit :reset_updated_properties - after_commit :cache_project_has_external_issue_tracker belongs_to :project, inverse_of: :services belongs_to :group, inverse_of: :services @@ -438,10 +437,6 @@ def async_execute(data) ProjectServiceWorker.perform_async(id, data) end - def external_issue_tracker? - category == :issue_tracker && active? - end - def external_wiki? type == 'ExternalWikiService' && active? end @@ -461,12 +456,6 @@ def validate_belongs_to_project_or_group errors.add(:project_id, 'The service cannot belong to both a project and a group') if project_id && group_id end - def cache_project_has_external_issue_tracker - if project && !project.destroyed? - project.cache_has_external_issue_tracker - end - end - def valid_recipients? activated? && !importing? end diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb index df78c3645c74ea..ae756d0856eb56 100644 --- a/app/services/bulk_create_integration_service.rb +++ b/app/services/bulk_create_integration_service.rb @@ -11,8 +11,6 @@ def execute service_list = ServiceList.new(batch, service_hash, association).to_array Service.transaction do - run_callbacks(batch) if association == 'project' - results = bulk_insert(*service_list) if integration.data_fields_present? @@ -33,14 +31,6 @@ def bulk_insert(klass, columns, values_array) klass.insert_all(items_to_insert, returning: [:id]) end - # rubocop: disable CodeReuse/ActiveRecord - def run_callbacks(batch) - if integration.external_issue_tracker? - Project.where(id: batch.select(:id)).update_all(has_external_issue_tracker: true) - end - end - # rubocop: enable CodeReuse/ActiveRecord - def service_hash if integration.template? integration.to_service_hash diff --git a/changelogs/unreleased/290715-has_external_issue_tracker_trigger.yml b/changelogs/unreleased/290715-has_external_issue_tracker_trigger.yml new file mode 100644 index 00000000000000..d9d40c99c5cea6 --- /dev/null +++ b/changelogs/unreleased/290715-has_external_issue_tracker_trigger.yml @@ -0,0 +1,5 @@ +--- +title: Add PostgreSQL trigger to maintain projects.has_external_issue_tracker +merge_request: 51852 +author: +type: changed diff --git a/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb b/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb new file mode 100644 index 00000000000000..7f29b0fd86c972 --- /dev/null +++ b/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +class AddHasExternalIssueTrackerTrigger < ActiveRecord::Migration[6.0] + include Gitlab::Database::SchemaHelpers + + DOWNTIME = false + FUNCTION_NAME = 'set_has_external_issue_tracker'.freeze + TRIGGER_ON_INSERT_NAME = 'trigger_has_external_issue_tracker_on_insert'.freeze + TRIGGER_ON_UPDATE_NAME = 'trigger_has_external_issue_tracker_on_update'.freeze + TRIGGER_ON_DELETE_NAME = 'trigger_has_external_issue_tracker_on_delete'.freeze + + def up + create_trigger_function(FUNCTION_NAME, replace: true) do + <<~SQL + UPDATE projects SET has_external_issue_tracker = ( + EXISTS + ( + SELECT 1 + FROM services + WHERE project_id = COALESCE(NEW.project_id, OLD.project_id) + AND active = TRUE + AND category = 'issue_tracker' + ) + ) + WHERE projects.id = COALESCE(NEW.project_id, OLD.project_id); + RETURN NULL; + SQL + end + + execute(<<~SQL) + CREATE TRIGGER #{TRIGGER_ON_INSERT_NAME} + AFTER INSERT ON services + FOR EACH ROW + WHEN (NEW.category = 'issue_tracker' AND NEW.active = TRUE AND NEW.project_id IS NOT NULL) + EXECUTE FUNCTION #{FUNCTION_NAME}(); + SQL + + execute(<<~SQL) + CREATE TRIGGER #{TRIGGER_ON_UPDATE_NAME} + AFTER UPDATE ON services + FOR EACH ROW + WHEN (NEW.category = 'issue_tracker' AND OLD.active != NEW.active AND NEW.project_id IS NOT NULL) + EXECUTE FUNCTION #{FUNCTION_NAME}(); + SQL + + execute(<<~SQL) + CREATE TRIGGER #{TRIGGER_ON_DELETE_NAME} + AFTER DELETE ON services + FOR EACH ROW + WHEN (OLD.category = 'issue_tracker' AND OLD.active = TRUE AND OLD.project_id IS NOT NULL) + EXECUTE FUNCTION #{FUNCTION_NAME}(); + SQL + end + + def down + drop_trigger(:services, TRIGGER_ON_INSERT_NAME) + drop_trigger(:services, TRIGGER_ON_UPDATE_NAME) + drop_trigger(:services, TRIGGER_ON_DELETE_NAME) + drop_function(FUNCTION_NAME) + end +end diff --git a/db/schema_migrations/20210117210226 b/db/schema_migrations/20210117210226 new file mode 100644 index 00000000000000..a68f7f6d93e87b --- /dev/null +++ b/db/schema_migrations/20210117210226 @@ -0,0 +1 @@ +c0d22d00d52a516347930e1a36f350113c0949214925176f08ceed81999746bd \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 38965f00aa60a4..811b6adbe88988 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -10,6 +10,26 @@ CREATE EXTENSION IF NOT EXISTS btree_gist; CREATE EXTENSION IF NOT EXISTS pg_trgm; +CREATE FUNCTION set_has_external_issue_tracker() RETURNS trigger + LANGUAGE plpgsql + AS $$ +BEGIN +UPDATE projects SET has_external_issue_tracker = ( + EXISTS + ( + SELECT 1 + FROM services + WHERE project_id = COALESCE(NEW.project_id, OLD.project_id) + AND active = TRUE + AND category = 'issue_tracker' + ) + ) +WHERE projects.id = COALESCE(NEW.project_id, OLD.project_id); +RETURN NULL; + +END +$$; + CREATE FUNCTION set_has_external_wiki() RETURNS trigger LANGUAGE plpgsql AS $$ @@ -23637,6 +23657,12 @@ ALTER INDEX product_analytics_events_experimental_pkey ATTACH PARTITION gitlab_p CREATE TRIGGER table_sync_trigger_ee39a25f9d AFTER INSERT OR DELETE OR UPDATE ON audit_events FOR EACH ROW EXECUTE PROCEDURE table_sync_function_2be879775d(); +CREATE TRIGGER trigger_has_external_issue_tracker_on_delete AFTER DELETE ON services FOR EACH ROW WHEN ((((old.category)::text = 'issue_tracker'::text) AND (old.active = true) AND (old.project_id IS NOT NULL))) EXECUTE PROCEDURE set_has_external_issue_tracker(); + +CREATE TRIGGER trigger_has_external_issue_tracker_on_insert AFTER INSERT ON services FOR EACH ROW WHEN ((((new.category)::text = 'issue_tracker'::text) AND (new.active = true) AND (new.project_id IS NOT NULL))) EXECUTE PROCEDURE set_has_external_issue_tracker(); + +CREATE TRIGGER trigger_has_external_issue_tracker_on_update AFTER UPDATE ON services FOR EACH ROW WHEN ((((new.category)::text = 'issue_tracker'::text) AND (old.active <> new.active) AND (new.project_id IS NOT NULL))) EXECUTE PROCEDURE set_has_external_issue_tracker(); + CREATE TRIGGER trigger_has_external_wiki_on_delete AFTER DELETE ON services FOR EACH ROW WHEN ((((old.type)::text = 'ExternalWikiService'::text) AND (old.project_id IS NOT NULL))) EXECUTE PROCEDURE set_has_external_wiki(); CREATE TRIGGER trigger_has_external_wiki_on_insert AFTER INSERT ON services FOR EACH ROW WHEN (((new.active = true) AND ((new.type)::text = 'ExternalWikiService'::text) AND (new.project_id IS NOT NULL))) EXECUTE PROCEDURE set_has_external_wiki(); diff --git a/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb b/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb new file mode 100644 index 00000000000000..cbd1e36c2c2c34 --- /dev/null +++ b/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb @@ -0,0 +1,146 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require_migration! + +RSpec.describe AddHasExternalIssueTrackerTrigger do + let(:migration) { described_class.new } + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:services) { table(:services) } + + before do + @namespace = namespaces.create!(name: 'foo', path: 'foo') + @project = projects.create!(namespace_id: @namespace.id) + end + + describe '#up' do + before do + migrate! + end + + describe 'INSERT trigger' do + it 'sets `has_external_issue_tracker` to true when active `issue_tracker` is inserted' do + expect do + services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + end.to change { @project.reload.has_external_issue_tracker }.to(true) + end + + it 'does not set `has_external_issue_tracker` to true when service is for a different project' do + different_project = projects.create!(namespace_id: @namespace.id) + + expect do + services.create!(category: 'issue_tracker', active: true, project_id: different_project.id) + end.not_to change { @project.reload.has_external_issue_tracker } + end + + it 'does not set `has_external_issue_tracker` to true when inactive `issue_tracker` is inserted' do + expect do + services.create!(category: 'issue_tracker', active: false, project_id: @project.id) + end.not_to change { @project.reload.has_external_issue_tracker } + end + + it 'does not set `has_external_issue_tracker` to true when active other service is inserted' do + expect do + services.create!(category: 'my_type', active: true, project_id: @project.id) + end.not_to change { @project.reload.has_external_issue_tracker } + end + end + + describe 'UPDATE trigger' do + it 'sets `has_external_issue_tracker` to true when `issue_tracker` is made active' do + service = services.create!(category: 'issue_tracker', active: false, project_id: @project.id) + + expect do + service.update!(active: true) + end.to change { @project.reload.has_external_issue_tracker }.to(true) + end + + it 'sets `has_external_issue_tracker` to false when `issue_tracker` is made inactive' do + service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + + expect do + service.update!(active: false) + end.to change { @project.reload.has_external_issue_tracker }.to(false) + end + + it 'does not change `has_external_issue_tracker` when `issue_tracker` is made inactive, if an active `issue_tracker` still exists' do + services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + + expect do + service.update!(active: false) + end.not_to change { @project.reload.has_external_issue_tracker } + end + + it 'does not change `has_external_issue_tracker` when service is for a different project' do + different_project = projects.create!(namespace_id: @namespace.id) + service = services.create!(category: 'issue_tracker', active: false, project_id: different_project.id) + + expect do + service.update!(active: true) + end.not_to change { @project.reload.has_external_issue_tracker } + end + end + + describe 'DELETE trigger' do + it 'sets `has_external_issue_tracker` to false when `issue_tracker` is deleted' do + service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + + expect do + service.delete + end.to change { @project.reload.has_external_issue_tracker }.to(false) + end + + it 'does not set `has_external_issue_tracker` to false when `issue_tracker` is deleted, if an active `issue_tracker` still exists' do + services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + + expect do + service.delete + end.not_to change { @project.reload.has_external_issue_tracker } + end + + it 'does not change `has_external_issue_tracker` when service is for a different project' do + different_project = projects.create!(namespace_id: @namespace.id) + service = services.create!(category: 'issue_tracker', active: true, project_id: different_project.id) + + expect do + service.delete + end.not_to change { @project.reload.has_external_issue_tracker } + end + end + end + + describe '#down' do + before do + migration.up + migration.down + end + + it 'drops the INSERT trigger' do + expect do + services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + end.not_to change { @project.reload.has_external_issue_tracker } + end + + it 'drops the UPDATE trigger' do + service = services.create!(category: 'issue_tracker', active: false, project_id: @project.id) + @project.update!(has_external_issue_tracker: false) + + expect do + service.update!(active: true) + end.not_to change { @project.reload.has_external_issue_tracker } + end + + it 'drops the DELETE trigger' do + service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + @project.update!(has_external_issue_tracker: true) + + expect do + service.delete + end.not_to change { @project.reload.has_external_issue_tracker } + end + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1cf197322f5d1c..b8a3f3bef6ebf5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -754,9 +754,8 @@ def expected_total_time(mrs) context 'when both internal and external issue trackers are enabled' do before do - subject.project.has_external_issue_tracker = true - subject.project.save! create(:jira_service, project: subject.project) + subject.project.reload end it 'does not cache issues from external trackers' do @@ -1263,9 +1262,10 @@ def set_compare(merge_request) describe '#issues_mentioned_but_not_closing' do let(:closing_issue) { create :issue, project: subject.project } let(:mentioned_issue) { create :issue, project: subject.project } - let(:commit) { double('commit', safe_message: "Fixes #{closing_issue.to_reference}") } + subject { create(:merge_request, source_project: create(:project)) } + it 'detects issues mentioned in description but not closed' do subject.project.add_developer(subject.author) subject.description = "Is related to #{mentioned_issue.to_reference} and #{closing_issue.to_reference}" @@ -1279,13 +1279,12 @@ def set_compare(merge_request) end context 'when the project has an external issue tracker' do - subject { create(:merge_request, source_project: create(:project, :repository)) } - before do subject.project.add_developer(subject.author) commit = double(:commit, safe_message: 'Fixes TEST-3') create(:jira_service, project: subject.project) + subject.project.reload allow(subject).to receive(:commits).and_return([commit]) allow(subject).to receive(:description).and_return('Is related to TEST-2 and TEST-3') @@ -1469,6 +1468,8 @@ def set_compare(merge_request) end describe '#default_merge_commit_message' do + subject { create(:merge_request, source_project: create(:project)) } + it 'includes merge information as the title' do request = build(:merge_request, source_branch: 'source', target_branch: 'target') @@ -1645,7 +1646,7 @@ def set_compare(merge_request) end it_behaves_like 'an editable mentionable' do - subject { create(:merge_request, :simple) } + subject { create(:merge_request, :simple, source_project: create(:project, :repository)) } let(:backref_text) { "merge request #{subject.to_reference}" } let(:set_mentionable_text) { ->(txt) { subject.description = txt } } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a2b51684d4d3ce..8d0b7336955767 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1002,103 +1002,125 @@ end end - describe '#external_issue_tracker' do - let(:project) { create(:project) } - let(:ext_project) { create(:redmine_project) } + describe '#has_wiki?' do + let(:no_wiki_project) { create(:project, :wiki_disabled, has_external_wiki: false) } + let(:wiki_enabled_project) { create(:project) } + let(:external_wiki_project) { create(:project, has_external_wiki: true) } - context 'on existing projects with no value for has_external_issue_tracker' do - before do - project.update_column(:has_external_issue_tracker, nil) - ext_project.update_column(:has_external_issue_tracker, nil) + it 'returns true if project is wiki enabled or has external wiki' do + expect(wiki_enabled_project).to have_wiki + expect(external_wiki_project).to have_wiki + expect(no_wiki_project).not_to have_wiki + end + end + + describe '#default_owner' do + let_it_be(:owner) { create(:user) } + let_it_be(:namespace) { create(:namespace, owner: owner) } + + context 'the project does not have a group' do + let(:project) { build(:project, namespace: namespace) } + + it 'is the namespace owner' do + expect(project.default_owner).to eq(owner) end + end - it 'updates the has_external_issue_tracker boolean' do - expect do - project.external_issue_tracker - end.to change { project.reload.has_external_issue_tracker }.to(false) + context 'the project is in a group' do + let(:group) { build(:group) } + let(:project) { build(:project, group: group, namespace: namespace) } - expect do - ext_project.external_issue_tracker - end.to change { ext_project.reload.has_external_issue_tracker }.to(true) + it 'is the group owner' do + allow(group).to receive(:default_owner).and_return(Object.new) + + expect(project.default_owner).to eq(group.default_owner) end end + end + + describe '#external_issue_tracker' do + it 'sets Project#has_external_issue_tracker when it is nil' do + project_with_no_tracker = create(:project, has_external_issue_tracker: nil) + project_with_tracker = create(:redmine_project, has_external_issue_tracker: nil) + + expect do + project_with_no_tracker.external_issue_tracker + end.to change { project_with_no_tracker.reload.has_external_issue_tracker }.from(nil).to(false) + + expect do + project_with_tracker.external_issue_tracker + end.to change { project_with_tracker.reload.has_external_issue_tracker }.from(nil).to(true) + end it 'returns nil and does not query services when there is no external issue tracker' do - expect(project).not_to receive(:services) + project = create(:project) + expect(project).not_to receive(:services) expect(project.external_issue_tracker).to eq(nil) end it 'retrieves external_issue_tracker querying services and cache it when there is external issue tracker' do - ext_project.reload # Factory returns a project with changed attributes - expect(ext_project).to receive(:services).once.and_call_original + project = create(:redmine_project) - 2.times { expect(ext_project.external_issue_tracker).to be_a_kind_of(RedmineService) } + expect(project).to receive(:services).once.and_call_original + 2.times { expect(project.external_issue_tracker).to be_a_kind_of(RedmineService) } end end - describe '#cache_has_external_issue_tracker' do - let_it_be(:project) { create(:project, has_external_issue_tracker: nil) } - - it 'stores true if there is any external_issue_tracker' do - services = double(:service, external_issue_trackers: [RedmineService.new]) - expect(project).to receive(:services).and_return(services) + describe '#has_external_issue_tracker' do + let_it_be(:project) { create(:project) } - expect do - project.cache_has_external_issue_tracker - end.to change { project.has_external_issue_tracker}.to(true) + def subject + project.reload.has_external_issue_tracker end - it 'stores false if there is no external_issue_tracker' do - services = double(:service, external_issue_trackers: []) - expect(project).to receive(:services).and_return(services) + it 'is false when external issue tracker service is not active' do + create(:service, project: project, category: 'issue_tracker', active: false) - expect do - project.cache_has_external_issue_tracker - end.to change { project.has_external_issue_tracker}.to(false) + is_expected.to eq(false) end - it 'does not cache data when in a read-only GitLab instance' do - allow(Gitlab::Database).to receive(:read_only?) { true } + it 'is false when other service is active' do + create(:service, project: project, category: 'not_issue_tracker', active: true) - expect do - project.cache_has_external_issue_tracker - end.not_to change { project.has_external_issue_tracker } + is_expected.to eq(false) end - end - describe '#has_wiki?' do - let(:no_wiki_project) { create(:project, :wiki_disabled, has_external_wiki: false) } - let(:wiki_enabled_project) { create(:project) } - let(:external_wiki_project) { create(:project, has_external_wiki: true) } - - it 'returns true if project is wiki enabled or has external wiki' do - expect(wiki_enabled_project).to have_wiki - expect(external_wiki_project).to have_wiki - expect(no_wiki_project).not_to have_wiki - end - end + context 'when there is an active external issue tracker service' do + let!(:service) do + create(:service, project: project, type: 'JiraService', category: 'issue_tracker', active: true) + end - describe '#default_owner' do - let_it_be(:owner) { create(:user) } - let_it_be(:namespace) { create(:namespace, owner: owner) } + specify { is_expected.to eq(true) } - context 'the project does not have a group' do - let(:project) { build(:project, namespace: namespace) } + it 'becomes false when external issue tracker service is destroyed' do + expect do + Service.find(service.id).delete + end.to change { subject }.to(false) + end - it 'is the namespace owner' do - expect(project.default_owner).to eq(owner) + it 'becomes false when external issue tracker service becomes inactive' do + expect do + service.update_column(:active, false) + end.to change { subject }.to(false) end - end - context 'the project is in a group' do - let(:group) { build(:group) } - let(:project) { build(:project, group: group, namespace: namespace) } + context 'when there are two active external issue tracker services' do + let_it_be(:second_service) do + create(:service, project: project, type: 'CustomIssueTracker', category: 'issue_tracker', active: true) + end - it 'is the group owner' do - allow(group).to receive(:default_owner).and_return(Object.new) + it 'does not become false when external issue tracker service is destroyed' do + expect do + Service.find(service.id).delete + end.not_to change { subject } + end - expect(project.default_owner).to eq(group.default_owner) + it 'does not become false when external issue tracker service becomes inactive' do + expect do + service.update_column(:active, false) + end.not_to change { subject } + end end end end diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 04b3920cd6c3fc..25e947b62224a6 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -753,38 +753,6 @@ end end - describe "callbacks" do - let!(:service) do - RedmineService.new( - project: project, - active: true, - properties: { - project_url: 'http://redmine/projects/project_name_in_redmine', - issues_url: "http://redmine/#{project.id}/project_name_in_redmine/:id", - new_issue_url: 'http://redmine/projects/project_name_in_redmine/issues/new' - } - ) - end - - describe "on create" do - it "updates the has_external_issue_tracker boolean" do - expect do - service.save! - end.to change { service.project.has_external_issue_tracker }.from(false).to(true) - end - end - - describe "on update" do - it "updates the has_external_issue_tracker boolean" do - service.save! - - expect do - service.update(active: false) - end.to change { service.project.has_external_issue_tracker }.from(true).to(false) - end - end - end - describe '#api_field_names' do let(:fake_service) do Class.new(Service) do @@ -864,20 +832,6 @@ def fields end end - describe '#external_issue_tracker?' do - where(:category, :active, :result) do - :issue_tracker | true | true - :issue_tracker | false | false - :common | true | false - end - - with_them do - it 'returns the right result' do - expect(build(:service, category: category, active: active).external_issue_tracker?).to eq(result) - end - end - end - describe '#external_wiki?' do where(:type, :active, :result) do 'ExternalWikiService' | true | true diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 674382ee14f00e..3ac993972c6f34 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -43,46 +43,6 @@ end end - shared_examples 'updates project callbacks' do - it 'updates projects#has_external_issue_tracker for issue tracker services' do - described_class.new(integration, batch, association).execute - - expect(project.reload.has_external_issue_tracker).to eq(true) - expect(excluded_project.reload.has_external_issue_tracker).to eq(false) - end - - context 'with an external wiki integration' do - before do - integration.update!(category: 'common', type: 'ExternalWikiService') - end - - it 'updates projects#has_external_wiki for external wiki services' do - described_class.new(integration, batch, association).execute - - expect(project.reload.has_external_wiki).to eq(true) - expect(excluded_project.reload.has_external_wiki).to eq(false) - end - end - end - - shared_examples 'does not update project callbacks' do - it 'does not update projects#has_external_issue_tracker for issue tracker services' do - described_class.new(integration, batch, association).execute - - expect(project.reload.has_external_issue_tracker).to eq(false) - end - - context 'with an inactive external wiki integration' do - let(:integration) { create(:external_wiki_service, :instance, active: false) } - - it 'does not update projects#has_external_wiki for external wiki services' do - described_class.new(integration, batch, association).execute - - expect(project.reload.has_external_wiki).to eq(false) - end - end - end - context 'passing an instance-level integration' do let(:integration) { instance_integration } let(:inherit_from_id) { integration.id } @@ -95,15 +55,6 @@ it_behaves_like 'creates integration from batch ids' it_behaves_like 'updates inherit_from_id' - it_behaves_like 'updates project callbacks' - - context 'when integration is not active' do - before do - integration.update!(active: false) - end - - it_behaves_like 'does not update project callbacks' - end end context 'with a group association' do @@ -130,7 +81,6 @@ it_behaves_like 'creates integration from batch ids' it_behaves_like 'updates inherit_from_id' - it_behaves_like 'updates project callbacks' end context 'with a group association' do @@ -157,7 +107,6 @@ let(:inherit_from_id) { integration.id } it_behaves_like 'creates integration from batch ids' - it_behaves_like 'updates project callbacks' end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index dc545f57d2364a..3cf45143594667 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -84,6 +84,7 @@ let!(:external_issue_tracker) { create(:jira_service, project: project) } it 'closes the issue on the external issue tracker' do + project.reload expect(project.external_issue_tracker).to receive(:close_issue) described_class.new(project, user).close_issue(external_issue) @@ -94,6 +95,7 @@ let!(:external_issue_tracker) { create(:jira_service, project: project, active: false) } it 'does not close the issue on the external issue tracker' do + project.reload expect(project.external_issue_tracker).not_to receive(:close_issue) described_class.new(project, user).close_issue(external_issue) @@ -104,6 +106,7 @@ let!(:external_issue_tracker) { create(:bugzilla_service, project: project) } it 'does not close the issue on the external issue tracker' do + project.reload expect(project.external_issue_tracker).not_to receive(:close_issue) described_class.new(project, user).close_issue(external_issue) diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index f83b8d98ce8624..22b3456708f886 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -252,6 +252,7 @@ def stub_compare issue.update!(iid: 123) else create(:"#{issue_tracker}_service", project: project) + project.reload end end @@ -351,6 +352,7 @@ def stub_compare issue.update!(iid: 123) else create(:"#{issue_tracker}_service", project: project) + project.reload end end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index 96afca2f2cbe62..ae18bc23c17ca7 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -729,12 +729,14 @@ def build_note(old_reviewers, new_reviewers) it 'is false with issue tracker supporting referencing' do create(:jira_service, project: project) + project.reload expect(service.cross_reference_disallowed?(noteable)).to be_falsey end it 'is true with issue tracker not supporting referencing' do create(:bugzilla_service, project: project) + project.reload expect(service.cross_reference_disallowed?(noteable)).to be_truthy end -- GitLab From 36d885432c2881abe3985d44d00ca8b8eb1aa25b Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 20 Jan 2021 16:09:46 +1300 Subject: [PATCH 2/3] Add backend reviewer feedback --- ...210117210226_add_has_external_issue_tracker_trigger.rb | 8 ++++---- .../add_has_external_issue_tracker_trigger_spec.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb b/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb index 7f29b0fd86c972..d12d377fe32b73 100644 --- a/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb +++ b/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb @@ -4,10 +4,10 @@ class AddHasExternalIssueTrackerTrigger < ActiveRecord::Migration[6.0] include Gitlab::Database::SchemaHelpers DOWNTIME = false - FUNCTION_NAME = 'set_has_external_issue_tracker'.freeze - TRIGGER_ON_INSERT_NAME = 'trigger_has_external_issue_tracker_on_insert'.freeze - TRIGGER_ON_UPDATE_NAME = 'trigger_has_external_issue_tracker_on_update'.freeze - TRIGGER_ON_DELETE_NAME = 'trigger_has_external_issue_tracker_on_delete'.freeze + FUNCTION_NAME = 'set_has_external_issue_tracker' + TRIGGER_ON_INSERT_NAME = 'trigger_has_external_issue_tracker_on_insert' + TRIGGER_ON_UPDATE_NAME = 'trigger_has_external_issue_tracker_on_update' + TRIGGER_ON_DELETE_NAME = 'trigger_has_external_issue_tracker_on_delete' def up create_trigger_function(FUNCTION_NAME, replace: true) do diff --git a/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb b/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb index cbd1e36c2c2c34..b26f0fcede68dc 100644 --- a/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb +++ b/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb @@ -41,7 +41,7 @@ end.not_to change { @project.reload.has_external_issue_tracker } end - it 'does not set `has_external_issue_tracker` to true when active other service is inserted' do + it 'does not set `has_external_issue_tracker` to true when a non-`issue tracker` active service is inserted' do expect do services.create!(category: 'my_type', active: true, project_id: @project.id) end.not_to change { @project.reload.has_external_issue_tracker } -- GitLab From 39ab90ca2abd58624bdcbf8a0f9524736fa70780 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 20 Jan 2021 16:10:01 +1300 Subject: [PATCH 3/3] Add database reviewer feedback --- ..._add_has_external_issue_tracker_trigger.rb | 2 +- ...has_external_issue_tracker_trigger_spec.rb | 22 +++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb b/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb index d12d377fe32b73..20fe0ee0fd1b8e 100644 --- a/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb +++ b/db/migrate/20210117210226_add_has_external_issue_tracker_trigger.rb @@ -20,8 +20,8 @@ def up WHERE project_id = COALESCE(NEW.project_id, OLD.project_id) AND active = TRUE AND category = 'issue_tracker' - ) ) + ) WHERE projects.id = COALESCE(NEW.project_id, OLD.project_id); RETURN NULL; SQL diff --git a/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb b/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb index b26f0fcede68dc..72983c7cfbfe6d 100644 --- a/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb +++ b/spec/migrations/add_has_external_issue_tracker_trigger_spec.rb @@ -65,7 +65,16 @@ end.to change { @project.reload.has_external_issue_tracker }.to(false) end - it 'does not change `has_external_issue_tracker` when `issue_tracker` is made inactive, if an active `issue_tracker` still exists' do + it 'sets `has_external_issue_tracker` to false when `issue_tracker` is made inactive, and an inactive `issue_tracker` exists' do + services.create!(category: 'issue_tracker', active: false, project_id: @project.id) + service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + + expect do + service.update!(active: false) + end.to change { @project.reload.has_external_issue_tracker }.to(false) + end + + it 'does not change `has_external_issue_tracker` when `issue_tracker` is made inactive, if an active `issue_tracker` exists' do services.create!(category: 'issue_tracker', active: true, project_id: @project.id) service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) @@ -93,7 +102,16 @@ end.to change { @project.reload.has_external_issue_tracker }.to(false) end - it 'does not set `has_external_issue_tracker` to false when `issue_tracker` is deleted, if an active `issue_tracker` still exists' do + it 'sets `has_external_issue_tracker` to false when `issue_tracker` is deleted, if an inactive `issue_tracker` still exists' do + services.create!(category: 'issue_tracker', active: false, project_id: @project.id) + service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) + + expect do + service.delete + end.to change { @project.reload.has_external_issue_tracker }.to(false) + end + + it 'does not change `has_external_issue_tracker` when `issue_tracker` is deleted, if an active `issue_tracker` still exists' do services.create!(category: 'issue_tracker', active: true, project_id: @project.id) service = services.create!(category: 'issue_tracker', active: true, project_id: @project.id) -- GitLab