Automatic merge of gitlab-org/gitlab master

app/finders/container_repositories_finder.rb

@@ -14,7 +14,8 @@ class ContainerRepositoriesFinder
     return unless authorized?
 
     repositories = @subject.is_a?(Project) ? project_repositories : group_repositories
-    filter_by_image_name(repositories)
+    repositories = filter_by_image_name(repositories)
+    sort(repositories)
   end
 
   private
@@ -39,6 +40,12 @@ class ContainerRepositoriesFinder
     repositories.search_by_name(@params[:name])
   end
 
+  def sort(repositories)
+    return repositories unless @params[:sort]
+
+    repositories.order_by(@params[:sort])
+  end
+
   def authorized?
     Ability.allowed?(@user, :read_container_image, @subject)
   end

app/graphql/resolvers/container_repositories_resolver.rb

@@ -10,8 +10,13 @@ module Resolvers
               required: false,
               description: 'Filter the container repositories by their name.'
 
-    def resolve(name: nil)
-      ContainerRepositoriesFinder.new(user: current_user, subject: object, params: { name: name })
+    argument :sort, Types::ContainerRepositorySortEnum,
+             description: 'Sort container repositories by this criteria.',
+             required: false,
+             default_value: :created_desc
+
+    def resolve(name: nil, sort: nil)
+      ContainerRepositoriesFinder.new(user: current_user, subject: object, params: { name: name, sort: sort })
                                  .execute
                                  .tap { track_event(:list_repositories, :container) }
     end

app/graphql/types/container_repository_sort_enum.rb

+# frozen_string_literal: true
+
+module Types
+  class ContainerRepositorySortEnum < SortEnum
+    graphql_name 'ContainerRepositorySort'
+    description 'Values for sorting container repositories'
+
+    value 'NAME_ASC', 'Name by ascending order', value: :name_asc
+    value 'NAME_DESC', 'Name by descending order', value: :name_desc
+  end
+end

app/models/container_repository.rb

@@ -4,6 +4,7 @@ class ContainerRepository < ApplicationRecord
   include Gitlab::Utils::StrongMemoize
   include Gitlab::SQL::Pattern
   include EachBatch
+  include Sortable
 
   WAITING_CLEANUP_STATUSES = %i[cleanup_scheduled cleanup_unfinished].freeze
 

app/models/note.rb

@@ -542,7 +542,7 @@ class Note < ApplicationRecord
   end
 
   def skip_notification?
-    review.present?
+    review.present? || author.ghost?
   end
 
   private

app/workers/new_note_worker.rb

@@ -13,7 +13,7 @@ class NewNoteWorker # rubocop:disable Scalability/IdempotentWorker
   # rubocop: disable CodeReuse/ActiveRecord
   def perform(note_id, _params = {})
     if note = Note.find_by(id: note_id)
-      NotificationService.new.new_note(note) unless note.skip_notification? || note.author.ghost?
+      NotificationService.new.new_note(note) unless note.skip_notification?
       Notes::PostProcessService.new(note).execute
     else
       Gitlab::AppLogger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")

changelogs/unreleased/290302-sort-container-repositories.yml

+---
+title: Add sort argument to container_repositories graphql resolver
+merge_request: 53404
+author:
+type: changed

doc/api/graphql/reference/gitlab_schema.graphql

@@ -4427,6 +4427,61 @@ Identifier of ContainerRepository.
 """
 scalar ContainerRepositoryID
 
+"""
+Values for sorting container repositories
+"""
+enum ContainerRepositorySort {
+  """
+  Created at ascending order
+  """
+  CREATED_ASC
+
+  """
+  Created at descending order
+  """
+  CREATED_DESC
+
+  """
+  Name by ascending order
+  """
+  NAME_ASC
+
+  """
+  Name by descending order
+  """
+  NAME_DESC
+
+  """
+  Updated at ascending order
+  """
+  UPDATED_ASC
+
+  """
+  Updated at descending order
+  """
+  UPDATED_DESC
+
+  """
+  Created at ascending order
+  """
+  created_asc @deprecated(reason: "Use CREATED_ASC. Deprecated in 13.5.")
+
+  """
+  Created at descending order
+  """
+  created_desc @deprecated(reason: "Use CREATED_DESC. Deprecated in 13.5.")
+
+  """
+  Updated at ascending order
+  """
+  updated_asc @deprecated(reason: "Use UPDATED_ASC. Deprecated in 13.5.")
+
+  """
+  Updated at descending order
+  """
+  updated_desc @deprecated(reason: "Use UPDATED_DESC. Deprecated in 13.5.")
+}
+
 """
 Status of a container repository
 """
@@ -10650,6 +10705,11 @@ type Group {
     Filter the container repositories by their name.
     """
     name: String
