Merge branch 'fix-api-sorting' into 'master'

Fix API issues sorting

## What does this MR do?

Fix the sorting of issues in the API.

## Are there points in the code the reviewer needs to double check?

Instead of removing the '_at' suffix manually, we could add those versions to the `Sortable` concern instead.

## Why was this MR needed?

There were a couple of bugs:

* The global and project-specific issues endpoints wouldn't sort at all.
* Group sorting would work, but only if you applied two undocumented workarounds:
  * Always pass both `order_by` and `sort` (both are optional, so only one should be needed to change ordering).
  * Instead of passing `created_at` or `updated_at`, you needed to pass `created` or `updated`.

This makes the API implementation match the docs.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/983.

See merge request !6281

CHANGELOG

@@ -16,6 +16,7 @@ v 8.12.0 (unreleased)
   - Change logo animation to CSS (ClemMakesApps)
   - Instructions for enabling Git packfile bitmaps !6104
   - Fix pagination on user snippets page
+  - Fix sorting of issues in API
   - Escape search term before passing it to Regexp.new !6241 (winniehell)
   - Fix pinned sidebar behavior in smaller viewports !6169
   - Change merge_error column from string to text type

lib/api/issues.rb

@@ -41,7 +41,8 @@ module API
         issues = current_user.issues.inc_notes_with_associations
         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)
+        issues = issues.reorder(issuable_order_by => issuable_sort)
+
         present paginate(issues), with: Entities::Issue, current_user: current_user
       end
     end
@@ -73,7 +74,11 @@ module API
         params[:group_id] = group.id
         params[:milestone_title] = params.delete(:milestone)
         params[:label_name] = params.delete(:labels)
-        params[:sort] = "#{params.delete(:order_by)}_#{params.delete(:sort)}" if params[:order_by] && params[:sort]
+
+        if params[:order_by] || params[:sort]
+          # The Sortable concern takes 'created_desc', not 'created_at_desc' (for example)
+          params[:sort] = "#{issuable_order_by.sub('_at', '')}_#{issuable_sort}"
+        end
 
         issues = IssuesFinder.new(current_user, params).execute
 
@@ -113,7 +118,8 @@ module API
           issues = filter_issues_milestone(issues, params[:milestone])
         end
 
-        issues.reorder(issuable_order_by => issuable_sort)
+        issues = issues.reorder(issuable_order_by => issuable_sort)
+
         present paginate(issues), with: Entities::Issue, current_user: current_user
       end
 

spec/requests/api/issues_spec.rb

@@ -17,21 +17,24 @@ describe API::API, api: true  do
            assignee: user,
            project: project,
            state: :closed,
-           milestone: milestone
+           milestone: milestone,
+           updated_at: 3.hours.ago
   end
   let!(:confidential_issue) do
     create :issue,
            :confidential,
            project: project,
            author: author,
-           assignee: assignee
+           assignee: assignee,
+           updated_at: 2.hours.ago
   end
   let!(:issue) do
     create :issue,
            author: user,
            assignee: user,
            project: project,
-           milestone: milestone
+           milestone: milestone,
+           updated_at: 1.hour.ago
   end
   let!(:label) do
     create(:label, title: 'label', color: '#FFAABB', project: project)
@@ -135,6 +138,42 @@ describe API::API, api: true  do
         expect(json_response).to be_an Array
         expect(json_response.length).to eq(0)
       end
+
+      it 'sorts by created_at descending by default' do
+        get api('/issues', user)
+        response_dates = json_response.map { |issue| issue['created_at'] }
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(response_dates).to eq(response_dates.sort.reverse)
+      end
+
+      it 'sorts ascending when requested' do
+        get api('/issues?sort=asc', user)
+        response_dates = json_response.map { |issue| issue['created_at'] }
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(response_dates).to eq(response_dates.sort)
+      end
+
+      it 'sorts by updated_at descending when requested' do
+        get api('/issues?order_by=updated_at', user)
+        response_dates = json_response.map { |issue| issue['updated_at'] }
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(response_dates).to eq(response_dates.sort.reverse)
+      end
+
+      it 'sorts by updated_at ascending when requested' do
+        get api('/issues?order_by=updated_at&sort=asc', user)
+        response_dates = json_response.map { |issue| issue['updated_at'] }
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(response_dates).to eq(response_dates.sort)
+      end
     end
   end
 
