Merge branch 'revert-57f6abcd' into 'master'

Revert "Merge branch 'sy-fix-assignee-n-plus-one' into 'master'"

See merge request gitlab-org/gitlab!34198

app/assets/javascripts/alert_management/components/alert_management_list.vue

@@ -282,9 +282,7 @@ export default {
     },
     getAssignees(assignees) {
       // TODO: Update to show list of assignee(s) after https://gitlab.com/gitlab-org/gitlab/-/issues/218405
-      return assignees.nodes?.length > 0
-        ? assignees.nodes[0]?.username
-        : s__('AlertManagement|Unassigned');
+      return assignees?.length > 0 ? assignees[0]?.username : s__('AlertManagement|Unassigned');
     },
     handlePageChange(page) {
       const { startCursor, endCursor } = this.alerts.pageInfo;

app/assets/javascripts/alert_management/graphql/fragments/list_item.fragment.graphql

@@ -6,10 +6,8 @@ fragment AlertListItem on AlertManagementAlert {
   startedAt
   endedAt
   eventCount
-  issueIid
+  issueIid,
   assignees {
-    nodes {
     username
-    }
-  }
+  },
 }

app/graphql/resolvers/alert_management/alerts/assignees_resolver.rb

-# frozen_string_literal: true
-
-module Resolvers
-  module AlertManagement
-    module Alerts
-      class AssigneesResolver < BaseResolver
-        type Types::UserType, null: true
-
-        # With lazy-loaded assignees, authorizations should be avoided
-        # in earlier phases to take advantage of batching. See
-        # AssigneeLoader for authorization steps.
-        # https://gitlab.com/gitlab-org/gitlab/-/issues/217767
-        def self.skip_authorizations?
-          true
-        end
-
-        def resolve(**args)
-          return [] unless Feature.enabled?(:alert_assignee)
-
-          ::Gitlab::Graphql::Loaders::AlertManagement::Alerts::AssigneesLoader
-            .new(object.id, user_authorization_filter)
-            .find
-        end
-
-        private
-
-        def user_authorization_filter
-          proc do |users|
-            users.select { |user| Ability.allowed?(context[:current_user], :read_user, user) }
-          end
-        end
-      end
-    end
-  end
-end

app/graphql/types/alert_management/alert_type.rb

@@ -85,10 +85,15 @@ module Types
             description: 'Timestamp the alert was last updated'
 
       field :assignees,
-            Types::UserType.connection_type,
+            [Types::UserType],
             null: true,
-            description: 'Assignees of the alert',
-            resolver: ::Resolvers::AlertManagement::Alerts::AssigneesResolver
+            description: 'Assignees of the alert'
+
+      def assignees
+        return User.none unless Feature.enabled?(:alert_assignee, object.project)
+
+        object.assignees
+      end
     end
   end
 end

app/models/alert_management/alert_assignee.rb

@@ -7,7 +7,5 @@ module AlertManagement
 
     validates :alert, presence: true
     validates :assignee, presence: true, uniqueness: { scope: :alert_id }
-
-    scope :for_alert_ids, -> (ids) { includes(:assignee).where(alert_id: ids) }
   end
 end

doc/api/graphql/reference/gitlab_schema.graphql

