diff options
Diffstat (limited to 'app/services/merge_requests')
13 files changed, 229 insertions, 26 deletions
diff --git a/app/services/merge_requests/add_context_service.rb b/app/services/merge_requests/add_context_service.rb index bb82fa23468..b693f8509a2 100644 --- a/app/services/merge_requests/add_context_service.rb +++ b/app/services/merge_requests/add_context_service.rb @@ -66,7 +66,8 @@ module MergeRequests relative_order: index, sha: sha, authored_date: Gitlab::Database.sanitize_timestamp(commit_hash[:authored_date]), - committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date]) + committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date]), + trailers: commit_hash.fetch(:trailers, {}).to_json ) end end diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index 150ec85fca9..59d8f553eff 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -14,6 +14,7 @@ module MergeRequests create_approval_note(merge_request) mark_pending_todos_as_done(merge_request) execute_approval_hooks(merge_request, current_user) + merge_request_activity_counter.track_approve_mr_action(user: current_user) success end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 0613c061f2e..6bd31e26748 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -108,7 +108,7 @@ module MergeRequests def filter_reviewer(merge_request) return if params[:reviewer_ids].blank? - unless can_admin_issuable?(merge_request) && merge_request.allows_reviewers? + unless can_admin_issuable?(merge_request) params.delete(:reviewer_ids) return diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 80991657688..12c901aa1a1 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -16,30 +16,17 @@ module MergeRequests merge_request.source_project = find_source_project merge_request.target_project = find_target_project - # Source project sets the default source branch removal setting - merge_request.merge_params['force_remove_source_branch'] = - if params.key?(:force_remove_source_branch) - params.delete(:force_remove_source_branch) - else - merge_request.source_project.remove_source_branch_after_merge? - end + # Force remove the source branch? + merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch + # Only assign merge requests params that are allowed self.params = assign_allowed_merge_params(merge_request, params) + # Filter out params that are either not allowed or invalid filter_params(merge_request) - # merge_request.assign_attributes(...) below is a Rails - # method that only work if all the params it is passed have - # corresponding fields in the database. As there are no fields - # in the database for :add_label_ids and :remove_label_ids, we - # need to remove them from the params before the call to - # merge_request.assign_attributes(...) - # - # IssuableBaseService#process_label_ids takes care - # of the removal. - params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a) - - merge_request.assign_attributes(params.to_h.compact) + # Filter out :add_label_ids and :remove_label_ids params + filter_label_id_params merge_request.compare_commits = [] set_merge_request_target_branch @@ -74,6 +61,29 @@ module MergeRequests :errors, to: :merge_request + def force_remove_source_branch + if params.key?(:force_remove_source_branch) + params.delete(:force_remove_source_branch) + else + merge_request.source_project.remove_source_branch_after_merge? + end + end + + def filter_label_id_params + # merge_request.assign_attributes(...) below is a Rails + # method that only work if all the params it is passed have + # corresponding fields in the database. As there are no fields + # in the database for :add_label_ids and :remove_label_ids, we + # need to remove them from the params before the call to + # merge_request.assign_attributes(...) + # + # IssuableBaseService#process_label_ids takes care + # of the removal. + params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a) + + merge_request.assign_attributes(params.to_h.compact) + end + def find_source_project source_project = project_from_params(:source_project) return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project) diff --git a/app/services/merge_requests/create_from_issue_service.rb b/app/services/merge_requests/create_from_issue_service.rb index 78b462174c9..b43e697d3ab 100644 --- a/app/services/merge_requests/create_from_issue_service.rb +++ b/app/services/merge_requests/create_from_issue_service.rb @@ -25,6 +25,7 @@ module MergeRequests new_merge_request = create(merge_request) if new_merge_request.valid? + merge_request_activity_counter.track_mr_create_from_issue(user: current_user) SystemNoteService.new_merge_request(issue, project, current_user, new_merge_request) success(new_merge_request) diff --git a/app/services/merge_requests/mark_reviewer_reviewed_service.rb b/app/services/merge_requests/mark_reviewer_reviewed_service.rb new file mode 100644 index 00000000000..766a4ca0a49 --- /dev/null +++ b/app/services/merge_requests/mark_reviewer_reviewed_service.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module MergeRequests + class MarkReviewerReviewedService < MergeRequests::BaseService + def execute(merge_request) + return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request) + + reviewer = merge_request.find_reviewer(current_user) + + if reviewer + return error("Failed to update reviewer") unless reviewer.update(state: :reviewed) + + success + else + error("Reviewer not found") + end + end + end +end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index f4454db0af8..fc4405ef704 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -8,6 +8,8 @@ module MergeRequests # Executed when you do merge via GitLab UI # class MergeService < MergeRequests::MergeBaseService + GENERIC_ERROR_MESSAGE = 'An error occurred while merging' + delegate :merge_jid, :state, to: :@merge_request def execute(merge_request, options = {}) @@ -79,7 +81,7 @@ module MergeRequests if commit_id log_info("Git merge finished on JID #{merge_jid} commit #{commit_id}") else - raise_error('Conflicts detected during merge') + raise_error(GENERIC_ERROR_MESSAGE) end merge_request.update!(merge_commit_sha: commit_id) @@ -96,7 +98,7 @@ module MergeRequests "Something went wrong during merge pre-receive hook. #{e.message}".strip rescue => e handle_merge_error(log_message: e.message) - raise_error('Something went wrong during merge') + raise_error(GENERIC_ERROR_MESSAGE) end def after_merge diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb index 96a2322f6a0..9fecab85cc1 100644 --- a/app/services/merge_requests/mergeability_check_service.rb +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -114,6 +114,7 @@ module MergeRequests merge_to_ref_success = merge_to_ref + reload_merge_head_diff update_diff_discussion_positions! if merge_to_ref_success if merge_to_ref_success && can_git_merge? @@ -123,6 +124,10 @@ module MergeRequests end end + def reload_merge_head_diff + MergeRequests::ReloadMergeHeadDiffService.new(merge_request).execute + end + def update_diff_discussion_positions! Discussions::CaptureDiffNotePositionsService.new(merge_request).execute end @@ -153,6 +158,7 @@ module MergeRequests def merge_to_ref params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) } result = MergeRequests::MergeToRefService.new(project, merge_request.author, params).execute(merge_request) + result[:status] == :success end diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index f04ec3c3e80..aafba9bfcef 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -9,6 +9,8 @@ module MergeRequests class PostMergeService < MergeRequests::BaseService include RemovesRefs + MAX_RETARGET_MERGE_REQUESTS = 4 + def execute(merge_request) merge_request.mark_as_merged close_issues(merge_request) @@ -18,6 +20,7 @@ module MergeRequests merge_request_activity_counter.track_merge_mr_action(user: current_user) notification_service.merge_mr(merge_request, current_user) execute_hooks(merge_request, 'merge') + retarget_chain_merge_requests(merge_request) invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers) merge_request.update_project_counter_caches delete_non_latest_diffs(merge_request) @@ -28,6 +31,34 @@ module MergeRequests private + def retarget_chain_merge_requests(merge_request) + return unless Feature.enabled?(:retarget_merge_requests, merge_request.target_project) + + # we can only retarget MRs that are targeting the same project + # and have a remove source branch set + return unless merge_request.for_same_project? && merge_request.remove_source_branch? + + # find another merge requests that + # - as a target have a current source project and branch + other_merge_requests = merge_request.source_project + .merge_requests + .opened + .by_target_branch(merge_request.source_branch) + .preload_source_project + .at_most(MAX_RETARGET_MERGE_REQUESTS) + + other_merge_requests.find_each do |other_merge_request| + # Update only MRs on projects that we have access to + next unless can?(current_user, :update_merge_request, other_merge_request.source_project) + + ::MergeRequests::UpdateService + .new(other_merge_request.source_project, current_user, + target_branch: merge_request.target_branch, + target_branch_was_deleted: true) + .execute(other_merge_request) + end + end + def close_issues(merge_request) return unless merge_request.target_branch == project.default_branch diff --git a/app/services/merge_requests/reload_merge_head_diff_service.rb b/app/services/merge_requests/reload_merge_head_diff_service.rb new file mode 100644 index 00000000000..f02a9bd3139 --- /dev/null +++ b/app/services/merge_requests/reload_merge_head_diff_service.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module MergeRequests + class ReloadMergeHeadDiffService + include BaseServiceUtility + + def initialize(merge_request) + @merge_request = merge_request + end + + def execute + return error("default_merge_ref_for_diffs feature flag is disabled") unless enabled? + return error("Merge request has no merge ref head.") unless merge_request.merge_ref_head.present? + + error_msg = recreate_merge_head_diff + + return error(error_msg) if error_msg + + success + end + + private + + attr_reader :merge_request + + def enabled? + Feature.enabled?(:default_merge_ref_for_diffs, merge_request.project, default_enabled: :yaml) + end + + def recreate_merge_head_diff + merge_request.merge_head_diff&.destroy! + + # n+1: https://gitlab.com/gitlab-org/gitlab/-/issues/19377 + Gitlab::GitalyClient.allow_n_plus_1_calls do + merge_request.create_merge_head_diff! + end + + # Reset the merge request so it won't load the merge head diff as the + # MergeRequest#merge_request_diff. + merge_request.reset + + nil + rescue StandardError => e + message = "Failed to recreate merge head diff: #{e.message}" + + Gitlab::AppLogger.error(message: message, merge_request_id: merge_request.id) + message + end + end +end diff --git a/app/services/merge_requests/remove_approval_service.rb b/app/services/merge_requests/remove_approval_service.rb index 3164d0b4069..f2bf5de61c1 100644 --- a/app/services/merge_requests/remove_approval_service.rb +++ b/app/services/merge_requests/remove_approval_service.rb @@ -16,6 +16,7 @@ module MergeRequests reset_approvals_cache(merge_request) create_note(merge_request) + merge_request_activity_counter.track_unapprove_mr_action(user: current_user) end success diff --git a/app/services/merge_requests/request_review_service.rb b/app/services/merge_requests/request_review_service.rb new file mode 100644 index 00000000000..b061ed45fee --- /dev/null +++ b/app/services/merge_requests/request_review_service.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module MergeRequests + class RequestReviewService < MergeRequests::BaseService + def execute(merge_request, user) + return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request) + + reviewer = merge_request.find_reviewer(user) + + if reviewer + return error("Failed to update reviewer") unless reviewer.update(state: :unreviewed) + + notify_reviewer(merge_request, user) + + success + else + error("Reviewer not found") + end + end + + private + + def notify_reviewer(merge_request, reviewer) + notification_service.async.review_requested_of_merge_request(merge_request, current_user, reviewer) + todo_service.create_request_review_todo(merge_request, current_user, reviewer) + end + end +end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index d2e5a2a1619..1707daff734 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -4,6 +4,12 @@ module MergeRequests class UpdateService < MergeRequests::BaseService extend ::Gitlab::Utils::Override + def initialize(project, user = nil, params = {}) + super + + @target_branch_was_deleted = @params.delete(:target_branch_was_deleted) + end + def execute(merge_request) # We don't allow change of source/target projects and source branch # after merge request was created @@ -36,7 +42,9 @@ module MergeRequests end if merge_request.previous_changes.include?('target_branch') - create_branch_change_note(merge_request, 'target', + create_branch_change_note(merge_request, + 'target', + target_branch_was_deleted ? 'delete' : 'update', merge_request.previous_changes['target_branch'].first, merge_request.target_branch) @@ -87,12 +95,51 @@ module MergeRequests MergeRequests::CloseService end + def before_update(issuable, skip_spam_check: false) + return unless issuable.changed? + + @issuable_changes = issuable.changes + end + def after_update(issuable) issuable.cache_merge_request_closes_issues!(current_user) + + return unless @issuable_changes + + %w(title description).each do |action| + next unless @issuable_changes.key?(action) + + # Track edits to title or description + # + merge_request_activity_counter + .public_send("track_#{action}_edit_action".to_sym, user: current_user) # rubocop:disable GitlabSecurity/PublicSend + + # Track changes to Draft/WIP status + # + if action == "title" + old_title, new_title = @issuable_changes["title"] + old_title_wip = MergeRequest.work_in_progress?(old_title) + new_title_wip = MergeRequest.work_in_progress?(new_title) + + if !old_title_wip && new_title_wip + # Marked as Draft/WIP + # + merge_request_activity_counter + .track_marked_as_draft_action(user: current_user) + elsif old_title_wip && !new_title_wip + # Unmarked as Draft/WIP + # + merge_request_activity_counter + .track_unmarked_as_draft_action(user: current_user) + end + end + end end private + attr_reader :target_branch_was_deleted + def handle_milestone_change(merge_request) return if skip_milestone_email @@ -109,6 +156,9 @@ module MergeRequests create_assignee_note(merge_request, old_assignees) notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees) todo_service.reassigned_assignable(merge_request, current_user, old_assignees) + + new_assignees = merge_request.assignees - old_assignees + merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees) end def handle_reviewers_change(merge_request, old_reviewers) @@ -117,11 +167,14 @@ module MergeRequests notification_service.async.changed_reviewer_of_merge_request(merge_request, current_user, old_reviewers) todo_service.reassigned_reviewable(merge_request, current_user, old_reviewers) invalidate_cache_counts(merge_request, users: affected_reviewers.compact) + + new_reviewers = merge_request.reviewers - old_reviewers + merge_request_activity_counter.track_users_review_requested(users: new_reviewers) end - def create_branch_change_note(issuable, branch_type, old_branch, new_branch) + def create_branch_change_note(issuable, branch_type, event_type, old_branch, new_branch) SystemNoteService.change_branch( - issuable, issuable.project, current_user, branch_type, + issuable, issuable.project, current_user, branch_type, event_type, old_branch, new_branch) end |