Merge branch 'dm-license-features' into 'master'

Simplify specification of EE license/plan features

Closes #2466

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

ee/app/models/ee/project.rb

@@ -179,14 +179,11 @@ module EE
       !public? && shared_runners_enabled? && namespace.shared_runners_minutes_limit_enabled?
     end
 
-    # Checks licensed feature availability if `feature` matches any
-    # key on License::FEATURE_CODES. Otherwise, check feature availability
-    # through ProjectFeature.
     def feature_available?(feature, user = nil)
-      if License::FEATURE_CODES.key?(feature)
-        licensed_feature_available?(feature)
-      else
+      if ProjectFeature::FEATURES.include?(feature)
         super
+      else
+        licensed_feature_available?(feature)
       end
     end
 

lib/gitlab/usage_data.rb

@@ -94,6 +94,7 @@ module Gitlab
           usage_data[:license_user_count] = license.restricted_user_count
           usage_data[:license_starts_at] = license.starts_at
           usage_data[:license_expires_at] = license.expires_at
+          usage_data[:license_plan] = license.plan
           usage_data[:license_add_ons] = license.add_ons
         end
 

spec/ee/spec/models/ee/project_spec.rb

@@ -107,57 +107,58 @@ describe Project do
         allow(namespace).to receive(:plan) { plan_license }
       end
 
-      License::FEATURE_CODES.each do |feature_sym, feature_code|
-        context feature_sym.to_s do
-          let(:feature) { feature_sym }
-          let(:feature_code) { feature_code }
+      License::EEU_FEATURES.each do |feature_sym|
+        let(:feature) { feature_sym }
 
-          context "checking #{feature_sym} availability both on Global and Namespace license" do
-            let(:check_namespace_plan) { true }
+        context feature_sym.to_s do
+          unless License::GLOBAL_FEATURES.include?(feature_sym)
+            context "checking #{feature_sym} availability both on Global and Namespace license" do
+              let(:check_namespace_plan) { true }
 
-            context 'allowed by Plan License AND Global License' do
-              let(:allowed_on_global_license) { true }
-              let(:plan_license) { Plan.find_by(name: 'gold') }
+              context 'allowed by Plan License AND Global License' do
+                let(:allowed_on_global_license) { true }
+                let(:plan_license) { Plan.find_by(name: 'gold') }
 
-              it 'returns true' do
-                is_expected.to eq(true)
+                it 'returns true' do
+                  is_expected.to eq(true)
+                end
               end
-            end
 
-            context 'not allowed by Plan License but project and namespace are public' do
-              let(:allowed_on_global_license) { true }
-              let(:plan_license) { Plan.find_by(name: 'bronze') }
+              context 'not allowed by Plan License but project and namespace are public' do
+                let(:allowed_on_global_license) { true }
+                let(:plan_license) { Plan.find_by(name: 'bronze') }
 
-              it 'returns true' do
-                allow(namespace).to receive(:public?) { true }
-                allow(project).to receive(:public?) { true }
+                it 'returns true' do
+                  allow(namespace).to receive(:public?) { true }
+                  allow(project).to receive(:public?) { true }
 
-                is_expected.to eq(true)
+                  is_expected.to eq(true)
+                end
               end
-            end
 
-            unless License.plan_includes_feature?(License::STARTER_PLAN, feature_sym)
-              context 'not allowed by Plan License' do
-                let(:allowed_on_global_license) { true }
-                let(:plan_license) { Plan.find_by(name: 'bronze') }
+              unless License.plan_includes_feature?(License::STARTER_PLAN, feature_sym)
+                context 'not allowed by Plan License' do
+                  let(:allowed_on_global_license) { true }
+                  let(:plan_license) { Plan.find_by(name: 'bronze') }
 
-                it 'returns false' do
-                  is_expected.to eq(false)
+                  it 'returns false' do
+                    is_expected.to eq(false)
+                  end
                 end
               end
-            end
 
-            context 'not allowed by Global License' do
-              let(:allowed_on_global_license) { false }
-              let(:plan_license) { Plan.find_by(name: 'gold') }
+              context 'not allowed by Global License' do
+                let(:allowed_on_global_license) { false }
+                let(:plan_license) { Plan.find_by(name: 'gold') }
 
-              it 'returns false' do
-                is_expected.to eq(false)
+                it 'returns false' do
+                  is_expected.to eq(false)
+                end
               end
             end
           end
 
-          context "when checking #{feature_code} only for Global license" do
+          context "when checking #{feature_sym} only for Global license" do
             let(:check_namespace_plan) { false }
 
             context 'allowed by Global License' do

spec/ee/support/license_helpers.rb

@@ -9,9 +9,6 @@ module EE
     # This enables `geo` and disables `deploy_board` features for a spec.
     # Other features are still enabled/disabled as defined in the licence.
     def stub_licensed_features(features)
-      unknown_features = features.keys - License::FEATURE_CODES.keys
-      raise "Unknown features: #{unknown_features.inspect}" unless unknown_features.empty?
-
       allow(License).to receive(:feature_available?).and_call_original
 
       features.each do |feature, enabled|

spec/lib/gitlab/usage_data_spec.rb

@@ -23,6 +23,7 @@ describe Gitlab::UsageData do
         counts
         historical_max_users
         license_add_ons
+        license_plan
         license_expires_at
         license_starts_at
         license_user_count

spec/models/license_spec.rb

