Merge branch '247846_feature_flag_correction' into 'master'

Always check both storage feature flags

See merge request gitlab-org/gitlab!45909

ee/app/helpers/ee/namespace_storage_limit_alert_helper.rb

@@ -50,7 +50,7 @@ module EE
       ::Gitlab.dev_env_or_com? &&
         ::Gitlab::CurrentSettings.automatic_purchased_storage_allocation? &&
         ::Feature.enabled?(:buy_storage_link) &&
-        ::Feature.enabled?(:additional_repo_storage_by_namespace, namespace)
+        namespace.additional_repo_storage_by_namespace_enabled?
     end
 
     def namespace_storage_usage_link(namespace)

ee/app/models/ee/namespace/root_excess_storage_size.rb

@@ -33,7 +33,7 @@ module EE
 
     def enforce_limit?
       return false unless ::Gitlab::CurrentSettings.automatic_purchased_storage_allocation?
-      return false unless ::Feature.enabled?(:additional_repo_storage_by_namespace, root_namespace)
+      return false unless root_namespace.additional_repo_storage_by_namespace_enabled?
 
       true
     end

ee/lib/ee/gitlab/repository_size_checker.rb

@@ -27,7 +27,7 @@ module EE
       def additional_repo_storage_available?
         return false unless ::Gitlab::CurrentSettings.automatic_purchased_storage_allocation?
 
-        ::Feature.enabled?(:additional_repo_storage_by_namespace, namespace)
+        !!namespace&.additional_repo_storage_by_namespace_enabled?
       end
 
       def total_repository_size_excess

ee/spec/helpers/ee/namespace_storage_limit_alert_helper_spec.rb

@@ -206,33 +206,33 @@ RSpec.describe EE::NamespaceStorageLimitAlertHelper do
   describe '#can_purchase_storage_for_namespace?' do
     subject { helper.can_purchase_storage_for_namespace?(namespace) }
 
-    let(:namespace) { build(:namespace) }
+    let_it_be(:namespace) { build(:namespace) }
 
-    where(:is_dev_or_com, :auto_storage_allocation_enabled, :buy_storage_link_enabled, :additional_storage_enabled, :result) do
-      true  | true  | true  | true  | true
-      true  | true  | true  | false | false
-      true  | true  | false | true  | false
-      true  | true  | false | false | false
-      true  | false | true  | true  | false
-      true  | false | true  | false | false
-      true  | false | false | true  | false
-      true  | false | false | false | false
-      false | true  | true  | true  | false
-      false | true  | true  | false | false
-      false | true  | false | true  | false
-      false | true  | false | false | false
-      false | false | true  | true  | false
-      false | false | true  | false | false
-      false | false | false | true  | false
-      false | false | false | false | false
-    end
+    where(
+      is_dev_or_com: [true, false],
+      auto_storage_allocation_enabled: [true, false],
+      buy_storage_link_enabled: [true, false],
+      namespace_storage_limit_enabled: [true, false],
+      additional_storage_enabled: [true, false]
+    )
 
     with_them do
+      let(:result) do
+        is_dev_or_com &&
+          auto_storage_allocation_enabled &&
+          buy_storage_link_enabled &&
+          !namespace_storage_limit_enabled &&
+          additional_storage_enabled
+      end
+
       before do
         allow(::Gitlab).to receive(:dev_env_or_com?).and_return(is_dev_or_com)
         stub_application_setting(automatic_purchased_storage_allocation: auto_storage_allocation_enabled)
-        stub_feature_flags(additional_repo_storage_by_namespace: additional_storage_enabled)
-        stub_feature_flags(buy_storage_link: buy_storage_link_enabled)
+        stub_feature_flags(
+          namespace_storage_limit: namespace_storage_limit_enabled,
+          additional_repo_storage_by_namespace: additional_storage_enabled,
+          buy_storage_link: buy_storage_link_enabled
+        )
       end
 
       it { is_expected.to eq(result) }

ee/spec/lib/ee/gitlab/repository_size_checker_spec.rb

@@ -23,68 +23,88 @@ RSpec.describe Gitlab::RepositorySizeChecker do
   before do
     allow(Gitlab::CurrentSettings).to receive(:automatic_purchased_storage_allocation?).and_return(gitlab_setting_enabled)
 
-    allow(namespace).to receive(:total_repository_size_excess).and_return(total_repository_size_excess.megabytes)
+    allow(namespace).to receive(:total_repository_size_excess).and_return(total_repository_size_excess.megabytes) if namespace
   end
 
   describe '#above_size_limit?' do
