Skip to content

Refactor how we call `MergeRequests::BaseService#reset_approvals` and `MergeRequests::BaseService#delete_approvals`

Background

We are no longer calling MergeRequests::BaseService#reset_approvals in other services aside from MergeRequests::ResetApprovalsService but it's still in MergeRequests::BaseService.

We are also calling MergeRequests::BaseService#delete_approvals in MergeRequests::UpdateService#create_branch_change_note which is not really its responsibility. We do it because we only want to delete approvals in EE when target branch changes.

Proposal

  1. Move MergeRequests::BaseService#reset_approvals to MergeRequests::ResetApprovalsService.
  2. Create a MergeRequests::UpdateService#delete_approvals_on_target_branch_change, call it in MergeRequests::UpdateService#handle_target_branch_change after calling #create_branch_change_note. Then in EE, override #delete_approvals_on_target_branch_change to call delete_approvals(merge_request) if reset_approvals?(merge_request, nil).

Thanks to @ebaque and @nmalcolm for the suggestions!

Edited by Patrick Bajao