Merge branch '18059-fix-faulty-not-logic' into 'master'

Fix faulty NOT logic for issuable array params

See merge request gitlab-org/gitlab!22555

app/finders/issuable_finder.rb

@@ -314,18 +314,21 @@ 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

app/models/concerns/issuable.rb

@@ -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 })

ee/app/finders/ee/issues_finder.rb

@@ -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

ee/spec/finders/issues_finder_spec.rb

@@ -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) }
 

spec/finders/issues_finder_spec.rb

@@ -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