Automatic merge of gitlab-org/gitlab-ce master

app/validators/url_validator.rb

@@ -93,6 +93,6 @@ class UrlValidator < ActiveModel::EachValidator
   end
 
   def allow_setting_local_requests?
-    ApplicationSetting.current&.allow_local_requests_from_hooks_and_services?
+    Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services?
   end
 end

changelogs/unreleased/54796-api-sort-tie-breaker-for-pagination.yml

+---
+title: 'API: Sort tie breaker with id DESC'
+merge_request: 25311
+author: Nermin Vehabovic
+type: changed

doc/user/admin_area/settings/index.md

@@ -15,6 +15,10 @@ include:
 - [Visibility and access controls](visibility_and_access_controls.md)
 - [Custom templates repository](instance_template_repository.md)
 
+NOTE: **Note:**
+You can change the [first day of the week](../../profile/preferences.md) for the entire GitLab instance
+in the **Localization** section of **Admin area > Settings > Preferences**.
+
 ## GitLab.com admin area settings
 
 Most of the settings under the admin area change the behavior of the whole

lib/api/helpers.rb

@@ -312,6 +312,12 @@ module API
       items.search(text)
     end
 
+    def order_options_with_tie_breaker
+      order_options = { params[:order_by] => params[:sort] }
+      order_options['id'] ||= 'desc'
+      order_options
+    end
+
     # error helpers
 
     def forbidden!(reason = nil)
@@ -406,7 +412,7 @@ module API
 
     # rubocop: disable CodeReuse/ActiveRecord
     def reorder_projects(projects)
-      projects.reorder(params[:order_by] => params[:sort])
+      projects.reorder(order_options_with_tie_breaker)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 

lib/api/issues.rb

@@ -31,8 +31,7 @@ module API
 
         issues = IssuesFinder.new(current_user, args).execute
           .preload(:assignees, :labels, :notes, :timelogs, :project, :author, :closed_by)
-
-        issues.reorder(args[:order_by] => args[:sort])
+        issues.reorder(order_options_with_tie_breaker)
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

lib/api/merge_requests.rb

@@ -40,7 +40,7 @@ module API
         args[:scope] = args[:scope].underscore if args[:scope]
 
         merge_requests = MergeRequestsFinder.new(current_user, args).execute
-                           .reorder(args[:order_by] => args[:sort])
+                           .reorder(order_options_with_tie_breaker)
         merge_requests = paginate(merge_requests)
                            .preload(:source_project, :target_project)
 

lib/api/notes.rb

@@ -39,7 +39,7 @@ module API
           # at the DB query level (which we cannot in that case), the current
           # page can have less elements than :per_page even if
           # there's more than one page.
-          raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
+          raw_notes = noteable.notes.with_metadata.reorder(order_options_with_tie_breaker)
           notes =
             # paginate() only works with a relation. This could lead to a
             # mismatch between the pagination headers info and the actual notes

lib/api/users.rb

@@ -26,7 +26,7 @@ module API
         # rubocop: disable CodeReuse/ActiveRecord
         def reorder_users(users)
           if params[:order_by] && params[:sort]
-            users.reorder(params[:order_by] => params[:sort])
+            users.reorder(order_options_with_tie_breaker)
           else
             users
           end

spec/lib/gitlab/current_settings_spec.rb

@@ -14,9 +14,8 @@ describe Gitlab::CurrentSettings do
 
   describe '#current_application_settings', :use_clean_rails_memory_store_caching do
     it 'allows keys to be called directly' do
-      db_settings = create(:application_setting,
-        home_page_url: 'http://mydomain.com',
-        signup_enabled: false)
+      db_settings = ApplicationSetting.first || create(:application_setting)
+      db_settings.update!(home_page_url: 'http://mydomain.com', signup_enabled: false)
 
       expect(described_class.home_page_url).to eq(db_settings.home_page_url)
       expect(described_class.signup_enabled?).to be_falsey
@@ -109,7 +108,7 @@ describe Gitlab::CurrentSettings do
 
         context 'with pending migrations' do
           before do
-            expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
+            allow(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
           end
 
           shared_examples 'a non-persisted ApplicationSetting object' do

spec/models/lfs_download_object_spec.rb

@@ -46,9 +46,7 @@ describe LfsDownloadObject do
         subject { described_class.new(oid: oid, size: size, link: 'http://192.168.1.1') }
 
         before do
-          allow(ApplicationSetting)
-            .to receive(:current)
-              .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: setting))
+          stub_application_setting(allow_local_requests_from_hooks_and_services: setting)
         end
 
         context 'are allowed' do

spec/requests/api/issues_spec.rb

@@ -363,12 +363,40 @@ describe API::Issues do
         expect_paginated_array_response([])
       end
 
+      context 'without sort params' do
         it 'sorts by created_at descending by default' do
           get api('/issues', user)
 
           expect_paginated_array_response([issue.id, closed_issue.id])
         end
 
+        context 'with 2 issues with same created_at' do
+          let!(:closed_issue2) do
+            create :closed_issue,
+                   author: user,
+                   assignees: [user],
+                   project: project,
+                   milestone: milestone,
+                   created_at: closed_issue.created_at,
+                   updated_at: 1.hour.ago,
+                   title: issue_title,
+                   description: issue_description
+          end
+
+          it 'page breaks first page correctly' do
+            get api('/issues?per_page=2', user)
+
+            expect_paginated_array_response([issue.id, closed_issue2.id])
+          end
+
+          it 'page breaks second page correctly' do
+            get api('/issues?per_page=2&page=2', user)
+
+            expect_paginated_array_response([closed_issue.id])
+          end
+        end
+      end
+
       it 'sorts ascending when requested' do
         get api('/issues?sort=asc', user)
 
