Merge branch '11107-fix-exposed-api-urls-for-approvals' into 'master'

Fix exposed api urls for approval rules

Closes #11107

See merge request gitlab-org/gitlab-ee!10819

ee/app/presenters/ee/merge_request_presenter.rb

@@ -6,32 +6,38 @@ module EE
     prepend VisibleApprovableForRule
 
     def approvals_path
-      if approval_feature_available?
-        approvals_project_merge_request_path(project, merge_request)
+      if expose_mr_approval_path?
+        expose_url(approvals_project_merge_request_path(project, merge_request))
       end
     end
 
     def api_approvals_path
-      if approval_feature_available?
-        api_v4_projects_merge_requests_approvals_path(id: project.id, merge_request_iid: merge_request.iid)
+      if expose_mr_approval_path?
+        expose_url(api_v4_projects_merge_requests_approvals_path(id: project.id, merge_request_iid: merge_request.iid))
       end
     end
 
     def api_approval_settings_path
+      if expose_mr_approval_path?
+        expose_url(api_v4_projects_merge_requests_approval_settings_path(id: project.id, merge_request_iid: merge_request.iid))
+      end
+    end
+
+    def api_project_approval_settings_path
       if approval_feature_available?
-        api_v4_projects_merge_requests_approval_settings_path(id: project.id, merge_request_iid: merge_request.iid)
+        expose_url(api_v4_projects_approval_settings_path(id: project.id))
       end
     end
 
     def api_approve_path
-      if approval_feature_available?
-        api_v4_projects_merge_requests_approve_path(id: project.id, merge_request_iid: merge_request.iid)
+      if expose_mr_approval_path?
+        expose_url(api_v4_projects_merge_requests_approve_path(id: project.id, merge_request_iid: merge_request.iid))
       end
     end
 
     def api_unapprove_path
-      if approval_feature_available?
-        api_v4_projects_merge_requests_unapprove_path(id: project.id, merge_request_iid: merge_request.iid)
+      if expose_mr_approval_path?
+        expose_url(api_v4_projects_merge_requests_unapprove_path(id: project.id, merge_request_iid: merge_request.iid))
       end
     end
 
@@ -46,5 +52,11 @@ module EE
     def approver_groups
       ::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user)
     end
+
+    private
+
+    def expose_mr_approval_path?
+      approval_feature_available? && merge_request.iid
+    end
   end
 end

ee/app/views/shared/issuable/_approvals.html.haml

@@ -12,7 +12,7 @@
     Approvers
   .col-sm-10
     - if Feature.enabled?(:approval_rules, @target_project, default_enabled: true)
-      = render 'shared/issuable/approvals_multiple_rule', issuable: issuable
+      = render 'shared/issuable/approvals_multiple_rule', issuable: issuable, presenter: presenter
     - else
       = render 'shared/issuable/approvals_single_rule', issuable: issuable, presenter: presenter, form: form
     - if can_update_approvers

ee/app/views/shared/issuable/_approvals_multiple_rule.html.haml

@@ -2,6 +2,6 @@
   'can_edit': can?(current_user, :update_approvers, issuable).to_s,
   'allow_multi_rule': @target_project.multiple_approval_rules_available?.to_s,
   'mr_id': issuable.iid,
-  'mr_settings_path': issuable.iid && api_v4_projects_merge_requests_approval_settings_path(id: @target_project.id, merge_request_iid: issuable.iid),
-  'project_settings_path': api_v4_projects_approval_settings_path(id: @target_project.id) } }
+  'mr_settings_path': presenter.api_approval_settings_path,
+  'project_settings_path': presenter.api_project_approval_settings_path } }
   = sprite_icon('spinner', size: 24, css_class: 'gl-spinner')

ee/app/views/shared/merge_request_approvals_settings/_multiple_rules_form.html.haml

@@ -2,9 +2,9 @@
   = form.label :approver_ids, class: 'label-bold' do
     = _("Add approvers")
   #js-mr-approvals-settings{ data: { 'project_id': @project.id,
-    'project_path': api_v4_projects_path(id: @project.id),
-    'settings_path': api_v4_projects_approval_settings_path(id: @project.id),
-    'rules_path': api_v4_projects_approval_settings_rules_path(id: @project.id),
+    'project_path': expose_url(api_v4_projects_path(id: @project.id)),
+    'settings_path': expose_url(api_v4_projects_approval_settings_path(id: @project.id)),
+    'rules_path': expose_url(api_v4_projects_approval_settings_rules_path(id: @project.id)),
     'allow_multi_rule': @project.multiple_approval_rules_available?.to_s } }
     .text-center.prepend-top-default
       = sprite_icon('spinner', size: 24, css_class: 'gl-spinner')

