Skip to content

G
gitlab-ce
Commit ee0fb98d authored by Sarah Yasonik's avatar Sarah Yasonik
Browse files
Options

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

This reverts merge request !33656
parent af7eff8f
Show whitespace changes
Inline Side-by-side
Showing with 77 additions and 382 deletions
app/assets/javascripts/alert_management/components/alert_management_list.vue
View file @ ee0fb98d
......@@ -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
View file @ ee0fb98d
......@@ -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 deleted 100644 → 0
View file @ af7eff8f
# 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
View file @ ee0fb98d
......@@ -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
View file @ ee0fb98d
......@@ -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
View file @ ee0fb98d
......@@ -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
View file @ ee0fb98d
......@@ -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
View file @ ee0fb98d
......@@ -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 deleted 100644 → 0
View file @ af7eff8f
# 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
View file @ ee0fb98d
......@@ -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
View file @ ee0fb98d
......@@ -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 deleted 100644 → 0
View file @ af7eff8f
# 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 deleted 100644 → 0
View file @ af7eff8f
# 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
View file @ ee0fb98d
......@@ -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 deleted 100644 → 0
View file @ af7eff8f
# 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
View file @ ee0fb98d
......@@ -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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7