diff --git a/app/controllers/admin/topics_controller.rb b/app/controllers/admin/topics_controller.rb index b451928e5919a5709a14cb5df796a099ff6ba054..69bcfdf4791f96a5ba46bdabf5eed97651cdcedc 100644 --- a/app/controllers/admin/topics_controller.rb +++ b/app/controllers/admin/topics_controller.rb @@ -45,6 +45,22 @@ def destroy notice: _('Topic %{topic_name} was successfully removed.') % { topic_name: @topic.title_or_name } end + def merge + source_topic = Projects::Topic.find(merge_params[:source_topic_id]) + target_topic = Projects::Topic.find(merge_params[:target_topic_id]) + + begin + ::Topics::MergeService.new(source_topic, target_topic).execute + rescue ArgumentError => e + return render status: :bad_request, json: { type: :alert, message: e.message } + end + + message = _('Topic %{source_topic} was successfully merged into topic %{target_topic}.') + redirect_to admin_topics_path, + status: :found, + notice: message % { source_topic: source_topic.name, target_topic: target_topic.name } + end + private def topic @@ -63,4 +79,8 @@ def allowed_topic_params :title ] end + + def merge_params + params.permit([:source_topic_id, :target_topic_id]) + end end diff --git a/app/services/topics/merge_service.rb b/app/services/topics/merge_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..0d256579fe04d7e2396c890e74cf155eed90b857 --- /dev/null +++ b/app/services/topics/merge_service.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +module Topics + class MergeService + attr_accessor :source_topic, :target_topic + + def initialize(source_topic, target_topic) + @source_topic = source_topic + @target_topic = target_topic + end + + def execute + validate_parameters! + + ::Projects::ProjectTopic.transaction do + move_project_topics + refresh_target_topic_counters + delete_source_topic + end + end + + private + + def validate_parameters! + raise ArgumentError, 'The source topic is not a topic.' unless source_topic.is_a?(Projects::Topic) + raise ArgumentError, 'The target topic is not a topic.' unless target_topic.is_a?(Projects::Topic) + raise ArgumentError, 'The source topic and the target topic are identical.' if source_topic == target_topic + end + + # rubocop: disable CodeReuse/ActiveRecord + def move_project_topics + project_ids_for_projects_currently_using_source_and_target = ::Projects::ProjectTopic + .where(topic_id: target_topic).select(:project_id) + # Only update for projects that exclusively use the source topic + ::Projects::ProjectTopic.where(topic_id: source_topic.id) + .where.not(project_id: project_ids_for_projects_currently_using_source_and_target) + .update_all(topic_id: target_topic.id) + + # Delete source topic for projects that were using source and target + ::Projects::ProjectTopic.where(topic_id: source_topic.id).delete_all + end + + def refresh_target_topic_counters + target_topic.update!( + total_projects_count: total_projects_count(target_topic.id), + non_private_projects_count: non_private_projects_count(target_topic.id) + ) + end + + def delete_source_topic + source_topic.destroy! + end + + def total_projects_count(topic_id) + ::Projects::ProjectTopic.where(topic_id: topic_id).count + end + + def non_private_projects_count(topic_id) + ::Projects::ProjectTopic.joins(:project).where(topic_id: topic_id).where('projects.visibility_level in (10, 20)') + .count + end + # rubocop: enable CodeReuse/ActiveRecord + end +end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index bbf00cd0b002bc0198f287fc15a9998357a605d2..2f4e286f5eb24f5f4d7701e4132c18b8f380e9b2 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -65,6 +65,7 @@ resource :avatar, controller: 'topics/avatars', only: [:destroy] collection do post :preview_markdown + post :merge end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 2c2ce654412c36305359f749dc4174f4f536c7dc..555eb473c1dfd3025d105ee46c7ac1c218a848ef 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -40998,6 +40998,9 @@ msgstr "" msgid "TopNav|Go back" msgstr "" +msgid "Topic %{source_topic} was successfully merged into topic %{target_topic}." +msgstr "" + msgid "Topic %{topic_name} was successfully created." msgstr "" diff --git a/spec/controllers/admin/topics_controller_spec.rb b/spec/controllers/admin/topics_controller_spec.rb index ee36d5f1def6694b12faf06b58cf114a314c8a23..87093e0263bbbfafe870f93d9884e3b38e32e6e1 100644 --- a/spec/controllers/admin/topics_controller_spec.rb +++ b/spec/controllers/admin/topics_controller_spec.rb @@ -173,4 +173,44 @@ end end end + + describe 'POST #merge' do + let_it_be(:source_topic) { create(:topic, name: 'source_topic') } + let_it_be(:project) { create(:project, topic_list: source_topic.name ) } + + it 'merges source topic into target topic' do + post :merge, params: { source_topic_id: source_topic.id, target_topic_id: topic.id } + + expect(response).to redirect_to(admin_topics_path) + expect(topic.projects).to contain_exactly(project) + expect { source_topic.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'renders a 404 error for non-existing id' do + post :merge, params: { source_topic_id: non_existing_record_id, target_topic_id: topic.id } + + expect(response).to have_gitlab_http_status(:not_found) + expect { topic.reload }.not_to raise_error + end + + it 'renders a 400 error for identical topic ids' do + post :merge, params: { source_topic_id: topic, target_topic_id: topic.id } + + expect(response).to have_gitlab_http_status(:bad_request) + expect { topic.reload }.not_to raise_error + end + + context 'as a normal user' do + before do + sign_in(user) + end + + it 'renders a 404 error' do + post :merge, params: { source_topic_id: source_topic.id, target_topic_id: topic.id } + + expect(response).to have_gitlab_http_status(:not_found) + expect { source_topic.reload }.not_to raise_error + end + end + end end diff --git a/spec/services/topics/merge_service_spec.rb b/spec/services/topics/merge_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..971917eb8e9bd4a5ab2bb543eaf519196718697b --- /dev/null +++ b/spec/services/topics/merge_service_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Topics::MergeService do + let_it_be(:source_topic) { create(:topic, name: 'source_topic') } + let_it_be(:target_topic) { create(:topic, name: 'target_topic') } + let_it_be(:project_1) { create(:project, :public, topic_list: source_topic.name ) } + let_it_be(:project_2) { create(:project, :private, topic_list: source_topic.name ) } + let_it_be(:project_3) { create(:project, :public, topic_list: target_topic.name ) } + let_it_be(:project_4) { create(:project, :public, topic_list: [source_topic.name, target_topic.name] ) } + + subject { described_class.new(source_topic, target_topic).execute } + + describe '#execute' do + it 'merges source topic into target topic' do + subject + + expect(target_topic.projects).to contain_exactly(project_1, project_2, project_3, project_4) + expect { source_topic.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'refreshes counters of target topic' do + expect { subject } + .to change { target_topic.reload.total_projects_count }.by(2) + .and change { target_topic.reload.non_private_projects_count }.by(1) + end + + context 'when source topic fails to delete' do + it 'reverts previous changes' do + allow(source_topic.reload).to receive(:destroy!).and_raise(ActiveRecord::RecordNotDestroyed) + + expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) + + expect(source_topic.projects).to contain_exactly(project_1, project_2, project_4) + expect(target_topic.projects).to contain_exactly(project_3, project_4) + end + end + + context 'for parameter validation' do + using RSpec::Parameterized::TableSyntax + + subject { described_class.new(source_topic_parameter, target_topic_parameter).execute } + + where(:source_topic_parameter, :target_topic_parameter, :expected_message) do + nil | ref(:target_topic) | 'The source topic is not a topic.' + ref(:source_topic) | nil | 'The target topic is not a topic.' + ref(:target_topic) | ref(:target_topic) | 'The source topic and the target topic are identical.' # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands + end + + with_them do + it 'raises correct error' do + expect { subject }.to raise_error(ArgumentError) do |error| + expect(error.message).to eq(expected_message) + end + end + end + end + end +end