Fix an issue when filtering merge requests with more than one label15529-fix=missing-from-clause-for-table-issues

Signed-off-by: Rémy Coutable <remy@rymai.me>

3 files changed, 188 insertions, 2 deletions

diff --git a/CHANGELOG b/CHANGELOG
index fbc3ee194cb..c16de0f7828 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -9,6 +9,7 @@ v 8.7.1 (unreleased)
   - Use the `can?` helper instead of `current_user.can?`. !3882
   - Prevent users from deleting Webhooks via API they do not own
   - Fix Error 500 due to stale cache when projects are renamed or transferred
+  - Fix an issue when filtering merge requests with more than one label
 
 v 8.7.0
   - Gitlab::GitAccess and Gitlab::GitAccessWiki are now instrumented
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index d5166e81474..e2f534d2483 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -123,8 +123,8 @@ module Issuable
     end
 
     def with_label(title)
-      if title.is_a?(Array) && title.count > 1
-        joins(:labels).where(labels: { title: title }).group('issues.id').having("count(distinct labels.title) = #{title.count}")
+      if title.is_a?(Array) && title.size > 1
+        joins(:labels).where(labels: { title: title }).group(arel_table[:id]).having("COUNT(DISTINCT labels.title) = #{title.size}")
       else
         joins(:labels).where(labels: { title: title })
       end
diff --git a/spec/features/merge_requests/user_lists_merge_requests_spec.rb b/spec/features/merge_requests/user_lists_merge_requests_spec.rb
new file mode 100644
index 00000000000..37deac65e6f
--- /dev/null
+++ b/spec/features/merge_requests/user_lists_merge_requests_spec.rb
@@ -0,0 +1,185 @@
+require 'spec_helper'
+
+describe 'Projects > Merge requests > User lists merge requests', feature: true do
+  include SortingHelper
+
+  let(:project) { create(:project, :public) }
+  let(:user) { create(:user) }
+
+  before { login_with(user) }
+
+  describe 'Filter merge requests' do
+    before do
+      %w[fix markdown].each do |branch|
+        create(:merge_request,
+               author: user,
+               assignee: user,
+               source_project: project,
+               source_branch: branch,
+               title: branch)
+      end
+      create(:merge_request,
+             author: user,
+             assignee: nil,
+             source_project: project,
+             source_branch: 'lfs',
+             title: 'lfs',
+             milestone: create(:milestone, project: project))
+    end
+
+    it 'allows filtering by merge requests with no specified assignee' do
+      visit namespace_project_merge_requests_path(project.namespace, project, assignee_id: IssuableFinder::NONE)
+
+      expect(current_path).to eq(namespace_project_merge_requests_path(project.namespace, project))
+      expect(page).to have_content 'lfs'
+      expect(page).not_to have_content 'fix'
+      expect(page).not_to have_content 'markdown'
+    end
+
+    it 'allows filtering by a specified assignee' do
+      visit namespace_project_merge_requests_path(project.namespace, project, assignee_id: user.id)
+
+      expect(page).not_to have_content 'lfs'
+      expect(page).to have_content 'fix'
+      expect(page).to have_content 'markdown'
+    end
+  end
+
+  describe 'Filter & Sort merge requests' do
+    %w[fix markdown lfs].each_with_index do |branch, index|
+      let!(branch.to_sym) do
+        create(:merge_request,
+               title: branch,
+               source_project: project,
+               source_branch: branch,
+               created_at: Time.now - (index * 60))
+      end
+    end
+    let(:newer_due_milestone) { create(:milestone, due_date: '2013-12-11') }
+    let(:later_due_milestone) { create(:milestone, due_date: '2013-12-12') }
+
+    describe 'Sort merge requests' do
+      it 'sorts by newest' do
+        visit namespace_project_merge_requests_path(project.namespace, project, sort: sort_value_recently_created)
+
+        expect(first_merge_request).to include('lfs')
+        expect(last_merge_request).to include('fix')
+      end
+
+      it 'sorts by oldest' do
+        visit namespace_project_merge_requests_path(project.namespace, project, sort: sort_value_oldest_created)
+
+        expect(first_merge_request).to include('fix')
+        expect(last_merge_request).to include('lfs')
+      end
+
+      it 'sorts by most recently updated' do
+        lfs.updated_at = Time.now + 100
+        lfs.save
+        visit namespace_project_merge_requests_path(project.namespace, project, sort: sort_value_recently_updated)
+
+        expect(first_merge_request).to include('lfs')
+      end
+
+      it 'sorts by least recently updated' do
+        lfs.updated_at = Time.now - 100
+        lfs.save
+        visit namespace_project_merge_requests_path(project.namespace, project, sort: sort_value_oldest_updated)
+
+        expect(first_merge_request).to include('lfs')
+      end
+
+      describe 'sorting by milestone' do
+        before do
+          fix.milestone = newer_due_milestone
+          fix.save
+          markdown.milestone = later_due_milestone
+          markdown.save
+        end
+
+        it 'sorts by recently due milestone' do
+          visit namespace_project_merge_requests_path(project.namespace, project, sort: sort_value_milestone_soon)
+
+          expect(first_merge_request).to include('fix')
+        end
+
+        it 'sorts by least recently due milestone' do
+          visit namespace_project_merge_requests_path(project.namespace, project, sort: sort_value_milestone_later)
+
+          expect(first_merge_request).to include('markdown')
+        end
+      end
+    end
+
+    describe 'Filter and sort at the same time' do
+      let(:user2) { create(:user) }
+
+      before do
+        fix.assignee = user2
+        fix.milestone = newer_due_milestone
+        fix.save
+        markdown.assignee = user2
+        markdown.milestone = later_due_milestone
+        markdown.save
+      end
+
+      context 'filter on one label' do
+        let(:label) { create(:label, project: project) }
+        before { create(:label_link, label: label, target: fix) }
+
+        it 'sorts by due soon' do
+          visit namespace_project_merge_requests_path(project.namespace, project,
+            label_name: [label.name],
+            sort: sort_value_due_date_soon)
+
+          expect(first_merge_request).to include('fix')
+        end
+      end
+
+      context 'filter on two labels' do
+        let(:label) { create(:label, project: project) }
+        let(:label2) { create(:label, project: project) }
+        before do
+          create(:label_link, label: label, target: fix)
+          create(:label_link, label: label2, target: fix)
+        end
+
+        it 'sorts by due soon' do
+          visit namespace_project_merge_requests_path(project.namespace, project,
+            label_name: [label.name, label2.name],
+            sort: sort_value_due_date_soon)
+
+          expect(first_merge_request).to include('fix')
+        end
+
+        context 'filter on assignee' do
+          it 'sorts by due soon' do
+            visit namespace_project_merge_requests_path(project.namespace, project,
+              label_name: [label.name, label2.name],
+              assignee_id: user2.id,
+              sort: sort_value_due_date_soon)
+
+            expect(first_merge_request).to include('fix')
+          end
+
+          it 'sorts by recently due milestone' do
+            visit namespace_project_merge_requests_path(project.namespace, project,
+              label_name: [label.name, label2.name],
+              assignee_id: user2.id,
+              sort: sort_value_milestone_soon)
+
+            expect(first_merge_request).to include('fix')
+          end
+        end
+      end
+    end
+  end
+
+  def first_merge_request
+    page.all('ul.mr-list > li').first.text
+  end
+
+  def last_merge_request
+    page.all('ul.mr-list > li').last.text
+  end
+end