ee/changelogs/unreleased/11107-fix-exposed-api-urls-for-approvals.yml

+---
+title: Fix approval rules when used with relative url root
+merge_request: 10819
+author:
+type: fixed

ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Merge request > User edits MR with approval rules', :js do
+  include_context 'project with approval rules'
+  let(:merge_request) { create(:merge_request, source_project: project) }
+  let(:mr_rule_names) { %w[foo lorem ipsum] }
+
+  def page_rule_names
+    page.all('.js-approval-rules table .js-name')
+  end
+
+  before do
+    project.update_attribute(:disable_overriding_approvers_per_merge_request, false)
+    stub_licensed_features(multiple_approval_rules: true)
+
+    mr_rule_names.each do |name|
+      create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1, name: name)
+    end
+
+    sign_in(author)
+    visit(edit_project_merge_request_path(project, merge_request))
+
+    wait_for_requests
+  end
+
+  it "shows approval rules" do
+    names = page_rule_names.map(&:text)
+
+    expect(names).to eq(mr_rule_names)
+  end
+end

ee/spec/features/merge_request/user_sets_approval_rules_spec.rb

@@ -5,29 +5,13 @@ require 'rails_helper'
 describe 'Merge request > User sets approval rules', :js do
   include ProjectForksHelper
 
-  let(:approver) { create(:user) }
-  let(:author) { create(:user) }
-  let(:project) { create(:project, :public, :repository) }
+  include_context 'project with approval rules'
 
   def page_rule_names
     page.all('.js-approval-rules table .js-name')
   end
 
-  before do
-    stub_licensed_features(multiple_approval_rules: true)
-
-    [approver, author].each do |member|
-      project.add_maintainer(member)
-    end
-  end
-
   context "with project approval rules" do
-    let!(:regular_rules) do
-      Array.new(3) do |i|
-        create(:approval_project_rule, project: project, users: [approver], name: "Regular Rule #{i}")
-      end
-    end
-
     context "from a fork" do
       let(:forked_project) { fork_project(project, nil, repository: true) }
 

ee/spec/presenters/merge_request_presenter_spec.rb

 require 'spec_helper'
 
 describe MergeRequestPresenter do
-  let(:resource) { create :merge_request, source_project: project }
-  let!(:project) { create(:project, :repository) }
-  let!(:user) { project.creator }
+  using RSpec::Parameterized::TableSyntax
 
-  describe '#approvals_path' do
-    subject { described_class.new(resource, current_user: user).approvals_path }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+  let(:project) { create(:project, :repository) }
+  let(:user) { project.creator }
+  let(:approval_feature_available) { true }
+
+  before do
+    stub_config_setting(relative_url_root: '/gitlab')
+    stub_licensed_features(merge_request_approvers: approval_feature_available)
+  end
 
-    it 'returns path' do
-      is_expected.to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}/approvals")
+  shared_examples 'is nil when needed' do
+    where(:approval_feature_available, :with_iid) do
+      false | false
+      false | true
+      true  | false
     end
+
+    with_them do
+      before do
+        merge_request.iid = nil unless with_iid
+      end
+
+      it { is_expected.to be_nil }
+    end
+  end
+
+  describe '#approvals_path' do
+    subject { described_class.new(merge_request, current_user: user).approvals_path }
+
+    it_behaves_like 'is nil when needed'
+
+    it { is_expected.to eq(expose_url("/#{merge_request.project.full_path}/merge_requests/#{merge_request.iid}/approvals")) }
   end
 
   describe '#api_approvals_path' do
-    subject { described_class.new(resource, current_user: user).api_approvals_path }
+    subject { described_class.new(merge_request, current_user: user).api_approvals_path }
 
-    it 'returns path' do
-      is_expected.to eq("/api/v4/projects/#{resource.project.id}/merge_requests/#{resource.iid}/approvals")
-    end
+    it_behaves_like 'is nil when needed'
+
+    it { is_expected.to eq(expose_url("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approvals")) }
   end
 
   describe '#api_approval_settings_path' do
-    subject { described_class.new(resource, current_user: user).api_approval_settings_path }
+    subject { described_class.new(merge_request, current_user: user).api_approval_settings_path }
+
+    it_behaves_like 'is nil when needed'
+
+    it { is_expected.to eq(expose_url("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approval_settings")) }
+  end
+
+  describe '#api_project_approval_settings_path' do
+    subject { described_class.new(merge_request, current_user: user).api_project_approval_settings_path }
 
-    it 'returns path' do
-      is_expected.to eq("/api/v4/projects/#{resource.project.id}/merge_requests/#{resource.iid}/approval_settings")
+    it { is_expected.to eq(expose_url("/api/v4/projects/#{merge_request.project.id}/approval_settings")) }
+
+    context "when approvals not available" do
+      let(:approval_feature_available) { false }
+
+      it { is_expected.to be_nil }
     end
   end
 
   describe '#api_approve_path' do