-    context 'when Gitlab app setting for automatic purchased storage allocation is not enabled' do
-      let(:gitlab_setting_enabled) { false }
-
+    shared_examples 'original logic (additional storage not considered)' do
       include_examples 'checker size above limit'
       include_examples 'checker size not over limit'
-    end
 
-    context 'with feature flag :additional_repo_storage_by_namespace enabled' do
-      shared_examples 'feature flag :additional_repo_storage_by_namespace enabled' do
-        context 'when there is available excess storage' do
-          it 'returns false' do
-            expect(subject.above_size_limit?).to eq(false)
-          end
+      context 'when over the default limit but would be under the limit if additional storage was enabled' do
+        let(:current_size) { 100 }
+        let(:additional_purchased_storage) { 60 }
+
+        it 'returns true' do
+          expect(subject.above_size_limit?).to eq(true)
         end
+      end
+    end
 
-        context 'when size is above the limit and there is no excess storage' do
-          let(:current_size) { 100 }
-          let(:total_repository_size_excess) { 20 }
-          let(:additional_purchased_storage) { 10 }
+    context 'when enabled is false' do
+      let(:enabled) { false }
 
-          it 'returns true' do
-            expect(subject.above_size_limit?).to eq(true)
-          end
+      context 'when size is under the limit' do
+        it 'returns false' do
+          expect(subject.above_size_limit?).to eq(false)
         end
+      end
 
-        it 'returns false when not over the limit' do
+      context 'when size is above the limit' do
+        let(:current_size) { 100 }
+
+        it 'returns false' do
           expect(subject.above_size_limit?).to eq(false)
         end
       end
+    end
 
-      include_examples 'feature flag :additional_repo_storage_by_namespace enabled'
+    include_examples 'original logic (additional storage not considered)'
 
-      context 'when only enabled for a specific namespace' do
-        before do
-          stub_feature_flags(additional_repo_storage_by_namespace: false)
-        end
+    context 'when Gitlab app setting for automatic purchased storage allocation is not enabled' do
+      let(:gitlab_setting_enabled) { false }
 
-        context 'when namespace is another one than the given one' do
-          let(:other_namespace) { create(:namespace) }
+      include_examples 'original logic (additional storage not considered)'
+    end
 
-          before do
-            stub_feature_flags(additional_repo_storage_by_namespace: other_namespace)
-          end
+    context 'when namespace is nil' do
+      let(:namespace) { nil }
 
-          include_examples 'checker size above limit'
-          include_examples 'checker size not over limit'
-        end
+      include_examples 'original logic (additional storage not considered)'
+    end
 
-        context 'when namespace is the given one' do
-          let(:namespace) { create(:namespace, additional_purchased_storage_size: additional_purchased_storage) }
+    context 'with feature flag :namespace_storage_limit disabled' do
+      before do
+        stub_feature_flags(namespace_storage_limit: false)
+      end
 
-          before do
-            stub_feature_flags(additional_repo_storage_by_namespace: namespace)
-          end
+      context 'when there are no locked projects (total repository excess < additional storage)' do
+        let(:current_size) { 100 } # current_size > limit
+        let(:total_repository_size_excess) { 5 }
+        let(:additional_purchased_storage) { 10 }
 
-          include_examples 'feature flag :additional_repo_storage_by_namespace enabled'
+        it 'returns false' do
+          expect(subject.above_size_limit?).to eq(false)
         end
       end
+
+      context 'when there are no locked projects (total repository excess == additional storage)' do
+        let(:current_size) { 100 } # current_size > limit
+        let(:total_repository_size_excess) { 10 }
+        let(:additional_purchased_storage) { 10 }
+
+        it 'returns false' do
+          expect(subject.above_size_limit?).to eq(false)
+        end
+      end
+
+      context 'when there are locked projects (total repository excess > additional storage)' do
+        let(:total_repository_size_excess) { 12 }
+        let(:additional_purchased_storage) { 10 }
+
+        include_examples 'checker size above limit'
+        include_examples 'checker size not over limit'
+      end
     end
 
     context 'with feature flag :additional_repo_storage_by_namespace disabled' do
@@ -92,17 +112,28 @@ RSpec.describe Gitlab::RepositorySizeChecker do
         stub_feature_flags(additional_repo_storage_by_namespace: false)
       end
 
-      include_examples 'checker size above limit'
-      include_examples 'checker size not over limit'
+      include_examples 'original logic (additional storage not considered)'
     end
   end
 
   describe '#exceeded_size' do
-    context 'with feature flag :additional_repo_storage_by_namespace enabled' do
-      context 'when Gitlab app setting for automatic purchased storage allocation is not enabled' do
-        let(:gitlab_setting_enabled) { false }
+    include_examples 'checker size exceeded'
 
