Merge branch 'issue_15557' into 'master'

Fix error 500 when sorting issues by milestone due date and filtering by labels

fixes #15557 

See merge request !4327

15 files changed, 73 insertions, 47 deletions

diff --git a/.rubocop.yml b/.rubocop.yml
index eece9ec69d8..9275f9537d5 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -318,7 +318,7 @@ Style/LambdaCall:
 
 # Comments should start with a space.
 Style/LeadingCommentSpace:
-  Enabled: false
+  Enabled: true
 
 # Use \ instead of + or << to concatenate two string literals at line end.
 Style/LineEndConcatenation:
diff --git a/CHANGELOG b/CHANGELOG
index cb1603eea37..79395b81abb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -15,6 +15,7 @@ v 8.9.0 (unreleased)
   - Changed the Slack build message to use the singular duration if necessary (Aran Koning)
   - Fix issues filter when ordering by milestone
   - Todos will display target state if issuable target is 'Closed' or 'Merged'
+  - Fix bug when sorting issues by milestone due date and filtering by two or more labels
   - Remove 'main language' feature
   - Projects pending deletion will render a 404 page
   - Measure queue duration between gitlab-workhorse and Rails
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 1429ee40bb7..9b2a9d298b3 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -263,7 +263,7 @@ class ApplicationController < ActionController::Base
       # internal repos where you are not a member. Enable this filter
       # or improve current implementation to filter only issues you
       # created or assigned or mentioned
-      #@filter_params[:authorized_only] = true
+      # @filter_params[:authorized_only] = true
     end
 
     @filter_params
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 17ce8e2ad20..d54284d7b20 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -205,7 +205,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def branch_from
-    #This is always source
+    # This is always source
     @source_project = @merge_request.nil? ? @project : @merge_request.source_project
     @commit = @repository.commit(params[:ref]) if params[:ref].present?
     render layout: false
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 8ed3ccf1c02..7d8c56f4c22 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -271,7 +271,7 @@ class IssuableFinder
       if filter_by_no_label?
         items = items.without_label
       else
-        items = items.with_label(label_names)
+        items = items.with_label(label_names, params[:sort])
         if projects
           items = items.where(labels: { project_id: projects })
         end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 91315b3459f..9aaaf787372 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -126,13 +126,29 @@ module Issuable
       joins(join_clause).group(issuable_table[:id]).reorder("COUNT(notes.id) DESC")
     end
 
-    def with_label(title)
+    def with_label(title, sort = nil)
       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}")
+        joins(:labels).where(labels: { title: title }).group(*grouping_columns(sort)).having("COUNT(DISTINCT labels.title) = #{title.size}")
       else
         joins(:labels).where(labels: { title: title })
       end
     end
+
+    # Includes table keys in group by clause when sorting
+    # preventing errors in postgres
+    #
+    # Returns an array of arel columns
+    def grouping_columns(sort)
+      grouping_columns = [arel_table[:id]]
+
+      if ["milestone_due_desc", "milestone_due_asc"].include?(sort)
+        milestone_table = Milestone.arel_table
+        grouping_columns << milestone_table[:id]
+        grouping_columns << milestone_table[:due_date]
+      end
+
+      grouping_columns
+    end
   end
 
   def today?
diff --git a/app/models/key.rb b/app/models/key.rb
index d52afda67d1..0532e84f47d 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -26,7 +26,7 @@ class Key < ActiveRecord::Base
   end
 
   def publishable_key
-    #Removes anything beyond the keytype and key itself
+    # Removes anything beyond the keytype and key itself
     self.key.split[0..1].join(' ')
   end
 
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index 393ad909b45..7bd13105045 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -52,7 +52,7 @@ Doorkeeper.configure do
   # For more information go to
   # https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes
   default_scopes  :api
-  #optional_scopes :write, :update
+  # optional_scopes :write, :update
 
   # Change the way client credentials are retrieved from the request object.
   # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then
diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb
index 4c164119fff..26c30e523a7 100644
--- a/config/initializers/omniauth.rb
+++ b/config/initializers/omniauth.rb
@@ -13,7 +13,7 @@ end
 
 OmniAuth.config.full_host = Settings.gitlab['base_url']
 OmniAuth.config.allowed_request_methods = [:post]
-#In case of auto sign-in, the GET method is used (users don't get to click on a button)
+# In case of auto sign-in, the GET method is used (users don't get to click on a button)
 OmniAuth.config.allowed_request_methods << :get if Gitlab.config.omniauth.auto_sign_in_with_provider.present?
 OmniAuth.config.before_request_phase do |env|
   OmniAuth::RequestForgeryProtection.call(env)
diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb
index 6fe7faa547a..522dd2b9428 100644
--- a/lib/gitlab/diff/parser.rb
+++ b/lib/gitlab/diff/parser.rb
@@ -17,16 +17,16 @@ module Gitlab
         Enumerator.new do |yielder|
           @lines.each do |line|
             next if filename?(line)
