[go: up one dir, main page]

Skip to content

Refactor MergeRequests::ApprovalService to return reason

Summary

As of this writing, when MergeRequests::ApprovalService#execute is called and it fails, it returns nil or false and the consumer doesn't know what's the reason for it.

This was initially brought up in #372972 (comment 1094005013).

Improvements

Once !97541 (merged) is merged, we can refactor the MergeRequests::ApprovalService

Risks

Approval failure handling at consumer level (controller and API endpoint) may break. This can be mitigated by making sure failure cases are covered in tests of specific consumers.

Involved components

Service

  • app/services/merge_requests/approval_service.rb
  • ee/app/services/ee/merge_requests/approval_service.rb

Consumers

  • app/controllers/projects/merge_requests/drafts_controller.rb
  • lib/api/merge_request_approvals.rb
  • lib/gitlab/quick_actions/merge_request_actions.rb