-        include_examples 'checker size exceeded'
+    context 'when Gitlab app setting for automatic purchased storage allocation is not enabled' do
+      let(:gitlab_setting_enabled) { false }
+
+      include_examples 'checker size exceeded'
+    end
+
+    context 'when namespace is nil' do
+      let(:namespace) { nil }
+
+      include_examples 'checker size exceeded'
+    end
+
+    context 'with feature flag :namespace_storage_limit disabled' do
+      before do
+        stub_feature_flags(namespace_storage_limit: false)
       end
 
       context 'when current size + total repository size excess are below or equal to the limit + additional purchased storage' do

ee/spec/models/ee/namespace/root_excess_storage_size_spec.rb

@@ -28,14 +28,28 @@ RSpec.describe EE::Namespace::RootExcessStorageSize do
     context 'when limit enforcement is on' do
       let(:storage_allocation_enabled) { true }
 
-      context 'when below limit' do
-        it { is_expected.to eq(false) }
+      context 'with feature flag :namespace_storage_limit disabled' do
+        before do
+          stub_feature_flags(namespace_storage_limit: false)
+        end
+
+        context 'when below limit' do
+          it { is_expected.to eq(false) }
+        end
+
+        context 'when above limit' do
+          let(:total_repository_size_excess) { 101.megabytes }
+
+          it { is_expected.to eq(true) }
+        end
       end
 
-      context 'when above limit' do
-        let(:total_repository_size_excess) { 101.megabytes }
+      context 'with feature flag :additional_repo_storage_by_namespace disabled' do
+        before do
+          stub_feature_flags(additional_repo_storage_by_namespace: false)
+        end
 
-        it { is_expected.to eq(true) }
+        it { is_expected.to eq(false) }
       end
     end
   end
@@ -91,14 +105,24 @@ RSpec.describe EE::Namespace::RootExcessStorageSize do
       allow(Gitlab::CurrentSettings).to receive(:automatic_purchased_storage_allocation?) { storage_allocation_enabled }
     end
 
-    it { is_expected.to eq(true) }
-
     context 'with application setting is disabled' do
       let(:storage_allocation_enabled) { false }
 
       it { is_expected.to eq(false) }
     end
 
+    context 'with feature flags (:namespace_storage_limit & :additional_repo_storage_by_namespace) enabled' do
+      it { is_expected.to eq(false) }
+    end
+
+    context 'with feature flag :namespace_storage_limit disabled' do
+      before do
+        stub_feature_flags(namespace_storage_limit: false)
+      end
+
+      it { is_expected.to eq(true) }
+    end
+
     context 'with feature flag :additional_repo_storage_by_namespace disabled' do
       before do
         stub_feature_flags(additional_repo_storage_by_namespace: false)

ee/spec/services/namespaces/check_excess_storage_size_service_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe Namespaces::CheckExcessStorageSizeService, '#execute' do
   let(:service) { described_class.new(namespace, user) }
   let(:total_repository_size_excess) { 150.megabytes }
   let(:additional_purchased_storage_size) { 100 }
+  let(:namespace_storage_limit_enabled) { false }
   let(:storage_allocation_enabled) { true }
   let(:actual_size_limit) { 10.gigabytes }
   let(:locked_project_count) { 1 }
@@ -15,6 +16,7 @@ RSpec.describe Namespaces::CheckExcessStorageSizeService, '#execute' do
   subject(:response) { service.execute }
 
   before do
+    stub_feature_flags(namespace_storage_limit: namespace_storage_limit_enabled)
     allow(Gitlab::CurrentSettings).to receive(:automatic_purchased_storage_allocation?) { storage_allocation_enabled }
 
     allow(namespace).to receive(:root_ancestor).and_return(namespace)
@@ -37,6 +39,12 @@ RSpec.describe Namespaces::CheckExcessStorageSizeService, '#execute' do
 
       it { is_expected.to be_success }
     end
+
+    context 'with feature flag :namespace_storage_limit enabled' do
+      let(:namespace_storage_limit_enabled) { true }
+
+      it { is_expected.to be_success }
+    end
   end
 
   context 'when additional_purchased_storage_size is set to 0' do

lib/gitlab/repository_size_checker.rb

@@ -32,7 +32,7 @@ module Gitlab
     def changes_will_exceed_size_limit?(change_size)
       return false unless enabled?
 
-      change_size > limit || exceeded_size(change_size) > 0
+      above_size_limit? || exceeded_size(change_size) > 0
     end
 
     # @param change_size [int] in bytes