Commit 23147066 authored by Rémy Coutable's avatar Rémy Coutable

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
parents 99af6da7 02f835c1
...@@ -16,6 +16,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -16,6 +16,7 @@ Please view this file on the master branch, on stable branches it's out of date.
- Refactor email, use setter method instead AR callbacks for email attribute (Semyon Pupkov) - Refactor email, use setter method instead AR callbacks for email attribute (Semyon Pupkov)
## 8.13.1 (unreleased) ## 8.13.1 (unreleased)
- Fix bug where labels would be assigned to issues that were moved
- Fix error in generating labels - Fix error in generating labels
- Fix reply-by-email not working due to queue name mismatch - Fix reply-by-email not working due to queue name mismatch
- Fixed hidden pipeline graph on commit and MR page !6895 - Fixed hidden pipeline graph on commit and MR page !6895
......
...@@ -35,8 +35,10 @@ class LabelsFinder < UnionFinder ...@@ -35,8 +35,10 @@ class LabelsFinder < UnionFinder
end end
def with_title(items) def with_title(items)
items = items.where(title: title) if title return items if title.nil?
items return items.none if title.blank?
items.where(title: title)
end end
def group_id def group_id
...@@ -52,7 +54,7 @@ class LabelsFinder < UnionFinder ...@@ -52,7 +54,7 @@ class LabelsFinder < UnionFinder
end end
def title def title
params[:title].presence || params[:name].presence params[:title] || params[:name]
end end
def project def project
......
...@@ -38,6 +38,14 @@ describe LabelsFinder do ...@@ -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] 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
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 end
context 'filtering by group_id' do context 'filtering by group_id' do
...@@ -64,6 +72,30 @@ describe LabelsFinder do ...@@ -64,6 +72,30 @@ describe LabelsFinder do
expect(finder.execute).to eq [group_label_2] expect(finder.execute).to eq [group_label_2]
end 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 end
end end
...@@ -23,14 +23,15 @@ describe Issues::MoveService, services: true do ...@@ -23,14 +23,15 @@ describe Issues::MoveService, services: true do
old_project.team << [user, :reporter] old_project.team << [user, :reporter]
new_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, old_issue.labels << create(:label,
project_id: old_project.id, project_id: old_project.id,
title: label) title: label)
end
new_project.labels << create(:label, title: 'label1') new_project.labels << create(:label, title: label)
new_project.labels << create(:label, title: 'label2') end
end end
end end
...@@ -277,5 +278,25 @@ describe Issues::MoveService, services: true do ...@@ -277,5 +278,25 @@ describe Issues::MoveService, services: true do
it { expect { move }.to raise_error(StandardError, /permissions/) } it { expect { move }.to raise_error(StandardError, /permissions/) }
end end
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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment