diff options
Diffstat (limited to 'app/services/merge_requests')
13 files changed, 156 insertions, 19 deletions
diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index 37c2676e51c..e3f0758699b 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -33,6 +33,8 @@ module MergeRequests def execute_approval_hooks(merge_request, current_user) # Only one approval is required for a merge request to be approved + notification_service.async.approve_mr(merge_request, current_user) + execute_hooks(merge_request, 'approved') end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index d197c13378a..44be254441d 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -43,6 +43,8 @@ module MergeRequests 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,17 +60,16 @@ module MergeRequests 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 def cleanup_environments(merge_request) Environments::StopService.new(merge_request.source_project, current_user) - .execute_for_merge_request(merge_request) + .execute_for_merge_request_pipeline(merge_request) end def cancel_review_app_jobs!(merge_request) @@ -246,7 +247,7 @@ module MergeRequests 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,9 +255,49 @@ module MergeRequests 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 = if assignee.user_id == current_user&.id + :unreviewed + else + merge_request.find_reviewer(assignee.assignee)&.state || :attention_requested + end + + [ + 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 = if reviewer.user_id == current_user&.id + :unreviewed + else + merge_request.find_assignee(reviewer.reviewer)&.state || :attention_requested + end + + [ + reviewer, + { state: MergeRequestReviewer.states[state], updated_state_by_user_id: current_user.id } + ] + end - ::MergeRequests::RemoveAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request).execute + ::Gitlab::Database::BulkUpdate.execute(%i[state updated_state_by_user_id], reviewers_map) end end end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 878e42172b7..ee6f204be45 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -203,6 +203,12 @@ module MergeRequests target_branch.blank? || target_project.commit(target_branch) end + def set_draft_title_if_needed + return unless compare_commits.empty? || Gitlab::Utils.to_boolean(params[:draft]) + + merge_request.title = wip_title + end + # When your branch name starts with an iid followed by a dash this pattern will be # interpreted as the user wants to close that issue on this project. # @@ -220,7 +226,7 @@ module MergeRequests assign_title_and_description_from_commits merge_request.title ||= title_from_issue if target_project.issues_enabled? || target_project.external_issue_tracker merge_request.title ||= source_branch.titleize.humanize - merge_request.title = wip_title if compare_commits.empty? + set_draft_title_if_needed append_closes_description end diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 9c525ae8489..8e0f72eb380 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -25,9 +25,7 @@ module MergeRequests # expose issuable create method so it can be called from email # handler CreateMergeRequestHandler - def create(merge_request) - super - end + public :create private diff --git a/app/services/merge_requests/handle_assignees_change_service.rb b/app/services/merge_requests/handle_assignees_change_service.rb index a169a6dc0b6..78c93d10f2a 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 @@ module MergeRequests 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/mergeability/run_checks_service.rb b/app/services/merge_requests/mergeability/run_checks_service.rb index 03c6d985c23..fd6907c976b 100644 --- a/app/services/merge_requests/mergeability/run_checks_service.rb +++ b/app/services/merge_requests/mergeability/run_checks_service.rb @@ -37,7 +37,7 @@ module MergeRequests attr_reader :merge_request, :params def run_check(check) - return check.execute unless Feature.enabled?(:mergeability_caching, merge_request.project, default_enabled: :yaml) + return check.execute unless Feature.enabled?(:mergeability_caching, merge_request.project) return check.execute unless check.cacheable? cached_result = results.read(merge_check: check) diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index e475b57e4a2..980c757bcbc 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -45,7 +45,19 @@ module MergeRequests closed_issues = merge_request.visible_closing_issues_for(current_user) closed_issues.each do |issue| - Issues::CloseService.new(project: project, current_user: current_user).execute(issue, commit: merge_request) + # We are intentionally only closing Issues asynchronously (excluding ExternalIssues) + # as the worker only supports finding an Issue. We are also only experiencing + # SQL timeouts when closing an Issue. + if Feature.enabled?(:async_mr_close_issue, project) && issue.is_a?(Issue) + MergeRequests::CloseIssueWorker.perform_async( + project.id, + current_user.id, + issue.id, + merge_request.id + ) + else + Issues::CloseService.new(project: project, current_user: current_user).execute(issue, commit: merge_request) + end end end diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb index adbe3ddfdad..076fe8c3b21 100644 --- a/app/services/merge_requests/push_options_handler_service.rb +++ b/app/services/merge_requests/push_options_handler_service.rb @@ -126,6 +126,7 @@ module MergeRequests params = { title: push_options[:title], description: push_options[:description], + draft: push_options[:draft], target_branch: push_options[:target], force_remove_source_branch: push_options[:remove_source_branch], label: push_options[:label], @@ -147,6 +148,10 @@ module MergeRequests params[:milestone] = milestone if milestone end + if params.key?(:description) + params[:description] = params[:description].gsub('\n', "\n") + end + params end diff --git a/app/services/merge_requests/remove_approval_service.rb b/app/services/merge_requests/remove_approval_service.rb index c7bc3532264..d9bb17a7b1b 100644 --- a/app/services/merge_requests/remove_approval_service.rb +++ b/app/services/merge_requests/remove_approval_service.rb @@ -33,6 +33,7 @@ module MergeRequests def trigger_approval_hooks(merge_request) yield + notification_service.async.unapprove_mr(merge_request, current_user) execute_hooks(merge_request, 'unapproved') end diff --git a/app/services/merge_requests/remove_attention_requested_service.rb b/app/services/merge_requests/remove_attention_requested_service.rb index a32a8071471..8a410fda691 100644 --- a/app/services/merge_requests/remove_attention_requested_service.rb +++ b/app/services/merge_requests/remove_attention_requested_service.rb @@ -2,22 +2,26 @@ module MergeRequests class RemoveAttentionRequestedService < MergeRequests::BaseService - attr_accessor :merge_request + attr_accessor :merge_request, :user - def initialize(project:, current_user:, merge_request:) + def initialize(project:, current_user:, merge_request:, user:) super(project: project, current_user: current_user) @merge_request = merge_request + @user = user end def execute return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request) if reviewer || assignee + return success if reviewer&.reviewed? || assignee&.reviewed? + update_state(reviewer) update_state(assignee) - current_user.invalidate_attention_requested_count + user.invalidate_attention_requested_count + create_remove_attention_request_note success else @@ -28,15 +32,19 @@ module MergeRequests private def assignee - merge_request.find_assignee(current_user) + @assignee ||= merge_request.find_assignee(user) end def reviewer - merge_request.find_reviewer(current_user) + @reviewer ||= merge_request.find_reviewer(user) end def update_state(reviewer_or_assignee) reviewer_or_assignee&.update(state: :reviewed) end + + def create_remove_attention_request_note + SystemNoteService.remove_attention_request(merge_request, merge_request.project, current_user, user) + end end end diff --git a/app/services/merge_requests/request_attention_service.rb b/app/services/merge_requests/request_attention_service.rb new file mode 100644 index 00000000000..07e9996f87b --- /dev/null +++ b/app/services/merge_requests/request_attention_service.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module MergeRequests + class RequestAttentionService < MergeRequests::BaseService + attr_accessor :merge_request, :user + + def initialize(project:, current_user:, merge_request:, user:) + super(project: project, current_user: current_user) + + @merge_request = merge_request + @user = user + end + + def execute + return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request) + + if reviewer || assignee + return success if reviewer&.attention_requested? || assignee&.attention_requested? + + update_state(reviewer) + update_state(assignee) + + user.invalidate_attention_requested_count + create_attention_request_note + notity_user + + if current_user.id != user.id + remove_attention_requested(merge_request) + end + + success + else + error("User is not a reviewer or assignee of the merge request") + end + end + + private + + def notity_user + notification_service.async.attention_requested_of_merge_request(merge_request, current_user, user) + todo_service.create_attention_requested_todo(merge_request, current_user, user) + end + + def create_attention_request_note + SystemNoteService.request_attention(merge_request, merge_request.project, current_user, user) + end + + def assignee + @assignee ||= merge_request.find_assignee(user) + end + + def reviewer + @reviewer ||= merge_request.find_reviewer(user) + end + + def update_state(reviewer_or_assignee) + reviewer_or_assignee&.update(state: :attention_requested, updated_state_by: current_user) + end + end +end diff --git a/app/services/merge_requests/update_assignees_service.rb b/app/services/merge_requests/update_assignees_service.rb index d52c1bbbcda..5b23f69ac4a 100644 --- a/app/services/merge_requests/update_assignees_service.rb +++ b/app/services/merge_requests/update_assignees_service.rb @@ -20,6 +20,8 @@ module MergeRequests 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/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 391079223ca..6e8afaecbba 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -11,6 +11,10 @@ module MergeRequests end def execute(merge_request) + if Gitlab::Utils.to_boolean(params[:draft]) + merge_request.title = merge_request.draft_title + end + update_merge_request_with_specialized_service(merge_request) || general_fallback(merge_request) end |