Commit 02f34cd7 authored by Mario de la Ossa's avatar Mario de la Ossa

Fix faulty NOT logic for issuable array params

Fixes labels and assignees NOT params to remove issuables containing ANY
value in the list instead of ALL values in the list.
parent cbc62341
......@@ -314,17 +314,20 @@ class IssuableFinder
params[:assignee_username].present?
end
# rubocop: disable CodeReuse/ActiveRecord
def assignee
return @assignee if defined?(@assignee)
assignees.first
end
@assignee =
# rubocop: disable CodeReuse/ActiveRecord
def assignees
strong_memoize(:assignees) do
if assignee_id?
User.find_by(id: params[:assignee_id])
User.where(id: params[:assignee_id])
elsif assignee_username?
User.find_by_username(params[:assignee_username])
User.where(username: params[:assignee_username])
else
nil
User.none
end
end
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -415,7 +418,7 @@ class IssuableFinder
# These are "helper" params that are required inside the NOT to get the right results. They usually come in
# at the top-level params, but if they do come in inside the `:not` params, they should take precedence.
not_helpers = params.slice(*NEGATABLE_PARAMS_HELPER_KEYS).merge(params[:not].slice(*NEGATABLE_PARAMS_HELPER_KEYS))
not_param = { key => value }.with_indifferent_access.merge(not_helpers)
not_param = { key => value }.with_indifferent_access.merge(not_helpers).merge(not_query: true)
items_to_negate = self.class.new(current_user, not_param).execute
......@@ -543,6 +546,8 @@ class IssuableFinder
# rubocop: enable CodeReuse/ActiveRecord
def by_assignee(items)
return items.assigned_to(assignees) if not_query? && assignees.any?
if filter_by_no_assignee?
items.unassigned
elsif filter_by_any_assignee?
......@@ -624,7 +629,7 @@ class IssuableFinder
elsif filter_by_any_label?
items.any_label
else
items.with_label(label_names, params[:sort])
items.with_label(label_names, params[:sort], not_query: not_query?)
end
items
......@@ -673,4 +678,8 @@ class IssuableFinder
def min_access_level
ProjectFeature.required_minimum_access_level(klass)
end
def not_query?
!!params[:not_query]
end
end
......@@ -108,7 +108,9 @@ module Issuable
where("NOT EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE #{to_ability_name}_id = #{to_ability_name}s.id)")
end
scope :assigned_to, ->(u) do
where("EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE user_id = ? AND #{to_ability_name}_id = #{to_ability_name}s.id)", u.id)
assignees_table = Arel::Table.new("#{to_ability_name}_assignees")
sql = assignees_table.project('true').where(assignees_table[:user_id].in(u)).where(Arel::Nodes::SqlLiteral.new("#{to_ability_name}_id = #{to_ability_name}s.id"))
where("EXISTS (#{sql.to_sql})")
end
# rubocop:enable GitlabSecurity/SqlInjection
......@@ -263,8 +265,9 @@ module Issuable
.reorder(Gitlab::Database.nulls_last_order('highest_priority', direction))
end
def with_label(title, sort = nil)
if title.is_a?(Array) && title.size > 1
def with_label(title, sort = nil, not_query: false)
multiple_labels = title.is_a?(Array) && title.size > 1
if multiple_labels && !not_query
joins(:labels).where(labels: { title: title }).group(*grouping_columns(sort)).having("COUNT(DISTINCT labels.title) = #{title.size}")
else
joins(:labels).where(labels: { title: title })
......
......@@ -49,9 +49,13 @@ module EE
params[:weight].to_s.downcase == ::IssuesFinder::FILTER_ANY
end
def assignee_ids?
params[:assignee_ids].present?
end
override :by_assignee
def by_assignee(items)
if assignees.any?
if assignees.any? && !not_query?
assignees.each do |assignee|
items = items.assigned_to(assignee)
end
......@@ -62,15 +66,14 @@ module EE
super
end
override :assignees
# rubocop: disable CodeReuse/ActiveRecord
def assignees
strong_memoize(:assignees) do
if params[:assignee_ids]
if assignee_ids?
::User.where(id: params[:assignee_ids])
elsif params[:assignee_username]
::User.where(username: params[:assignee_username])
else
[]
super
end
end
end
......
......@@ -54,6 +54,27 @@ describe IssuesFinder do
end
end
context 'filter by username' do
let_it_be(:user3) { create(:user) }
let(:issuables) { issues }
before do
project2.add_developer(user3)
issue2.assignees = [user, user2]
issue3.assignees = [user2, user3]
end
it_behaves_like 'assignee username filter' do
let(:params) { { assignee_username: [user2.username, user3.username] } }
let(:expected_issuables) { [issue3] }
end
it_behaves_like 'assignee NOT username filter' do
let(:params) { { not: { assignee_username: [user.username, user2.username] } } }
let(:expected_issuables) { [issue4] }
end
end
context 'filter by epic' do
let_it_be(:group) { create(:group) }
......
......@@ -33,17 +33,22 @@ describe IssuesFinder do
before do
project2.add_developer(user3)
issue3.assignees = [user2, user3]
issue2.assignees = [user2]
issue3.assignees = [user3]
end
it_behaves_like 'assignee username filter' do
let(:params) { { assignee_username: [user2.username, user3.username] } }
let(:expected_issuables) { [issue3] }
let(:params) { { assignee_username: [user2.username] } }
let(:expected_issuables) { [issue2] }
end
it_behaves_like 'assignee NOT username filter' do
let(:params) { { not: { assignee_username: [user2.username, user3.username] } } }
let(:expected_issuables) { [issue1, issue2, issue4] }
before do
issue2.assignees = [user2]
end
let(:params) { { not: { assignee_username: [user.username, user2.username] } } }
let(:expected_issuables) { [issue3, issue4] }
end
end
......@@ -395,8 +400,8 @@ describe IssuesFinder do
context 'using NOT' do
let(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
it 'returns issues that do not have ALL labels provided' do
expect(issues).to contain_exactly(issue1, issue3, issue4)
it 'returns issues that do not have any of the labels provided' do
expect(issues).to contain_exactly(issue1, issue4)
end
end
end
......@@ -417,8 +422,8 @@ describe IssuesFinder do
context 'using NOT' do
let(:params) { { not: { label_name: [label.title, label2.title].join(',') } } }
it 'returns issues that do not have ALL labels provided' do
expect(issues).to contain_exactly(issue1, issue3, issue4)
it 'returns issues that do not have ANY ONE of the labels provided' do
expect(issues).to contain_exactly(issue1, issue4)
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