-  
+
             full_line = line.delete("\n")
-  
+
             if line.match(/^@@ -/)
               type = "match"
-  
+
               line_old = line.match(/\-[0-9]*/)[0].to_i.abs rescue 0
               line_new = line.match(/\+[0-9]*/)[0].to_i.abs rescue 0
-  
-              next if line_old <= 1 && line_new <= 1 #top of file
+
+              next if line_old <= 1 && line_new <= 1 # top of file
               yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
               line_obj_index += 1
               next
@@ -39,8 +39,8 @@ module Gitlab
               yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new)
               line_obj_index += 1
             end
-  
-  
+
+
             case line[0]
             when "+"
               line_new += 1
diff --git a/lib/gitlab/gitlab_import/importer.rb b/lib/gitlab/gitlab_import/importer.rb
index e32ef8a44c2..3f76ec97977 100644
--- a/lib/gitlab/gitlab_import/importer.rb
+++ b/lib/gitlab/gitlab_import/importer.rb
@@ -17,7 +17,7 @@ module Gitlab
       def execute
         project_identifier = CGI.escape(project.import_source)
 
-        #Issues && Comments
+        # Issues && Comments
         issues = client.issues(project_identifier)
 
         issues.each do |issue|
diff --git a/spec/features/merge_requests/user_lists_merge_requests_spec.rb b/spec/features/merge_requests/user_lists_merge_requests_spec.rb
index 2c7e1c748ad..1c130057c56 100644
--- a/spec/features/merge_requests/user_lists_merge_requests_spec.rb
+++ b/spec/features/merge_requests/user_lists_merge_requests_spec.rb
@@ -131,6 +131,15 @@ describe 'Projects > Merge requests > User lists merge requests', feature: true
         expect(first_merge_request).to include('fix')
         expect(count_merge_requests).to eq(1)
       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: user.id,
+          sort: sort_value_milestone_soon)
+
+        expect(first_merge_request).to include('fix')
+      end
     end
   end
 
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 70bbe633269..fb20578d8d3 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -118,10 +118,10 @@ describe Issue, "Issuable" do
     let(:project) { build_stubbed(:empty_project) }
 
     context "by milestone due date" do
-      #Correct order is:
-      #Issues/MRs with milestones ordered by date
-      #Issues/MRs with milestones without dates
-      #Issues/MRs without milestones
+      # Correct order is:
+      # Issues/MRs with milestones ordered by date
+      # Issues/MRs with milestones without dates
+      # Issues/MRs without milestones
 
       let!(:issue) { create(:issue, project: project) }
       let!(:early_milestone) { create(:milestone, project: project, due_date: 10.days.from_now) }
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 40b24c125b5..a7690f430c4 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -20,7 +20,7 @@ describe API::API, api: true  do
     end
 
     context "when authenticated" do
-      #These specs are written just in case API authentication is not required anymore
+      # These specs are written just in case API authentication is not required anymore
       context "when public level is restricted" do
         before do
           stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
diff --git a/spec/teaspoon_env.rb b/spec/teaspoon_env.rb
index 58f45ff8610..69b2b9b6d5b 100644
--- a/spec/teaspoon_env.rb
+++ b/spec/teaspoon_env.rb
@@ -41,11 +41,11 @@ Teaspoon.configure do |config|
     suite.matcher = "{spec/javascripts,app/assets}/**/*_spec.{js,js.coffee,coffee}"
 
     # Load additional JS files, but requiring them in your spec helper is the preferred way to do this.
-    #suite.javascripts = []
+    # suite.javascripts = []
 
     # You can include your own stylesheets if you want to change how Teaspoon looks.
     # Note: Spec related CSS can and should be loaded using fixtures.
-    #suite.stylesheets = ["teaspoon"]
+    # suite.stylesheets = ["teaspoon"]
 
     # This suites spec helper, which can require additional support files. This file is loaded before any of your test
     # files are loaded.
@@ -62,19 +62,19 @@ Teaspoon.configure do |config|
 
     # Hooks allow you to use `Teaspoon.hook("fixtures")` before, after, or during your spec run. This will make a
     # synchronous Ajax request to the server that will call all of the blocks you've defined for that hook name.
-    #suite.hook :fixtures, &proc{}
+    # suite.hook :fixtures, &proc{}
 
     # Determine whether specs loaded into the test harness should be embedded as individual script tags or concatenated
-    # into a single file. Similar to Rails' asset `debug: true` and `config.assets.debug = true` options. By default, 
+    # into a single file. Similar to Rails' asset `debug: true` and `config.assets.debug = true` options. By default,
     # Teaspoon expands all assets to provide more valuable stack traces that reference individual source files.
-    #suite.expand_assets = true
+    # suite.expand_assets = true
   end
 
   # Example suite. Since we're just filtering to files already within the root test/javascripts, these files will also
   # be run in the default suite -- but can be focused into a more specific suite.
