Merge branch '9908-hide-approver-ui-in-free' into 'master'

Fix Approval UI showing up for free plan

Closes #9908

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

app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue

@@ -112,7 +112,7 @@ export default {
       return enableSquashBeforeMerge && commitsCount > 1;
     },
     isApprovalNeeded() {
-      return this.mr.approvalsRequired ? !this.mr.isApproved : false;
+      return this.mr.hasApprovalsAvailable ? !this.mr.isApproved : false;
     },
     shouldShowMergeControls() {
       return this.mr.isMergeAllowed || this.shouldShowMergeWhenPipelineSucceedsText;

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/single_rule/approvals.vue

@@ -59,7 +59,10 @@ export default {
 </script>
 <template>
   <mr-widget-container>
-    <div v-if="mr.approvalsRequired" class="media media-section js-mr-approvals align-items-center">
+    <div
+      v-if="mr.hasApprovalsAvailable"
+      class="media media-section js-mr-approvals align-items-center"
+    >
       <mr-widget-icon name="approval" />
       <div v-show="fetchingApprovals" class="mr-approvals-loading-state media-body">
         <span class="approvals-loading-text"> {{ $options.FETCH_LOADING }} </span>

ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue

@@ -32,7 +32,7 @@ export default {
   },
   computed: {
     shouldRenderApprovals() {
-      return this.mr.approvalsRequired && this.mr.state !== 'nothingToMerge';
+      return this.mr.hasApprovalsAvailable && this.mr.state !== 'nothingToMerge';
     },
     shouldRenderCodeQuality() {
       const { codeclimate } = this.mr;

ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -44,8 +44,10 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     this.isApproved = this.isApproved || false;
     this.approvals = this.approvals || null;
     this.approvalRules = this.approvalRules || [];
+    this.hasApprovalsAvailable = Boolean(
+      data.has_approvals_available || this.hasApprovalsAvailable,
+    );
     this.approvalsPath = data.approvals_path || this.approvalsPath;
-    this.approvalsRequired = data.approvalsRequired || Boolean(this.approvalsPath);
     this.apiApprovalsPath = data.api_approvals_path || this.apiApprovalsPath;
     this.apiApprovalSettingsPath = data.api_approval_settings_path || this.apiApprovalSettingsPath;
     this.apiApprovePath = data.api_approve_path || this.apiApprovePath;
@@ -60,7 +62,7 @@ export default class MergeRequestStore extends CEMergeRequestStore {
       this.preventMerge = !this.isApproved;
     } else {
       this.isApproved = !this.approvalsLeft || false;
-      this.preventMerge = this.approvalsRequired && this.approvalsLeft;
+      this.preventMerge = this.hasApprovalsAvailable && this.approvalsLeft;
     }
   }
 

ee/app/models/approval_state.rb

@@ -10,7 +10,7 @@ class ApprovalState
 
   attr_reader :merge_request, :project
 
-  def_delegators :@merge_request, :merge_status, :approved_by_users, :approvals
+  def_delegators :@merge_request, :merge_status, :approved_by_users, :approvals, :approval_feature_available?
   alias_method :approved_approvers, :approved_by_users
 
   def initialize(merge_request)
@@ -31,6 +31,8 @@ class ApprovalState
 
   def wrapped_approval_rules
     strong_memoize(:wrapped_approval_rules) do
+      next [] unless approval_feature_available?
+
       result = use_fallback? ? [fallback_rule] : regular_rules
       result += code_owner_rules
       result

ee/app/models/concerns/approvable.rb

@@ -7,6 +7,16 @@ module Approvable
   # such as approver_groups and target_project in presenters
   include ::VisibleApprovable
 
+  def approval_feature_available?
+    strong_memoize(:approval_feature_available) do
+      if project
+        project.feature_available?(:merge_request_approvers)
+      else
+        false
+      end
+    end
+  end
+
   def approval_state
     @approval_state ||= ApprovalState.new(self)
   end
@@ -40,7 +50,7 @@ module Approvable
   end
 
   def approvals_before_merge
-    return nil unless project&.feature_available?(:merge_request_approvers)
+    return nil unless approval_feature_available?
 
     super
   end

ee/app/models/concerns/visible_approvable.rb

@@ -5,11 +5,6 @@
 module VisibleApprovable
   include ::Gitlab::Utils::StrongMemoize
 
-  def requires_approve?
-    # keep until UI changes implemented
-    true
-  end
-
   # Users in the list of approvers who have not already approved this MR.
   #
   def approvers_left

ee/app/presenters/ee/merge_request_presenter.rb

@@ -6,31 +6,31 @@ module EE
     prepend VisibleApprovableForRule
 
     def approvals_path
-      if requires_approve?
+      if approval_feature_available?
         approvals_project_merge_request_path(project, merge_request)
       end
     end
 
     def api_approvals_path
-      if requires_approve?
+      if approval_feature_available?
         api_v4_projects_merge_requests_approvals_path(id: project.id, merge_request_iid: merge_request.iid)
       end
     end
 
     def api_approval_settings_path
-      if requires_approve?
+      if approval_feature_available?
         api_v4_projects_merge_requests_approval_settings_path(id: project.id, merge_request_iid: merge_request.iid)
       end
     end
 
     def api_approve_path
-      if requires_approve?
+      if approval_feature_available?
         api_v4_projects_merge_requests_approve_path(id: project.id, merge_request_iid: merge_request.iid)
       end
     end
 
     def api_unapprove_path
-      if requires_approve?
+      if approval_feature_available?
         api_v4_projects_merge_requests_unapprove_path(id: project.id, merge_request_iid: merge_request.iid)
       end
     end

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -122,6 +122,9 @@ module EE
       expose :can_push_to_source_branch do |merge_request|
         presenter(merge_request).can_push_to_source_branch?
       end
+      expose :has_approvals_available do |merge_request|
+        merge_request.approval_feature_available?
+      end
       expose :rebase_path do |merge_request|
         presenter(merge_request).rebase_path
       end

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

@@ -3,7 +3,7 @@
 - form = local_assigns.fetch(:form)
 
 - return unless issuable.is_a?(MergeRequest)
-- return unless presenter.requires_approve?
+- return unless issuable.approval_feature_available?
 
 - can_update_approvers = can?(current_user, :update_approvers, issuable)
 

ee/changelogs/unreleased/9908-hide-approver-ui-in-free.yml

+---
+title: Fix approval-related UI showing up in free plan
+merge_request: 9819
+author:
+type: fixed

ee/lib/ee/api/entities.rb

@@ -389,6 +389,10 @@ module EE
           approval_state.has_non_fallback_rules?
         end
 
+        expose :merge_request_approvers_available do |approval_state|
+          approval_state.project.feature_available?(:merge_request_approvers)
+        end
+
         expose :multiple_approval_rules_available do |approval_state|
           approval_state.project.multiple_approval_rules_available?
         end

ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js

@@ -720,7 +720,7 @@ describe('ee merge request widget options', () => {
         vm = mountComponent(Component, {
           mrData: {
             ...mockData,
-            approvalsRequired: false,
+            has_approvals_available: false,
           },
         });
         vm.mr.state = 'readyToMerge';
@@ -732,7 +732,7 @@ describe('ee merge request widget options', () => {
         vm = mountComponent(Component, {
           mrData: {
             ...mockData,
-            approvalsRequired: true,
+            has_approvals_available: true,
           },
         });
         vm.mr.state = 'nothingToMerge';
@@ -744,7 +744,7 @@ describe('ee merge request widget options', () => {
         vm = mountComponent(Component, {
           mrData: {
             ...mockData,
-            approvalsRequired: true,
+            has_approvals_available: true,
           },
         });
         vm.mr.state = 'readyToMerge';

ee/spec/models/approvable_spec.rb

@@ -9,6 +9,24 @@ describe Approvable do
     stub_feature_flags(approval_rules: false)
   end
 
+  describe '#approval_feature_available?' do
+    let(:project) { create(:project) }
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+    subject { merge_request.approval_feature_available? }
+
+    it 'is false when feature is disabled' do
+      allow(project).to receive(:feature_available?).with(:merge_request_approvers).and_return(false)
+
+      is_expected.to be false
+    end
+
+    it 'is true when feature is enabled' do
+      allow(project).to receive(:feature_available?).with(:merge_request_approvers).and_return(true)
+
+      is_expected.to be true
+    end
+  end
+
   describe '#approvers_overwritten?' do
     subject { merge_request.approvers_overwritten? }
 

ee/spec/models/approval_state_spec.rb

@@ -17,6 +17,10 @@ describe ApprovalState do
     end
   end
 
+  def disable_feature
+    allow(subject).to receive(:approval_feature_available?).and_return(false)
+  end
+
   let(:merge_request) { create(:merge_request) }
   let(:project) { merge_request.target_project }
   let(:approver1) { create(:user) }
@@ -108,6 +112,14 @@ describe ApprovalState do
         expect(subject.wrapped_approval_rules).to all(be_an(ApprovalWrappedRule))
         expect(subject.wrapped_approval_rules.size).to eq(2)
       end
+
+      context 'when approval feature is unavailable' do
+        it 'returns empty array' do
+          disable_feature
+
+          expect(subject.wrapped_approval_rules).to eq([])
+        end
+      end
     end
 
     describe '#approval_needed?' do
@@ -150,6 +162,14 @@ describe ApprovalState do
           expect(subject.approval_needed?).to eq(false)
         end
       end
+
+      context 'when approval feature is unavailable' do
+        it 'returns false' do
+          disable_feature
+
+          expect(subject.approval_needed?).to eq(false)
+        end
+      end
     end
 
     describe '#approved?' do
@@ -216,6 +236,15 @@ describe ApprovalState do
 
         it_behaves_like 'when rules are present'
       end
+
+      context 'when approval feature is unavailable' do
+        it 'returns true' do
+          disable_feature
+          create_rule(users: [create(:user)], approvals_required: 1)
+
+          expect(subject.approved?).to eq(true)
+        end
+      end
     end
 
     describe '#any_approver_allowed?' do
@@ -261,6 +290,14 @@ describe ApprovalState do
       it 'sums approvals_left from rules' do
         expect(subject.approvals_left).to eq(12)
       end
+
+      context 'when approval feature is unavailable' do
+        it 'returns 0' do
+          disable_feature
+
+          expect(subject.approvals_left).to eq(0)
+        end
+      end
     end
 
     describe '#approval_rules_left' do
@@ -268,12 +305,21 @@ describe ApprovalState do
         create_rule(approvals_required: 1, users: [create(:user)])
       end
 
-      it 'counts approval_rules left' do
-        create_unapproved_rule
-        create_unapproved_rule
+      before do
+        2.times { create_unapproved_rule }
+      end
 
+      it 'counts approval_rules left' do
         expect(subject.approval_rules_left.size).to eq(2)
       end
+
+      context 'when approval feature is unavailable' do
+        it 'returns empty array' do
+          disable_feature
+
+          expect(subject.approval_rules_left).to eq([])
+        end
+      end
     end
 
     describe '#approvers' do

ee/spec/models/visible_approvable_spec.rb

@@ -9,12 +9,6 @@ describe VisibleApprovable do
     stub_feature_flags(approval_rules: false)
   end
 
-  describe '#requires_approve' do
-    subject { resource.requires_approve? }
-
-    it { is_expected.to be true }
-  end
-
   describe '#approvers_left' do
     let!(:private_group) { create(:group_with_members, :private) }
     let!(:public_group) { create(:group_with_members) }

ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb

@@ -15,7 +15,6 @@ describe 'shared/issuable/_approvals.html.haml' do
     allow(view).to receive(:current_user).and_return(user)
     allow(form).to receive(:label)
     allow(form).to receive(:number_field)
-    allow(merge_request).to receive(:requires_approve?).and_return(true)
     allow(MergeRequestApproverPresenter).to receive(:new).and_return(approver_presenter)
     assign(:project, project)
   end

spec/fixtures/api/schemas/entities/merge_request_widget.json

@@ -128,6 +128,7 @@
     "conflicts_docs_path": { "type": ["string", "null"] },
 
     // EE-specific
+    "has_approvals_available": { "type": ["boolean"] },
     "approvals_before_merge": { "type": ["integer", "null"] },
     "approved": { "type": "boolean" },
     "approvals_path": { "type": ["string", "null"] },