Merge branch 'cablett-231472-group-labels' into 'master'

Use existing group label (if there is one) when promoting

See merge request gitlab-org/gitlab!45122

app/services/labels/promote_service.rb

@@ -10,81 +10,79 @@ module Labels
           label.is_a?(ProjectLabel)
 
       Label.transaction do
-        new_label = clone_label_to_group_label(label)
+        # use the existing group label if it exists
+        group_label = find_or_create_group_label(label)
 
-        label_ids_for_merge(new_label).find_in_batches(batch_size: BATCH_SIZE) do |batched_ids|
-          update_old_label_relations(new_label, batched_ids)
+        label_ids_for_merge(group_label).find_in_batches(batch_size: BATCH_SIZE) do |batched_ids|
+          update_old_label_relations(group_label, batched_ids)
           destroy_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
+        group_label
       end
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     private
 
-    def update_old_label_relations(new_label, old_label_ids)
-      update_issuables(new_label, old_label_ids)
-      update_resource_label_events(new_label, old_label_ids)
-      update_issue_board_lists(new_label, old_label_ids)
-      update_priorities(new_label, old_label_ids)
-      subscribe_users(new_label, old_label_ids)
+    def update_old_label_relations(group_label, old_label_ids)
+      update_issuables(group_label, old_label_ids)
+      update_resource_label_events(group_label, old_label_ids)
+      update_issue_board_lists(group_label, old_label_ids)
+      update_priorities(group_label, old_label_ids)
+      subscribe_users(group_label, old_label_ids)
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def subscribe_users(new_label, label_ids)
+    def subscribe_users(group_label, label_ids)
       # users can be subscribed to multiple labels that will be merged into the group one
       # we want to keep only one subscription / user
       ids_to_update = Subscription.where(subscribable_id: label_ids, subscribable_type: 'Label')
         .group(:user_id)
         .pluck('MAX(id)')
-      Subscription.where(id: ids_to_update).update_all(subscribable_id: new_label.id)
+      Subscription.where(id: ids_to_update).update_all(subscribable_id: group_label.id)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def label_ids_for_merge(new_label)
+    def label_ids_for_merge(group_label)
       LabelsFinder
-        .new(current_user, title: new_label.title, group_id: project.group.id)
+        .new(current_user, title: group_label.title, group_id: project.group.id)
         .execute(skip_authorization: true)
-        .where.not(id: new_label)
+        .where.not(id: group_label)
         .select(:id) # Can't use pluck() to avoid object-creation because of the batching
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def update_issuables(new_label, label_ids)
+    def update_issuables(group_label, label_ids)
       LabelLink
         .where(label: label_ids)
-        .update_all(label_id: new_label.id)
+        .update_all(label_id: group_label.id)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def update_resource_label_events(new_label, label_ids)
+    def update_resource_label_events(group_label, label_ids)
       ResourceLabelEvent
         .where(label: label_ids)
-        .update_all(label_id: new_label.id)
+        .update_all(label_id: group_label.id)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def update_issue_board_lists(new_label, label_ids)
+    def update_issue_board_lists(group_label, label_ids)
       List
         .where(label: label_ids)
-        .update_all(label_id: new_label.id)
+        .update_all(label_id: group_label.id)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def update_priorities(new_label, label_ids)
+    def update_priorities(group_label, label_ids)
       LabelPriority
         .where(label: label_ids)
-        .update_all(label_id: new_label.id)
+        .update_all(label_id: group_label.id)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -92,18 +90,12 @@ module Labels
     def destroy_project_labels(label_ids)
       Label.where(id: label_ids).destroy_all # rubocop: disable Cop/DestroyAll
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    def clone_label_to_group_label(label)
+    def find_or_create_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 = GroupLabel.create_with(params).find_or_initialize_by(group_id: project.group.id, title: label.title)
 
+      new_label.save! unless new_label.persisted?
       new_label
     end
   end

changelogs/unreleased/cablett-231472-group-labels.yml

+---
+title: Use existing group label when promoting project label
+merge_request: 45122
+author:
+type: changed

doc/api/labels.md

@@ -249,9 +249,12 @@ An older endpoint `PUT /projects/:id/labels` with `name` or `label_id` in the pa
 
 ## Promote a project label to a group label
 
-> [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/25218) in GitLab 12.3.
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/25218) in GitLab 12.3.
+> - In [GitLab 13.6 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/231472), promoting a
+>   project label keeps that label's ID and changes it into a group label. Previously, promoting a
+>   project label created a new group label with a new ID and deleted the old label.
 
