Abillity to promote project labels to group labels

Fixes #24021

app/controllers/projects/labels_controller.rb

@@ -2,12 +2,13 @@ class Projects::LabelsController < Projects::ApplicationController
   include ToggleSubscriptionAction
 
   before_action :module_enabled
-  before_action :label, only: [:edit, :update, :destroy]
+  before_action :label, only: [:edit, :update, :destroy, :promote]
   before_action :find_labels, only: [:index, :set_priorities, :remove_priority, :toggle_subscription]
   before_action :authorize_read_label!
   before_action :authorize_admin_labels!, only: [:new, :create, :edit, :update,
                                                  :generate, :destroy, :remove_priority,
                                                  :set_priorities]
+  before_action :authorize_admin_group!, only: [:promote]
 
   respond_to :js, :html
 
@@ -108,6 +109,32 @@ class Projects::LabelsController < Projects::ApplicationController
     end
   end
 
+  def promote
+    promote_service = Labels::PromoteService.new(@project, @current_user)
+
+    begin
+      return render_404 unless promote_service.execute(@label)
+      respond_to do |format|
+        format.html do
+          redirect_to(namespace_project_labels_path(@project.namespace, @project),
+                      notice: 'Label was promoted to a Group Label')
+        end
+        format.js
+      end
+    rescue ActiveRecord::RecordInvalid => e
+      Gitlab::AppLogger.error "Failed to promote label \"#{@label.title}\" to group label"
+      Gitlab::AppLogger.error e
+
+      respond_to do |format|
+        format.html do
+          redirect_to(namespace_project_labels_path(@project.namespace, @project),
+                      notice: 'Failed to promote label due to internal error. Please contact administrators.')
+        end
+        format.js
+      end
+    end
+  end
+
   protected
 
   def module_enabled
@@ -135,4 +162,8 @@ class Projects::LabelsController < Projects::ApplicationController
   def authorize_admin_labels!
     return render_404 unless can?(current_user, :admin_label, @project)
   end
+
+  def authorize_admin_group!
+    return render_404 unless can?(current_user, :admin_group, @project.group)
+  end
 end

app/services/labels/promote_service.rb

@@ -0,0 +1,71 @@
+module Labels
+  class PromoteService < BaseService
+    BATCH_SIZE = 1000
+
+    def execute(label)
+      return unless project.group &&
+          label.is_a?(ProjectLabel)
+
+      Label.transaction do
+        new_label = clone_label_to_group_label(label)
+
+        label_ids_for_merge(new_label).find_in_batches(batch_size: BATCH_SIZE) do |batched_ids|
+          update_issuables(new_label, batched_ids)
+          update_issue_board_lists(new_label, batched_ids)
+          update_priorities(new_label, batched_ids)
+          # Order is important, project labels need to be last
+          update_project_labels(batched_ids)
+        end
+
+        # We skipped validations during creation. Let's run them now, after deleting conflicting labels
+        raise ActiveRecord::RecordInvalid.new(new_label) unless new_label.valid?
+        new_label
+      end
+    end
+
+    private
+
+    def label_ids_for_merge(new_label)
+      LabelsFinder.
+        new(current_user, title: new_label.title, group_id: project.group.id).
+        execute(skip_authorization: true).
+        where.not(id: new_label).
+        select(:id)  # Can't use pluck() to avoid object-creation because of the batching
+    end
+
+    def update_issuables(new_label, label_ids)
+      LabelLink.
+        where(label: label_ids).
+        update_all(label_id: new_label)
+    end
+
+    def update_issue_board_lists(new_label, label_ids)
+      List.
+        where(label: label_ids).
+        update_all(label_id: new_label)
+    end
+
+    def update_priorities(new_label, label_ids)
+      LabelPriority.
+        where(label: label_ids).
+        update_all(label_id: new_label)
+    end
+
+    def update_project_labels(label_ids)
+      Label.where(id: label_ids).delete_all
+    end
+
+    def clone_label_to_group_label(label)
+      params = label.attributes.slice('title', 'description', 'color')
+      # Since the title of the new label has to be the same as the previous labels
+      # and we're merging old labels in batches we'll skip validation to omit 2-step
+      # merge process and do it in one batch
+      # We'll be forcing validation at the end of the transaction to ensure everything
+      # was merged correctly
+      new_label = GroupLabel.new(params.merge(group: project.group))
+      new_label.save(validate: false)
+
+      new_label
+    end
+  end
+end

app/views/shared/_label.html.haml

@@ -66,6 +66,10 @@
                 %a.js-subscribe-button{ data: { url: toggle_subscription_group_label_path(label.group, label) } }
                   Group level
 
+    - if label.is_a?(ProjectLabel) && label.project.group && can?(current_user, :admin_group, label.project.group)
+      = link_to promote_namespace_project_label_path(label.project.namespace, label.project, label), title: "Promote to Group Label", class: 'btn btn-transparent btn-action', data: {confirm: "Promoting this label will make this label available to all projects inside this group. Existing project labels with the same name will be merged. Are you sure?", toggle: "tooltip"}, method: :post do
+        %span.sr-only Promote to Group
+        = icon('level-up')
     - if can?(current_user, :admin_label, label)
       = link_to edit_label_path(label), title: "Edit", class: 'btn btn-transparent btn-action', data: {toggle: "tooltip"} do
         %span.sr-only Edit

