Merge branch 'sh-fix-labels-move-issue' into 'master'

Fix bug where labels would be assigned to issues that were moved

If you attempt to move an issue from one project to another and leave
labels blank, LabelsFinder would assign all labels in the new project
to that issue. The issue is that :title is passed along to the Finder,
but since it appears empty no filtering is done. As a result, all
labels in the group are returned. This fix handles that case.

Closes #23668

See merge request !7065
Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG.md

@@ -13,6 +13,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Stop clearing the database cache on `rake cache:clear`. !7056
   - Only show register tab if signup enabled. !7058
   - Expire and build repository cache after project import. !7064
+  - Fix bug where labels would be assigned to issues that were moved. !7065
 
 ## 8.13.0 (2016-10-22)
 

app/finders/labels_finder.rb

@@ -35,8 +35,10 @@ class LabelsFinder < UnionFinder
   end
 
   def with_title(items)
-    items = items.where(title: title) if title
-    items
+    return items if title.nil?
+    return items.none if title.blank?
+
+    items.where(title: title)
   end
 
   def group_id
@@ -52,7 +54,7 @@ class LabelsFinder < UnionFinder
   end
 
   def title
-    params[:title].presence || params[:name].presence
+    params[:title] || params[:name]
   end
 
   def project

spec/finders/labels_finder_spec.rb

@@ -38,6 +38,14 @@ describe LabelsFinder do
 
         expect(finder.execute).to eq [group_label_2, group_label_3, project_label_1, group_label_1, project_label_2, project_label_4]
       end
+
+      it 'returns labels available if nil title is supplied' do
+        group_2.add_developer(user)
+        # params[:title] will return `nil` regardless whether it is specified
+        finder = described_class.new(user, title: nil)
+
+        expect(finder.execute).to eq [group_label_2, group_label_3, project_label_1, group_label_1, project_label_2, project_label_4]
+      end
     end
 
     context 'filtering by group_id' do
@@ -64,6 +72,30 @@ describe LabelsFinder do
 
         expect(finder.execute).to eq [group_label_2]
       end
+
+      it 'returns label with title alias' do
+        finder = described_class.new(user, name: 'Group Label 2')
+
+        expect(finder.execute).to eq [group_label_2]
+      end
+
+      it 'returns no labels if empty title is supplied' do
+        finder = described_class.new(user, title: [])
+
+        expect(finder.execute).to be_empty
+      end
+
+      it 'returns no labels if blank title is supplied' do
+        finder = described_class.new(user, title: '')
+
+        expect(finder.execute).to be_empty
+      end
+
+      it 'returns no labels if empty name is supplied' do
+        finder = described_class.new(user, name: [])
+
+        expect(finder.execute).to be_empty
+      end
     end
   end
 end

spec/services/issues/move_service_spec.rb

@@ -23,14 +23,15 @@ describe Issues::MoveService, services: true do
       old_project.team << [user, :reporter]
       new_project.team << [user, :reporter]
 
-      ['label1', 'label2'].each do |label|
+      labels = Array.new(2) { |x| "label%d" % (x + 1) }
+
+      labels.each do |label|
         old_issue.labels << create(:label,
           project_id: old_project.id,
           title: label)
-      end
 
-      new_project.labels << create(:label, title: 'label1')
-      new_project.labels << create(:label, title: 'label2')
+        new_project.labels << create(:label, title: label)
+      end
     end
   end
 
@@ -277,5 +278,25 @@ describe Issues::MoveService, services: true do
         it { expect { move }.to raise_error(StandardError, /permissions/) }
       end
     end
+
+    context 'movable issue with no assigned labels' do
+      before do
+        old_project.team << [user, :reporter]
+        new_project.team << [user, :reporter]
+
+        labels = Array.new(2) { |x| "label%d" % (x + 1) }
+
+        labels.each do |label|
+          new_project.labels << create(:label, title: label)
+        end
+      end
+
+      include_context 'issue move executed'
+
+      it 'does not assign labels to new issue' do
+        expected_label_titles = new_issue.reload.labels.map(&:title)
+        expect(expected_label_titles.size).to eq 0
+      end
+    end
   end
 end