-    subject { described_class.new(resource, current_user: user).api_approve_path }
+    subject { described_class.new(merge_request, current_user: user).api_approve_path }
 
-    it 'returns path' do
-      is_expected.to eq("/api/v4/projects/#{resource.project.id}/merge_requests/#{resource.iid}/approve")
-    end
+    it_behaves_like 'is nil when needed'
+
+    it { is_expected.to eq(expose_url("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approve")) }
   end
 
   describe '#api_unapprove_path' do
-    subject { described_class.new(resource, current_user: user).api_unapprove_path }
+    subject { described_class.new(merge_request, current_user: user).api_unapprove_path }
 
-    it 'returns path' do
-      is_expected.to eq("/api/v4/projects/#{resource.project.id}/merge_requests/#{resource.iid}/unapprove")
-    end
+    it_behaves_like 'is nil when needed'
+
+    it { is_expected.to eq(expose_url("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/unapprove")) }
   end
 
   describe '#approvers_left' do
     let!(:private_group) { create(:group_with_members, :private) }
     let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
-    let!(:approver) { create(:approver, target: resource) }
+    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
+    let!(:approver) { create(:approver, target: merge_request) }
 
     before do
       stub_feature_flags(approval_rules: false)
-      resource.approvals.create!(user: approver.user)
+      merge_request.approvals.create!(user: approver.user)
     end
 
-    subject { described_class.new(resource, current_user: user).approvers_left }
+    subject { described_class.new(merge_request, current_user: user).approvers_left }
 
     it { is_expected.to match_array(public_approver_group.users) }
 
@@ -77,15 +113,15 @@ describe MergeRequestPresenter do
   describe '#approvers_left with approval_rule enabled' do
     let!(:private_group) { create(:group_with_members, :private) }
     let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
-    let!(:approver) { create(:approver, target: resource) }
+    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
+    let!(:approver) { create(:approver, target: merge_request) }
 
     before do
-      resource.approvals.create!(user: approver.user)
+      merge_request.approvals.create!(user: approver.user)
     end
 
-    subject { described_class.new(resource, current_user: user).approvers_left }
+    subject { described_class.new(merge_request, current_user: user).approvers_left }
 
     it 'contains all approvers' do
       approvers = public_approver_group.users + private_approver_group.users - [user]
@@ -97,10 +133,10 @@ describe MergeRequestPresenter do
   describe '#overall_approver_groups' do
     let!(:private_group) { create(:group_with_members, :private) }
     let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
+    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
 
-    subject { described_class.new(resource, current_user: user).overall_approver_groups }
+    subject { described_class.new(merge_request, current_user: user).overall_approver_groups }
 
     it { is_expected.to match_array([public_approver_group]) }
 
@@ -116,11 +152,11 @@ describe MergeRequestPresenter do
   describe '#all_approvers_including_groups' do
     let!(:private_group) { create(:group_with_members, :private) }
     let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
-    let!(:approver) { create(:approver, target: resource) }
+    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
+    let!(:approver) { create(:approver, target: merge_request) }
 
-    subject { described_class.new(resource, current_user: user).all_approvers_including_groups }
+    subject { described_class.new(merge_request, current_user: user).all_approvers_including_groups }
 
     before do
       stub_feature_flags(approval_rules: false)
@@ -144,11 +180,11 @@ describe MergeRequestPresenter do
   describe '#all_approvers_including_groups with approval_rule enabled' do
     let!(:private_group) { create(:group_with_members, :private) }
     let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
-    let!(:approver) { create(:approver, target: resource) }
+    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
+    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
+    let!(:approver) { create(:approver, target: merge_request) }
 
-    subject { described_class.new(resource, current_user: user).all_approvers_including_groups }
+    subject { described_class.new(merge_request, current_user: user).all_approvers_including_groups }
 
     it do
       approvers = [public_approver_group.users, private_approver_group.users, approver.user].flatten - [user]

ee/spec/support/shared_contexts/project_approval_rules_shared_context.rb

+# frozen_string_literal: true
+
+shared_context 'project with approval rules' do
+  let(:approver) { create(:user) }
+  let(:author) { create(:user) }
+  let(:project) { create(:project, :public, :repository) }
+
+  before do
+    stub_licensed_features(multiple_approval_rules: true)
+
+    [approver, author].each do |member|
+      project.add_maintainer(member)
+    end
+  end
+
+  let!(:regular_rules) do
+    Array.new(3) do |i|
+      create(:approval_project_rule, project: project, users: [approver], name: "Regular Rule #{i}")
+    end
+  end
+end