Merge branch 'bvl-expand-namespace-feature-checks' into 'master'

Improve Namespace feature check

Closes #2761

See merge request !2279

app/models/ee/namespace.rb

@@ -33,11 +33,19 @@ module EE
     # for a given Namespace plan. This method should consider ancestor groups
     # being licensed.
     def feature_available?(feature)
-      @features_available ||= Hash.new do |h, feature|
+      @feature_available ||= Hash.new do |h, feature|
+        h[feature] = load_feature_available(feature)
+      end
+
+      @feature_available[feature]
+    end
+
+    def feature_available_in_plan?(feature)
+      @features_available_in_plan ||= Hash.new do |h, feature|
         h[feature] = plans.any? { |plan| License.plan_includes_feature?(EE_PLANS[plan], feature) }
       end
 
-      @features_available[feature]
+      @features_available_in_plan[feature]
     end
 
     def actual_shared_runners_minutes_limit
@@ -57,6 +65,16 @@ module EE
 
     private
 
+    def load_feature_available(feature)
+      globally_available = License.feature_available?(feature)
+
+      if current_application_settings.should_check_namespace_plan?
+        globally_available && feature_available_in_plan?(feature)
+      else
+        globally_available
+      end
+    end
+
     def plans
       @ancestors_plans ||=
         if parent_id

app/models/ee/project.rb

@@ -344,11 +344,19 @@ module EE
     private
 
     def licensed_feature_available?(feature)
+      @licensed_feature_available ||= Hash.new do |h, feature|
+        h[feature] = load_licensed_feature_available(feature)
+      end
+
+      @licensed_feature_available[feature]
+    end
+
+    def load_licensed_feature_available(feature)
       globally_available = License.feature_available?(feature)
 
       if current_application_settings.should_check_namespace_plan?
         globally_available &&
-          (public? && namespace.public? || namespace.feature_available?(feature))
+          (public? && namespace.public? || namespace.feature_available_in_plan?(feature))
       else
         globally_available
       end

spec/models/ee/namespace_spec.rb

@@ -43,17 +43,43 @@ describe Namespace, models: true do
   end
 
   describe '#feature_available?' do
+    let(:plan_license) { Namespace::BRONZE_PLAN }
     let(:group) { create(:group, plan: plan_license) }
+    let(:feature) { :service_desk }
 
     subject { group.feature_available?(feature) }
 
-    context 'when feature available' do
-      let(:feature) { :deploy_board }
+    before do
+      stub_licensed_features(feature => true)
+    end
+
+    it 'uses the global setting when running on premise' do
+      stub_application_setting_on_object(group, should_check_namespace_plan: false)
+
+      is_expected.to be_truthy
+    end
+
+    it 'only checks the plan once' do
+      expect(group).to receive(:load_feature_available).once.and_call_original
+
+      2.times { group.feature_available?(:service_desk) }
+    end
+
+    context 'when checking namespace plan' do
+      before do
+        stub_application_setting_on_object(group, should_check_namespace_plan: true)
+      end
+
+      it 'combines the global setting with the group setting when not running on premise' do
+        is_expected.to be_falsy
+      end
+
+      context 'when feature available on the plan' do
         let(:plan_license) { Namespace::GOLD_PLAN }
 
         context 'when feature available for current group' do
-        it 'returns false' do
-          is_expected.to eq(true)
+          it 'returns true' do
+            is_expected.to be_truthy
           end
         end
 
@@ -62,18 +88,19 @@ describe Namespace, models: true do
             let(:child_group) { create :group, parent: group }
 
             it 'child group has feature available' do
-            expect(child_group.feature_available?(feature)).to eq(true)
+              expect(child_group.feature_available?(feature)).to be_truthy
             end
           end
         end
       end
 
-    context 'when feature not available' do
+      context 'when feature not available in the plan' do
         let(:feature) { :deploy_board }
         let(:plan_license) { Namespace::BRONZE_PLAN }
 
         it 'returns false' do
-        is_expected.to eq(false)
+          is_expected.to be_falsy
+        end
       end
     end
   end

spec/models/ee/project_spec.rb

@@ -22,7 +22,7 @@ describe Project, models: true do
       before do
         stub_application_setting('check_namespace_plan?' => check_namespace_plan)
         allow(Gitlab).to receive(:com?) { true }
-        expect(License).to receive(:feature_available?).with(feature) { allowed_on_global_license }
+        stub_licensed_features(feature => allowed_on_global_license)
         allow(namespace).to receive(:plan) { plan_license }
       end
 
@@ -99,6 +99,13 @@ describe Project, models: true do
       end
     end
 
+    it 'only loads licensed availability once' do
+      expect(project).to receive(:load_licensed_feature_available)
+                             .once.and_call_original
+
+      2.times { project.feature_available?(:service_desk) }
+    end
+
     context 'when feature symbol is not included on Namespace features code' do
       let(:feature) { :issues }
 

spec/support/stub_configuration.rb

@@ -9,6 +9,16 @@ module StubConfiguration
       .to receive_messages(messages)
   end
 
+  def stub_application_setting_on_object(object, messages)
+    add_predicates(messages)
+
+    allow(Gitlab::CurrentSettings.current_application_settings)
+      .to receive_messages(messages)
+    messages.each do |setting, value|
+      allow(object).to receive_message_chain(:current_application_settings, setting) { value }
+    end
+  end
+
   def stub_config_setting(messages)
     allow(Gitlab.config.gitlab).to receive_messages(messages)
   end