-Promotes a project label to a group label.
+Promotes a project label to a group label. The label keeps its ID.
 
 ```plaintext
 PUT /projects/:id/labels/:label_id/promote

doc/user/project/labels.md

@@ -95,6 +95,8 @@ If you delete a label, it is permanently deleted. All references to the label ar
 
 #### Promote a project label to a group label
 
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/231472) in GitLab 13.6: promoting a project label keeps that label's ID and changes it into a group label. Previously, promoting a project label created a new group label with a new ID and deleted the old label.
+
 If you previously created a project label and now want to make it available for other
 projects within the same group, you can promote it to a group label.
 
@@ -105,6 +107,8 @@ also merged.
 All issues, merge requests, issue board lists, issue board filters, and label subscriptions
 with the old labels are assigned to the new group label.
 
+The new group label has the same ID as the previous project label.
+
 CAUTION: **Caution:**
 Promoting a label is a permanent action, and cannot be reversed.
 

spec/services/labels/promote_service_spec.rb

@@ -63,139 +63,149 @@ RSpec.describe Labels::PromoteService do
         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
+      shared_examples 'promoting a project label to a group label' do
+        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 'removes all project labels with that title within the 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 'copies title, description and color' do
-        service.execute(project_label_1_1)
+        it 'keeps users\' subscriptions' do
+          user2 = create(:user)
+          project_label_1_1.subscriptions.create!(user: user, subscribed: true)
+          project_label_2_1.subscriptions.create!(user: user, subscribed: true)
+          project_label_3_2.subscriptions.create!(user: user, subscribed: true)
+          project_label_2_1.subscriptions.create!(user: user2, subscribed: true)
 
-        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
+          expect { service.execute(project_label_1_1) }.to change { Subscription.count }.from(4).to(3)
 
-      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 'keeps users\' subscriptions' do
-        user2 = create(:user)
-        project_label_1_1.subscriptions.create!(user: user, subscribed: true)
-        project_label_2_1.subscriptions.create!(user: user, subscribed: true)
-        project_label_3_2.subscriptions.create!(user: user, subscribed: true)
-        project_label_2_1.subscriptions.create!(user: user2, subscribed: true)
+          expect(new_label.subscribed?(user)).to be_truthy
+          expect(new_label.subscribed?(user2)).to be_truthy
+        end
 
-        expect { service.execute(project_label_1_1) }.to change { Subscription.count }.from(4).to(3)
+        it 'recreates priorities' do
+          service.execute(project_label_1_1)
 
-        expect(new_label.subscribed?(user)).to be_truthy
-        expect(new_label.subscribed?(user2)).to be_truthy
-      end
+          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 'recreates priorities' do
-        service.execute(project_label_1_1)
+        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(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
+          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 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)
+        it 'does not touch out of group priority' do
+          service.execute(project_label_1_1)
 
-        expect(project_4_new_label).not_to be_nil
-        expect(project_4_new_label.id).to eq(project_label_4_1.id)
-      end
+          expect(new_label.priority(project_4)).to be_nil
+        end
 
-      it 'does not touch out of group priority' do
-        service.execute(project_label_1_1)
+        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(new_label.priority(project_4)).to be_nil
-      end
+          expect(issue_label).not_to be_nil
+          expect(issue_label.id).to eq(new_label.id)
+        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)
+        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
 
-        expect(issue_label).not_to be_nil
-        expect(issue_label.id).to eq(new_label.id)
-      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)
 
-      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
+          expect(issue_label).not_to be_nil
+          expect(issue_label.id).to eq(project_label_1_2.id)
+        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)
+        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(project_label_1_2.id)
-      end
+          expect(issue_label).not_to be_nil
+          expect(issue_label.id).to eq(new_label.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)
+        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(new_label.id)
-      end
+          expect(issue_label).not_to be_nil
+          expect(issue_label.id).to eq(project_label_4_1.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)
+        it 'updates merge request' do
+          service.execute(project_label_1_1)
+          merge_label = merge_3_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
+          expect(merge_label).not_to be_nil
+          expect(merge_label.id).to eq(new_label.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)
+        it 'updates board lists' do
+          service.execute(project_label_1_1)
+          list = issue_board_2_1.lists.find_by(label: new_label)
 
-        expect(merge_label).not_to be_nil
-        expect(merge_label.id).to eq(new_label.id)
-      end
+          expect(list).not_to be_nil
+        end
 
-      it 'updates board lists' do
-        service.execute(project_label_1_1)
-        list = issue_board_2_1.lists.find_by(label: new_label)
+        # 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)
 
-        expect(list).not_to be_nil
+          Label.reflect_on_all_associations.each do |association|
+            expect(project_label_1_1.send(association.name).any?).to be_falsey
+          end
+        end
       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)
+      context 'if there is an existing identical group label' do
+        let!(:existing_group_label) { create(:group_label, group: group_1, title: project_label_1_1.title ) }
+
+        it 'uses the existing group label' do
+          expect { service.execute(project_label_1_1) }
+            .to change(project_1.labels, :count).by(-1)
+            .and not_change(group_1.labels, :count)
+          expect(new_label).not_to be_nil
+        end
 
-        Label.reflect_on_all_associations.each do |association|
-          expect(project_label_1_1.send(association.name).any?).to be_falsey
+        it 'does not create a new group label clone' do
+          expect { service.execute(project_label_1_1) }.not_to change { GroupLabel.maximum(:id) }
         end
+
+        it_behaves_like 'promoting a project label to a group label'
       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)
+      context 'if there is no existing identical group label' do
+        let(:existing_group_label) { nil }
 
-            label
-          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 'raises an exception' do
-          expect { service.execute(project_label_1_1) }.to raise_error(ActiveRecord::RecordInvalid)
+        it 'copies title, description and color to cloned group label' 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_behaves_like 'promoting a project label to a group label'
       end
     end
   end