changelogs/unreleased/label-promotion.yml

@@ -0,0 +1,4 @@
+---
+title: "Project labels can now be promoted to group labels"
+merge_request: 7242
+author: Olaf Tomalka

config/routes/project.rb

@@ -220,6 +220,7 @@ constraints(ProjectUrlConstrainer.new) do
         end
 
         member do
+          post :promote
           post :toggle_subscription
           delete :remove_priority
         end

spec/controllers/projects/labels_controller_spec.rb

@@ -112,4 +112,49 @@ describe Projects::LabelsController do
       post :toggle_subscription, namespace_id: project.namespace.to_param, project_id: project.to_param, id: label.to_param
     end
   end
+
+  describe 'POST #promote' do
+    let!(:promoted_label_name) { "Promoted Label" }
+    let!(:label_1) { create(:label, title: promoted_label_name, project: project) }
+
+    context 'not group owner' do
+      it 'denies access' do
+        post :promote, namespace_id: project.namespace.to_param, project_id: project.to_param, id: label_1.to_param
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context 'group owner' do
+      before do
+        GroupMember.add_users_to_group(group, [user], :owner)
+      end
+
+      it 'gives access' do
+        post :promote, namespace_id: project.namespace.to_param, project_id: project.to_param, id: label_1.to_param
+
+        expect(response).to redirect_to(namespace_project_labels_path)
+      end
+
+      it 'promotes the label' do
+        post :promote, namespace_id: project.namespace.to_param, project_id: project.to_param, id: label_1.to_param
+
+        expect(Label.where(id: label_1.id)).to be_empty
+        expect(GroupLabel.find_by(title: promoted_label_name)).not_to be_nil
+      end
+
+      context 'service raising InvalidRecord' do
+        before do
+          expect_any_instance_of(Labels::PromoteService).to receive(:execute) do |label|
+            raise ActiveRecord::RecordInvalid.new(label_1)
+          end
+        end
+
+        it 'returns to label list' do
+          post :promote, namespace_id: project.namespace.to_param, project_id: project.to_param, id: label_1.to_param
+          expect(response).to redirect_to(namespace_project_labels_path)
+        end
+      end
+    end
+  end
 end

spec/services/labels/promote_service_spec.rb

