Merge branch '335960-skip-user-limits-check' into 'master'

Skip true up flag in License causes user limit check to be run instead

See merge request gitlab-org/gitlab!66158

ee/app/models/license.rb

@@ -251,8 +251,9 @@ class License < ApplicationRecord
   LICENSEE_ATTRIBUTES = %w[Name Email Company].freeze
 
   validate :valid_license
-  validate :check_users_limit, if: :new_record?, unless: :validate_with_trueup?
-  validate :check_trueup, unless: :persisted?, if: :validate_with_trueup?
+  validate :check_users_limit, if: :new_record?, unless: [:validate_with_trueup?, :reconciliation_completed?]
+  validate :check_trueup, unless: [:persisted?, :reconciliation_completed?], if: :validate_with_trueup?
+  validate :check_restricted_user_count, if: :reconciliation_completed?
   validate :not_expired, unless: :persisted?
 
   before_validation :reset_license, if: :data_changed?
@@ -444,6 +445,10 @@ class License < ApplicationRecord
     restricted_attr(:subscription_id)
   end
 
+  def reconciliation_completed?
+    restricted_attr(:reconciliation_completed)
+  end
+
   def features_from_add_ons
     add_ons.map { |name, count| FEATURES_FOR_ADD_ONS[name] if count.to_i > 0 }.compact
   end
@@ -498,8 +503,6 @@ class License < ApplicationRecord
   end
 
   def validate_with_trueup?
-    return false if restricted_attr(:skip_true_up)
-
     [restricted_attr(:trueup_quantity),
      restricted_attr(:trueup_from),
      restricted_attr(:trueup_to)].all?(&:present?)
@@ -692,9 +695,7 @@ class License < ApplicationRecord
                           end
 
     if trueup_qty >= expected_trueup_qty
-      if restricted_user_count < daily_billable_users_count
-        add_limit_error(user_count: daily_billable_users_count)
-      end
+      check_restricted_user_count
     else
       message = ["You have applied a True-up for #{trueup_qty} #{"user".pluralize(trueup_qty)}"]
       message << "but you need one for #{expected_trueup_qty} #{"user".pluralize(expected_trueup_qty)}."
@@ -704,6 +705,12 @@ class License < ApplicationRecord
     end
   end
 
+  def check_restricted_user_count
+    return unless restricted_user_count && restricted_user_count < daily_billable_users_count
+
+    add_limit_error(user_count: daily_billable_users_count)
+  end
+
   def add_limit_error(current_period: true, user_count:)
     overage_count = overage(user_count)
 

ee/spec/models/license_spec.rb

@@ -12,9 +12,7 @@ RSpec.describe License do
   describe 'validations' do
     describe '#valid_license' do
       context 'when the license is provided' do
-        it 'is valid' do
-          expect(license).to be_valid
-        end
+        it { is_expected.to be_valid }
       end
 
       context 'when no license is provided' do
@@ -22,9 +20,7 @@ RSpec.describe License do
           license.data = nil
         end
 
-        it 'is invalid' do
-          expect(license).not_to be_valid
-        end
+        it { is_expected.not_to be_valid }
       end
     end
 
@@ -36,17 +32,17 @@ RSpec.describe License do
         create(:historical_data, recorded_at: date, active_user_count: active_user_count)
       end
 
-      context 'when skip_true_up is true on the license' do
-        it 'does not add errors for invalid true up' do
-          set_restrictions(restricted_user_count: 10, trueup_quantity: 8, skip_true_up: true)
-
-          expect(license).to be_valid
+      context 'when reconciliation_completed is true on the license' do
+        before do
+          set_restrictions(restricted_user_count: 10, trueup_quantity: 8, reconciliation_completed: true)
         end
+
+        it { is_expected.to be_valid }
       end
 
-      context 'when skip_true_up is false on the license' do
+      context 'when reconciliation_completed is false on the license' do
         it 'adds errors for invalid true up figures' do
