Commit 138ad560 authored by Igor Drozdov's avatar Igor Drozdov

Move async mergeability check into widget

Currently Projects::MergeRequestsController#show goes
into primary sometimes due to the mergeability check

Let's move it to the widget and check it async when
we load the merge request the first time and sync
when it's auto refreshed

Visually we have the same behaviour, but merge_requests#show
action no longer goes to primary
parent 7a19c4a6
...@@ -60,6 +60,7 @@ export default class MergeRequestStore { ...@@ -60,6 +60,7 @@ export default class MergeRequestStore {
this.rebaseInProgress = data.rebase_in_progress; this.rebaseInProgress = data.rebase_in_progress;
this.mergeRequestDiffsPath = data.diffs_path; this.mergeRequestDiffsPath = data.diffs_path;
this.approvalsWidgetType = data.approvals_widget_type; this.approvalsWidgetType = data.approvals_widget_type;
this.mergeRequestWidgetPath = data.merge_request_widget_path;
if (data.issues_links) { if (data.issues_links) {
const links = data.issues_links; const links = data.issues_links;
......
...@@ -14,6 +14,8 @@ class Projects::MergeRequests::ContentController < Projects::MergeRequests::Appl ...@@ -14,6 +14,8 @@ class Projects::MergeRequests::ContentController < Projects::MergeRequests::Appl
SLOW_POLLING_INTERVAL = 5.minutes.in_milliseconds SLOW_POLLING_INTERVAL = 5.minutes.in_milliseconds
def widget def widget
check_mergeability_async!
respond_to do |format| respond_to do |format|
format.json do format.json do
render json: serializer(MergeRequestPollWidgetEntity) render json: serializer(MergeRequestPollWidgetEntity)
...@@ -38,6 +40,13 @@ class Projects::MergeRequests::ContentController < Projects::MergeRequests::Appl ...@@ -38,6 +40,13 @@ class Projects::MergeRequests::ContentController < Projects::MergeRequests::Appl
def serializer(entity) def serializer(entity)
serializer = MergeRequestSerializer.new(current_user: current_user, project: merge_request.project) serializer = MergeRequestSerializer.new(current_user: current_user, project: merge_request.project)
serializer.represent(merge_request, {}, entity) serializer.represent(merge_request, { async_mergeability_check: params[:async_mergeability_check] }, entity)
end
def check_mergeability_async!
return unless Feature.enabled?(:check_mergeability_async_in_widget, merge_request.project, default_enabled: :yaml)
return if params[:async_mergeability_check].blank?
merge_request.check_mergeability(async: true)
end end
end end
...@@ -92,7 +92,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -92,7 +92,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
def show def show
close_merge_request_if_no_source_project close_merge_request_if_no_source_project
@merge_request.check_mergeability(async: true)
if Feature.disabled?(:check_mergeability_async_in_widget, @project, default_enabled: :yaml)
@merge_request.check_mergeability(async: true)
end
respond_to do |format| respond_to do |format|
format.html do format.html do
......
...@@ -64,6 +64,10 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity ...@@ -64,6 +64,10 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity
presenter(merge_request).target_branch_commits_path presenter(merge_request).target_branch_commits_path
end end
expose :merge_request_widget_path do |merge_request|
widget_project_json_merge_request_path(merge_request.target_project, merge_request, format: :json)
end
expose :target_branch_tree_path do |merge_request| expose :target_branch_tree_path do |merge_request|
presenter(merge_request).target_branch_tree_path presenter(merge_request).target_branch_tree_path
end end
......
...@@ -29,7 +29,12 @@ class MergeRequestPollWidgetEntity < Grape::Entity ...@@ -29,7 +29,12 @@ class MergeRequestPollWidgetEntity < Grape::Entity
expose :default_merge_commit_message expose :default_merge_commit_message
expose :mergeable?, as: :mergeable expose :mergeable do |merge_request, options|
next merge_request.mergeable? if Feature.disabled?(:check_mergeability_async_in_widget, merge_request.project, default_enabled: :yaml)
next false if options[:async_mergeability_check].present? && merge_request.checking?
merge_request.mergeable?
end
expose :default_merge_commit_message_with_description do |merge_request| expose :default_merge_commit_message_with_description do |merge_request|
merge_request.default_merge_commit_message(include_description: true) merge_request.default_merge_commit_message(include_description: true)
......
...@@ -36,7 +36,7 @@ class MergeRequestWidgetEntity < Grape::Entity ...@@ -36,7 +36,7 @@ class MergeRequestWidgetEntity < Grape::Entity
end end
expose :merge_request_widget_path do |merge_request| expose :merge_request_widget_path do |merge_request|
widget_project_json_merge_request_path(merge_request.target_project, merge_request, format: :json) widget_project_json_merge_request_path(merge_request.target_project, merge_request, async_mergeability_check: true, format: :json)
end end
expose :merge_request_cached_widget_path do |merge_request| expose :merge_request_cached_widget_path do |merge_request|
......
...@@ -62,7 +62,7 @@ ...@@ -62,7 +62,7 @@
- add_page_startup_api_call notes_url - add_page_startup_api_call notes_url
- else - else
- add_page_startup_api_call discussions_path(@merge_request) - add_page_startup_api_call discussions_path(@merge_request)
- add_page_startup_api_call widget_project_json_merge_request_path(@project, @merge_request, format: :json) - add_page_startup_api_call widget_project_json_merge_request_path(@project, @merge_request, async_mergeability_check: true, format: :json)
- add_page_startup_api_call cached_widget_project_json_merge_request_path(@project, @merge_request, format: :json) - add_page_startup_api_call cached_widget_project_json_merge_request_path(@project, @merge_request, format: :json)
#js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request, Feature.enabled?(:paginated_notes, @project)).to_json, #js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request, Feature.enabled?(:paginated_notes, @project)).to_json,
endpoint_metadata: @endpoint_metadata_url, endpoint_metadata: @endpoint_metadata_url,
......
---
name: check_mergeability_async_in_widget
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/58178
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/326567
milestone: '13.11'
type: development
group: group::source code
default_enabled: false
...@@ -11,13 +11,13 @@ RSpec.describe Projects::MergeRequests::ContentController do ...@@ -11,13 +11,13 @@ RSpec.describe Projects::MergeRequests::ContentController do
sign_in(user) sign_in(user)
end end
def do_request(action = :cached_widget) def do_request(action = :cached_widget, params = {})
get action, params: { get action, params: {
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
project_id: project, project_id: project,
id: merge_request.iid, id: merge_request.iid,
format: :json format: :json
} }.merge(params)
end end
context 'user has access to the project' do context 'user has access to the project' do
...@@ -42,6 +42,10 @@ RSpec.describe Projects::MergeRequests::ContentController do ...@@ -42,6 +42,10 @@ RSpec.describe Projects::MergeRequests::ContentController do
end end
describe 'GET widget' do describe 'GET widget' do
before do
merge_request.mark_as_unchecked!
end
it 'checks whether the MR can be merged' do it 'checks whether the MR can be merged' do
controller.instance_variable_set(:@merge_request, merge_request) controller.instance_variable_set(:@merge_request, merge_request)
...@@ -53,6 +57,17 @@ RSpec.describe Projects::MergeRequests::ContentController do ...@@ -53,6 +57,17 @@ RSpec.describe Projects::MergeRequests::ContentController do
expect(response.headers['Poll-Interval']).to eq('10000') expect(response.headers['Poll-Interval']).to eq('10000')
end end
context 'when async_mergeability_check param is passed' do
it 'checks mergeability asynchronously' do
expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
expect(service).not_to receive(:execute)
expect(service).to receive(:async_execute).and_call_original
end
do_request(:widget, { async_mergeability_check: true })
end
end
context 'merged merge request' do context 'merged merge request' do
let(:merge_request) do let(:merge_request) do
create(:merged_merge_request, :with_test_reports, target_project: project, source_project: project) create(:merged_merge_request, :with_test_reports, target_project: project, source_project: project)
......
...@@ -82,13 +82,19 @@ RSpec.describe Projects::MergeRequestsController do ...@@ -82,13 +82,19 @@ RSpec.describe Projects::MergeRequestsController do
merge_request.mark_as_unchecked! merge_request.mark_as_unchecked!
end end
it 'checks mergeability asynchronously' do context 'check_mergeability_async_in_widget feature flag is disabled' do
expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service| before do
expect(service).not_to receive(:execute) stub_feature_flags(check_mergeability_async_in_widget: false)
expect(service).to receive(:async_execute)
end end
go it 'checks mergeability asynchronously' do
expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
expect(service).not_to receive(:execute)
expect(service).to receive(:async_execute)
end
go
end
end end
end end
......
...@@ -11,9 +11,10 @@ RSpec.describe MergeRequestPollWidgetEntity do ...@@ -11,9 +11,10 @@ RSpec.describe MergeRequestPollWidgetEntity do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let(:request) { double('request', current_user: user, project: project) } let(:request) { double('request', current_user: user, project: project) }
let(:options) { {} }
subject do subject do
described_class.new(resource, request: request).as_json described_class.new(resource, { request: request }.merge(options)).as_json
end end
it 'has default_merge_commit_message_with_description' do it 'has default_merge_commit_message_with_description' do
...@@ -278,4 +279,39 @@ RSpec.describe MergeRequestPollWidgetEntity do ...@@ -278,4 +279,39 @@ RSpec.describe MergeRequestPollWidgetEntity do
]) ])
end end
end end
describe '#mergeable' do
it 'shows whether a merge request is mergeable' do
expect(subject[:mergeable]).to eq(true)
end
context 'when merge request is in checking state' do
before do
resource.mark_as_unchecked!
resource.mark_as_checking!
end
it 'calculates mergeability and returns true' do
expect(subject[:mergeable]).to eq(true)
end
context 'when async_mergeability_check is passed' do
let(:options) { { async_mergeability_check: true } }
it 'returns false' do
expect(subject[:mergeable]).to eq(false)
end
context 'when check_mergeability_async_in_widget is disabled' do
before do
stub_feature_flags(check_mergeability_async_in_widget: false)
end
it 'calculates mergeability and returns true' do
expect(subject[:mergeable]).to eq(true)
end
end
end
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