+
+    """
+    Sort container repositories by this criteria.
+    """
+    sort: ContainerRepositorySort = created_desc
   ): ContainerRepositoryConnection
 
   """
@@ -18690,6 +18750,11 @@ type Project {
     Filter the container repositories by their name.
     """
     name: String
+
+    """
+    Sort container repositories by this criteria.
+    """
+    sort: ContainerRepositorySort = created_desc
   ): ContainerRepositoryConnection
 
   """

doc/api/graphql/reference/gitlab_schema.json

@@ -12061,6 +12061,77 @@
           "enumValues": null,
           "possibleTypes": null
         },
+        {
+          "kind": "ENUM",
+          "name": "ContainerRepositorySort",
+          "description": "Values for sorting container repositories",
+          "fields": null,
+          "inputFields": null,
+          "interfaces": null,
+          "enumValues": [
+            {
+              "name": "updated_desc",
+              "description": "Updated at descending order",
+              "isDeprecated": true,
+              "deprecationReason": "Use UPDATED_DESC. Deprecated in 13.5."
+            },
+            {
+              "name": "updated_asc",
+              "description": "Updated at ascending order",
+              "isDeprecated": true,
+              "deprecationReason": "Use UPDATED_ASC. Deprecated in 13.5."
+            },
+            {
+              "name": "created_desc",
+              "description": "Created at descending order",
+              "isDeprecated": true,
+              "deprecationReason": "Use CREATED_DESC. Deprecated in 13.5."
+            },
+            {
+              "name": "created_asc",
+              "description": "Created at ascending order",
+              "isDeprecated": true,
+              "deprecationReason": "Use CREATED_ASC. Deprecated in 13.5."
+            },
+            {
+              "name": "UPDATED_DESC",
+              "description": "Updated at descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "UPDATED_ASC",
+              "description": "Updated at ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "CREATED_DESC",
+              "description": "Created at descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "CREATED_ASC",
+              "description": "Created at ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "NAME_ASC",
+              "description": "Name by ascending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "NAME_DESC",
+              "description": "Name by descending order",
+              "isDeprecated": false,
+              "deprecationReason": null
+            }
+          ],
+          "possibleTypes": null
+        },
         {
           "kind": "ENUM",
           "name": "ContainerRepositoryStatus",
@@ -29330,6 +29401,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "sort",
+                  "description": "Sort container repositories by this criteria.",
+                  "type": {
+                    "kind": "ENUM",
+                    "name": "ContainerRepositorySort",
+                    "ofType": null
+                  },
+                  "defaultValue": "created_desc"
+                },
                 {
                   "name": "after",
                   "description": "Returns the elements in the list that come after the specified cursor.",
@@ -54963,6 +55044,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "sort",
+                  "description": "Sort container repositories by this criteria.",
+                  "type": {
+                    "kind": "ENUM",
+                    "name": "ContainerRepositorySort",
+                    "ofType": null
+                  },
+                  "defaultValue": "created_desc"
+                },
                 {
                   "name": "after",
                   "description": "Returns the elements in the list that come after the specified cursor.",

doc/api/graphql/reference/index.md

@@ -4698,6 +4698,23 @@ Status of the tags cleanup of a container repository.
 | `UNFINISHED` | The tags cleanup has been partially executed. There are still remaining tags to delete. |
 | `UNSCHEDULED` | The tags cleanup is not scheduled. This is the default state. |
 
+### ContainerRepositorySort
+
+Values for sorting container repositories.
+
+| Value | Description |
+| ----- | ----------- |
+| `CREATED_ASC` | Created at ascending order |
+| `CREATED_DESC` | Created at descending order |
+| `NAME_ASC` | Name by ascending order |
+| `NAME_DESC` | Name by descending order |
+| `UPDATED_ASC` | Updated at ascending order |
+| `UPDATED_DESC` | Updated at descending order |
+| `created_asc` **{warning-solid}** | **Deprecated:** Use CREATED_ASC. Deprecated in 13.5. |
+| `created_desc` **{warning-solid}** | **Deprecated:** Use CREATED_DESC. Deprecated in 13.5. |
+| `updated_asc` **{warning-solid}** | **Deprecated:** Use UPDATED_ASC. Deprecated in 13.5. |
+| `updated_desc` **{warning-solid}** | **Deprecated:** Use UPDATED_DESC. Deprecated in 13.5. |
+
 ### ContainerRepositoryStatus
 
 Status of a container repository.

doc/development/documentation/index.md

@@ -207,7 +207,7 @@ To add a redirect:
    1. Assign the MR to a technical writer for review and merge.
 1. If the redirect is to one of the 4 internal docs projects (not an external URL),
    create an MR in [`gitlab-docs`](https://gitlab.com/gitlab-org/gitlab-docs):
-   1. Update [`redirects.yaml`](https://gitlab.com/gitlab-org/gitlab-docs/-/blob/master/content/_data/redirects.yaml)
+   1. Update [`content/_data/redirects.yaml`](https://gitlab.com/gitlab-org/gitlab-docs/-/blob/master/content/_data/redirects.yaml)
       with one redirect entry for each renamed or moved file. This code works for
       <https://docs.gitlab.com> links only. Keep them alphabetically sorted:
 

ee/app/assets/javascripts/environments/components/environment_alert.vue

@@ -53,7 +53,7 @@ export default {
       return this.alert?.prometheusAlert?.humanizedText;
     },
     severity() {
-      return this.alert?.severity || '';
+      return this.alert?.severity;
     },
   },
   classes: [
@@ -71,7 +71,7 @@ export default {
   <div v-if="alert" :class="$options.classes" data-testid="alert">
     <gl-sprintf :message="$options.translations.alertText">
       <template #severity>
-        <severity-badge :severity="severity" class="gl-display-inline" />
+        <severity-badge v-if="severity" :severity="severity" class="gl-display-inline" />
       </template>
       <template #startedAt>
         <span v-gl-tooltip :title="tooltipTitle(alert.startedAt)">

ee/app/assets/javascripts/security_dashboard/components/security_dashboard_table_row.vue

@@ -43,9 +43,6 @@ export default {
   computed: {
     ...mapState(['dashboardType']),
     ...mapState('vulnerabilities', ['selectedVulnerabilities']),
-    severity() {
-      return this.vulnerability.severity || ' ';
-    },
     vulnerabilityIdentifier() {
       return getPrimaryIdentifier(this.vulnerability.identifiers, 'external_type');
     },
@@ -126,7 +123,11 @@ export default {
     <div class="table-section section-15">
       <div class="table-mobile-header" role="rowheader">{{ s__('Reports|Severity') }}</div>
       <div class="table-mobile-content">
-        <severity-badge :severity="severity" class="text-right text-md-left" />
+        <severity-badge
+          v-if="vulnerability.severity"
+          :severity="vulnerability.severity"
+          class="text-right text-md-left"
+        />
       </div>
     </div>
 

ee/app/assets/javascripts/vue_shared/security_reports/components/severity_badge.vue

@@ -22,7 +22,7 @@ export default {
       return Object.keys(SEVERITY_CLASS_NAME_MAP).includes(this.severityKey);
     },
     severityKey() {
-      return this.severity.toLowerCase();
+      return this.severity?.toLowerCase();
     },
     className() {
       return SEVERITY_CLASS_NAME_MAP[this.severityKey];

ee/spec/frontend/environments/environment_alert_spec.js

@@ -23,6 +23,8 @@ describe('Environment Alert', () => {
     });
   };
 
+  const findSeverityBadge = () => wrapper.find(SeverityBadge);
+
   beforeEach(() => {
     factory();
   });
@@ -59,14 +61,17 @@ describe('Environment Alert', () => {
       expect(link.attributes('href')).toBe('/alert/details');
     });
 
-    it('should show a severity badge', () => {
-      expect(wrapper.find(SeverityBadge).props('severity')).toBe('CRITICAL');
+    it('should show a severity badge with the correct severity', () => {
+      const badge = findSeverityBadge();
+      expect(badge.exists()).toBe(true);
+      expect(badge.props('severity')).toBe('CRITICAL');
     });
   });
 
   describe('has no alert', () => {
     it('should display nothing', () => {
       expect(wrapper.find('[data-testid="alert"]').exists()).toBe(false);
+      expect(findSeverityBadge().exists()).toBe(false);
     });
   });
 });

ee/spec/frontend/security_dashboard/components/security_dashboard_table_row_spec.js

@@ -6,6 +6,7 @@ import { VULNERABILITY_MODAL_ID } from 'ee/vue_shared/security_reports/component
 import createStore from 'ee/security_dashboard/store';
 import { DASHBOARD_TYPES } from 'ee/security_dashboard/store/constants';
 import { trimText } from 'helpers/text_helper';
+import SeverityBadge from 'ee/vue_shared/security_reports/components/severity_badge.vue';
 import { BV_SHOW_MODAL } from '~/lib/utils/constants';
 import mockDataVulnerabilities from '../store/modules/vulnerabilities/data/mock_data_vulnerabilities';
 
@@ -41,6 +42,7 @@ describe('Security Dashboard Table Row', () => {
   const findAllIssueCreated = () => wrapper.findAll('[data-testid="issues-icon"]');
   const hasSelectedClass = () => wrapper.classes('gl-bg-blue-50');
   const findCheckbox = () => wrapper.find(GlFormCheckbox);
+  const findSeverityBadge = () => wrapper.find(SeverityBadge);
 
   describe('when loading', () => {
     beforeEach(() => {
@@ -51,9 +53,8 @@ describe('Security Dashboard Table Row', () => {
       expect(findLoader().exists()).toBeTruthy();
     });
 
-    it('should render a ` ` for severity', () => {
-      expect(wrapper.vm.severity).toEqual(' ');
-      expect(findContent(0).text()).toEqual('');
+    it('should not render the severity', () => {
+      expect(findSeverityBadge().exists()).toBe(false);
     });
 
     it('should render a `` for the report type and scanner', () => {
@@ -78,7 +79,7 @@ describe('Security Dashboard Table Row', () => {
     });
 
     it('should render the severity', () => {
-      expect(findContent(0).text().toLowerCase()).toContain(vulnerability.severity);
+      expect(findSeverityBadge().text().toLowerCase()).toBe(vulnerability.severity);
     });
 
     it('should render the identifier cell', () => {

spec/finders/container_repositories_finder_spec.rb

@@ -32,6 +32,34 @@ RSpec.describe ContainerRepositoriesFinder do
     end
   end
 
+  shared_examples 'with sorting' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project) { create(:project, group: group) }
+    let_it_be(:sort_repository) do
+      create(:container_repository, name: 'bar', project: project, created_at: 1.day.ago)
+    end
+
+    let_it_be(:sort_repository2) do
+      create(:container_repository, name: 'foo', project: project, created_at: 1.hour.ago, updated_at: 1.hour.ago)
+    end
+
+    [:created_desc, :updated_asc, :name_desc].each do |order|
+      context "with sort set to #{order}" do
+        let(:params) { { sort: order } }
+
+        it { is_expected.to eq([sort_repository2, sort_repository])}
+      end
+    end
+
+    [:created_asc, :updated_desc, :name_asc].each do |order|
+      context "with sort set to #{order}" do
+        let(:params) { { sort: order } }
+
+        it { is_expected.to eq([sort_repository, sort_repository2])}
+      end
+    end
+  end
+
   describe '#execute' do
     context 'with authorized user' do
       subject { described_class.new(user: reporter, subject: subject_object, params: params).execute }
@@ -47,6 +75,7 @@ RSpec.describe ContainerRepositoriesFinder do
         it { is_expected.to match_array([project_repository, other_repository]) }
 
         it_behaves_like 'with name search'
+        it_behaves_like 'with sorting'
       end
 
       context 'when subject_type is project' do
@@ -55,6 +84,7 @@ RSpec.describe ContainerRepositoriesFinder do
         it { is_expected.to match_array([project_repository]) }
 
         it_behaves_like 'with name search'
+        it_behaves_like 'with sorting'
       end
 
       context 'with invalid subject_type' do

spec/graphql/resolvers/container_repositories_resolver_spec.rb

@@ -27,6 +27,34 @@ RSpec.describe Resolvers::ContainerRepositoriesResolver do
 
         it { is_expected.to contain_exactly(named_container_repository) }
       end
+
+      context 'with a sort argument' do
+        let_it_be(:group) { create(:group) }
+        let_it_be(:project) { create(:project, group: group) }
+        let_it_be(:sort_repository) do
+          create(:container_repository, name: 'bar', project: project, created_at: 1.day.ago)
+        end
+
+        let_it_be(:sort_repository2) do
+          create(:container_repository, name: 'foo', project: project, created_at: 1.hour.ago, updated_at: 1.hour.ago)
+        end
+
+        [:created_desc, :updated_asc, :name_desc].each do |order|
+          context "#{order}" do
+            let(:args) { { sort: order } }
+
+            it { is_expected.to eq([sort_repository2, sort_repository]) }
+          end
+        end
+
+        [:created_asc, :updated_desc, :name_asc].each do |order|
+          context "#{order}" do
+            let(:args) { { sort: order } }
+
+            it { is_expected.to eq([sort_repository, sort_repository2]) }
+          end
+        end
+      end
     end
 
     context 'with authorized user' do

spec/graphql/types/container_repository_sort_enum_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GitlabSchema.types['ContainerRepositorySort'] do
+  specify { expect(described_class.graphql_name).to eq('ContainerRepositorySort') }
+
+  it_behaves_like 'common sort values'
+
+  it 'exposes all the existing issue sort values' do
+    expect(described_class.values.keys).to include(
+      *%w[NAME_ASC NAME_DESC]
+    )
+  end
+end

spec/requests/api/graphql/project/container_repositories_spec.rb

@@ -156,4 +156,51 @@ RSpec.describe 'getting container repositories in a project' do
 
     expect(container_repositories_count_response).to eq(container_repositories.size)
   end
+
+  describe 'sorting and pagination' do
+    let_it_be(:data_path) { [:project, :container_repositories] }
+    let_it_be(:sort_project) { create(:project, :public) }
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:container_repository1) { create(:container_repository, name: 'b', project: sort_project) }
+    let_it_be(:container_repository2) { create(:container_repository, name: 'a', project: sort_project) }
+    let_it_be(:container_repository3) { create(:container_repository, name: 'd', project: sort_project) }
+    let_it_be(:container_repository4) { create(:container_repository, name: 'c', project: sort_project) }
+    let_it_be(:container_repository5) { create(:container_repository, name: 'e', project: sort_project) }
+
+    before do
+      stub_container_registry_tags(repository: container_repository1.path, tags: %w(tag1 tag1 tag3), with_manifest: false)
+      stub_container_registry_tags(repository: container_repository2.path, tags: %w(tag4 tag5 tag6), with_manifest: false)
+      stub_container_registry_tags(repository: container_repository3.path, tags: %w(tag7 tag8), with_manifest: false)
+      stub_container_registry_tags(repository: container_repository4.path, tags: %w(tag9), with_manifest: false)
+      stub_container_registry_tags(repository: container_repository5.path, tags: %w(tag10 tag11), with_manifest: false)
+    end
+
+    def pagination_query(params)
+      graphql_query_for(:project, { full_path: sort_project.full_path },
+        query_nodes(:container_repositories, :name, include_pagination_info: true, args: params)
+      )
+    end
+
+    def pagination_results_data(data)
+      data.map { |container_repository| container_repository.dig('name') }
+    end
+
+    context 'when sorting by name' do
+      context 'when ascending' do
+        it_behaves_like 'sorted paginated query' do
+          let(:sort_param) { :NAME_ASC }
+          let(:first_param) { 2 }
+          let(:expected_results) { [container_repository2.name, container_repository1.name, container_repository4.name, container_repository3.name, container_repository5.name] }
+        end
+      end
+
+      context 'when descending' do
+        it_behaves_like 'sorted paginated query' do
+          let(:sort_param) { :NAME_DESC }
+          let(:first_param) { 2 }
+          let(:expected_results) { [container_repository5.name, container_repository3.name, container_repository4.name, container_repository1.name, container_repository2.name] }
+        end
+      end
+    end
+  end
 end