Merge branch 'api-default-order' into 'master'

Sorting refactoring

* refactor sorting logic for web UI
* refactor and document ordering for API: project, issues, merge requests
* explicitly define ordering in models using default_scope

See merge request !1476

4 files changed, 72 insertions, 60 deletions

diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index be9e4280d65..8fa30460ba6 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -154,6 +154,22 @@ module API
       Gitlab::Access.options_with_owner.values.include? level.to_i
     end
 
+    def issuable_order_by
+      if params["order_by"] == 'updated_at'
+        'updated_at'
+      else
+        'created_at'
+      end
+    end
+
+    def issuable_sort
+      if params["sort"] == 'asc'
+        :asc
+      else
+        :desc
+      end
+    end
+
     # error helpers
 
     def forbidden!(reason = nil)
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index d2828b24c36..ff062be6040 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -27,7 +27,9 @@ module API
       # Parameters:
       #   state (optional) - Return "opened" or "closed" issues
       #   labels (optional) - Comma-separated list of label names
-
+      #   order_by (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at`
+      #   sort (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
+      #
       # Example Requests:
       #   GET /issues
       #   GET /issues?state=opened
@@ -39,8 +41,7 @@ module API
         issues = current_user.issues
         issues = filter_issues_state(issues, params[:state]) unless params[:state].nil?
         issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil?
-        issues = issues.order('issues.id DESC')
-
+        issues.reorder(issuable_order_by => issuable_sort)
         present paginate(issues), with: Entities::Issue
       end
     end
@@ -53,6 +54,8 @@ module API
       #   state (optional) - Return "opened" or "closed" issues
       #   labels (optional) - Comma-separated list of label names
       #   milestone (optional) - Milestone title
+      #   order_by (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at`
+      #   sort (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
       #
       # Example Requests:
       #   GET /projects/:id/issues
@@ -67,11 +70,12 @@ module API
         issues = user_project.issues
         issues = filter_issues_state(issues, params[:state]) unless params[:state].nil?
         issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil?
+
         unless params[:milestone].nil?
           issues = filter_issues_milestone(issues, params[:milestone])
         end
-        issues = issues.order('issues.id DESC')
 
+        issues.reorder(issuable_order_by => issuable_sort)
         present paginate(issues), with: Entities::Issue
       end
 
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index a0ebd8d0c1b..25b7857f4b1 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -25,6 +25,8 @@ module API
       # Parameters:
       #   id (required) - The ID of a project
       #   state (optional) - Return requests "merged", "opened" or "closed"
+      #   order_by (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at`
+      #   sort (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
       #
       # Example:
       #   GET /projects/:id/merge_requests
@@ -37,25 +39,18 @@ module API
       #
       get ":id/merge_requests" do
         authorize! :read_merge_request, user_project
+        merge_requests = user_project.merge_requests
+
+        merge_requests =
+          case params["state"]
+          when "opened" then merge_requests.opened
+          when "closed" then merge_requests.closed
+          when "merged" then merge_requests.merged
+          else merge_requests
+          end
 
-        mrs = case params["state"]
-              when "opened" then user_project.merge_requests.opened
-              when "closed" then user_project.merge_requests.closed
-              when "merged" then user_project.merge_requests.merged
-              else user_project.merge_requests
-              end
-
-        sort = case params["sort"]
-               when 'desc' then 'DESC'
-               else 'ASC'
-               end
-
-        mrs = case params["order_by"]
-              when 'updated_at' then mrs.order("updated_at #{sort}")
-              else  mrs.order("created_at #{sort}")
-              end
-
-        present paginate(mrs), with: Entities::MergeRequest
+        merge_requests.reorder(issuable_order_by => issuable_sort)
+        present paginate(merge_requests), with: Entities::MergeRequest
       end
 
       # Show MR
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index d96288bb982..0677e85beab 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -11,6 +11,37 @@ module API
           attrs[:visibility_level] = Gitlab::VisibilityLevel::PUBLIC if !attrs[:visibility_level].present? && publik == true
           attrs
         end
+
+        def filter_projects(projects)
+          # If the archived parameter is passed, limit results accordingly
+          if params[:archived].present?
+            projects = projects.where(archived: parse_boolean(params[:archived]))
+          end
+
+          if params[:search].present?
+            projects = projects.search(params[:search])
+          end
+
+          projects.reorder(project_order_by => project_sort)
+        end
+
+        def project_order_by
+          order_fields = %w(id name path created_at updated_at last_activity_at)
+
+          if order_fields.include?(params['order_by'])
+            params['order_by']
+          else
+            'created_at'
+          end
+        end
+
+        def project_sort
+          if params["sort"] == 'asc'
+            :asc
+          else
+            :desc
+          end
+        end
       end
 
       # Get a projects list for authenticated user
@@ -19,25 +50,7 @@ module API
       #   GET /projects
       get do
         @projects = current_user.authorized_projects
-        sort = params[:sort] == 'desc' ? 'desc' : 'asc'
-
-        @projects = case params["order_by"]
-                    when 'id' then @projects.reorder("id #{sort}")
-                    when 'name' then @projects.reorder("name #{sort}")
-                    when 'created_at' then @projects.reorder("created_at #{sort}")
-                    when 'last_activity_at' then @projects.reorder("last_activity_at #{sort}")
-                    else @projects
-                    end
-
-        # If the archived parameter is passed, limit results accordingly
-        if params[:archived].present?
-          @projects = @projects.where(archived: parse_boolean(params[:archived]))
-        end
-
-        if params[:search].present?
-          @projects = @projects.search(params[:search])
-        end
-
+        @projects = filter_projects(@projects)
         @projects = paginate @projects
         present @projects, with: Entities::Project
       end
@@ -47,16 +60,8 @@ module API
       # Example Request:
       #   GET /projects/owned
       get '/owned' do
-        sort = params[:sort] == 'desc' ? 'desc' : 'asc'
         @projects = current_user.owned_projects
-        @projects = case params["order_by"]
-                    when 'id' then @projects.reorder("id #{sort}")
-                    when 'name' then @projects.reorder("name #{sort}")
-                    when 'created_at' then @projects.reorder("created_at #{sort}")
-                    when 'last_activity_at' then @projects.reorder("last_activity_at #{sort}")
-                    else @projects
-                    end
-
+        @projects = filter_projects(@projects)
         @projects = paginate @projects
         present @projects, with: Entities::Project
       end
@@ -67,16 +72,8 @@ module API
       #   GET /projects/all
       get '/all' do
         authenticated_as_admin!
-        sort = params[:sort] == 'desc' ? 'desc' : 'asc'
-
-        @projects = case params["order_by"]
-                    when 'id' then Project.order("id #{sort}")
-                    when 'name' then Project.order("name #{sort}")
-                    when 'created_at' then Project.order("created_at #{sort}")
-                    when 'last_activity_at' then Project.order("last_activity_at #{sort}")
-                    else Project
-                    end
-
+        @projects = Project.all
+        @projects = filter_projects(@projects)
         @projects = paginate @projects
         present @projects, with: Entities::Project
       end