Merge branch 'pedropombeiro/372117/revert-remove-default-sort-order' into 'master'

Revert "Merge branch 'pedropombeiro/372117/remove-default-sort-order' into 'master'"

See merge request !119082



Merged-by: Miguel Rincon <mrincon@gitlab.com>
Approved-by: Miguel Rincon <mrincon@gitlab.com>
Approved-by: Fiona Neill <fneill@gitlab.com>
Approved-by: Tarun Vellishetty <tvellishetty@gitlab.com>
Approved-by: Stan Hu <stanhu@gmail.com>
Reviewed-by: Pedro Pombeiro <noreply@pedro.pombei.ro>
Co-authored-by: Pedro Pombeiro <noreply@pedro.pombei.ro>

app/assets/javascripts/ci/runner/components/runner_projects.vue

@@ -71,7 +71,6 @@ export default {
       return {
         id: runner.id,
         search: search.length >= SHORT_SEARCH_LENGTH ? search : '',
-        sort: 'ID_ASC',
         ...getPaginationVariables(this.pagination, RUNNER_DETAILS_PROJECTS_PAGE_SIZE),
       };
     },

app/assets/javascripts/ci/runner/graphql/show/runner_projects.query.graphql

@@ -3,7 +3,6 @@
 query getRunnerProjects(
   $id: CiRunnerID!
   $search: String
-  $sort: String
   $first: Int
   $last: Int
   $before: String
@@ -15,14 +14,7 @@ query getRunnerProjects(
       id
     }
     projectCount
-    projects(
-      search: $search
-      sort: $sort
-      first: $first
-      last: $last
-      before: $before
-      after: $after
-    ) {
+    projects(search: $search, first: $first, last: $last, before: $before, after: $after) {
       nodes {
         id
         avatarUrl

app/graphql/resolvers/ci/runner_projects_resolver.rb

@@ -13,6 +13,17 @@ class RunnerProjectsResolver < BaseResolver
 
       alias_method :runner, :object
 
+      argument :sort, GraphQL::Types::String,
+               required: false,
+               default_value: 'id_asc', # TODO: Remove in %17.0 and move :sort to ProjectSearchArguments, see https://gitlab.com/gitlab-org/gitlab/-/issues/372117
+               deprecated: {
+                 reason: 'Default sort order will change in GitLab 17.0. ' \
+                   'Specify `"id_asc"` if you require the query results to be ordered by ascending IDs',
+                 milestone: '15.4'
+               },
+               description: "Sort order of results. Format: `<field_name>_<sort_direction>`, " \
+                 "for example: `id_desc` or `name_asc`"
+
       def resolve_with_lookahead(**args)
         return unless runner.project_type?
 

app/graphql/resolvers/concerns/project_search_arguments.rb

@@ -19,12 +19,6 @@ module ProjectSearchArguments
     argument :topics, type: [GraphQL::Types::String],
                       required: false,
                       description: 'Filter projects by topics.'
-
-    argument :sort, GraphQL::Types::String,
-             required: false,
-             default_value: 'id_desc',
-             description: "Sort order of results. Format: `<field_name>_<sort_direction>`, " \
-                          "for example: `id_desc` or `name_asc`"
   end
 
   private

app/graphql/resolvers/projects_resolver.rb

@@ -10,6 +10,11 @@ class ProjectsResolver < BaseResolver
              required: false,
              description: 'Filter projects by IDs.'
 
+    argument :sort, GraphQL::Types::String,
+             required: false,
+             description: "Sort order of results. Format: `<field_name>_<sort_direction>`, " \
+                 "for example: `id_desc` or `name_asc`"
+
     argument :with_issues_enabled, GraphQL::Types::Boolean,
              required: false,
              description: "Return only projects with issues enabled."

data/deprecations/16-0-CiRunner-projects-default-sort.yml

+#
+# REQUIRED FIELDS
+#
+- title: "CiRunner.projects default sort is changing to `id_desc`"  # (required) Clearly explain the change, or planned change. For example, "The `confidential` field for a `Note` is deprecated" or "CI/CD job names will be limited to 250 characters."
+  removal_milestone: "17.0"  # (required) The milestone when this feature is planned to be removed
+  announcement_milestone: "16.0"  # (required) The milestone when this feature was first announced as deprecated.
+  breaking_change: true  # (required) Change to false if this is not a breaking change.
+  reporter: pedropombeiro  # (required) GitLab username of the person reporting the change
+  stage: Verify  # (required) String value of the stage that the feature was created in. e.g., Growth
+  issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/372117 # (required) Link to the deprecation issue in GitLab
+  body: |  # (required) Do not modify this line, instead modify the lines below.
+    The `CiRunner.projects`'s field default sort order value will change from `id_asc` to `id_desc`.
+    If you rely on the order of the returned projects to be `id_asc`, change your scripts to make the choice explicit.
+#
+# OPTIONAL END OF SUPPORT FIELDS
+#
+# If an End of Support period applies, the announcement should be shared with GitLab Support
+# in the `#spt_managers` channel in Slack, and mention `@gitlab-com/support` in this MR.
+#
+  end_of_support_milestone:  # (optional) Use "XX.YY" format. The milestone when support for this feature will end.
+  #
+  # OTHER OPTIONAL FIELDS
+  #
+  tiers:  # (optional - may be required in the future) An array of tiers that the feature is available in currently.  e.g., [Free, Silver, Gold, Core, Premium, Ultimate]
+  documentation_url: https://docs.gitlab.com/ee/api/graphql/reference/index.html#cirunnerprojects # (optional) This is a link to the current documentation page
+  image_url:  # (optional) This is a link to a thumbnail image depicting the feature
+  video_url:  # (optional) Use the youtube thumbnail URL with the structure of https://img.youtube.com/vi/UNIQUEID/hqdefault.jpg

doc/api/graphql/reference/index.md

@@ -12253,7 +12253,7 @@ four standard [pagination arguments](#connection-pagination-arguments):
 | <a id="cirunnerprojectsmembership"></a>`membership` | [`Boolean`](#boolean) | Return only projects that the current user is a member of. |
 | <a id="cirunnerprojectssearch"></a>`search` | [`String`](#string) | Search query, which can be for the project name, a path, or a description. |
 | <a id="cirunnerprojectssearchnamespaces"></a>`searchNamespaces` | [`Boolean`](#boolean) | Include namespace in project search. |
-| <a id="cirunnerprojectssort"></a>`sort` | [`String`](#string) | Sort order of results. Format: `<field_name>_<sort_direction>`, for example: `id_desc` or `name_asc`. |
+| <a id="cirunnerprojectssort"></a>`sort` **{warning-solid}** | [`String`](#string) | **Deprecated** in 15.4. Default sort order will change in GitLab 17.0. Specify `"id_asc"` if you require the query results to be ordered by ascending IDs. |
 | <a id="cirunnerprojectstopics"></a>`topics` | [`[String!]`](#string) | Filter projects by topics. |
 ##### `CiRunner.status`

doc/update/deprecations.md

@@ -107,6 +107,20 @@ Due to limited customer usage, Browser Performance Testing is deprecated and wil
 
 <div class="deprecation breaking-change" data-milestone="17.0">
 
+### CiRunner.projects default sort is changing to `id_desc`
+
+<div class="deprecation-notes">
+- Announced in: GitLab <span class="milestone">16.0</span>
+- [Breaking change](https://docs.gitlab.com/ee/development/deprecation_guidelines/)
+</div>
+
+The `CiRunner.projects`'s field default sort order value will change from `id_asc` to `id_desc`.
+If you rely on the order of the returned projects to be `id_asc`, change your scripts to make the choice explicit.
+
+</div>
+
+<div class="deprecation breaking-change" data-milestone="17.0">
+
 ### DAST ZAP advanced configuration variables deprecation
 
 <div class="deprecation-notes">

spec/frontend/ci/runner/components/runner_projects_spec.js

@@ -67,7 +67,6 @@ describe('RunnerProjects', () => {
     expect(mockRunnerProjectsQuery).toHaveBeenCalledWith({
       id: mockRunner.id,
       search: '',
-      sort: 'ID_ASC',
       first: RUNNER_DETAILS_PROJECTS_PAGE_SIZE,
     });
   });
@@ -109,6 +108,7 @@ describe('RunnerProjects', () => {
         name,
         fullName: nameWithNamespace,
         avatarUrl,
+        isOwner: true, // first project is always owner
       });
     });
 
@@ -124,7 +124,6 @@ describe('RunnerProjects', () => {
         expect(mockRunnerProjectsQuery).toHaveBeenLastCalledWith({
           id: mockRunner.id,
           search: '',
-          sort: 'ID_ASC',
           first: RUNNER_DETAILS_PROJECTS_PAGE_SIZE,
           after: 'AFTER_CURSOR',
         });
@@ -139,7 +138,6 @@ describe('RunnerProjects', () => {
         expect(mockRunnerProjectsQuery).toHaveBeenLastCalledWith({
           id: mockRunner.id,
           search: '',
-          sort: 'ID_ASC',
           last: RUNNER_DETAILS_PROJECTS_PAGE_SIZE,
           before: 'BEFORE_CURSOR',
         });
@@ -153,7 +151,6 @@ describe('RunnerProjects', () => {
         expect(mockRunnerProjectsQuery).toHaveBeenLastCalledWith({
           id: mockRunner.id,
           search: 'my search',
-          sort: 'ID_ASC',
           first: RUNNER_DETAILS_PROJECTS_PAGE_SIZE,
         });
       });
@@ -170,7 +167,6 @@ describe('RunnerProjects', () => {
         expect(mockRunnerProjectsQuery).toHaveBeenLastCalledWith({
           id: mockRunner.id,
           search: 'my search',
-          sort: 'ID_ASC',
           first: RUNNER_DETAILS_PROJECTS_PAGE_SIZE,
         });
       });

spec/graphql/resolvers/ci/runner_projects_resolver_spec.rb

@@ -16,7 +16,7 @@
 
   describe '#resolve' do
     context 'with authorized user', :enable_admin_mode do
-      let_it_be(:current_user) { create(:user, :admin) }
+      let(:current_user) { create(:user, :admin) }
 
       context 'with search argument' do
         let(:args) { { search: 'Project1.' } }
@@ -69,15 +69,15 @@
       end
 
       context 'without arguments' do
-        it 'returns a lazy value with all projects sorted by :id_desc' do
+        it 'returns a lazy value with all projects sorted by :id_asc' do
           expect(subject).to be_a(GraphQL::Execution::Lazy)
-          expect(subject.value.items).to eq([project3, project2, project1])
+          expect(subject.value.items).to eq([project1, project2, project3])
         end
       end
     end
 
     context 'with unauthorized user' do
-      let_it_be(:current_user) { create(:user) }
+      let(:current_user) { create(:user) }
 
       it { is_expected.to be_nil }
     end

spec/requests/api/graphql/ci/runner_spec.rb

@@ -628,8 +628,8 @@
           'projectCount' => 2,
           'projects' => {
             'nodes' => [
-              a_graphql_entity_for(project2),
-              a_graphql_entity_for(project1)
+              a_graphql_entity_for(project1),
+              a_graphql_entity_for(project2)
             ]
           })
         expect(runner2_data).to match a_hash_including(