Commit 1ca1c452 authored by Paul Slaughter's avatar Paul Slaughter Committed by Mark Chao

Cleanup has approvals available check in MR widget

- Removes `requires_approve?`. This was only being used
   in `merge_request_presenter` to hide/show the API paths.
- Adds `approval_feature_available?`
   The API path checks have been updated to use this call.
- Replaces `approvalsRequired` in the mr_widget_store with
   `hasApprovalsAvailable`. The previous property actually
   only checked if the `approvalsPath` was truthy or not. It
   looked like it checked the `approvalsRequired` from the
   server, but since this was camel case, this value never existed.
parent f150b0ee
......@@ -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;
......
......@@ -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>
......
......@@ -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;
......
......@@ -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;
}
}
......
......@@ -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,7 +31,7 @@ class ApprovalState
def wrapped_approval_rules
strong_memoize(:wrapped_approval_rules) do
next [] unless project.feature_available?(:merge_request_approvers)
next [] unless approval_feature_available?
result = use_fallback? ? [fallback_rule] : regular_rules
result += code_owner_rules
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -3,7 +3,7 @@
- form = local_assigns.fetch(:form)
- return unless issuable.is_a?(MergeRequest)
- return unless @project.feature_available?(:merge_request_approvers)
- return unless issuable.approval_feature_available?
- can_update_approvers = can?(current_user, :update_approvers, issuable)
......
......@@ -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
......
......@@ -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';
......
......@@ -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? }
......
......@@ -18,8 +18,7 @@ describe ApprovalState do
end
def disable_feature
allow(subject.project).to receive(:feature_available?).and_call_original
allow(subject.project).to receive(:feature_available?).with(:merge_request_approvers).and_return(false)
allow(subject).to receive(:approval_feature_available?).and_return(false)
end
let(:merge_request) { create(:merge_request) }
......
......@@ -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) }
......
......@@ -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"] },
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment