From 76c57ba39d92d4ed06607432d174a1aec6dd194a Mon Sep 17 00:00:00 2001 From: Mark Chao <mchao@gitlab.com> Date: Thu, 25 Oct 2018 21:46:53 +0800 Subject: [PATCH] Include code owner in `overall_approvers` Its return type is change from `Approver` to `User`, as code owner is type `User`, and it does not make sense to wrap them in `Approver`. When approvers are overriden, code owner is not included, because the persisted approvers will include code owners already. Add endpoint to delete approver by user_id, since overall_approvers now returns User, and this avoids one query for fetching user's approver association. --- config/routes/project.rb | 1 + .../merge_requests/merge_request_approvals.md | 48 ++++++++----------- .../projects/approvers_controller.rb | 7 +++ ee/app/models/approver.rb | 4 ++ ee/app/models/concerns/visible_approvable.rb | 21 ++++---- .../ee/merge_requests/update_service.rb | 4 +- ee/app/services/ee/notification_service.rb | 4 +- ee/app/services/ee/todo_service.rb | 2 +- .../shared/issuable/_approvals.html.haml | 8 ++-- .../1012-assign-code-owner-as-approver.yml | 5 ++ ee/spec/models/visible_approvable_spec.rb | 10 ++-- 11 files changed, 64 insertions(+), 50 deletions(-) create mode 100644 ee/changelogs/unreleased/1012-assign-code-owner-as-approver.yml diff --git a/config/routes/project.rb b/config/routes/project.rb index 7209b0132d4..0261c62a035 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -156,6 +156,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do ## EE-specific resources :approvers, only: :destroy + delete 'approvers', to: 'approvers#destroy_via_user_id', as: :approver_via_user_id resources :approver_groups, only: :destroy ## EE-specific diff --git a/doc/user/project/merge_requests/merge_request_approvals.md b/doc/user/project/merge_requests/merge_request_approvals.md index 4e12e3f0184..2623adc27b5 100644 --- a/doc/user/project/merge_requests/merge_request_approvals.md +++ b/doc/user/project/merge_requests/merge_request_approvals.md @@ -19,7 +19,6 @@ To edit the merge request approvals: 1. Navigate to your project's **Settings > General** and expand the **Merge requests settings** -1. Tick the "Merge requests approvals" checkbox 1. Search for users or groups that will be [eligible to approve](#eligible-approvers) merge requests and click the **Add** button to add them as approvers 1. Set the minimum number of required approvals under the "Approvals required" @@ -36,36 +35,31 @@ suitable to your workflow: [overridden per merge request](#overriding-the-merge-request-approvals-default-settings) - Choose whether [approvals will be reset with new pushed commits](#resetting-approvals-on-push) -NOTE: **Note:** -If the approvers are changed via the project's settings after a merge request -is created, the merge request retains the previous approvers, but you can always -change them by [editing the merge request](#overriding-the-merge-request-approvals-default-settings). - ## Eligible approvers -An individual user is an eligible approver if they are a member of the given project, -a member of the project's immediate parent group, or a member of a group that has share access -to the project via a [share](../members/share_project_with_groups.md). +The following can approve merge requests: + +- Users being added as approvers at project or merge request level. +- [Code owners](../code_owners.md) related to the merge request ([introduced](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7933) in [GitLab Starter](https://about.gitlab.com/pricing/) 11.5). -A group is also an eligible approver. [In the future](https://gitlab.com/gitlab-org/gitlab-ee/issues/2048), +An individual user can be added as an approver for a project if they are a member of: + +- The project. +- The project's immediate parent group. +- A group that has access to the project via a [share](../members/share_project_with_groups.md). + +A group can also be added as an approver. [In the future](https://gitlab.com/gitlab-org/gitlab-ee/issues/2048), group approvers will be restricted. If a user is added as an individual approver and is also part of a group approver, then that user is just counted once. The merge request author does not count as an eligible approver, unless [self-approval] is explicitly enabled on the project settings. -Let's say that `m` is the number of required approvals, and `Ω` is the set of -explicit approvers. Depending on their number, there are different cases: - -- If `m <= count(Ω)`, then only those explicit approvers can approve the merge request. -- If `m > count(Ω)` , then all the explicit approvers _and_ the members of the given - project with Developer role or higher are eligible approvers of the merge - request. +### Implicit approvers -NOTE: **Note:** -If the approvals settings are [overridden](#overriding-the-merge-request-approvals-default-settings) -for the particular merge request, then the set of explicit approvers is the -union of the default approvers and the extra approvers set in the merge request. +If the number of required approvals is greater than the number of approvers, +other users will become implicit approvers to fill the gap. +Those implicit approvers include members of the given project with Developer role or higher. ## Adding or removing an approval @@ -124,6 +118,12 @@ First, you have to enable this option in the project's settings: 1. Click **Save changes** +NOTE: **Note:** +If approver overriding is enabled +and the project level approvers are changed after a merge request is created, +the merge request retains the previous approvers. +However, the approvers can be changed by [editing the merge request](#overriding-the-merge-request-approvals-default-settings). + --- The default approval settings can now be overridden when creating a @@ -200,9 +200,3 @@ request itself. It belongs to the target branch's project. ## Approver suggestions Approvers are suggested for merge requests based on the previous authors of the files affected by the merge request. - -### CODEOWNERS file - -> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7437>) in [GitLab Premium](https://about.gitlab.com/pricing/) 11.4. - -If the [CODEOWNERS](../code_owners.md) file is present in the target branch, more precise suggestions are provided based on its rules. diff --git a/ee/app/controllers/projects/approvers_controller.rb b/ee/app/controllers/projects/approvers_controller.rb index 826b434c9a3..11e6d914064 100644 --- a/ee/app/controllers/projects/approvers_controller.rb +++ b/ee/app/controllers/projects/approvers_controller.rb @@ -1,12 +1,19 @@ class Projects::ApproversController < Projects::ApplicationController before_action :authorize_for_subject! + # @deprecated def destroy subject.approvers.find(params[:id]).destroy redirect_back_or_default(default: { action: 'index' }) end + def destroy_via_user_id + subject.approvers.find_by_user_id(params[:user_id]).destroy + + redirect_back_or_default(default: { action: 'index' }) + end + private def authorize_for_subject! diff --git a/ee/app/models/approver.rb b/ee/app/models/approver.rb index 77a53e700a3..2c2575fa2c0 100644 --- a/ee/app/models/approver.rb +++ b/ee/app/models/approver.rb @@ -3,4 +3,8 @@ class Approver < ActiveRecord::Base belongs_to :user validates :user, presence: true + + def find_by_user_id(user_id) + find_by(user_id: user_id) + end end diff --git a/ee/app/models/concerns/visible_approvable.rb b/ee/app/models/concerns/visible_approvable.rb index 9eef34d9d53..f86576a3092 100644 --- a/ee/app/models/concerns/visible_approvable.rb +++ b/ee/app/models/concerns/visible_approvable.rb @@ -23,14 +23,23 @@ module VisibleApprovable # Before a merge request has been created, author will be nil, so pass the current user # on the MR create page. # + # @return [Array<User>] def overall_approvers - approvers_relation = approvers_overwritten? ? approvers : target_project.approvers + if approvers_overwritten? + code_owners = [] # already persisted into database, no need to recompute + approvers_relation = approvers + else + code_owners = self.code_owners.dup + approvers_relation = target_project.approvers + end if author && !authors_can_approve? approvers_relation = approvers_relation.where.not(user_id: author.id) end - approvers_relation.includes(:user) + results = code_owners.concat(approvers_relation.includes(:user).map(&:user)) + results.uniq! + results end def overall_approver_groups @@ -41,8 +50,8 @@ module VisibleApprovable strong_memoize(:all_approvers_including_groups) do approvers = [] - # Approvers from direct assignment - approvers << approvers_from_users + # Approvers not sourced from group level + approvers << overall_approvers approvers << approvers_from_groups @@ -50,10 +59,6 @@ module VisibleApprovable end end - def approvers_from_users - overall_approvers.map(&:user) - end - def approvers_from_groups group_approvers = overall_approver_groups.flat_map(&:users) group_approvers.delete(author) unless authors_can_approve? diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb index 810b86bbade..360997aa0e4 100644 --- a/ee/app/services/ee/merge_requests/update_service.rb +++ b/ee/app/services/ee/merge_requests/update_service.rb @@ -8,11 +8,11 @@ module EE override :execute def execute(merge_request) should_remove_old_approvers = params.delete(:remove_old_approvers) - old_approvers = merge_request.overall_approvers.to_a + old_approvers = merge_request.overall_approvers merge_request = super(merge_request) - new_approvers = merge_request.overall_approvers.to_a - old_approvers + new_approvers = merge_request.overall_approvers - old_approvers if new_approvers.any? todo_service.add_merge_request_approvers(merge_request, new_approvers) diff --git a/ee/app/services/ee/notification_service.rb b/ee/app/services/ee/notification_service.rb index 242f2b7a454..87e42659aa9 100644 --- a/ee/app/services/ee/notification_service.rb +++ b/ee/app/services/ee/notification_service.rb @@ -70,9 +70,7 @@ module EE def add_mr_approvers_email(merge_request, approvers, current_user) approvers.each do |approver| - recipient = approver.user - - mailer.add_merge_request_approver_email(recipient.id, merge_request.id, current_user.id).deliver_later + mailer.add_merge_request_approver_email(approver.id, merge_request.id, current_user.id).deliver_later end end diff --git a/ee/app/services/ee/todo_service.rb b/ee/app/services/ee/todo_service.rb index 16c4aaddc86..0ada74ff65a 100644 --- a/ee/app/services/ee/todo_service.rb +++ b/ee/app/services/ee/todo_service.rb @@ -42,7 +42,7 @@ module EE def create_approval_required_todos(merge_request, approvers, author) attributes = attributes_for_todo(merge_request.project, merge_request, author, ::Todo::APPROVAL_REQUIRED) - create_todos(approvers.map(&:user), attributes) + create_todos(approvers, attributes) end end end diff --git a/ee/app/views/shared/issuable/_approvals.html.haml b/ee/app/views/shared/issuable/_approvals.html.haml index 56e65b0df5c..fbce3f902ed 100644 --- a/ee/app/views/shared/issuable/_approvals.html.haml +++ b/ee/app/views/shared/issuable/_approvals.html.haml @@ -42,16 +42,16 @@ - unsaved_approvers = !presenter.approvers_overwritten? - item_classes = unsaved_approvers ? ['unsaved-approvers'] : [] - presenter.overall_approvers.each do |approver| - %li{ id: dom_id(approver.user), class: item_classes + ['approver'] } - = link_to approver.user.name, approver.user + %li{ id: dom_id(approver), class: item_classes + ['approver'] } + = link_to approver.name, approver - if can_update_approvers .float-right - if unsaved_approvers - = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-sm btn btn-remove", title: 'Remove approver' do + = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.name}"}, class: "btn-sm btn btn-remove", title: 'Remove approver' do = icon("sign-out") Remove - else - = link_to project_merge_request_approver_path(@project, issuable, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-sm btn btn-remove", title: 'Remove approver' do + = link_to project_merge_request_approver_via_user_id_path(@project, issuable, user_id: approver.id), data: { confirm: "Are you sure you want to remove approver #{approver.name}"}, method: :delete, class: "btn-sm btn btn-remove", title: 'Remove approver' do = icon("sign-out") Remove - presenter.overall_approver_groups.each do |approver_group| diff --git a/ee/changelogs/unreleased/1012-assign-code-owner-as-approver.yml b/ee/changelogs/unreleased/1012-assign-code-owner-as-approver.yml new file mode 100644 index 00000000000..a892ee7d395 --- /dev/null +++ b/ee/changelogs/unreleased/1012-assign-code-owner-as-approver.yml @@ -0,0 +1,5 @@ +--- +title: Assign code owner as approver +merge_request: 7933 +author: +type: added diff --git a/ee/spec/models/visible_approvable_spec.rb b/ee/spec/models/visible_approvable_spec.rb index 3437117c32b..87e66c73a6b 100644 --- a/ee/spec/models/visible_approvable_spec.rb +++ b/ee/spec/models/visible_approvable_spec.rb @@ -39,20 +39,20 @@ describe VisibleApprovable do subject { resource.overall_approvers } it 'returns a list of all the project approvers' do - is_expected.to match_array(project_approver) + is_expected.to match_array(project_approver.user) end context 'when author is approver' do let!(:author_approver) { create(:approver, target: project, user: resource.author) } it 'excludes author if authors cannot approve' do - is_expected.not_to include(author_approver) + is_expected.not_to include(author_approver.user) end it 'includes author if authors are able to approve' do project.update(merge_requests_author_approval: true) - is_expected.to include(author_approver) + is_expected.to include(author_approver.user) end end @@ -60,7 +60,7 @@ describe VisibleApprovable do let!(:approver) { create(:approver, target: resource) } it 'returns the list of all the merge request user approvers' do - is_expected.to match_array(approver) + is_expected.to match_array(approver.user) end end end @@ -95,7 +95,7 @@ describe VisibleApprovable do subject { resource.all_approvers_including_groups } it 'only queries once' do - expect(resource).to receive(:approvers_from_users).and_call_original.once + expect(resource).to receive(:overall_approvers).and_call_original.once 3.times { subject } end -- 2.30.9