diff options
Diffstat (limited to 'app/services/merge_requests')
11 files changed, 72 insertions, 34 deletions
diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index 64ae33c9b15..5761e34caff 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -3,7 +3,7 @@ module MergeRequests class ApprovalService < MergeRequests::BaseService def execute(merge_request) - return unless can_be_approved?(merge_request) + return unless eligible_for_approval?(merge_request) approval = merge_request.approvals.new(user: current_user) @@ -28,8 +28,8 @@ module MergeRequests private - def can_be_approved?(merge_request) - merge_request.can_be_approved_by?(current_user) + def eligible_for_approval?(merge_request) + merge_request.eligible_for_approval_by?(current_user) end def save_approval(approval) diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 6cefd9169f5..cfd7c645b7e 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -58,6 +58,7 @@ 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) + trigger_merge_request_reviewers_updated(merge_request) end def cleanup_environments(merge_request) @@ -244,6 +245,10 @@ module MergeRequests Milestones::MergeRequestsCountService.new(milestone).delete_cache end + + def trigger_merge_request_reviewers_updated(merge_request) + GraphqlTriggers.merge_request_reviewers_updated(merge_request) + end end end diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index f83b14c7269..da3a9652d69 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -23,6 +23,7 @@ module MergeRequests cleanup_environments(merge_request) abort_auto_merge(merge_request, 'merge request was closed') cleanup_refs(merge_request) + trigger_merge_request_merge_status_updated(merge_request) end merge_request @@ -38,5 +39,9 @@ module MergeRequests merge_request_metrics_service(merge_request).close(close_event) end end + + def trigger_merge_request_merge_status_updated(merge_request) + GraphqlTriggers.merge_request_merge_status_updated(merge_request) + end end end diff --git a/app/services/merge_requests/mark_reviewer_reviewed_service.rb b/app/services/merge_requests/mark_reviewer_reviewed_service.rb index 766a4ca0a49..96747eabcf6 100644 --- a/app/services/merge_requests/mark_reviewer_reviewed_service.rb +++ b/app/services/merge_requests/mark_reviewer_reviewed_service.rb @@ -10,6 +10,8 @@ module MergeRequests if reviewer return error("Failed to update reviewer") unless reviewer.update(state: :reviewed) + trigger_merge_request_reviewers_updated(merge_request) + success else error("Reviewer not found") diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb index 3e630d40b3d..2a3c1e8bc26 100644 --- a/app/services/merge_requests/merge_base_service.rb +++ b/app/services/merge_requests/merge_base_service.rb @@ -9,12 +9,12 @@ module MergeRequests attr_reader :merge_request # Overridden in EE. - def hooks_validation_pass?(_merge_request) + def hooks_validation_pass?(merge_request, validate_squash_message: false) true end # Overridden in EE. - def hooks_validation_error(_merge_request) + def hooks_validation_error(merge_request, validate_squash_message: false) # No-op end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 6d31a29f5a7..6b4f9dbe509 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -26,6 +26,7 @@ module MergeRequests @merge_request = merge_request @options = options + jid = merge_jid validate! @@ -37,7 +38,7 @@ module MergeRequests end end - log_info("Merge process finished on JID #{merge_jid} with state #{state}") + log_info("Merge process finished on JID #{jid} with state #{state}") rescue MergeError => e handle_merge_error(log_message: e.message, save_message_on_model: true) ensure @@ -159,17 +160,32 @@ module MergeRequests end def handle_merge_error(log_message:, save_message_on_model: false) - Gitlab::AppLogger.error("MergeService ERROR: #{merge_request_info} - #{log_message}") + log_error("MergeService ERROR: #{merge_request_info} - #{log_message}") @merge_request.update(merge_error: log_message) if save_message_on_model end def log_info(message) + payload = log_payload("#{merge_request_info} - #{message}") + logger.info(**payload) + end + + def log_error(message) + payload = log_payload(message) + logger.error(**payload) + end + + def logger @logger ||= Gitlab::AppLogger - @logger.info("#{merge_request_info} - #{message}") + end + + def log_payload(message) + Gitlab::ApplicationContext.current + .merge(merge_request_info: merge_request_info, + message: message) end def merge_request_info - merge_request.to_reference(full: true) + @merge_request_info ||= merge_request.to_reference(full: true) end def source_matches? diff --git a/app/services/merge_requests/mergeability/logger.rb b/app/services/merge_requests/mergeability/logger.rb index 8b45d231e03..88ef6d81eaa 100644 --- a/app/services/merge_requests/mergeability/logger.rb +++ b/app/services/merge_requests/mergeability/logger.rb @@ -11,16 +11,12 @@ module MergeRequests end def commit - return unless enabled? - commit_logs end def instrument(mergeability_name:) raise ArgumentError, 'block not given' unless block_given? - return yield unless enabled? - op_start_db_counters = current_db_counter_payload op_started_at = current_monotonic_time @@ -38,15 +34,11 @@ module MergeRequests attr_reader :destination, :merge_request def observe(name, value) - return unless enabled? - observations[name.to_s].push(value) end def commit_logs - attributes = Gitlab::ApplicationContext.current.merge({ - mergeability_project_id: merge_request.project.id - }) + attributes = Gitlab::ApplicationContext.current.merge({ mergeability_project_id: merge_request.project.id }) attributes[:mergeability_merge_request_id] = merge_request.id attributes.merge!(observations_hash) @@ -89,12 +81,6 @@ module MergeRequests ::Gitlab::Metrics::Subscribers::ActiveRecord.db_counter_payload end - def enabled? - strong_memoize(:enabled) do - ::Feature.enabled?(:mergeability_checks_logger, merge_request.project) - end - end - def current_monotonic_time ::Gitlab::Metrics::System.monotonic_time end diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb index ef251f121ae..aa52349b0ee 100644 --- a/app/services/merge_requests/push_options_handler_service.rb +++ b/app/services/merge_requests/push_options_handler_service.rb @@ -104,7 +104,7 @@ module MergeRequests merge_request = ::MergeRequests::CreateService.new( project: project, current_user: current_user, - params: merge_request.attributes.merge(assignees: merge_request.assignees, + params: merge_request.attributes.merge(assignee_ids: merge_request.assignee_ids, label_ids: merge_request.label_ids) ).execute end @@ -140,8 +140,8 @@ module MergeRequests params[:add_labels] = params.delete(:label).keys if params.has_key?(:label) params[:remove_labels] = params.delete(:unlabel).keys if params.has_key?(:unlabel) - params[:add_assignee_ids] = params.delete(:assign).keys if params.has_key?(:assign) - params[:remove_assignee_ids] = params.delete(:unassign).keys if params.has_key?(:unassign) + params[:add_assignee_ids] = convert_to_user_ids(params.delete(:assign).keys) if params.has_key?(:assign) + params[:remove_assignee_ids] = convert_to_user_ids(params.delete(:unassign).keys) if params.has_key?(:unassign) if push_options[:milestone] milestone = Milestone.for_projects_and_groups(@project, @project.ancestors_upto)&.find_by_name(push_options[:milestone]) @@ -169,7 +169,7 @@ module MergeRequests params = base_params params.merge!( - assignees: [current_user], + assignee_ids: [current_user.id], source_branch: branch, source_project: project, target_project: target_project @@ -186,6 +186,12 @@ module MergeRequests base_params.merge(merge_params(merge_request.source_branch)) end + def convert_to_user_ids(ids_or_usernames) + ids, usernames = ids_or_usernames.partition { |id_or_username| id_or_username.is_a?(Numeric) || id_or_username.match?(/\A\d+\z/) } + ids += User.by_username(usernames).pluck(:id) unless usernames.empty? # rubocop:disable CodeReuse/ActiveRecord + ids + end + def collect_errors_from_merge_request(merge_request) merge_request.errors.full_messages.each do |error| errors << error diff --git a/app/services/merge_requests/request_review_service.rb b/app/services/merge_requests/request_review_service.rb index b061ed45fee..ebbae98352b 100644 --- a/app/services/merge_requests/request_review_service.rb +++ b/app/services/merge_requests/request_review_service.rb @@ -11,6 +11,7 @@ module MergeRequests return error("Failed to update reviewer") unless reviewer.update(state: :unreviewed) notify_reviewer(merge_request, user) + trigger_merge_request_reviewers_updated(merge_request) success else diff --git a/app/services/merge_requests/update_assignees_service.rb b/app/services/merge_requests/update_assignees_service.rb index a13db52e34b..79a3e9f3c22 100644 --- a/app/services/merge_requests/update_assignees_service.rb +++ b/app/services/merge_requests/update_assignees_service.rb @@ -18,7 +18,17 @@ module MergeRequests return merge_request if old_ids.to_set == new_ids.to_set # no-change attrs = update_attrs.merge(assignee_ids: new_ids) - merge_request.update!(**attrs) + + # We now have assignees validation on merge request + # If we use an update with bang, it will explode, + # instead we need to check if its valid then return if its not valid. + if Feature.enabled?(:limit_assignees_per_issuable) + merge_request.update(**attrs) + + return merge_request unless merge_request.valid? + else + merge_request.update!(**attrs) + end # Defer the more expensive operations (handle_assignee_changes) to the background MergeRequests::HandleAssigneesChangeService diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 6d518edc88f..745647b727c 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -38,7 +38,6 @@ module MergeRequests handle_target_branch_change(merge_request) handle_milestone_change(merge_request) handle_draft_status_change(merge_request, changed_fields) - handle_label_changes(merge_request, old_labels) track_title_and_desc_edits(changed_fields) track_discussion_lock_toggle(merge_request, changed_fields) @@ -71,7 +70,8 @@ module MergeRequests MergeRequests::CloseService end - def after_update(issuable) + def after_update(issuable, old_associations) + super issuable.cache_merge_request_closes_issues!(current_user) end @@ -179,9 +179,12 @@ module MergeRequests old_title_draft = MergeRequest.draft?(old_title) new_title_draft = MergeRequest.draft?(new_title) - # notify the draft status changed. Added/removed message is handled in the - # email template itself, see `change_in_merge_request_draft_status_email` template. - notify_draft_status_changed(merge_request) if old_title_draft || new_title_draft + if old_title_draft || new_title_draft + # notify the draft status changed. Added/removed message is handled in the + # email template itself, see `change_in_merge_request_draft_status_email` template. + notify_draft_status_changed(merge_request) + trigger_merge_request_status_updated(merge_request) + end if !old_title_draft && new_title_draft # Marked as Draft @@ -320,6 +323,10 @@ module MergeRequests def filter_sentinel_values(param) param.reject { _1 == 0 } end + + def trigger_merge_request_status_updated(merge_request) + GraphqlTriggers.merge_request_merge_status_updated(merge_request) + end end end |