@@ -617,12 +645,40 @@ describe API::Issues do
         expect_paginated_array_response(group_confidential_issue.id)
       end
 
+      context 'without sort params' do
         it 'sorts by created_at descending by default' do
           get api(base_url, user)
 
           expect_paginated_array_response([group_closed_issue.id, group_confidential_issue.id, group_issue.id])
         end
 
+        context 'with 2 issues with same created_at' do
+          let!(:group_issue2) do
+            create :issue,
+                   author: user,
+                   assignees: [user],
+                   project: group_project,
+                   milestone: group_milestone,
+                   updated_at: 1.hour.ago,
+                   title: issue_title,
+                   description: issue_description,
+                   created_at: group_issue.created_at
+          end
+
+          it 'page breaks first page correctly' do
+            get api("#{base_url}?per_page=3", user)
+
+            expect_paginated_array_response([group_closed_issue.id, group_confidential_issue.id, group_issue2.id])
+          end
+
+          it 'page breaks second page correctly' do
+            get api("#{base_url}?per_page=3&page=2", user)
+
+            expect_paginated_array_response([group_issue.id])
+          end
+        end
+      end
+
       it 'sorts ascending when requested' do
         get api("#{base_url}?sort=asc", user)
 
@@ -832,12 +888,40 @@ describe API::Issues do
       expect_paginated_array_response([issue.id, closed_issue.id])
     end
 
+    context 'without sort params' do
       it 'sorts by created_at descending by default' do
         get api("#{base_url}/issues", user)
 
         expect_paginated_array_response([issue.id, confidential_issue.id, closed_issue.id])
       end
 
+      context 'with 2 issues with same created_at' do
+        let!(:closed_issue2) do
+          create :closed_issue,
+                 author: user,
+                 assignees: [user],
+                 project: project,
+                 milestone: milestone,
+                 created_at: closed_issue.created_at,
+                 updated_at: 1.hour.ago,
+                 title: issue_title,
+                 description: issue_description
+        end
+
+        it 'page breaks first page correctly' do
+          get api("#{base_url}/issues?per_page=3", user)
+
+          expect_paginated_array_response([issue.id, confidential_issue.id, closed_issue2.id])
+        end
+
+        it 'page breaks second page correctly' do
+          get api("#{base_url}/issues?per_page=3&page=2", user)
+
+          expect_paginated_array_response([closed_issue.id])
+        end
+      end
+    end
+
     it 'sorts ascending when requested' do
       get api("#{base_url}/issues", user), params: { sort: :asc }
 

spec/support/shared_examples/requests/api/merge_requests_list.rb

@@ -257,6 +257,38 @@ shared_examples 'merge requests list' do
         expect(response_dates).to eq(response_dates.sort.reverse)
       end
 
+      context '2 merge requests with equal created_at' do
+        let!(:closed_mr2) do
+          create :merge_request,
+                 state: 'closed',
+                 milestone: milestone1,
+                 author: user,
+                 assignee: user,
+                 source_project: project,
+                 target_project: project,
+                 title: "Test",
+                 created_at: @mr_earlier.created_at
+        end
+
+        it 'page breaks first page correctly' do
+          get api("#{endpoint_path}?sort=desc&per_page=4", user)
+
+          response_ids = json_response.map { |merge_request| merge_request['id'] }
+
+          expect(response_ids).to include(closed_mr2.id)
+          expect(response_ids).not_to include(@mr_earlier.id)
+        end
+
+        it 'page breaks second page correctly' do
+          get api("#{endpoint_path}?sort=desc&per_page=4&page=2", user)
+
+          response_ids = json_response.map { |merge_request| merge_request['id'] }
+
+          expect(response_ids).not_to include(closed_mr2.id)
+          expect(response_ids).to include(@mr_earlier.id)
+        end
+      end
+
       it 'returns an array of merge_requests ordered by updated_at' do
         path = endpoint_path + '?order_by=updated_at'
 

spec/support/shared_examples/requests/api/notes.rb

@@ -8,6 +8,7 @@ shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
         create_list(:note, 3, params)
       end
 
+      context 'without sort params' do
         it 'sorts by created_at in descending order by default' do
           get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user)
 
@@ -17,6 +18,37 @@ shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
           expect(response_dates).to eq(response_dates.sort.reverse)
         end
 
+        context '2 notes with equal created_at' do
+          before do
+            @first_note = Note.first
+
+            params = { noteable: noteable, author: user }
+            params[:project] = parent if parent.is_a?(Project)
+            params[:created_at] = @first_note.created_at
+
+            @note2 = create(:note, params)
+          end
+
+          it 'page breaks first page correctly' do
+            get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes?per_page=4", user)
+
+            response_ids = json_response.map { |note| note['id'] }
+
+            expect(response_ids).to include(@note2.id)
+            expect(response_ids).not_to include(@first_note.id)
+          end
+
+          it 'page breaks second page correctly' do
+            get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes?per_page=4&page=2", user)
+
+            response_ids = json_response.map { |note| note['id'] }
+
+            expect(response_ids).not_to include(@note2.id)
+            expect(response_ids).to include(@first_note.id)
+          end
+        end
+      end
+
       it 'sorts by ascending order when requested' do
         get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes?sort=asc", user)