From 81c488266125280ed8bbfcc0c686425adf42056c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Tue, 16 Aug 2022 16:36:50 +0200 Subject: [PATCH 1/4] Allow admins to merge topics [API] Changelog: added --- doc/api/topics.md | 42 ++++++++++++++++++++++- lib/api/topics.rb | 25 ++++++++++++++ spec/requests/api/topics_spec.rb | 59 ++++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 1 deletion(-) diff --git a/doc/api/topics.md b/doc/api/topics.md index ee88a43ff1c412..38d99244bb6ef0 100644 --- a/doc/api/topics.md +++ b/doc/api/topics.md @@ -217,7 +217,7 @@ curl --request PUT \ > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80725) in GitLab 14.9. -You must be an administrator to delete a project. +You must be an administrator to delete a project topic. When you delete a project topic, you also delete the topic assignment for projects. ```plaintext @@ -237,3 +237,43 @@ curl --request DELETE \ --header "PRIVATE-TOKEN: " \ "https://gitlab.example.com/api/v4/topics/1" ``` + +## Merge topics + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/95501) in GitLab 15.4. + +You must be an administrator to merge a source topic into a target topic. +When you merge topics, you delete the source topic and move all assigned projects to the target topic. + +```plaintext +POST /topics/merge +``` + +Supported attributes: + +| Attribute | Type | Required | Description | +| ----------------- | ------- | ---------------------- | -------------------------- | +| `source_topic_id` | integer | **{check-circle}** Yes | ID of source project topic | +| `target_topic_id` | integer | **{check-circle}** Yes | ID of target project topic | + +Example request: + +```shell +curl --request POST \ + --data "source_topic_id=2&target_topic_id=1" \ + --header "PRIVATE-TOKEN: " \ + "https://gitlab.example.com/api/v4/topics/merge" +``` + +Example response: + +```json +{ + "id": 1, + "name": "topic1", + "title": "Topic 1", + "description": null, + "total_projects_count": 0, + "avatar_url": null +} +``` diff --git a/lib/api/topics.rb b/lib/api/topics.rb index a08b4c6c1070e7..c4d795a23572e1 100644 --- a/lib/api/topics.rb +++ b/lib/api/topics.rb @@ -94,5 +94,30 @@ class Topics < ::API::Base destroy_conditionally!(topic) end + + desc 'Merge topics' do + detail 'This feature was introduced in GitLab 15.4.' + success Entities::Projects::Topic + end + params do + requires :source_topic_id, type: Integer, desc: 'ID of source project topic' + requires :target_topic_id, type: Integer, desc: 'ID of target project topic' + end + post 'topics/merge' do + authenticated_as_admin! + + source_topic = ::Projects::Topic.find(params[:source_topic_id]) + target_topic = ::Projects::Topic.find(params[:target_topic_id]) + + begin + ::Topics::MergeService.new(source_topic, target_topic).execute + rescue ArgumentError => e + render_api_error!(e.message, 400) + rescue StandardError + render_api_error!('Topics could not be merged!', 400) + end + + present target_topic, with: Entities::Projects::Topic + end end end diff --git a/spec/requests/api/topics_spec.rb b/spec/requests/api/topics_spec.rb index 72221e3fb6a815..9cbf6cfa3e303a 100644 --- a/spec/requests/api/topics_spec.rb +++ b/spec/requests/api/topics_spec.rb @@ -317,4 +317,63 @@ end end end + + describe 'POST /topics/merge', :aggregate_failures do + context 'as administrator' do + let_it_be(:api_url) { api('/topics/merge', admin) } + + it 'merge topics' do + post api_url, params: { source_topic_id: topic_3.id, target_topic_id: topic_2.id } + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['id']).to eq(topic_2.id) + expect { topic_2.reload }.not_to raise_error + expect { topic_3.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'returns 404 for non existing source topic id' do + post api_url, params: { source_topic_id: non_existing_record_id, target_topic_id: topic_2.id } + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns 404 for non existing target topic id' do + post api_url, params: { source_topic_id: topic_3.id, target_topic_id: non_existing_record_id } + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns 400 for identical topic ids' do + post api_url, params: { source_topic_id: topic_2.id, target_topic_id: topic_2.id } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eql('The source topic and the target topic are identical.') + end + + it 'returns 400 if merge failed' do + allow_any_instance_of(Projects::Topic).to receive(:destroy!).and_raise(ActiveRecord::RecordNotDestroyed) # rubocop:disable RSpec/AnyInstanceOf + + post api_url, params: { source_topic_id: topic_3.id, target_topic_id: topic_2.id } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eql('Topics could not be merged!') + end + end + + context 'as normal user' do + it 'returns 403 Forbidden' do + post api('/topics/merge', user), params: { source_topic_id: topic_3.id, target_topic_id: topic_2.id } + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context 'as anonymous' do + it 'returns 401 Unauthorized' do + post api('/topics/merge'), params: { source_topic_id: topic_3.id, target_topic_id: topic_2.id } + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end end -- GitLab From 1fce46f6ba2a081f8109b1ad64c9cf3edef886e0 Mon Sep 17 00:00:00 2001 From: Roy Zwambag Date: Wed, 24 Aug 2022 09:38:40 +0000 Subject: [PATCH 2/4] Use allow_next_found_instance_of for spec --- spec/requests/api/topics_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/requests/api/topics_spec.rb b/spec/requests/api/topics_spec.rb index 9cbf6cfa3e303a..dc1ec3e852e403 100644 --- a/spec/requests/api/topics_spec.rb +++ b/spec/requests/api/topics_spec.rb @@ -351,7 +351,9 @@ end it 'returns 400 if merge failed' do - allow_any_instance_of(Projects::Topic).to receive(:destroy!).and_raise(ActiveRecord::RecordNotDestroyed) # rubocop:disable RSpec/AnyInstanceOf + allow_next_found_instance_of(Projects::Topic) do |topic| + allow(topic).to receive(:destroy!).and_raise(ActiveRecord::RecordNotDestroyed) + end post api_url, params: { source_topic_id: topic_3.id, target_topic_id: topic_2.id } -- GitLab From 32c42f53f447cc0c8143d2c50d4933767c3e8799 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Fri, 2 Sep 2022 11:00:55 +0200 Subject: [PATCH 3/4] Use ServiceResponse for Topics::MergeService --- app/controllers/admin/topics_controller.rb | 7 ++----- app/services/topics/merge_service.rb | 6 ++++++ lib/api/topics.rb | 9 ++------- spec/controllers/admin/topics_controller_spec.rb | 2 +- spec/requests/api/topics_spec.rb | 3 ++- spec/services/topics/merge_service_spec.rb | 10 ++++++---- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/app/controllers/admin/topics_controller.rb b/app/controllers/admin/topics_controller.rb index c3b1c6793adb65..e97ead12f71796 100644 --- a/app/controllers/admin/topics_controller.rb +++ b/app/controllers/admin/topics_controller.rb @@ -49,11 +49,8 @@ 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 + response = ::Topics::MergeService.new(source_topic, target_topic).execute + return render status: :bad_request, json: { type: :alert, message: response.message } if response.error? message = _('Topic %{source_topic} was successfully merged into topic %{target_topic}.') flash[:toast] = message % { source_topic: source_topic.name, target_topic: target_topic.name } diff --git a/app/services/topics/merge_service.rb b/app/services/topics/merge_service.rb index 0d256579fe04d7..c0ce59c27ff463 100644 --- a/app/services/topics/merge_service.rb +++ b/app/services/topics/merge_service.rb @@ -17,6 +17,12 @@ def execute refresh_target_topic_counters delete_source_topic end + + ServiceResponse.success + rescue ArgumentError => e + ServiceResponse.error(message: e.message) + rescue StandardError + ServiceResponse.error(message: 'Topics could not be merged!') end private diff --git a/lib/api/topics.rb b/lib/api/topics.rb index c4d795a23572e1..38cfdc44021adc 100644 --- a/lib/api/topics.rb +++ b/lib/api/topics.rb @@ -109,13 +109,8 @@ class Topics < ::API::Base source_topic = ::Projects::Topic.find(params[:source_topic_id]) target_topic = ::Projects::Topic.find(params[:target_topic_id]) - begin - ::Topics::MergeService.new(source_topic, target_topic).execute - rescue ArgumentError => e - render_api_error!(e.message, 400) - rescue StandardError - render_api_error!('Topics could not be merged!', 400) - end + response = ::Topics::MergeService.new(source_topic, target_topic).execute + render_api_error!(response.message, :bad_request) if response.error? present target_topic, with: Entities::Projects::Topic end diff --git a/spec/controllers/admin/topics_controller_spec.rb b/spec/controllers/admin/topics_controller_spec.rb index 87093e0263bbbf..111fdcc3be6c47 100644 --- a/spec/controllers/admin/topics_controller_spec.rb +++ b/spec/controllers/admin/topics_controller_spec.rb @@ -194,7 +194,7 @@ end it 'renders a 400 error for identical topic ids' do - post :merge, params: { source_topic_id: topic, target_topic_id: topic.id } + post :merge, params: { source_topic_id: topic.id, target_topic_id: topic.id } expect(response).to have_gitlab_http_status(:bad_request) expect { topic.reload }.not_to raise_error diff --git a/spec/requests/api/topics_spec.rb b/spec/requests/api/topics_spec.rb index dc1ec3e852e403..1ad6f876fabdfd 100644 --- a/spec/requests/api/topics_spec.rb +++ b/spec/requests/api/topics_spec.rb @@ -326,9 +326,10 @@ post api_url, params: { source_topic_id: topic_3.id, target_topic_id: topic_2.id } expect(response).to have_gitlab_http_status(:created) - expect(json_response['id']).to eq(topic_2.id) expect { topic_2.reload }.not_to raise_error expect { topic_3.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(json_response['id']).to eq(topic_2.id) + expect(json_response['total_projects_count']).to eq(topic_2.total_projects_count) end it 'returns 404 for non existing source topic id' do diff --git a/spec/services/topics/merge_service_spec.rb b/spec/services/topics/merge_service_spec.rb index 971917eb8e9bd4..eef31817aa8d42 100644 --- a/spec/services/topics/merge_service_spec.rb +++ b/spec/services/topics/merge_service_spec.rb @@ -30,7 +30,9 @@ it 'reverts previous changes' do allow(source_topic.reload).to receive(:destroy!).and_raise(ActiveRecord::RecordNotDestroyed) - expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) + response = subject + expect(response).to be_error + expect(response.message).to eq('Topics could not be merged!') expect(source_topic.projects).to contain_exactly(project_1, project_2, project_4) expect(target_topic.projects).to contain_exactly(project_3, project_4) @@ -50,9 +52,9 @@ with_them do it 'raises correct error' do - expect { subject }.to raise_error(ArgumentError) do |error| - expect(error.message).to eq(expected_message) - end + response = subject + expect(response).to be_error + expect(response.message).to eq(expected_message) end end end -- GitLab From a1d6f8f2883e60f473cd35fa1dc3299317c6ccc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Mon, 5 Sep 2022 08:52:57 +0200 Subject: [PATCH 4/4] Track MergeService error and use i18n --- app/services/topics/merge_service.rb | 11 ++++++----- locale/gitlab.pot | 12 ++++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/app/services/topics/merge_service.rb b/app/services/topics/merge_service.rb index c0ce59c27ff463..58f3d5305b48ef 100644 --- a/app/services/topics/merge_service.rb +++ b/app/services/topics/merge_service.rb @@ -21,16 +21,17 @@ def execute ServiceResponse.success rescue ArgumentError => e ServiceResponse.error(message: e.message) - rescue StandardError - ServiceResponse.error(message: 'Topics could not be merged!') + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e, source_topic_id: source_topic.id, target_topic_id: target_topic.id) + ServiceResponse.error(message: _('Topics could not be merged!')) 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 + 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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bcfc87fa07845f..9d33cbedb36c73 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -39553,6 +39553,12 @@ msgstr "" msgid "The source project of this merge request has been removed." msgstr "" +msgid "The source topic and the target topic are identical." +msgstr "" + +msgid "The source topic is not a topic." +msgstr "" + msgid "The specified tab is invalid, please select another" msgstr "" @@ -39565,6 +39571,9 @@ msgstr "" msgid "The tag name can't be changed for an existing release." msgstr "" +msgid "The target topic is not a topic." +msgstr "" + msgid "The time period in seconds that the maximum requests per project limit applies to." msgstr "" @@ -41270,6 +41279,9 @@ msgstr "" msgid "Topics" msgstr "" +msgid "Topics could not be merged!" +msgstr "" + msgid "Total" msgstr "" -- GitLab