-  #config.suite :targeted do |suite|
+  # config.suite :targeted do |suite|
   #  suite.matcher = "spec/javascripts/targeted/*_spec.{js,js.coffee,coffee}"
-  #end
+  # end
 
   # CONSOLE RUNNER SPECIFIC
   #
@@ -94,45 +94,45 @@ Teaspoon.configure do |config|
   # PhantomJS: https://github.com/modeset/teaspoon/wiki/Using-PhantomJS
   # Selenium Webdriver: https://github.com/modeset/teaspoon/wiki/Using-Selenium-WebDriver
   # Capybara Webkit: https://github.com/modeset/teaspoon/wiki/Using-Capybara-Webkit
-  #config.driver = :phantomjs
+  # config.driver = :phantomjs
 
   # Specify additional options for the driver.
   #
   # PhantomJS: https://github.com/modeset/teaspoon/wiki/Using-PhantomJS
   # Selenium Webdriver: https://github.com/modeset/teaspoon/wiki/Using-Selenium-WebDriver
   # Capybara Webkit: https://github.com/modeset/teaspoon/wiki/Using-Capybara-Webkit
-  #config.driver_options = nil
+  # config.driver_options = nil
 
   # Specify the timeout for the driver. Specs are expected to complete within this time frame or the run will be
   # considered a failure. This is to avoid issues that can arise where tests stall.
-  #config.driver_timeout = 180
+  # config.driver_timeout = 180
 
   # Specify a server to use with Rack (e.g. thin, mongrel). If nil is provided Rack::Server is used.
-  #config.server = nil
+  # config.server = nil
 
   # Specify a port to run on a specific port, otherwise Teaspoon will use a random available port.
-  #config.server_port = nil
+  # config.server_port = nil
 
   # Timeout for starting the server in seconds. If your server is slow to start you may have to bump this, or you may
   # want to lower this if you know it shouldn't take long to start.
-  #config.server_timeout = 20
+  # config.server_timeout = 20
 
   # Force Teaspoon to fail immediately after a failing suite. Can be useful to make Teaspoon fail early if you have
   # several suites, but in environments like CI this may not be desirable.
-  #config.fail_fast = true
+  # config.fail_fast = true
 
   # Specify the formatters to use when outputting the results.
   # Note: Output files can be specified by using `"junit>/path/to/output.xml"`.
   #
   # Available: :dot, :clean, :documentation, :json, :junit, :pride, :rspec_html, :snowday, :swayze_or_oprah, :tap, :tap_y, :teamcity
-  #config.formatters = [:dot]
+  # config.formatters = [:dot]
 
   # Specify if you want color output from the formatters.
-  #config.color = true
+  # config.color = true
 
   # Teaspoon pipes all console[log/debug/error] to $stdout. This is useful to catch places where you've forgotten to
   # remove them, but in verbose applications this may not be desirable.
-  #config.suppress_log = false
+  # config.suppress_log = false
 
   # COVERAGE REPORTS / THRESHOLD ASSERTIONS
   #
@@ -149,7 +149,7 @@ Teaspoon.configure do |config|
   # Specify that you always want a coverage configuration to be used. Otherwise, specify that you want coverage
   # on the CLI.
   # Set this to "true" or the name of your coverage config.
-  #config.use_coverage = nil
+  # config.use_coverage = nil
 
   # You can have multiple coverage configs by passing a name to config.coverage.
   # e.g. config.coverage :ci do |coverage|
@@ -158,21 +158,21 @@ Teaspoon.configure do |config|
     # Which coverage reports Istanbul should generate. Correlates directly to what Istanbul supports.
     #
     # Available: text-summary, text, html, lcov, lcovonly, cobertura, teamcity
-    #coverage.reports = ["text-summary", "html"]
+    # coverage.reports = ["text-summary", "html"]
 
     # The path that the coverage should be written to - when there's an artifact to write to disk.
     # Note: Relative to `config.root`.
-    #coverage.output_path = "coverage"
+    # coverage.output_path = "coverage"
 
     # Assets to be ignored when generating coverage reports. Accepts an array of filenames or regular expressions. The
     # default excludes assets from vendor, gems and support libraries.
-    #coverage.ignore = [%r{/lib/ruby/gems/}, %r{/vendor/assets/}, %r{/support/}, %r{/(.+)_helper.}]
+    # coverage.ignore = [%r{/lib/ruby/gems/}, %r{/vendor/assets/}, %r{/support/}, %r{/(.+)_helper.}]
 
     # Various thresholds requirements can be defined, and those thresholds will be checked at the end of a run. If any
     # aren't met the run will fail with a message. Thresholds can be defined as a percentage (0-100), or nil.
-    #coverage.statements = nil
-    #coverage.functions = nil
-    #coverage.branches = nil
-    #coverage.lines = nil
+    # coverage.statements = nil
+    # coverage.functions = nil
+    # coverage.branches = nil
+    # coverage.lines = nil
   end
 end