@@ -214,21 +214,21 @@ describe License do
     describe '.features_for_plan' do
       it 'returns features for starter plan' do
         expect(described_class.features_for_plan('starter'))
-          .to include({ 'GitLab_MultipleIssueAssignees' => 1 })
+          .to include(:multiple_issue_assignees)
       end
 
       it 'returns features for premium plan' do
         expect(described_class.features_for_plan('premium'))
-          .to include({ 'GitLab_MultipleIssueAssignees' => 1, 'GitLab_DeployBoard' => 1, 'GitLab_FileLocks' => 1 })
+          .to include(:multiple_issue_assignees, :deploy_board, :file_locks)
       end
 
       it 'returns features for early adopter plan' do
         expect(described_class.features_for_plan('premium'))
-          .to include({ 'GitLab_DeployBoard' => 1, 'GitLab_FileLocks' => 1 } )
+          .to include(:deploy_board, :file_locks)
       end
 
-      it 'returns empty Hash if no features for given plan' do
-        expect(described_class.features_for_plan('bronze')).to eq({})
+      it 'returns empty array if no features for given plan' do
+        expect(described_class.features_for_plan('bronze')).to eq([])
       end
     end
 
@@ -264,8 +264,8 @@ describe License do
         let(:plan) { 'premium' }
         let(:feature) { nil }
 
-        it 'raises KeyError' do
-          expect { subject }.to raise_error(KeyError)
+        it 'returns false' do
+          is_expected.to eq(false)
         end
       end
     end
@@ -420,73 +420,60 @@ describe License do
       end
     end
 
-    describe '#add_ons' do
+    describe '#features_from_add_ons' do
       context 'without add-ons' do
-        it 'returns an empty Hash' do
+        it 'returns an empty array' do
           license = build_license_with_add_ons({}, plan: 'unknown')
 
-          expect(license.add_ons).to eq({})
+          expect(license.features_from_add_ons).to eq([])
         end
       end
 
       context 'with add-ons' do
         it 'returns all available add-ons' do
-          license = build_license_with_add_ons({ License::DEPLOY_BOARD_FEATURE => 1, License::FILE_LOCKS_FEATURE => 2 })
-
-          expect(license.add_ons.keys).to include(License::DEPLOY_BOARD_FEATURE, License::FILE_LOCKS_FEATURE)
-        end
-
-        it 'can return details about a single add-on' do
-          license = build_license_with_add_ons({ License::DEPLOY_BOARD_FEATURE => 2 })
-
-          expect(license.add_ons[License::DEPLOY_BOARD_FEATURE]).to eq(2)
-        end
-      end
-
-      context 'with extra features mapped by plan' do
-        it 'returns all available add-ons and extra features' do
-          license = build_license_with_add_ons({ License::DEPLOY_BOARD_FEATURE => 1 }, plan: License::PREMIUM_PLAN)
-          eep_features = License::EEP_FEATURES.reduce({}, :merge).keys
+          license = build_license_with_add_ons({ 'GitLab_DeployBoard' => 1, 'GitLab_FileLocks' => 2 })
 
-          expect(license.add_ons.keys).to include(License::DEPLOY_BOARD_FEATURE, *eep_features)
+          expect(license.features_from_add_ons).to match_array([:deploy_board, :file_locks])
         end
       end
     end
 
     describe '#feature_available?' do
       it 'returns true if add-on exists and have a quantity greater than 0' do
-        license = build_license_with_add_ons({ License::DEPLOY_BOARD_FEATURE => 1 })
+        license = build_license_with_add_ons({ 'GitLab_DeployBoard' => 1 })
 
         expect(license.feature_available?(:deploy_board)).to eq(true)
       end
 
-      it 'returns false if add-on exists but have a quantity of 0' do
-        license = build_license_with_add_ons({ License::DEPLOY_BOARD_FEATURE => 0 })
+      it 'returns true if the feature is included in the plan do' do
+        license = build_license_with_add_ons({}, plan: License::PREMIUM_PLAN)
 
-        expect(license.feature_available?(:deploy_board)).to eq(false)
+        expect(license.feature_available?(:auditor_user)).to eq(true)
       end
 
-      it 'returns false if add-on does not exists' do
-        license = build_license_with_add_ons({})
+      it 'returns false if add-on exists but have a quantity of 0' do
+        license = build_license_with_add_ons({ 'GitLab_DeployBoard' => 0 })
 
         expect(license.feature_available?(:deploy_board)).to eq(false)
       end
 
-      it 'raises error if invalid symbol is sent' do
+      it 'returns false if add-on does not exists' do
         license = build_license_with_add_ons({})
 
-        expect { license.feature_available?(:invalid) }.to raise_error(KeyError)
+        expect(license.feature_available?(:deploy_board)).to eq(false)
+        expect(license.feature_available?(:auditor_user)).to eq(false)
       end
 
       context 'with an expired trial license' do
+        let(:license) { create(:license, trial: true, expired: true) }
+
         before(:all) do
           described_class.destroy_all
-          create(:license, trial: true, expired: true)
         end
 
-        ::License::FEATURE_CODES.keys do |feature_code|
-          it "returns false for #{feature_code}" do
-            expect(license.feature_available?(feature_code)).to eq(false)
+        ::License::EES_FEATURES.each do |feature|
+          it "returns false for #{feature}" do
+            expect(license.feature_available?(feature)).to eq(false)
           end
         end
       end