diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index f93b39dbfd36a13988bd1623f56c0d329d303c18..6e46135dd8fce3de4ae29e5348a6a64c5f6eae36 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -44,6 +44,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
push_frontend_feature_flag(:issue_assignees_widget, @project, default_enabled: :yaml)
push_frontend_feature_flag(:realtime_labels, project, default_enabled: :yaml)
push_frontend_feature_flag(:updated_diff_expansion_buttons, project, default_enabled: :yaml)
+ push_frontend_feature_flag(:mr_attention_requests, current_user, default_enabled: :yaml)
end
before_action do
diff --git a/app/graphql/mutations/merge_requests/remove_attention_request.rb b/app/graphql/mutations/merge_requests/remove_attention_request.rb
index ab8617edb36ff2b468aa50dea0d0087ed8f67e37..3b12b09528b7fef217b40178bbe5007a4e9e8ca2 100644
--- a/app/graphql/mutations/merge_requests/remove_attention_request.rb
+++ b/app/graphql/mutations/merge_requests/remove_attention_request.rb
@@ -13,6 +13,8 @@ class RemoveAttentionRequest < Base
DESC
def resolve(project_path:, iid:, user:)
+ raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'Feature disabled' unless feature_enabled?
+
merge_request = authorized_find!(project_path: project_path, iid: iid)
result = ::MergeRequests::RemoveAttentionRequestedService.new(
@@ -27,6 +29,12 @@ def resolve(project_path:, iid:, user:)
errors: Array(result[:message])
}
end
+
+ private
+
+ def feature_enabled?
+ current_user&.mr_attention_requests_enabled?
+ end
end
end
end
diff --git a/app/graphql/mutations/merge_requests/request_attention.rb b/app/graphql/mutations/merge_requests/request_attention.rb
index 9a9f2f4cd435b656e9bb0a465819b8abd3586787..5f5565285f62f33e0355e06d4df66ac5d5bb61e6 100644
--- a/app/graphql/mutations/merge_requests/request_attention.rb
+++ b/app/graphql/mutations/merge_requests/request_attention.rb
@@ -13,6 +13,8 @@ class RequestAttention < Base
DESC
def resolve(project_path:, iid:, user:)
+ raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'Feature disabled' unless feature_enabled?
+
merge_request = authorized_find!(project_path: project_path, iid: iid)
result = ::MergeRequests::RequestAttentionService.new(
@@ -27,6 +29,12 @@ def resolve(project_path:, iid:, user:)
errors: Array(result[:message])
}
end
+
+ private
+
+ def feature_enabled?
+ current_user&.mr_attention_requests_enabled?
+ end
end
end
end
diff --git a/app/graphql/mutations/merge_requests/toggle_attention_requested.rb b/app/graphql/mutations/merge_requests/toggle_attention_requested.rb
index f316f23fb85049cb426676f394a52a436db7dc39..8913ca4853143c7320355a88ed289a5cd49fdacc 100644
--- a/app/graphql/mutations/merge_requests/toggle_attention_requested.rb
+++ b/app/graphql/mutations/merge_requests/toggle_attention_requested.rb
@@ -13,6 +13,8 @@ class ToggleAttentionRequested < Base
DESC
def resolve(project_path:, iid:, user:)
+ raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'Feature disabled' unless current_user&.mr_attention_requests_enabled?
+
merge_request = authorized_find!(project_path: project_path, iid: iid)
result = ::MergeRequests::ToggleAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request, user: user).execute
diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb
index 5c46145c846bd9495f866a9766f292e3b1ac7c90..c9f6a4648ddb25fe41a73cc98b232877914b57ca 100644
--- a/app/graphql/types/mutation_type.rb
+++ b/app/graphql/types/mutation_type.rb
@@ -69,9 +69,9 @@ class MutationType < BaseObject
mount_mutation Mutations::MergeRequests::SetDraft, calls_gitaly: true
mount_mutation Mutations::MergeRequests::SetAssignees
mount_mutation Mutations::MergeRequests::ReviewerRereview
- mount_mutation Mutations::MergeRequests::RequestAttention, feature_flag: :mr_attention_requests
- mount_mutation Mutations::MergeRequests::RemoveAttentionRequest, feature_flag: :mr_attention_requests
- mount_mutation Mutations::MergeRequests::ToggleAttentionRequested, feature_flag: :mr_attention_requests
+ mount_mutation Mutations::MergeRequests::RequestAttention
+ mount_mutation Mutations::MergeRequests::RemoveAttentionRequest
+ mount_mutation Mutations::MergeRequests::ToggleAttentionRequested
mount_mutation Mutations::Metrics::Dashboard::Annotations::Create
mount_mutation Mutations::Metrics::Dashboard::Annotations::Delete
mount_mutation Mutations::Notes::Create::Note, calls_gitaly: true
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index ba09735ef7ccdc74fc1470c4098e5b069302a8dd..576b85fdeb1027d1390a1f9b1f4dcde4b7fa4526 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -156,7 +156,7 @@ def user_merge_requests_counts
total: total_count
}
- if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
+ if current_user&.mr_attention_requests_enabled?
attention_requested_count = attention_requested_merge_requests_count
counts[:attention_requested_count] = attention_requested_count
diff --git a/app/models/concerns/merge_request_reviewer_state.rb b/app/models/concerns/merge_request_reviewer_state.rb
index 893d06b4da8364a4622f1a3c629f7c7e2051b687..18ec1c253e127c8fe2754fbc7884d37bac0748ff 100644
--- a/app/models/concerns/merge_request_reviewer_state.rb
+++ b/app/models/concerns/merge_request_reviewer_state.rb
@@ -16,8 +16,6 @@ module MergeRequestReviewerState
belongs_to :updated_state_by, class_name: 'User', foreign_key: :updated_state_by_user_id
- after_initialize :set_state, unless: :persisted?
-
def attention_requested_by
return unless attention_requested?
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 4c6ed399bf96ad3c19d32b4f651f2f9e8f682d4b..74095fad5e483deb8dee304e6b4ddb715afcda08 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1977,10 +1977,6 @@ def context_commits_diff
end
end
- def attention_requested_enabled?
- Feature.enabled?(:mr_attention_requests, project, default_enabled: :yaml)
- end
-
private
attr_accessor :skip_fetch_ref
diff --git a/app/models/merge_request_assignee.rb b/app/models/merge_request_assignee.rb
index 77b46fa50f470d522edefaabf36632134ef2c095..fd8e5860040fc310851b2d6cad803b995e130cad 100644
--- a/app/models/merge_request_assignee.rb
+++ b/app/models/merge_request_assignee.rb
@@ -10,12 +10,6 @@ class MergeRequestAssignee < ApplicationRecord
scope :in_projects, ->(project_ids) { joins(:merge_request).where(merge_requests: { target_project_id: project_ids }) }
- def set_state
- if Feature.enabled?(:mr_attention_requests, self.merge_request&.project, default_enabled: :yaml)
- self.state = MergeRequestReviewer.find_by(user_id: self.user_id, merge_request_id: self.merge_request_id)&.state || :attention_requested
- end
- end
-
def cache_key
[model_name.cache_key, id, state, assignee.cache_key]
end
diff --git a/app/models/merge_request_reviewer.rb b/app/models/merge_request_reviewer.rb
index 8c75fb2e4e676a661707e72e1e24c771de714f82..4abf0fa09f0f398ebe550964966bd7dea57d144d 100644
--- a/app/models/merge_request_reviewer.rb
+++ b/app/models/merge_request_reviewer.rb
@@ -6,12 +6,6 @@ class MergeRequestReviewer < ApplicationRecord
belongs_to :merge_request
belongs_to :reviewer, class_name: 'User', foreign_key: :user_id, inverse_of: :merge_request_reviewers
- def set_state
- if Feature.enabled?(:mr_attention_requests, self.merge_request&.project, default_enabled: :yaml)
- self.state = MergeRequestAssignee.find_by(user_id: self.user_id, merge_request_id: self.merge_request_id)&.state || :attention_requested
- end
- end
-
def cache_key
[model_name.cache_key, id, state, reviewer.cache_key]
end
diff --git a/app/models/user.rb b/app/models/user.rb
index 26d47de4f00483ee2e5d9f0a3c90f1b693f3c38b..0408cc2cb5d39412e770798d20a6a0c41843f36a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2070,6 +2070,10 @@ def user_readme
end
end
+ def mr_attention_requests_enabled?
+ Feature.enabled?(:mr_attention_requests, self, default_enabled: :yaml)
+ end
+
protected
# override, from Devise::Validatable
diff --git a/app/serializers/merge_request_user_entity.rb b/app/serializers/merge_request_user_entity.rb
index 97912656bbb7f1f2ab5f1295e4ea1428d06698b7..12c573d1a131850ccb00cd6bdd776c1a407cc882 100644
--- a/app/serializers/merge_request_user_entity.rb
+++ b/app/serializers/merge_request_user_entity.rb
@@ -20,7 +20,7 @@ def self.satisfies(*methods)
find_reviewer_or_assignee(user, options)&.reviewed?
end
- expose :attention_requested, if: satisfies(:present?, :allows_reviewers?, :attention_requested_enabled?) do |user, options|
+ expose :attention_requested, if: ->(_, options) { options[:merge_request].present? && options[:merge_request].allows_reviewers? && request.current_user&.mr_attention_requests_enabled? } do |user, options|
find_reviewer_or_assignee(user, options)&.attention_requested?
end
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index e4784d1de79fa5d676bad7264065f74b7c8c4317..30ad549db915bbeedc802c1406b8d5b50f0c052f 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -43,6 +43,8 @@ def handle_changes(merge_request, options)
end
def handle_assignees_change(merge_request, old_assignees)
+ bulk_update_assignees_state(merge_request, merge_request.assignees - old_assignees)
+
MergeRequests::HandleAssigneesChangeService
.new(project: project, current_user: current_user)
.async_execute(merge_request, old_assignees)
@@ -58,11 +60,10 @@ def handle_reviewers_change(merge_request, old_reviewers)
new_reviewers = merge_request.reviewers - old_reviewers
merge_request_activity_counter.track_users_review_requested(users: new_reviewers)
merge_request_activity_counter.track_reviewers_changed_action(user: current_user)
+ bulk_update_reviewers_state(merge_request, new_reviewers)
unless new_reviewers.include?(current_user)
remove_attention_requested(merge_request)
-
- merge_request.merge_request_reviewers_with(new_reviewers).update_all(updated_state_by_user_id: current_user.id)
end
end
@@ -246,7 +247,7 @@ def delete_milestone_total_merge_requests_counter_cache(milestone)
end
def remove_all_attention_requests(merge_request)
- return unless merge_request.attention_requested_enabled?
+ return unless current_user.mr_attention_requests_enabled?
users = merge_request.reviewers + merge_request.assignees
@@ -254,10 +255,42 @@ def remove_all_attention_requests(merge_request)
end
def remove_attention_requested(merge_request)
- return unless merge_request.attention_requested_enabled?
+ return unless current_user.mr_attention_requests_enabled?
::MergeRequests::RemoveAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request, user: current_user).execute
end
+
+ def bulk_update_assignees_state(merge_request, new_assignees)
+ return unless current_user.mr_attention_requests_enabled?
+ return if new_assignees.empty?
+
+ assignees_map = merge_request.merge_request_assignees_with(new_assignees).to_h do |assignee|
+ state = merge_request.find_reviewer(assignee.assignee)&.state || :attention_requested
+
+ [
+ assignee,
+ { state: MergeRequestAssignee.states[state], updated_state_by_user_id: current_user.id }
+ ]
+ end
+
+ ::Gitlab::Database::BulkUpdate.execute(%i[state updated_state_by_user_id], assignees_map)
+ end
+
+ def bulk_update_reviewers_state(merge_request, new_reviewers)
+ return unless current_user.mr_attention_requests_enabled?
+ return if new_reviewers.empty?
+
+ reviewers_map = merge_request.merge_request_reviewers_with(new_reviewers).to_h do |reviewer|
+ state = merge_request.find_assignee(reviewer.reviewer)&.state || :attention_requested
+
+ [
+ reviewer,
+ { state: MergeRequestReviewer.states[state], updated_state_by_user_id: current_user.id }
+ ]
+ end
+
+ ::Gitlab::Database::BulkUpdate.execute(%i[state updated_state_by_user_id], reviewers_map)
+ end
end
end
diff --git a/app/services/merge_requests/handle_assignees_change_service.rb b/app/services/merge_requests/handle_assignees_change_service.rb
index a169a6dc0b63ea29249d0092d1fc6e1feb9e1149..78c93d10f2a79c4665dbc26a801fc304b195987b 100644
--- a/app/services/merge_requests/handle_assignees_change_service.rb
+++ b/app/services/merge_requests/handle_assignees_change_service.rb
@@ -21,8 +21,6 @@ def execute(merge_request, old_assignees, options = {})
merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees)
merge_request_activity_counter.track_assignees_changed_action(user: current_user)
- merge_request.merge_request_assignees_with(new_assignees).update_all(updated_state_by_user_id: current_user.id)
-
execute_assignees_hooks(merge_request, old_assignees) if options[:execute_hooks]
unless new_assignees.include?(current_user)
diff --git a/app/services/merge_requests/update_assignees_service.rb b/app/services/merge_requests/update_assignees_service.rb
index d52c1bbbcdab03ad23b4321ed7364c0576d6652c..5b23f69ac4a15b6cedc0ba91cf0763c660792cad 100644
--- a/app/services/merge_requests/update_assignees_service.rb
+++ b/app/services/merge_requests/update_assignees_service.rb
@@ -20,6 +20,8 @@ def execute(merge_request)
attrs = update_attrs.merge(assignee_ids: new_ids)
merge_request.update!(**attrs)
+ bulk_update_assignees_state(merge_request, merge_request.assignees - old_assignees)
+
# Defer the more expensive operations (handle_assignee_changes) to the background
MergeRequests::HandleAssigneesChangeService
.new(project: project, current_user: current_user)
diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml
index ddd7dc721cdf9e6a0f5a6a840297ef9292559dd3..3d62683da43a0223c6701425456720ef2b16d47d 100644
--- a/app/views/layouts/header/_default.html.haml
+++ b/app/views/layouts/header/_default.html.haml
@@ -60,7 +60,7 @@
= number_with_delimiter(issues_count)
- if header_link?(:merge_requests)
= nav_link(path: 'dashboard#merge_requests', html_options: { class: "user-counter dropdown" }) do
- - top_level_link = Feature.enabled?(:mr_attention_requests, default_enabled: :yaml) ? attention_requested_mrs_dashboard_path : assigned_mrs_dashboard_path
+ - top_level_link = current_user.mr_attention_requests_enabled? ? attention_requested_mrs_dashboard_path : assigned_mrs_dashboard_path
= link_to top_level_link, class: 'dashboard-shortcuts-merge_requests', title: _('Merge requests'), aria: { label: _('Merge requests') },
data: { qa_selector: 'merge_requests_shortcut_button',
toggle: "dropdown",
@@ -77,7 +77,7 @@
%ul
%li.dropdown-header
= _('Merge requests')
- - if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
+ - if current_user.mr_attention_requests_enabled?
%li#js-need-attention-nav
#js-need-attention-nav-onboarding
= link_to attention_requested_mrs_dashboard_path, class: 'gl-display-flex! gl-align-items-center js-prefetch-document' do
diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml
index 008f2588dbd0bfa66156b79d490a59d99c599f93..c356b9d5f039f3b40f72499f74bf1c02aeb532b8 100644
--- a/app/views/projects/merge_requests/show.html.haml
+++ b/app/views/projects/merge_requests/show.html.haml
@@ -92,7 +92,7 @@
#js-review-bar
-- if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
+- if current_user&.mr_attention_requests_enabled?
#js-need-attention-sidebar-onboarding
= render 'projects/invite_members_modal', project: @project
diff --git a/app/views/shared/issuable/_assignees.html.haml b/app/views/shared/issuable/_assignees.html.haml
index 73f1e35f03f5e293a453d858fce6dc818728b35b..112b0368a3a46452e5b3bd768de9cc2d38418171 100644
--- a/app/views/shared/issuable/_assignees.html.haml
+++ b/app/views/shared/issuable/_assignees.html.haml
@@ -3,7 +3,7 @@
- render_count = assignees_rendering_overflow ? max_render - 1 : max_render
- more_assignees_count = issuable.assignees.size - render_count
-- if issuable.instance_of?(MergeRequest) && Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
+- if issuable.instance_of?(MergeRequest) && current_user&.mr_attention_requests_enabled?
= render 'shared/issuable/merge_request_assignees', issuable: issuable, count: render_count
- else
- issuable.assignees.take(render_count).each do |assignee| # rubocop: disable CodeReuse/ActiveRecord
diff --git a/app/views/shared/issuable/_reviewers.html.haml b/app/views/shared/issuable/_reviewers.html.haml
index 4af2cb0085974d719a0b1abe4d704045e47ee998..3bf923eb94608f22eb5351ab16a2b8ee28fa59bf 100644
--- a/app/views/shared/issuable/_reviewers.html.haml
+++ b/app/views/shared/issuable/_reviewers.html.haml
@@ -3,7 +3,7 @@
- render_count = reviewers_rendering_overflow ? max_render - 1 : max_render
- more_reviewers_count = issuable.reviewers.size - render_count
-- if issuable.instance_of?(MergeRequest) && Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
+- if issuable.instance_of?(MergeRequest) && current_user&.mr_attention_requests_enabled?
= render 'shared/issuable/merge_request_reviewers', issuable: issuable, count: render_count
- else
- issuable.reviewers.take(render_count).each do |reviewer| # rubocop: disable CodeReuse/ActiveRecord
diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml
index 7fdf8ea77962d0acfdeeee064098612f1320118e..6394e05ae247f6bfa0ebde2d9cc9113df630097e 100644
--- a/app/views/shared/issuable/_search_bar.html.haml
+++ b/app/views/shared/issuable/_search_bar.html.haml
@@ -88,7 +88,7 @@
= render 'shared/issuable/user_dropdown_item',
user: User.new(username: '{{username}}', name: '{{name}}'),
avatar: { lazy: true, url: '{{avatar_url}}' }
- - if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
+ - if current_user&.mr_attention_requests_enabled?
#js-dropdown-attention-requested.filtered-search-input-dropdown-menu.dropdown-menu
- if current_user
%ul{ data: { dropdown: true } }
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index eb06030ddb146088517f01daed6f01fd5f641cb3..25364361cc10f377b4537eabf494dfee97189871 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -3441,8 +3441,6 @@ Input type: `MergeRequestCreateInput`
### `Mutation.mergeRequestRemoveAttentionRequest`
-Available only when feature flag `mr_attention_requests` is enabled. This flag is disabled by default, because the feature is experimental and is subject to change without notice.
-
Input type: `MergeRequestRemoveAttentionRequestInput`
#### Arguments
@@ -3464,8 +3462,6 @@ Input type: `MergeRequestRemoveAttentionRequestInput`
### `Mutation.mergeRequestRequestAttention`
-Available only when feature flag `mr_attention_requests` is enabled. This flag is disabled by default, because the feature is experimental and is subject to change without notice.
-
Input type: `MergeRequestRequestAttentionInput`
#### Arguments
@@ -3636,8 +3632,6 @@ Input type: `MergeRequestSetSubscriptionInput`
### `Mutation.mergeRequestToggleAttentionRequested`
-Available only when feature flag `mr_attention_requests` is enabled. This flag is disabled by default, because the feature is experimental and is subject to change without notice.
-
Input type: `MergeRequestToggleAttentionRequestedInput`
#### Arguments
diff --git a/ee/spec/services/ee/merge_requests/update_service_spec.rb b/ee/spec/services/ee/merge_requests/update_service_spec.rb
index 30890180b45cc67462292b6064d67561cc8bdcf0..d4df1925470d0d9266200c9059802be5c921ce86 100644
--- a/ee/spec/services/ee/merge_requests/update_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/update_service_spec.rb
@@ -311,7 +311,7 @@ def update_merge_request(opts)
end
end
- context 'updating reviewers_ids' do
+ context 'updating reviewer_ids' do
it 'updates the tracking when user ids are valid' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.to receive(:track_users_review_requested)
@@ -319,6 +319,94 @@ def update_merge_request(opts)
update_merge_request(reviewer_ids: [user.id, user2.id])
end
+
+ context 'setting state of reviewers' do
+ before do
+ stub_feature_flags(mr_attention_requests: false)
+ end
+
+ it 'does not set state as attention_requested if feature flag is disabled' do
+ update_merge_request({
+ reviewer_ids: [user.id, user2.id]
+ })
+
+ expect(merge_request.merge_request_reviewers[0].state).not_to eq('attention_requested')
+ expect(merge_request.merge_request_reviewers[1].state).not_to eq('attention_requested')
+ end
+
+ context 'feature flag is enabled for current_user' do
+ before do
+ stub_feature_flags(mr_attention_requests: user)
+ end
+
+ it 'updates state to attention_requested' do
+ update_merge_request({
+ reviewer_ids: [user.id, user2.id]
+ })
+
+ expect(merge_request.merge_request_reviewers[0].state).to eq('attention_requested')
+ expect(merge_request.merge_request_reviewers[1].state).to eq('attention_requested')
+ end
+
+ it 'keeps original reviewers state' do
+ update_merge_request(reviewer_ids: [user2.id])
+
+ merge_request.find_reviewer(user2).update!(state: :unreviewed)
+
+ update_merge_request({
+ reviewer_ids: [user.id, user2.id]
+ })
+
+ expect(merge_request.find_reviewer(user).state).to eq('attention_requested')
+ expect(merge_request.find_reviewer(user2).state).to eq('unreviewed')
+ end
+ end
+ end
+ end
+
+ context 'updating assignee_ids' do
+ context 'setting state of assignees' do
+ before do
+ stub_feature_flags(mr_attention_requests: false)
+ end
+
+ it 'does not set state as attention_requested if feature flag is disabled' do
+ update_merge_request({
+ assignee_ids: [user.id, user2.id]
+ })
+
+ expect(merge_request.merge_request_assignees[0].state).not_to eq('attention_requested')
+ expect(merge_request.merge_request_assignees[1].state).not_to eq('attention_requested')
+ end
+
+ context 'feature flag is enabled for current_user' do
+ before do
+ stub_feature_flags(mr_attention_requests: user)
+ end
+
+ it 'updates state to attention_requested' do
+ update_merge_request({
+ assignee_ids: [user.id, user2.id]
+ })
+
+ expect(merge_request.merge_request_assignees[0].state).to eq('attention_requested')
+ expect(merge_request.merge_request_assignees[1].state).to eq('attention_requested')
+ end
+
+ it 'keeps original assignees state' do
+ update_merge_request(assignee_ids: [user2.id])
+
+ merge_request.find_assignee(user2).update!(state: :unreviewed)
+
+ update_merge_request({
+ assignee_ids: [user.id, user2.id]
+ })
+
+ expect(merge_request.find_assignee(user).state).to eq('attention_requested')
+ expect(merge_request.find_assignee(user2).state).to eq('unreviewed')
+ end
+ end
+ end
end
end
end
diff --git a/lib/api/user_counts.rb b/lib/api/user_counts.rb
index e5dfac3b1a157213406572426cec2baf9a8bb5c5..756901c571743cd92d5338cb4c8bf4b1254c3da8 100644
--- a/lib/api/user_counts.rb
+++ b/lib/api/user_counts.rb
@@ -19,7 +19,7 @@ class UserCounts < ::API::Base
todos: current_user.todos_pending_count
}
- if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
+ if current_user&.mr_attention_requests_enabled?
counts[:attention_requests] = current_user.attention_requested_open_merge_requests_count
end
diff --git a/lib/gitlab/gon_helper.rb b/lib/gitlab/gon_helper.rb
index 3cb7cefcc075e92a50a03669fde4cd70c38b8b08..98668c72ff425a7a7bfc2a8c9ec94a219668ef53 100644
--- a/lib/gitlab/gon_helper.rb
+++ b/lib/gitlab/gon_helper.rb
@@ -58,7 +58,7 @@ def add_gon_variables
push_frontend_feature_flag(:sandboxed_mermaid, default_enabled: :yaml)
push_frontend_feature_flag(:source_editor_toolbar, default_enabled: :yaml)
push_frontend_feature_flag(:gl_avatar_for_all_user_avatars, default_enabled: :yaml)
- push_frontend_feature_flag(:mr_attention_requests, default_enabled: :yaml)
+ push_frontend_feature_flag(:mr_attention_requests, current_user, default_enabled: :yaml)
push_frontend_feature_flag(:follow_in_user_popover, current_user, default_enabled: :yaml)
end
diff --git a/lib/gitlab/quick_actions/merge_request_actions.rb b/lib/gitlab/quick_actions/merge_request_actions.rb
index 4efa29337d113c897653e0895eb3479a6915da42..abf55f56c73ce00fb75560da25b05f14a59b513d 100644
--- a/lib/gitlab/quick_actions/merge_request_actions.rb
+++ b/lib/gitlab/quick_actions/merge_request_actions.rb
@@ -285,7 +285,7 @@ module MergeRequestActions
end
types MergeRequest
condition do
- Feature.enabled?(:mr_attention_requests, project, default_enabled: :yaml) &&
+ current_user.mr_attention_requests_enabled? &&
current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
end
parse_params do |attention_param|
@@ -321,7 +321,7 @@ module MergeRequestActions
end
types MergeRequest
condition do
- Feature.enabled?(:mr_attention_requests, project, default_enabled: :yaml) &&
+ current_user.mr_attention_requests_enabled? &&
current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
end
parse_params do |attention_param|
diff --git a/spec/features/dashboard/issuables_counter_spec.rb b/spec/features/dashboard/issuables_counter_spec.rb
index aa445265eec1970e068200cf3dca16abec292617..f8b68be7f93222e491c03bf112212f24261314c9 100644
--- a/spec/features/dashboard/issuables_counter_spec.rb
+++ b/spec/features/dashboard/issuables_counter_spec.rb
@@ -53,6 +53,11 @@
describe 'feature flag mr_attention_requests is enabled' do
before do
merge_request.update!(assignees: [user])
+
+ merge_request.find_assignee(user).update!(state: :attention_requested)
+
+ user.invalidate_attention_requested_count
+
sign_in(user)
end
diff --git a/spec/features/dashboard/merge_requests_spec.rb b/spec/features/dashboard/merge_requests_spec.rb
index 7507ef4e4539d35bc19757c433fd272dc816fe6c..38bef5c614332a80f77e59ef163937e39db282a9 100644
--- a/spec/features/dashboard/merge_requests_spec.rb
+++ b/spec/features/dashboard/merge_requests_spec.rb
@@ -112,8 +112,8 @@
end
it 'includes assigned and reviewers in badge' do
- within("span[aria-label='#{n_("%d merge request", "%d merge requests", 3) % 3}']") do
- expect(page).to have_content('3')
+ within("span[aria-label='#{n_("%d merge request", "%d merge requests", 0) % 0}']") do
+ expect(page).to have_content('0')
end
expect(find('.js-assigned-mr-count')).to have_content('2')
expect(find('.js-reviewer-mr-count')).to have_content('1')
diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb
index 1f63f69a4113f807d8a3af7d7bdd8d860436d9fe..96466e99105e44990969d6c4b8d831a5eed46f4b 100644
--- a/spec/finders/merge_requests_finder_spec.rb
+++ b/spec/finders/merge_requests_finder_spec.rb
@@ -414,6 +414,9 @@ def find(label_name)
before do
reviewer = merge_request1.find_reviewer(user2)
reviewer.update!(state: :reviewed)
+
+ merge_request2.find_reviewer(user2).update!(state: :attention_requested)
+ merge_request3.find_assignee(user2).update!(state: :attention_requested)
end
context 'by username' do
diff --git a/spec/graphql/types/user_merge_request_interaction_type_spec.rb b/spec/graphql/types/user_merge_request_interaction_type_spec.rb
index 1eaaa0c23d080a5def4176e2290ecb34be94b285..4782a1faf8de0d55992d4a3f1ffdbec5e19256db 100644
--- a/spec/graphql/types/user_merge_request_interaction_type_spec.rb
+++ b/spec/graphql/types/user_merge_request_interaction_type_spec.rb
@@ -76,6 +76,7 @@ def resolve(field_name)
context 'when the user has been asked to review the MR' do
before do
merge_request.reviewers << user
+ merge_request.find_reviewer(user).update!(state: :attention_requested)
end
it { is_expected.to eq(Types::MergeRequestReviewStateEnum.values['ATTENTION_REQUESTED'].value) }
diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb
index 38f2efd75a8add76eebf9bec07163644255241a7..f7762069b68badf437ea13f28b7b1ea58bab5678 100644
--- a/spec/helpers/merge_requests_helper_spec.rb
+++ b/spec/helpers/merge_requests_helper_spec.rb
@@ -84,7 +84,7 @@
describe 'mr_attention_requests disabled' do
before do
- stub_feature_flags(mr_attention_requests: false)
+ allow(user).to receive(:mr_attention_requests_enabled?).and_return(false)
end
it "returns assigned, review requested and total merge request counts" do
@@ -97,6 +97,10 @@
end
describe 'mr_attention_requests enabled' do
+ before do
+ allow(user).to receive(:mr_attention_requests_enabled?).and_return(true)
+ end
+
it "returns assigned, review requested, attention requests and total merge request counts" do
expect(subject).to eq(
assigned: user.assigned_open_merge_requests_count,
diff --git a/spec/models/merge_request_assignee_spec.rb b/spec/models/merge_request_assignee_spec.rb
index 1591c5170491eb654b57049282078c013e681ec3..387d17d7823c2d5b9245891ec2af44b828da877e 100644
--- a/spec/models/merge_request_assignee_spec.rb
+++ b/spec/models/merge_request_assignee_spec.rb
@@ -41,22 +41,11 @@
it_behaves_like 'having unique enum values'
- it_behaves_like 'having reviewer state'
-
- describe 'syncs to reviewer state' do
- before do
- reviewer = merge_request.merge_request_reviewers.build(reviewer: assignee)
- reviewer.update!(state: :reviewed)
- end
-
- it { is_expected.to have_attributes(state: 'reviewed') }
- end
-
describe '#attention_requested_by' do
let(:current_user) { create(:user) }
before do
- subject.update!(updated_state_by: current_user)
+ subject.update!(updated_state_by: current_user, state: :attention_requested)
end
context 'attention requested' do
diff --git a/spec/models/merge_request_reviewer_spec.rb b/spec/models/merge_request_reviewer_spec.rb
index dd00c4d8627553c56eacf936dd2dcce9cdfe091f..4df2dba3a7df95b6c7bdbaec66548ed082bc5056 100644
--- a/spec/models/merge_request_reviewer_spec.rb
+++ b/spec/models/merge_request_reviewer_spec.rb
@@ -10,17 +10,6 @@
it_behaves_like 'having unique enum values'
- it_behaves_like 'having reviewer state'
-
- describe 'syncs to assignee state' do
- before do
- assignee = merge_request.merge_request_assignees.build(assignee: reviewer)
- assignee.update!(state: :reviewed)
- end
-
- it { is_expected.to have_attributes(state: 'reviewed') }
- end
-
describe 'associations' do
it { is_expected.to belong_to(:merge_request).class_name('MergeRequest') }
it { is_expected.to belong_to(:reviewer).class_name('User').inverse_of(:merge_request_reviewers) }
@@ -30,7 +19,7 @@
let(:current_user) { create(:user) }
before do
- subject.update!(updated_state_by: current_user)
+ subject.update!(updated_state_by: current_user, state: :attention_requested)
end
context 'attention requested' do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 8545c7bc6c7e28f71694190fb128664ea6d87d2b..14cad1174efc64e6c4a22790e7ae7efd9e1d8092 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -151,6 +151,8 @@
before do
assignee = merge_request6.find_assignee(user2)
assignee.update!(state: :reviewed)
+ merge_request2.find_reviewer(user2).update!(state: :attention_requested)
+ merge_request5.find_assignee(user2).update!(state: :attention_requested)
end
it 'returns MRs that have any attention requests' do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 263f8ba4afdc170d25234ca6ad06bb598d151779..05dcabd96ba327704def4cc2cbdfab668a42f866 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -5006,9 +5006,13 @@ def add_user(access)
let(:archived_project) { create(:project, :public, :archived) }
before do
- create(:merge_request, source_project: project, author: user, reviewers: [user])
- create(:merge_request, :closed, source_project: project, author: user, reviewers: [user])
- create(:merge_request, source_project: archived_project, author: user, reviewers: [user])
+ mr1 = create(:merge_request, source_project: project, author: user, reviewers: [user])
+ mr2 = create(:merge_request, :closed, source_project: project, author: user, reviewers: [user])
+ mr3 = create(:merge_request, source_project: archived_project, author: user, reviewers: [user])
+
+ mr1.find_reviewer(user).update!(state: :attention_requested)
+ mr2.find_reviewer(user).update!(state: :attention_requested)
+ mr3.find_reviewer(user).update!(state: :attention_requested)
end
it 'returns number of open merge requests from non-archived projects' do
@@ -6817,4 +6821,23 @@ def access_levels(groups)
it_behaves_like 'it has loose foreign keys' do
let(:factory_name) { :user }
end
+
+ describe 'mr_attention_requests_enabled?' do
+ let(:user) { create(:user) }
+
+ before do
+ stub_feature_flags(mr_attention_requests: false)
+ end
+
+ it { expect(user.mr_attention_requests_enabled?).to be(false) }
+
+ it 'feature flag is enabled for user' do
+ stub_feature_flags(mr_attention_requests: user)
+
+ another_user = create(:user)
+
+ expect(user.mr_attention_requests_enabled?).to be(true)
+ expect(another_user.mr_attention_requests_enabled?).to be(false)
+ end
+ end
end
diff --git a/spec/models/users/merge_request_interaction_spec.rb b/spec/models/users/merge_request_interaction_spec.rb
index 12c7fa43a60b6ed5372c7f06806fbea34f6c2d55..a499a7c68e8bc1ce8324571d3bb6ba7166a08cd2 100644
--- a/spec/models/users/merge_request_interaction_spec.rb
+++ b/spec/models/users/merge_request_interaction_spec.rb
@@ -59,6 +59,7 @@
context 'when the user has been asked to review the MR' do
before do
merge_request.reviewers << user
+ merge_request.find_reviewer(user).update!(state: :attention_requested)
end
it { is_expected.to eq(Types::MergeRequestReviewStateEnum.values['ATTENTION_REQUESTED'].value) }
diff --git a/spec/requests/api/graphql/mutations/merge_requests/request_attention_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/request_attention_spec.rb
index e96f8ce55395d775493d1e243d132aa739a58ec7..9c7519138277f9cab7a4cdaabbe3a6d70cd030ca 100644
--- a/spec/requests/api/graphql/mutations/merge_requests/request_attention_spec.rb
+++ b/spec/requests/api/graphql/mutations/merge_requests/request_attention_spec.rb
@@ -63,4 +63,17 @@ def mutation_errors
expect(mutation_errors).not_to be_empty
end
end
+
+ context 'feature flag is disabled' do
+ before do
+ stub_feature_flags(mr_attention_requests: false)
+ end
+
+ it 'returns an error' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(response).to have_gitlab_http_status(:success)
+ expect(graphql_errors[0]["message"]).to eq "Feature disabled"
+ end
+ end
end
diff --git a/spec/requests/api/graphql/mutations/remove_attention_request_spec.rb b/spec/requests/api/graphql/mutations/remove_attention_request_spec.rb
index 0a7ced1a30ce18946748d366aad1bda87fe94bb4..053559b039d8c974ab03a0be0554723ae9c160ab 100644
--- a/spec/requests/api/graphql/mutations/remove_attention_request_spec.rb
+++ b/spec/requests/api/graphql/mutations/remove_attention_request_spec.rb
@@ -63,4 +63,17 @@ def mutation_errors
expect(mutation_errors).not_to be_empty
end
end
+
+ context 'feature flag is disabled' do
+ before do
+ stub_feature_flags(mr_attention_requests: false)
+ end
+
+ it 'returns an error' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(response).to have_gitlab_http_status(:success)
+ expect(graphql_errors[0]["message"]).to eq "Feature disabled"
+ end
+ end
end
diff --git a/spec/requests/api/graphql/project/merge_request_spec.rb b/spec/requests/api/graphql/project/merge_request_spec.rb
index 395b052369e0180b59fc324b367d2de5eedcf48e..d2f34080be3294e1d82c583fd57245735c48226f 100644
--- a/spec/requests/api/graphql/project/merge_request_spec.rb
+++ b/spec/requests/api/graphql/project/merge_request_spec.rb
@@ -425,7 +425,7 @@ def interaction_data
other_users.each do |user|
assign_user(user)
- merge_request.merge_request_reviewers.find_or_create_by!(reviewer: user)
+ merge_request.merge_request_reviewers.find_or_create_by!(reviewer: user, state: :attention_requested)
end
expect { post_graphql(query) }.not_to exceed_query_limit(baseline)
@@ -466,7 +466,7 @@ def interaction_data
let(:can_update) { false }
def assign_user(user)
- merge_request.merge_request_reviewers.create!(reviewer: user)
+ merge_request.merge_request_reviewers.create!(reviewer: user, state: :attention_requested)
end
end
diff --git a/spec/requests/api/user_counts_spec.rb b/spec/requests/api/user_counts_spec.rb
index 27ebf02dd811281d2d7c0e7deb29c83a11808aae..2d4705920cf46758a64cb4d213a89d17acd13456 100644
--- a/spec/requests/api/user_counts_spec.rb
+++ b/spec/requests/api/user_counts_spec.rb
@@ -43,7 +43,7 @@
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_a Hash
expect(json_response['merge_requests']).to eq(2)
- expect(json_response['attention_requests']).to eq(2)
+ expect(json_response['attention_requests']).to eq(0)
end
describe 'mr_attention_requests is disabled' do
diff --git a/spec/serializers/merge_request_user_entity_spec.rb b/spec/serializers/merge_request_user_entity_spec.rb
index 72d1b0c0dd27cba417c2f5b29d73560e63d486f4..7877356ff0f59c10392a0b833b5ac23e0ad75808 100644
--- a/spec/serializers/merge_request_user_entity_spec.rb
+++ b/spec/serializers/merge_request_user_entity_spec.rb
@@ -58,6 +58,10 @@
end
context 'attention_requested' do
+ before do
+ merge_request.find_assignee(user).update!(state: :attention_requested)
+ end
+
it { is_expected.to include(attention_requested: true ) }
end
diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb
index 34c598e6cc0830117e72445b1848e1283c71bc63..fa3b1614e21a9efebc315fd87661dc9324d57834 100644
--- a/spec/services/merge_requests/handle_assignees_change_service_spec.rb
+++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb
@@ -95,12 +95,6 @@ def execute
execute
end
- it 'updates attention requested by of assignee' do
- execute
-
- expect(merge_request.find_assignee(assignee).updated_state_by).to eq(user)
- end
-
it 'tracks users assigned event' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.to receive(:track_users_assigned_to_mr).once.with(users: [assignee])
diff --git a/spec/services/merge_requests/update_assignees_service_spec.rb b/spec/services/merge_requests/update_assignees_service_spec.rb
index 3a0b17c2768755ff1375c9d97858b463625b7cab..f5f6f0ca301c54fb2e04c118867b717ae27b9e88 100644
--- a/spec/services/merge_requests/update_assignees_service_spec.rb
+++ b/spec/services/merge_requests/update_assignees_service_spec.rb
@@ -113,6 +113,49 @@ def update_merge_request
expect { service.execute(merge_request) }
.to issue_fewer_queries_than { update_service.execute(other_mr) }
end
+
+ context 'setting state of assignees' do
+ before do
+ stub_feature_flags(mr_attention_requests: false)
+ end
+
+ it 'does not set state as attention_requested if feature flag is disabled' do
+ update_merge_request
+
+ expect(merge_request.merge_request_assignees[0].state).not_to eq('attention_requested')
+ end
+
+ context 'feature flag is enabled for current_user' do
+ before do
+ stub_feature_flags(mr_attention_requests: user)
+ end
+
+ it 'sets state as attention_requested' do
+ update_merge_request
+
+ expect(merge_request.merge_request_assignees[0].state).to eq('attention_requested')
+ expect(merge_request.merge_request_assignees[0].updated_state_by).to eq(user)
+ end
+
+ it 'uses reviewers state if it is same user as new assignee' do
+ merge_request.reviewers << user2
+
+ update_merge_request
+
+ expect(merge_request.merge_request_assignees[0].state).to eq('unreviewed')
+ end
+
+ context 'when assignee_ids matches existing assignee' do
+ let(:opts) { { assignee_ids: [user3.id] } }
+
+ it 'keeps original assignees state' do
+ update_merge_request
+
+ expect(merge_request.find_assignee(user3).state).to eq('unreviewed')
+ end
+ end
+ end
+ end
end
end
end
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 30095ebeb50fbb6f71c774f2c93879b6819dbafd..c15486dc916fac65af1fda3c0ae5eb566847ddd4 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -328,6 +328,49 @@ def update_merge_request(opts)
update_merge_request(reviewer_ids: [user.id])
end
+
+ context 'setting state of reviewers' do
+ before do
+ stub_feature_flags(mr_attention_requests: false)
+ end
+
+ it 'does not set state as attention_requested if feature flag is disabled' do
+ update_merge_request(reviewer_ids: [user.id])
+
+ expect(merge_request.merge_request_reviewers[0].state).not_to eq('attention_requested')
+ end
+
+ context 'feature flag is enabled for current_user' do
+ before do
+ stub_feature_flags(mr_attention_requests: user)
+ end
+
+ it 'sets state as attention_requested' do
+ update_merge_request(reviewer_ids: [user.id])
+
+ expect(merge_request.merge_request_reviewers[0].state).to eq('attention_requested')
+ expect(merge_request.merge_request_reviewers[0].updated_state_by).to eq(user)
+ end
+
+ it 'keeps original reviewers state' do
+ merge_request.find_reviewer(user2).update!(state: :unreviewed)
+
+ update_merge_request({
+ reviewer_ids: [user2.id]
+ })
+
+ expect(merge_request.find_reviewer(user2).state).to eq('unreviewed')
+ end
+
+ it 'uses reviewers state if it is same user as new assignee' do
+ merge_request.assignees << user
+
+ update_merge_request(reviewer_ids: [user.id])
+
+ expect(merge_request.merge_request_reviewers[0].state).to eq('unreviewed')
+ end
+ end
+ end
end
it 'creates a resource label event' do
@@ -1066,6 +1109,53 @@ def update_merge_request(opts)
end
end
end
+
+ context 'setting state of assignees' do
+ before do
+ stub_feature_flags(mr_attention_requests: false)
+ end
+
+ it 'does not set state as attention_requested if feature flag is disabled' do
+ update_merge_request({
+ assignee_ids: [user2.id]
+ })
+
+ expect(merge_request.merge_request_assignees[0].state).not_to eq('attention_requested')
+ end
+
+ context 'feature flag is enabled for current_user' do
+ before do
+ stub_feature_flags(mr_attention_requests: user)
+ end
+
+ it 'sets state as attention_requested' do
+ update_merge_request({
+ assignee_ids: [user2.id]
+ })
+
+ expect(merge_request.merge_request_assignees[0].state).to eq('attention_requested')
+ expect(merge_request.merge_request_assignees[0].updated_state_by).to eq(user)
+ end
+
+ it 'keeps original assignees state' do
+ update_merge_request({
+ assignee_ids: [user3.id]
+ })
+
+ expect(merge_request.find_assignee(user3).state).to eq('unreviewed')
+ end
+
+ it 'uses reviewers state if it is same user as new assignee' do
+ merge_request.reviewers << user2
+
+ update_merge_request({
+ assignee_ids: [user2.id]
+ })
+
+ expect(merge_request.merge_request_assignees[0].state).to eq('unreviewed')
+ end
+ end
+ end
end
context 'when adding time spent' do
diff --git a/spec/support/shared_examples/models/reviewer_state_shared_examples.rb b/spec/support/shared_examples/models/reviewer_state_shared_examples.rb
deleted file mode 100644
index f1392768b068a5e9d379e3786f70e47da561b97e..0000000000000000000000000000000000000000
--- a/spec/support/shared_examples/models/reviewer_state_shared_examples.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.shared_examples 'having reviewer state' do
- describe 'mr_attention_requests feature flag is disabled' do
- before do
- stub_feature_flags(mr_attention_requests: false)
- end
-
- it { is_expected.to have_attributes(state: 'unreviewed') }
- end
-
- describe 'mr_attention_requests feature flag is enabled' do
- it { is_expected.to have_attributes(state: 'attention_requested') }
- end
-end