diff --git a/CHANGELOG-EE b/CHANGELOG-EE index 8469779e6f..ef9a625dc4 100644 --- a/CHANGELOG-EE +++ b/CHANGELOG-EE @@ -2,6 +2,7 @@ v 7.13 (Unrelease) - Fix git hook validation on initial push to master branch. - Reset approvals on push - Fix 500 error when the source project of an MR is deleted + - Ability to define merge request approvers v 7.12.2 - Fixed the alignment of project settings icons diff --git a/app/controllers/projects/approvers_controller.rb b/app/controllers/projects/approvers_controller.rb new file mode 100644 index 0000000000..27f39aaa92 --- /dev/null +++ b/app/controllers/projects/approvers_controller.rb @@ -0,0 +1,14 @@ +class Projects::ApproversController < ApplicationController + def destroy + if params[:merge_request_id] + authorize_create_merge_request! + merge_request = project.merge_requests.find_by!(iid: params[:merge_request_id]) + merge_request.approvers.find(params[:id]).destroy + else + authorize_admin_project! + project.approvers.find(params[:id]).destroy + end + + redirect_to :back + end +end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index b954f85420..526f7bafc7 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -149,6 +149,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController def automerge return access_denied! unless @merge_request.can_be_merged_by?(current_user) + return render_404 unless @merge_request.approved? if @merge_request.automergeable? AutoMergeWorker.perform_async(@merge_request.id, current_user.id, params) @@ -201,6 +202,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def approve + unless @merge_request.can_approve?(current_user) + return render_404 + end + @approval = @merge_request.approvals.new @approval.user = current_user @@ -286,7 +291,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController def merge_request_params params.require(:merge_request).permit( :title, :assignee_id, :source_project_id, :source_branch, - :target_project_id, :target_branch, :milestone_id, + :target_project_id, :target_branch, :milestone_id, :approver_ids, :state_event, :description, :task_num, label_ids: [] ) end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8fbd2b31c3..39ddb74e10 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -195,7 +195,7 @@ class ProjectsController < ApplicationController :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id, :default_branch, :wiki_enabled, :merge_requests_template, :visibility_level, :merge_requests_rebase_enabled, :import_url, :last_activity_at, :namespace_id, :avatar, :merge_requests_rebase_default, - :approvals_before_merge + :approvals_before_merge, :approver_ids ) end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 2ac3dd03bd..c148ab2d08 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -68,4 +68,42 @@ module MergeRequestsHelper } ) end + + def render_items_list(items, separator = "and") + items_cnt = items.size + + case items_cnt + when 1 + items.first + when 2 + "#{items.first} #{separator} #{items.last}" + else + last_item = items.pop + "#{items.join(", ")} #{separator} #{last_item}" + end + end + + def render_require_section(merge_request) + str = if merge_request.approvals_left == 1 + "Requires one more approval" + else + "Requires #{merge_request.approvals_left} more approvals" + end + + if merge_request.approvers_left.any? + more_approvals = merge_request.approvals_left - merge_request.approvers_left.count + approvers_names = merge_request.approvers_left.map(&:name) + + case true + when more_approvals > 0 + str << " (from #{render_items_list(approvers_names + ["#{more_approvals} more"])})" + when more_approvals < 0 + str << " (from #{render_items_list(approvers_names, "or")})" + else + str << " (from #{render_items_list(approvers_names)})" + end + end + + str + end end diff --git a/app/models/approval.rb b/app/models/approval.rb index 21d4f1a99a..5ac0cab9bc 100644 --- a/app/models/approval.rb +++ b/app/models/approval.rb @@ -1,3 +1,14 @@ +# == Schema Information +# +# Table name: approvals +# +# id :integer not null, primary key +# merge_request_id :integer not null +# user_id :integer not null +# created_at :datetime +# updated_at :datetime +# + class Approval < ActiveRecord::Base belongs_to :user belongs_to :merge_request diff --git a/app/models/approver.rb b/app/models/approver.rb new file mode 100644 index 0000000000..984692658c --- /dev/null +++ b/app/models/approver.rb @@ -0,0 +1,16 @@ +# == Schema Information +# +# Table name: approvers +# +# id :integer not null, primary key +# target_id :integer not null +# target_type :string(255) +# user_id :integer not null +# created_at :datetime +# updated_at :datetime +# + +class Approver < ActiveRecord::Base + belongs_to :target, polymorphic: true + belongs_to :user +end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ad8f469306..5f7da74680 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -36,6 +36,7 @@ class MergeRequest < ActiveRecord::Base has_one :merge_request_diff, dependent: :destroy has_many :approvals, dependent: :destroy + has_many :approvers, as: :target, dependent: :destroy after_create :create_merge_request_diff after_update :update_merge_request_diff @@ -419,16 +420,29 @@ class MergeRequest < ActiveRecord::Base approvals_required - approvals.count end + def approvers_left + user_ids = overall_approvers.map(&:user_id) - approvals.map(&:user_id) + User.where id: user_ids + end + def approvals_required target_project.approvals_before_merge end def requires_approve? - !approvals_required.zero? + approvals_required.nonzero? + end + + def overall_approvers + if approvers.any? + approvers + else + target_project.approvers + end end def approved? - approvals.count >= approvals_required + approvals_left.zero? end def approved_by?(user) @@ -439,6 +453,15 @@ class MergeRequest < ActiveRecord::Base approvals.map(&:user) end + def can_approve?(user) + approvers_left.include?(user) || + (any_approver_allowed? && !approved_by?(user)) + end + + def any_approver_allowed? + approvals_left > approvers_left.count + end + def has_ci? source_project.ci_service && commits.any? end @@ -460,4 +483,10 @@ class MergeRequest < ActiveRecord::Base "Open" end end + + def approver_ids=(value) + value.split(",").map(&:strip).each do |user_id| + approvers.find_or_create_by(user_id: user_id, target_id: id) + end + end end diff --git a/app/models/project.rb b/app/models/project.rb index 5084e5e2d6..f897b43f41 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -117,6 +117,7 @@ class Project < ActiveRecord::Base has_many :deploy_keys, through: :deploy_keys_projects has_many :users_star_projects, dependent: :destroy has_many :starrers, through: :users_star_projects, source: :user + has_many :approvers, as: :target, dependent: :destroy has_many :project_group_links, dependent: :destroy has_many :invited_groups, through: :project_group_links, source: :group @@ -760,4 +761,10 @@ class Project < ActiveRecord::Base def jira_tracker_active? jira_tracker? && jira_service.active end + + def approver_ids=(value) + value.split(",").map(&:strip).each do |user_id| + approvers.find_or_create_by(user_id: user_id, target_id: id) + end + end end diff --git a/app/views/projects/_merge_request_settings.html.haml b/app/views/projects/_merge_request_settings.html.haml index a55ee66384..5cc5621df1 100644 --- a/app/views/projects/_merge_request_settings.html.haml +++ b/app/views/projects/_merge_request_settings.html.haml @@ -36,3 +36,31 @@ = f.label :reset_approvers_on_push do = f.check_box :reset_approvers_on_push %span.descr Reset approvers on push + + .form-group + = f.label :approver_ids, class: 'control-label' do + Approvers + .col-sm-10 + = users_select_tag("project[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true) + .help-block + Default approvers for each merge request. + + .panel.panel-default.prepend-top-10 + .panel-heading + Approvers + %small + (#{@project.approvers.count(:all)}) + %ul.well-list + - @project.approvers.each do |approver| + %li + = link_to approver.user.name, approver.user + .pull-right + = link_to namespace_project_approver_path(@project.namespace, @project, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do + = icon("sign-out") + Remove + - if @project.approvers.empty? + %li There are no approvers + + +:coffeescript + new UsersSelect() diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index a05cf8034b..adcb87b976 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -1,7 +1,7 @@ .project-edit-container .project-edit-errors .project-edit-content - %div + .panel %h3.page-title Project settings %hr diff --git a/app/views/projects/merge_requests/widget/open/_approve.html.haml b/app/views/projects/merge_requests/widget/open/_approve.html.haml index cb2d76d2ee..20dfa25351 100644 --- a/app/views/projects/merge_requests/widget/open/_approve.html.haml +++ b/app/views/projects/merge_requests/widget/open/_approve.html.haml @@ -1,8 +1,8 @@ %div %h4 - Requires #{pluralize(@merge_request.approvals_left, 'more approval')} - %p Each merge request in this project must be approved by #{pluralize(@merge_request.approvals_required, 'person')} before it can be accepted. - - unless @merge_request.approved_by?(current_user) + = render_require_section(@merge_request) + + - if @merge_request.can_approve?(current_user) .append-bottom-10 = form_for [:approve, @project.namespace.becomes(Namespace), @project, @merge_request], method: :post do |f| - = f.submit "Approve Merge Request", class: "btn btn-primary approve-btn" \ No newline at end of file + = f.submit "Approve Merge Request", class: "btn btn-primary approve-btn" diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index e434e1b6b9..ede9cc3986 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -81,6 +81,33 @@ = link_to 'Create new label', new_namespace_project_label_path(issuable.project.namespace, issuable.project), target: :blank - if issuable.is_a?(MergeRequest) + - if @project.approvals_before_merge.nonzero? + .form-group + = f.label :approver_ids, class: 'control-label' do + Approvers + .col-sm-10 + = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true) + .help-block + Merge Request should be approved by these users. + - if @project.approvers.any? + By default: #{render_items_list(@project.approvers.map{ |approver| approver.user.name})}. + You can override the project settings by setting your own list of approvers. + + .panel.panel-default.prepend-top-10 + .panel-heading + Approvers + %small + (#{@merge_request.approvers.count(:all)}) + %ul.well-list + - @merge_request.approvers.each do |approver| + %li + = link_to approver.user.name, approver.user + .pull-right + = link_to namespace_project_merge_request_approver_path(@project.namespace, @project, @merge_request, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do + = icon("sign-out") + Remove + - if @merge_request.approvers.empty? + %li There are no approvers %hr - if @merge_request.new_record? .form-group diff --git a/config/routes.rb b/config/routes.rb index 3bff1c20fc..64f12e6bc2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -507,6 +507,7 @@ Gitlab::Application.routes.draw do get :branch_to get :update_branches end + resources :approvers, only: :destroy end resources :branches, only: [:index, :new, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex } @@ -569,6 +570,8 @@ Gitlab::Application.routes.draw do get ":secret/:filename", action: :show, as: :show, constraints: { filename: /[^\/]+/ } end end + + resources :approvers, only: :destroy end get "/audit_events" => "audit_events#project_log" diff --git a/db/migrate/20150707222220_add_approvers_table.rb b/db/migrate/20150707222220_add_approvers_table.rb new file mode 100644 index 0000000000..4af7fe1c5c --- /dev/null +++ b/db/migrate/20150707222220_add_approvers_table.rb @@ -0,0 +1,14 @@ +class AddApproversTable < ActiveRecord::Migration + def change + create_table :approvers do |t| + t.integer :target_id, null: false + t.string :target_type + t.integer :user_id, null: false + + t.timestamps + + t.index [:target_id, :target_type] + t.index :user_id + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 97edb560f4..f10ceac594 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -57,6 +57,17 @@ ActiveRecord::Schema.define(version: 20150713160110) do t.datetime "updated_at" end + create_table "approvers", force: true do |t| + t.integer "target_id", null: false + t.string "target_type" + t.integer "user_id", null: false + t.datetime "created_at" + t.datetime "updated_at" + end + + add_index "approvers", ["target_id", "target_type"], name: "index_approvers_on_target_id_and_target_type", using: :btree + add_index "approvers", ["user_id"], name: "index_approvers_on_user_id", using: :btree + create_table "audit_events", force: true do |t| t.integer "author_id", null: false t.string "type", null: false diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 2d236601b7..ee222ccf66 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -240,6 +240,21 @@ Feature: Project Merge Requests When I click link "Approve" Then I should see approved merge request "Bug NS-04" + Scenario: I approve merge request if I am an approver + Given merge request 'Bug NS-04' must be approved by current user + And I click link "Bug NS-04" + And I should not see merge button + And I should see message that MR require an approval from me + When I click link "Approve" + Then I should see approved merge request "Bug NS-04" + + Scenario: I can not approve merge request if I am not an approver + Given merge request 'Bug NS-04' must be approved by some user + And I click link "Bug NS-04" + And I should not see merge button + When I should not see Approve button + And I should see message that MR require an approval + Scenario: I should see rebase checkbox Given project "Shop" have "Bug NS-05" open merge request with diffs inside And rebase before merge enabled diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 358c0ab19f..47b53aeaad 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -346,6 +346,24 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps project.save! end + step 'merge request \'Bug NS-04\' must be approved by current user' do + merge_request = MergeRequest.find_by!(title: "Bug NS-04") + project = merge_request.target_project + project.approvals_before_merge = 1 + project.approvers.create(user_id: current_user.id) + project.ensure_satellite_exists + project.save! + end + + step 'merge request \'Bug NS-04\' must be approved by some user' do + merge_request = MergeRequest.find_by!(title: "Bug NS-04") + project = merge_request.target_project + project.approvals_before_merge = 1 + project.approvers.create(user_id: create(:user).id) + project.ensure_satellite_exists + project.save! + end + step 'I click link "Approve"' do page.within '.mr-state-widget' do click_button 'Approve Merge Request' @@ -358,12 +376,30 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps end end + step 'I should not see Approve button' do + page.within '.mr-state-widget' do + expect(page).not_to have_button("Approve") + end + end + step 'I should see approved merge request "Bug NS-04"' do page.within '.mr-state-widget' do expect(page).to have_button("Accept Merge Request") end end + step 'I should see message that MR require an approval from me' do + page.within '.mr-state-widget' do + expect(page).to have_content("Requires one more approval (from #{current_user.name})") + end + end + + step 'I should see message that MR require an approval' do + page.within '.mr-state-widget' do + expect(page).to have_content("Requires one more approval") + end + end + step 'rebase before merge enabled' do project = merge_request.target_project project.merge_requests_rebase_enabled = true diff --git a/spec/helpers/merge_request_helper_spec.rb b/spec/helpers/merge_request_helper_spec.rb index 34c2cd4907..39e14ae2fc 100644 --- a/spec/helpers/merge_request_helper_spec.rb +++ b/spec/helpers/merge_request_helper_spec.rb @@ -40,4 +40,61 @@ describe MergeRequestsHelper do it { is_expected.to eq('#JIRA-123, #JIRA-456, and #FOOBAR-7890') } end end + + describe 'render_items_list' do + it "returns one item in the list" do + expect(render_items_list(["user"])).to eq("user") + end + + it "returns two items in the list" do + expect(render_items_list(["user", "user1"])).to eq("user and user1") + end + + it "returns three items in the list" do + expect(render_items_list(["user", "user1", "user2"])).to eq("user, user1 and user2") + end + end + + describe 'render_require_section' do + it "returns correct value in case - one approval" do + project.update(approvals_before_merge: 1) + merge_request = create(:merge_request, target_project: project, source_project: project) + expect(render_require_section(merge_request)).to eq("Requires one more approval") + end + + it "returns correct value in case - two approval" do + project.update(approvals_before_merge: 2) + merge_request = create(:merge_request, target_project: project, source_project: project) + expect(render_require_section(merge_request)).to eq("Requires 2 more approvals") + end + + it "returns correct value in case - one approver" do + project.update(approvals_before_merge: 1) + merge_request = create(:merge_request, target_project: project, source_project: project) + user = create :user + merge_request.approvers.create(user_id: user.id) + + expect(render_require_section(merge_request)).to eq("Requires one more approval (from #{user.name})") + end + + it "returns correct value in case - one approver and one more" do + project.update(approvals_before_merge: 2) + merge_request = create(:merge_request, target_project: project, source_project: project) + user = create :user + merge_request.approvers.create(user_id: user.id) + + expect(render_require_section(merge_request)).to eq("Requires 2 more approvals (from #{user.name} and 1 more)") + end + + it "returns correct value in case - two approver and one more" do + project.update(approvals_before_merge: 3) + merge_request = create(:merge_request, target_project: project, source_project: project) + user = create :user + user1 = create :user + merge_request.approvers.create(user_id: user.id) + merge_request.approvers.create(user_id: user1.id) + + expect(render_require_section(merge_request)).to eq("Requires 3 more approvals (from #{user1.name}, #{user.name} and 1 more)") + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c15dbd8e4e..f3621b122c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -175,6 +175,30 @@ describe MergeRequest do end end + describe "approvers_left" do + let(:merge_request) {create :merge_request} + + it "returns correct value" do + user = create(:user) + user1 = create(:user) + merge_request.target_project.approvers.create(user_id: user.id) + merge_request.target_project.approvers.create(user_id: user1.id) + merge_request.approvals.create(user_id: user1.id) + + expect(merge_request.approvers_left).to eq [user] + end + end + + describe "approvals_required" do + let(:merge_request) {create :merge_request} + + it "takes approvals_before_merge" do + merge_request.target_project.update(approvals_before_merge: 2) + + expect(merge_request.approvals_required).to eq 2 + end + end + it_behaves_like 'an editable mentionable' do subject { create(:merge_request, source_project: project) }