Refactor and improve sorting objects in API for projects, issues and merge requests

8 files changed, 101 insertions, 63 deletions

diff --git a/doc/api/issues.md b/doc/api/issues.md
index 8d073c46d33..5a2f6a4c229 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -18,6 +18,8 @@ Parameters:
 
 - `state` (optional) - Return `all` issues or just those that are `opened` or `closed`
 - `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`
 
 ```json
 [
@@ -105,6 +107,8 @@ Parameters:
 - `state` (optional) - Return `all` issues or just those that are `opened` or `closed`
 - `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`
 
 ## Single issue
 
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index acae55d07ef..1f3fd26a241 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -2,7 +2,9 @@
 
 ## List merge requests
 
-Get all merge requests for this project. The `state` parameter can be used to get only merge requests with a given state (`opened`, `closed`, or `merged`) or all of them (`all`). The pagination parameters `page` and `per_page` can be used to restrict the list of merge requests.
+Get all merge requests for this project. 
+The `state` parameter can be used to get only merge requests with a given state (`opened`, `closed`, or `merged`) or all of them (`all`). 
+The pagination parameters `page` and `per_page` can be used to restrict the list of merge requests.
 
 ```
 GET /projects/:id/merge_requests
@@ -14,8 +16,8 @@ Parameters:
 
 - `id` (required) - The ID of a project
 - `state` (optional) - Return `all` requests or just those that are `merged`, `opened` or `closed`
-- `order_by` (optional) - Return requests ordered by `created_at` or `updated_at` fields
-- `sort` (optional) - Return requests sorted in `asc` or `desc` order
+- `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`
 
 ```json
 [
diff --git a/doc/api/projects.md b/doc/api/projects.md
index 559d35d316a..454f6fa2e91 100644
--- a/doc/api/projects.md
+++ b/doc/api/projects.md
@@ -11,8 +11,8 @@ GET /projects
 Parameters:
 
 - `archived` (optional) - if passed, limit by archived status
-- `order_by` (optional) - Return requests ordered by `id`, `name`, `created_at` or `last_activity_at` fields
-- `sort` (optional) - Return requests sorted in `asc` or `desc` order
+- `order_by` (optional) - Return requests ordered by `id`, `name`, `path`, `created_at`, `updated_at` or `last_activity_at` fields. Default is `created_at`
+- `sort` (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
 - `search` (optional) - Return list of authorized projects according to a search criteria
 
 ```json
@@ -98,6 +98,13 @@ Get a list of projects which are owned by the authenticated user.
 GET /projects/owned
 ```
 
+Parameters:
+
+- `archived` (optional) - if passed, limit by archived status
+- `order_by` (optional) - Return requests ordered by `id`, `name`, `path`, `created_at`, `updated_at` or `last_activity_at` fields. Default is `created_at`
+- `sort` (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
+- `search` (optional) - Return list of authorized projects according to a search criteria
+
 ### List ALL projects
 
 Get a list of all GitLab projects (admin only).
@@ -106,6 +113,13 @@ Get a list of all GitLab projects (admin only).
 GET /projects/all
 ```
 
+Parameters:
+
+- `archived` (optional) - if passed, limit by archived status
+- `order_by` (optional) - Return requests ordered by `id`, `name`, `path`, `created_at`, `updated_at` or `last_activity_at` fields. Default is `created_at`
+- `sort` (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
+- `search` (optional) - Return list of authorized projects according to a search criteria
+
 ### Get single project
 
 Get a specific project, identified by project ID or NAMESPACE/PROJECT_NAME, which is owned by the authenticated user.
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 e2c2cd4c3da..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,7 +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.reorder(issuable_order_by => issuable_sort)
         present paginate(issues), with: Entities::Issue
       end
     end
@@ -52,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
@@ -66,10 +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.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
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 5795082f5cb..0870a298eff 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -28,6 +28,7 @@ describe API::API, api: true  do
         json_response.length.should == 3
         json_response.first['title'].should == merge_request.title
       end
+
       it "should return an array of all merge_requests" do
         get api("/projects/#{project.id}/merge_requests?state", user)
         response.status.should == 200
@@ -35,6 +36,7 @@ describe API::API, api: true  do
         json_response.length.should == 3
         json_response.first['title'].should == merge_request.title
       end
+
       it "should return an array of open merge_requests" do
         get api("/projects/#{project.id}/merge_requests?state=opened", user)
         response.status.should == 200
@@ -42,6 +44,7 @@ describe API::API, api: true  do
         json_response.length.should == 1
         json_response.first['title'].should == merge_request.title
       end
+
       it "should return an array of closed merge_requests" do
         get api("/projects/#{project.id}/merge_requests?state=closed", user)
         response.status.should == 200
@@ -50,6 +53,7 @@ describe API::API, api: true  do
         json_response.first['title'].should == merge_request_closed.title
         json_response.second['title'].should == merge_request_merged.title
       end
+
       it "should return an array of merged merge_requests" do
         get api("/projects/#{project.id}/merge_requests?state=merged", user)
         response.status.should == 200