-          set_restrictions(restricted_user_count: 10, trueup_quantity: 8, skip_true_up: false)
+          set_restrictions(restricted_user_count: 10, trueup_quantity: 8, reconciliation_completed: false)
 
           expect(license).not_to be_valid
           expect(license.errors.full_messages.to_sentence)
@@ -54,7 +50,7 @@ RSpec.describe License do
         end
       end
 
-      context 'when skip_true_up is not present on the license' do
+      context 'when reconciliation_completed is not present on the license' do
         it 'adds errors for invalid true up figures' do
           set_restrictions(restricted_user_count: 10, trueup_quantity: 8)
 
@@ -69,25 +65,23 @@ RSpec.describe License do
           set_restrictions(restricted_user_count: 5, trueup_quantity: 10)
         end
 
-        it 'is valid' do
-          expect(license).to be_valid
-        end
+        it { is_expected.to be_valid }
 
         context 'but active users exceeds restricted user count' do
-          it 'is invalid' do
+          before do
             create_list(:user, 6)
-
-            expect(license).not_to be_valid
           end
+
+          it { is_expected.not_to be_valid }
         end
       end
 
       context 'when quantity is wrong' do
-        it 'is invalid' do
+        before do
           set_restrictions(restricted_user_count: 5, trueup_quantity: 8)
-
-          expect(license).not_to be_valid
         end
+
+        it { is_expected.not_to be_valid }
       end
 
       context 'when previous user count is not present' do
@@ -102,11 +96,11 @@ RSpec.describe License do
         end
 
         context 'with wrong true-up quantity' do
-          it 'is invalid' do
+          before do
             create_list(:user, 2)
-
-            expect(license).not_to be_valid
           end
+
+          it { is_expected.not_to be_valid }
         end
       end
 
@@ -115,8 +109,55 @@ RSpec.describe License do
           set_restrictions(restricted_user_count: 5, trueup_quantity: 6, previous_user_count: 4)
         end
 
-        it 'uses it to calculate the expected true-up' do
-          expect(license).to be_valid
+        it { is_expected.to be_valid }
+      end
+    end
+
+    describe '#check_restricted_user_count' do
+      context 'when reconciliation_completed is true' do
+        context 'when restricted_user_count is equal or more than active_user_count' do
+          before do
+            set_restrictions(restricted_user_count: 10, reconciliation_completed: true)
+          end
+
+          it { is_expected.to be_valid }
+        end
+
+        context 'when the restricted_user_count is less than active_user_count' do
+          before do
+            set_restrictions(restricted_user_count: 2, reconciliation_completed: true)
+            create_list(:user, 6)
+            create(:historical_data, recorded_at: described_class.current.starts_at, active_user_count: 100)
+          end
+
+          it 'add limit error' do
+            expect(license.valid?).to be_falsey
+
+            expect(license.errors.full_messages.to_sentence).to include(
+              'This GitLab installation currently has 6 active users, exceeding this license\'s limit of 2 by 4 users'
+            )
+            expect(license.errors.full_messages.to_sentence).not_to include(
+              'During the year before this license started'
+            )
+          end
+        end
+      end
+
+      context 'when reconciliation_completed is false' do
+        context 'when the restricted_user_count is less than active_user_count' do
+          before do
+            set_restrictions(restricted_user_count: 2, reconciliation_completed: false)
+            create_list(:user, 6)
+            create(:historical_data, recorded_at: described_class.current.starts_at, active_user_count: 100)
+          end
+
+          it 'add limit error' do
+            expect(license.valid?).to be_falsey
+
+            expect(license.errors.full_messages.to_sentence).to include(
+              'During the year before this license started'
+            )
+          end
         end
       end
     end
@@ -232,9 +273,7 @@ RSpec.describe License do
         let!(:historical_data)  { create(:historical_data, recorded_at: date, active_user_count: active_user_count) }
 
         context 'when there is no active user count restriction' do
-          it 'is valid' do
-            expect(license).to be_valid
-          end
+          it { is_expected.to be_valid }
         end
 
         context 'without historical data' do