@@ -0,0 +1,187 @@
+require 'spec_helper'
+
+describe Labels::PromoteService, services: true do
+  describe '#execute' do
+    let!(:user) { create(:user) }
+
+    context 'project without group' do
+      let!(:project_1)  { create(:empty_project) }
+
+      let!(:project_label_1_1)  { create(:label, project: project_1) }
+
+      subject(:service) { described_class.new(project_1, user) }
+
+      it 'fails on project without group' do
+        expect(service.execute(project_label_1_1)).to be_falsey
+      end
+    end
+
+    context 'project with group' do
+      let!(:promoted_label_name)  { "Promoted Label" }
+      let!(:untouched_label_name) { "Untouched Label" }
+      let!(:promoted_description) { "Promoted Description" }
+      let!(:promoted_color)       { "#0000FF" }
+      let!(:label_2_1_priority)   { 1 }
+      let!(:label_3_1_priority)   { 2 }
+
+      let!(:group_1)  { create(:group) }
+      let!(:group_2)  { create(:group) }
+
+      let!(:project_1)  { create(:empty_project, namespace: group_1) }
+      let!(:project_2)  { create(:empty_project, namespace: group_1) }
+      let!(:project_3)  { create(:empty_project, namespace: group_1) }
+      let!(:project_4)  { create(:empty_project, namespace: group_2) }
+
+      # Labels/issues can't be lazily created so we might as well eager initialize
+      # all other objects too since we use them inside
+      let!(:project_label_1_1)  { create(:label, project: project_1, name: promoted_label_name, color: promoted_color, description: promoted_description) }
+      let!(:project_label_1_2)  { create(:label, project: project_1, name: untouched_label_name) }
+      let!(:project_label_2_1)  { create(:label, project: project_2, priority: label_2_1_priority, name: promoted_label_name, color: "#FF0000") }
+      let!(:project_label_3_1)  { create(:label, project: project_3, priority: label_3_1_priority, name: promoted_label_name) }
+      let!(:project_label_3_2)  { create(:label, project: project_3, priority: 1, name: untouched_label_name) }
+      let!(:project_label_4_1)  { create(:label, project: project_4, name: promoted_label_name) }
+
+      let!(:issue_1_1)  { create(:labeled_issue, project: project_1, labels: [project_label_1_1, project_label_1_2]) }
+      let!(:issue_1_2)  { create(:labeled_issue, project: project_1, labels: [project_label_1_2]) }
+      let!(:issue_2_1)  { create(:labeled_issue, project: project_2, labels: [project_label_2_1]) }
+      let!(:issue_4_1)  { create(:labeled_issue, project: project_4, labels: [project_label_4_1]) }
+
+      let!(:merge_3_1)  { create(:labeled_merge_request, source_project: project_3, target_project: project_3, labels: [project_label_3_1, project_label_3_2]) }
+
+      let!(:issue_board_2_1)      { create(:board, project: project_2) }
+      let!(:issue_board_list_2_1) { create(:list, board: issue_board_2_1, label: project_label_2_1) }
+
+      let(:new_label) { group_1.labels.find_by(title: promoted_label_name) }
+
+      subject(:service) { described_class.new(project_1, user) }
+
+      it 'fails on group label' do
+        group_label = create(:group_label, group: group_1)
+
+        expect(service.execute(group_label)).to be_falsey
+      end
+
+      it 'is truthy on success' do
+        expect(service.execute(project_label_1_1)).to be_truthy
+      end
+
+      it 'recreates the label as a group label' do
+        expect { service.execute(project_label_1_1) }.
+          to change(project_1.labels, :count).by(-1).
+          and change(group_1.labels, :count).by(1)
+        expect(new_label).not_to be_nil
+      end
+
+      it 'copies title, description and color' do
+        service.execute(project_label_1_1)
+
+        expect(new_label.title).to eq(promoted_label_name)
+        expect(new_label.description).to eq(promoted_description)
+        expect(new_label.color).to eq(promoted_color)
+      end
+
+      it 'merges labels with the same name in group' do
+        expect { service.execute(project_label_1_1) }.to change(project_2.labels, :count).by(-1).and \
+          change(project_3.labels, :count).by(-1)
+      end
+
+      it 'recreates priorities' do
+        service.execute(project_label_1_1)
+
+        expect(new_label.priority(project_1)).to be_nil
+        expect(new_label.priority(project_2)).to eq(label_2_1_priority)
+        expect(new_label.priority(project_3)).to eq(label_3_1_priority)
+      end
+
+      it 'does not touch project out of promoted group' do
+        service.execute(project_label_1_1)
+        project_4_new_label = project_4.labels.find_by(title: promoted_label_name)
+
+        expect(project_4_new_label).not_to be_nil
+        expect(project_4_new_label.id).to eq(project_label_4_1.id)
+      end
+
+      it 'does not touch out of group priority' do
+        service.execute(project_label_1_1)
+
+        expect(new_label.priority(project_4)).to be_nil
+      end
+
+      it 'relinks issue with the promoted label' do
+        service.execute(project_label_1_1)
+        issue_label = issue_1_1.labels.find_by(title: promoted_label_name)
+
+        expect(issue_label).not_to be_nil
+        expect(issue_label.id).to eq(new_label.id)
+      end
+
+      it 'does not remove untouched labels from issue' do
+        expect { service.execute(project_label_1_1) }.not_to change(issue_1_1.labels, :count)
+      end
+
+      it 'does not relink untouched label in issue' do
+        service.execute(project_label_1_1)
+        issue_label = issue_1_2.labels.find_by(title: untouched_label_name)
+
+        expect(issue_label).not_to be_nil
+        expect(issue_label.id).to eq(project_label_1_2.id)
+      end
+
+      it 'relinks issues with merged labels' do
+        service.execute(project_label_1_1)
+        issue_label = issue_2_1.labels.find_by(title: promoted_label_name)
+
+        expect(issue_label).not_to be_nil
+        expect(issue_label.id).to eq(new_label.id)
+      end
+
+      it 'does not relink issues from other group' do
+        service.execute(project_label_1_1)
+        issue_label = issue_4_1.labels.find_by(title: promoted_label_name)
+
+        expect(issue_label).not_to be_nil
+        expect(issue_label.id).to eq(project_label_4_1.id)
+      end
+
+      it 'updates merge request' do
+        service.execute(project_label_1_1)
+        merge_label = merge_3_1.labels.find_by(title: promoted_label_name)
+
+        expect(merge_label).not_to be_nil
+        expect(merge_label.id).to eq(new_label.id)
+      end
+
+      it 'updates board lists' do
+        service.execute(project_label_1_1)
+        list = issue_board_2_1.lists.find_by(label: new_label)
+
+        expect(list).not_to be_nil
+      end
+
+      # In case someone adds a new relation to Label.rb and forgets to relink it
+      # and the database doesn't have foreign key constraints
+      it 'relinks all relations' do
+        service.execute(project_label_1_1)
+
+        Label.reflect_on_all_associations.each do |association|
+          expect(project_label_1_1.send(association.name).any?).to be_falsey
+        end
+      end
+
+      context 'with invalid group label' do
+        before do
+          allow(service).to receive(:clone_label_to_group_label).and_wrap_original do |m, *args|
+            label = m.call(*args)
+            allow(label).to receive(:valid?).and_return(false)
+
+            label
+          end
+        end
+
+        it 'raises an exception' do
+          expect { service.execute(project_label_1_1) }.to raise_error(ActiveRecord::RecordInvalid)
+        end
+      end
+    end
+  end
+end