@@ -172,27 +172,7 @@ type AlertManagementAlert {
   """
   Assignees of the alert
   """
-  assignees(
-    """
-    Returns the elements in the list that come after the specified cursor.
-    """
-    after: String
-
-    """
-    Returns the elements in the list that come before the specified cursor.
-    """
-    before: String
-
-    """
-    Returns the first _n_ elements from the list.
-    """
-    first: Int
-
-    """
-    Returns the last _n_ elements from the list.
-    """
-    last: Int
-  ): UserConnection
+  assignees: [User!]
 
   """
   Timestamp the alert was created

doc/api/graphql/reference/gitlab_schema.json

@@ -486,51 +486,20 @@
               "name": "assignees",
               "description": "Assignees of the alert",
               "args": [
-                {
-                  "name": "after",
-                  "description": "Returns the elements in the list that come after the specified cursor.",
-                  "type": {
-                    "kind": "SCALAR",
-                    "name": "String",
-                    "ofType": null
-                  },
-                  "defaultValue": null
-                },
-                {
-                  "name": "before",
-                  "description": "Returns the elements in the list that come before the specified cursor.",
-                  "type": {
-                    "kind": "SCALAR",
-                    "name": "String",
-                    "ofType": null
-                  },
-                  "defaultValue": null
-                },
-                {
-                  "name": "first",
-                  "description": "Returns the first _n_ elements from the list.",
-                  "type": {
-                    "kind": "SCALAR",
-                    "name": "Int",
-                    "ofType": null
-                  },
-                  "defaultValue": null
-                },
-                {
-                  "name": "last",
-                  "description": "Returns the last _n_ elements from the list.",
-                  "type": {
-                    "kind": "SCALAR",
-                    "name": "Int",
-                    "ofType": null
-                  },
-                  "defaultValue": null
-                }
+
               ],
               "type": {
+                "kind": "LIST",
+                "name": null,
+                "ofType": {
+                  "kind": "NON_NULL",
+                  "name": null,
+                  "ofType": {
                     "kind": "OBJECT",
-                "name": "UserConnection",
+                    "name": "User",
                     "ofType": null
+                  }
+                }
               },
               "isDeprecated": false,
               "deprecationReason": null

doc/api/graphql/reference/index.md

@@ -61,6 +61,7 @@ Describes an alert from the project's Alert Management
 
 | Name  | Type  | Description |
 | ---   |  ---- | ----------  |
+| `assignees` | User! => Array | Assignees of the alert |
 | `createdAt` | Time | Timestamp the alert was created |
 | `description` | String | Description of the alert |
 | `details` | JSON | Alert details |

lib/gitlab/graphql/loaders/alert_management/alerts/assignees_loader.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Graphql
-    module Loaders
-      module AlertManagement
-        module Alerts
-          # Batches requests for AlertManagement::Alert#assignees
-          # to avoid N+1 queries
-          class AssigneesLoader
-            # @param alert_id [Integer]
-            # @param authorization_filter [Proc] Filter to be applied
-            #                             to output assignees
-            def initialize(alert_id, authorization_filter)
-              @alert_id = alert_id
-              @authorization_filter = authorization_filter
-            end
-
-            # Returns BatchLoader::GraphQL which evaluates
-            # to a Gitlab::Graphql::FilterableArray of User objects
-            def find
-              BatchLoader::GraphQL.for(alert_id).batch(default_value: default_value) do |alert_ids, loader|
-                load_assignees(loader, alert_ids)
-              end
-            end
-
-            private
-
-            attr_reader :alert_id, :authorization_filter
-
-            def default_value
-              Gitlab::Graphql::FilterableArray.new(authorization_filter)
-            end
-
-            def load_assignees(loader, alert_ids)
-              ::AlertManagement::AlertAssignee
-                .for_alert_ids(alert_ids)
-                .each { |alert_assignee| add_assignee_for_alert(loader, alert_assignee) }
-            end
-
-            def add_assignee_for_alert(loader, alert_assignee)
-              # loader optionally accepts a block, allowing
-              # access to the current expected output, allowing
-              # us to collect assignees
-              loader.call(alert_assignee.alert_id) { |assignees| assignees << alert_assignee.assignee }
-            end
-          end
-        end
-      end
-    end
-  end
-end

spec/frontend/alert_management/components/alert_management_list_spec.js

@@ -262,7 +262,7 @@ describe('AlertManagementList', () => {
         findAssignees()
           .at(1)
           .text(),
-      ).toBe(mockAlerts[1].assignees.nodes[0].username);
+      ).toBe(mockAlerts[1].assignees[0].username);
     });
 
     it('navigates to the detail page when alert row is clicked', () => {
@@ -291,7 +291,6 @@ describe('AlertManagementList', () => {
                   startedAt: '2020-03-17T23:18:14.996Z',
                   endedAt: '2020-04-17T23:18:14.996Z',
                   severity: 'high',
-                  assignees: { nodes: [] },
                 },
               ],
             },

spec/frontend/alert_management/mocks/alerts.json

@@ -8,7 +8,7 @@
       "startedAt": "2020-04-17T23:18:14.996Z",
       "endedAt": "2020-04-17T23:18:14.996Z",
       "status": "TRIGGERED",
-    "assignees": { "nodes": [] }
+      "assignees": []
     },
     {
       "iid": "1527543",
@@ -18,7 +18,7 @@
       "startedAt": "2020-04-17T23:18:14.996Z",
       "endedAt": "2020-04-17T23:18:14.996Z",
       "status": "ACKNOWLEDGED",
-    "assignees": { "nodes": [{ "username": "root" }] }
+      "assignees": [{"username": "root"}]
     },
     {
       "iid": "1527544",
@@ -28,6 +28,6 @@
       "startedAt": "2020-04-17T23:18:14.996Z",
       "endedAt": "2020-04-17T23:18:14.996Z",
       "status": "RESOLVED",
-    "assignees": { "nodes": [{ "username": "root" }] }
+      "assignees": [{"username": "root"}]
     }
-]
+  ]

spec/graphql/resolvers/alert_management/alerts/assignees_resolver_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Resolvers::AlertManagement::Alerts::AssigneesResolver do
-  include GraphqlHelpers
-
-  describe '#resolve' do
-    let_it_be(:current_user) { create(:user) }
-    let_it_be(:project) { create(:project) }
-    let_it_be(:alert) { create(:alert_management_alert, :all_fields, project: project) }
-    let_it_be(:another_alert) { create(:alert_management_alert, :all_fields, project: project) }
-
-    it 'resolves for a single alert' do
-      result = batch_sync(max_queries: 2) { resolve_assignees(alert) }
-
-      expect(result).to match_array(alert.assignees)
-    end
-
-    it 'resolves for multiple alerts' do
-      result = batch_sync(max_queries: 2) { [resolve_assignees(alert), resolve_assignees(another_alert)] }
-
-      expect(result).to match_array([alert.assignees, another_alert.assignees])
-    end
-
-    private
-
-    def resolve_assignees(alert, args = {}, context = { current_user: current_user })
-      resolve(described_class, obj: alert, args: args, ctx: context)
-    end
-  end
-end

spec/lib/gitlab/graphql/loaders/alert_management/alerts/assignees_loader_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Gitlab::Graphql::Loaders::AlertManagement::Alerts::AssigneesLoader do
-  describe '#find' do
-    let_it_be(:user) { create(:user) }
-    let_it_be(:project) { create(:project) }
-    let_it_be(:alert1) { create(:alert_management_alert, project: project, assignees: [user]) }
-    let_it_be(:alert2) { create(:alert_management_alert, :all_fields, project: project) }
-    let_it_be(:alert3) { create(:alert_management_alert, project: project) }
-    let(:filter) { proc {} }
-
-    subject do
-      [
-        described_class.new(alert1.id, filter).find,
-        described_class.new(alert2.id, filter).find,
-        described_class.new(alert3.id, filter).find
-      ].map(&:sync)
-    end
-
-    it 'only queries once for alert assignees' do
-      # One query for alert_assignees, one query for users
-      expect { subject }.not_to exceed_query_limit(2)
-    end
-
-    it 'returns appropriate assignees for alerts' do
-      expect(subject).to eq [alert1.assignees.to_a, alert2.assignees.to_a, alert3.assignees.to_a]
-    end
-
-    context 'with a filter' do
-      let(:filter) { proc { |users| users.select { |u| u == user } } }
-
-      it 'limits assignees by the filter' do
-        expect(subject).to eq [alert1.assignees.to_a, alert2.assignees.to_a, alert3.assignees.to_a]
-        expect(subject.all?(Gitlab::Graphql::FilterableArray)).to be_truthy
-
-        filtered_result = subject.map { |assignees| assignees.filter_callback.call(assignees) }
-
-        expect(filtered_result).to eq [[user], [], []]
-      end
-    end
-  end
-end

spec/models/alert_management/alert_assignee_spec.rb

@@ -3,37 +3,19 @@
 require 'spec_helper'
 
 describe AlertManagement::AlertAssignee do
-  let_it_be(:user1) { create(:user) }
-  let_it_be(:user2) { create(:user) }
-  let_it_be(:alert1) { create(:alert_management_alert, assignees: [user1, user2]) }
-  let_it_be(:alert2) { create(:alert_management_alert, assignees: [user2]) }
-
   describe 'associations' do
     it { is_expected.to belong_to(:alert) }
     it { is_expected.to belong_to(:assignee) }
   end
 
   describe 'validations' do
-    subject { alert1.alert_assignees.build(assignee: user1) }
+    let(:alert) { create(:alert_management_alert) }
+    let(:user) { create(:user) }
+
+    subject { alert.alert_assignees.build(assignee: user) }
 
     it { is_expected.to validate_presence_of(:alert) }
     it { is_expected.to validate_presence_of(:assignee) }
     it { is_expected.to validate_uniqueness_of(:assignee).scoped_to(:alert_id) }
   end
-
-  describe 'scopes' do
-    describe '.for_alert_ids' do
-      let(:alert_ids) { alert1.id }
-
-      subject { described_class.for_alert_ids(alert_ids) }
-
-      it { is_expected.to contain_exactly(*alert1.reload.alert_assignees) }
-
-      context 'with multiple ids' do
-        let(:alert_ids) { [alert1.id, alert2.id] }
-
-        it { is_expected.to contain_exactly(*alert1.reload.alert_assignees, *alert2.reload.alert_assignees) }
-      end
-    end
-  end
 end

spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb

-# frozen_string_literal: true
-require 'spec_helper'
-
-describe 'getting Alert Management Alert Assignees' do
-  include GraphqlHelpers
-
-  let_it_be(:project) { create(:project) }
-  let_it_be(:current_user) { create(:user) }
-  let_it_be(:first_alert) { create(:alert_management_alert, project: project, assignees: [current_user]) }
-  let_it_be(:second_alert) { create(:alert_management_alert, project: project) }
-
-  let(:params) { {} }
-
-  let(:fields) do
-    <<~QUERY
-      nodes {
-        #{all_graphql_fields_for('AlertManagementAlert')}
-      }
-    QUERY
-  end
-
-  let(:query) do
-    graphql_query_for(
-      'project',
-      { 'fullPath' => project.full_path },
-      query_graphql_field('alertManagementAlerts', params, fields)
-    )
-  end
-
-  let(:alerts) { graphql_data.dig('project', 'alertManagementAlerts', 'nodes') }
-  let(:assignees) { alerts.map { |alert| [alert['iid'], alert['assignees']['nodes']] }.to_h }
-  let(:first_assignees) { assignees[first_alert.iid.to_s] }
-  let(:second_assignees) { assignees[second_alert.iid.to_s] }
-
-  before do
-    project.add_developer(current_user)
-  end
-
-  it 'returns the correct assignees' do
-    post_graphql(query, current_user: current_user)
-
-    expect(first_assignees.length).to eq(1)
-    expect(first_assignees.first).to include('username' => current_user.username)
-    expect(second_assignees).to be_empty
-  end
-
-  it 'applies appropriate filters for non-visible users' do
-    allow(Ability).to receive(:allowed?).and_call_original
-    allow(Ability).to receive(:allowed?).with(current_user, :read_user, current_user).and_return(false)
-
-    post_graphql(query, current_user: current_user)
-
-    expect(first_assignees).to be_empty
-    expect(second_assignees).to be_empty
-  end
-
-  it 'avoids N+1 queries' do
-    base_count = ActiveRecord::QueryRecorder.new do
-      post_graphql(query, current_user: current_user)
-    end
-
-    # An N+1 would mean a new alert would increase the query count
-    third_alert = create(:alert_management_alert, project: project, assignees: [current_user])
-
-    expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(base_count)
-
-    third_assignees = assignees[third_alert.iid.to_s]
-
-    expect(third_assignees.length).to eq(1)
-    expect(third_assignees.first).to include('username' => current_user.username)
-  end
-
-  context 'with alert_assignee flag disabled' do
-    before do
-      stub_feature_flags(alert_assignee: false)
-    end
-
-    it 'excludes assignees' do
-      post_graphql(query, current_user: current_user)
-
-      expect(first_assignees).to be_empty
-      expect(second_assignees).to be_empty
-    end
-  end
-end

spec/requests/api/graphql/project/alert_management/alerts_spec.rb

@@ -75,7 +75,7 @@ describe 'getting Alert Management Alerts' do
           'updatedAt' => triggered_alert.updated_at.strftime('%Y-%m-%dT%H:%M:%SZ')
         )
 
-        expect(first_alert['assignees']['nodes'].first).to include('username' => triggered_alert.assignees.first.username)
+        expect(first_alert['assignees'].first).to include('username' => triggered_alert.assignees.first.username)
 
         expect(second_alert).to include(
           'iid' => resolved_alert.iid.to_s,
@@ -137,5 +137,18 @@ describe 'getting Alert Management Alerts' do
         end
       end
     end
+
+    context 'with alert_assignee flag disabled' do
+      before do
+        stub_feature_flags(alert_assignee: false)
+        project.add_developer(current_user)
+
+        post_graphql(query, current_user: current_user)
+      end
+
+      it 'excludes assignees' do
+        expect(alerts.first['assignees']).to be_empty
+      end
+    end
   end
 end