@@ -250,9 +289,7 @@ RSpec.describe License do
           end
 
           context 'with previous_user_count and active users above of license limit' do
-            it 'is invalid' do
-              expect(license).to be_invalid
-            end
+            it { is_expected.not_to be_valid }
 
             it 'shows the proper error message' do
               license.valid?
@@ -271,33 +308,25 @@ RSpec.describe License do
           end
 
           context 'when the license started' do
-            it 'is invalid' do
-              expect(license).not_to be_valid
-            end
+            it { is_expected.not_to be_valid }
           end
 
           context 'after the license started' do
             let(:date) { Date.current }
 
-            it 'is valid' do
-              expect(license).to be_valid
-            end
+            it { is_expected.to be_valid }
           end
 
           context 'in the year before the license started' do
             let(:date) { described_class.current.starts_at - 6.months }
 
-            it 'is invalid' do
-              expect(license).not_to be_valid
-            end
+            it { is_expected.not_to be_valid }
           end
 
           context 'earlier than a year before the license started' do
             let(:date) { described_class.current.starts_at - 2.years }
 
-            it 'is valid' do
-              expect(license).to be_valid
-            end
+            it { is_expected.to be_valid }
           end
         end
 
@@ -306,9 +335,7 @@ RSpec.describe License do
             gl_license.restrictions = { active_user_count: active_user_count + 1 }
           end
 
-          it 'is valid' do
-            expect(license).to be_valid
-          end
+          it { is_expected.to be_valid }
         end
 
         context 'when the active user count is met exactly' do
@@ -324,9 +351,7 @@ RSpec.describe License do
 
     describe '#not_expired' do
       context "when the license doesn't expire" do
-        it 'is valid' do
-          expect(license).to be_valid
-        end
+        it { is_expected.to be_valid }
       end
 
       context 'when the license has expired' do
@@ -334,9 +359,7 @@ RSpec.describe License do
           gl_license.expires_at = Date.yesterday
         end
 
-        it 'is invalid' do
-          expect(license).not_to be_valid
-        end
+        it { is_expected.not_to be_valid }
       end
 
       context 'when the license has yet to expire' do
@@ -344,9 +367,7 @@ RSpec.describe License do
           gl_license.expires_at = Date.tomorrow
         end
 
-        it 'is valid' do
-          expect(license).to be_valid
-        end
+        it { is_expected.to be_valid }
       end
     end
 
@@ -370,9 +391,7 @@ RSpec.describe License do
           set_restrictions(restricted_user_count: 10, previous_user_count: 15)
         end
 
-        it 'is valid' do
-          expect(license).to be_valid
-        end
+        it { is_expected.to be_valid }
       end
     end
   end
@@ -518,34 +537,26 @@ RSpec.describe License do
       context 'when addon included' do
         let(:plan) { 'premium' }
 
-        it 'returns true' do
-          is_expected.to eq(true)
-        end
+        it { is_expected.to eq(true) }
       end
 
       context 'when addon not included' do
         let(:plan) { 'starter' }
 
-        it 'returns false' do
-          is_expected.to eq(false)
-        end
+        it { is_expected.to eq(false) }
       end
 
       context 'when plan is not set' do
         let(:plan) { nil }
 
-        it 'returns false' do
-          is_expected.to eq(false)
-        end
+        it { is_expected.to eq(false) }
       end
 
       context 'when feature does not exists' do
         let(:plan) { 'premium' }
         let(:feature) { nil }
 
-        it 'returns false' do
-          is_expected.to eq(false)
-        end
+        it { is_expected.to eq(false) }
       end
     end
 
@@ -1474,7 +1485,7 @@ RSpec.describe License do
       trueup_quantity: opts[:trueup_quantity],
       trueup_from: (date - 1.year).to_s,
       trueup_to: date.to_s,
-      skip_true_up: opts[:skip_true_up]
+      reconciliation_completed: opts[:reconciliation_completed]
     }.compact
   end