@@ -147,21 +186,24 @@ describe API::API, api: true  do
              assignee: user,
              project: group_project,
              state: :closed,
-             milestone: group_milestone
+             milestone: group_milestone,
+             updated_at: 3.hours.ago
     end
     let!(:group_confidential_issue) do
       create :issue,
              :confidential,
              project: group_project,
              author: author,
-             assignee: assignee
+             assignee: assignee,
+             updated_at: 2.hours.ago
     end
     let!(:group_issue) do
       create :issue,
              author: user,
              assignee: user,
              project: group_project,
-             milestone: group_milestone
+             milestone: group_milestone,
+             updated_at: 1.hour.ago
     end
     let!(:group_label) do
       create(:label, title: 'group_lbl', color: '#FFAABB', project: group_project)
@@ -278,6 +320,42 @@ describe API::API, api: true  do
       expect(json_response.length).to eq(1)
       expect(json_response.first['id']).to eq(group_closed_issue.id)
     end
+
+    it 'sorts by created_at descending by default' do
+      get api(base_url, user)
+      response_dates = json_response.map { |issue| issue['created_at'] }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_an Array
+      expect(response_dates).to eq(response_dates.sort.reverse)
+    end
+
+    it 'sorts ascending when requested' do
+      get api("#{base_url}?sort=asc", user)
+      response_dates = json_response.map { |issue| issue['created_at'] }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_an Array
+      expect(response_dates).to eq(response_dates.sort)
+    end
+
+    it 'sorts by updated_at descending when requested' do
+      get api("#{base_url}?order_by=updated_at", user)
+      response_dates = json_response.map { |issue| issue['updated_at'] }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_an Array
+      expect(response_dates).to eq(response_dates.sort.reverse)
+    end
+
+    it 'sorts by updated_at ascending when requested' do
+      get api("#{base_url}?order_by=updated_at&sort=asc", user)
+      response_dates = json_response.map { |issue| issue['updated_at'] }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_an Array
+      expect(response_dates).to eq(response_dates.sort)
+    end
   end
 
   describe "GET /projects/:id/issues" do
@@ -386,6 +464,42 @@ describe API::API, api: true  do
       expect(json_response.length).to eq(1)
       expect(json_response.first['id']).to eq(closed_issue.id)
     end
+
+    it 'sorts by created_at descending by default' do
+      get api("#{base_url}/issues", user)
+      response_dates = json_response.map { |issue| issue['created_at'] }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_an Array
+      expect(response_dates).to eq(response_dates.sort.reverse)
+    end
+
+    it 'sorts ascending when requested' do
+      get api("#{base_url}/issues?sort=asc", user)
+      response_dates = json_response.map { |issue| issue['created_at'] }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_an Array
+      expect(response_dates).to eq(response_dates.sort)
+    end
+
+    it 'sorts by updated_at descending when requested' do
+      get api("#{base_url}/issues?order_by=updated_at", user)
+      response_dates = json_response.map { |issue| issue['updated_at'] }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_an Array
+      expect(response_dates).to eq(response_dates.sort.reverse)
+    end
+
+    it 'sorts by updated_at ascending when requested' do
+      get api("#{base_url}/issues?order_by=updated_at&sort=asc", user)
+      response_dates = json_response.map { |issue| issue['updated_at'] }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_an Array
+      expect(response_dates).to eq(response_dates.sort)
+    end
   end
 
   describe "GET /projects/:id/issues/:issue_id" do