Fix previous license period

ee/app/controllers/concerns/admin/license_request.rb

@@ -8,7 +8,6 @@ module Admin
       @license ||= begin
         License.reset_current
         License.reset_future_dated
-        License.reset_previous
         License.current
       end
     end

ee/app/models/license.rb

@@ -258,16 +258,15 @@ class License < ApplicationRecord
 
   validate :valid_license
   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_trueup, unless: :reconciliation_completed?, if: [:new_record?, :validate_with_trueup?]
   validate :check_restricted_user_count, if: :reconciliation_completed?
-  validate :not_expired, unless: :persisted?
+  validate :not_expired, if: :new_record?
 
   before_validation :reset_license, if: :data_changed?
 
   after_create :update_trial_setting
   after_commit :reset_current
   after_commit :reset_future_dated, on: [:create, :destroy]
-  after_commit :reset_previous, on: [:create, :destroy]
 
   scope :cloud, -> { where(cloud: true) }
   scope :recent, -> { reorder(id: :desc) }
@@ -331,14 +330,6 @@ class License < ApplicationRecord
       Gitlab::SafeRequestStore.delete(:future_dated_license)
     end
 
-    def previous
-      Gitlab::SafeRequestStore.fetch(:previous_license) { load_previous }
-    end
-
-    def reset_previous
-      Gitlab::SafeRequestStore.delete(:previous_license)
-    end
-
     def global_feature?(feature)
       GLOBAL_FEATURES.include?(feature)
     end
@@ -377,10 +368,6 @@ class License < ApplicationRecord
     def load_future_dated
       self.last_hundred.find { |license| license.valid? && license.future_dated? }
     end
-
-    def load_previous
-      self.last_hundred.find { |license| license.valid? && !license.future_dated? && license != License.current }
-    end
   end
 
   def data_filename
@@ -641,10 +628,6 @@ class License < ApplicationRecord
     self.class.reset_future_dated
   end
 
-  def reset_previous
-    self.class.reset_previous
-  end
-
   def reset_license
     @license = nil
   end
@@ -655,12 +638,27 @@ class License < ApplicationRecord
     self.errors.add(:base, _('The license key is invalid. Make sure it is exactly as you received it from GitLab Inc.'))
   end
 
+  # This method, `previous_started_at` and `previous_expired_at` are
+  # only used in the validation methods `check_users_limit` and check_trueup
+  # which are only used when uploading/creating a new license.
+  # The method will not work in other workflows since it has a dependency to
+  # use the current license as the previous in the system.
   def prior_historical_max
-    @prior_historical_max ||= begin
+    strong_memoize(:prior_historical_max) do
       historical_max(from: previous_started_at, to: previous_expired_at)
     end
   end
 
+  # See comment for `prior_historical_max`.
+  def previous_started_at
+    (License.current&.starts_at || starts_at - 1.year).beginning_of_day
+  end
+
+  # See comment for `prior_historical_max`.
+  def previous_expired_at
+    (License.current&.expires_at || expires_at && expires_at - 1.year || starts_at).end_of_day
+  end
+
   def restricted_user_count_with_threshold
     (restricted_user_count * (1 + ALLOWED_PERCENTAGE_OF_USERS_OVERAGE)).to_i
   end
@@ -669,15 +667,19 @@ class License < ApplicationRecord
     return if cloud_license?
     return unless restricted_user_count
 
+    user_count = daily_billable_users_count
+    current_period = true
+
     if previous_user_count && (prior_historical_max <= previous_user_count)
       return if restricted_user_count_with_threshold >= daily_billable_users_count
     else
       return if restricted_user_count_with_threshold >= prior_historical_max
-    end
 
-    user_count = prior_historical_max == 0 ? daily_billable_users_count : prior_historical_max
+      user_count = prior_historical_max
+      current_period = false
+    end
 
-    add_limit_error(current_period: prior_historical_max == 0, user_count: user_count)
+    add_limit_error(current_period: current_period, user_count: user_count)
   end
 
   def check_trueup
@@ -731,14 +733,6 @@ class License < ApplicationRecord
     self.errors.add(:base, _('This license has already expired.'))
   end
 
-  def previous_started_at
-    (License.previous&.starts_at || starts_at - 1.year).beginning_of_day
-  end
-
-  def previous_expired_at
-    (License.previous&.expires_at || starts_at).end_of_day
-  end
-
   def starts_at_for_historical_data
     (starts_at || Time.current - 1.year).beginning_of_day
   end

ee/spec/models/license_spec.rb

@@ -185,6 +185,30 @@ RSpec.describe License do
     end
 
     describe '#check_users_limit' do
+      let(:expires_at) { 11.months.from_now.to_date }
+      let(:restrictions) { { active_user_count: 9 } }
+      let(:gl_license) do
+        build(
+          :gitlab_license,
+          starts_at: 1.month.ago.to_date,
+          expires_at: expires_at,
+          restrictions: restrictions
+        )
+      end
+
+      def create_historical_data(recorded_at, prior_active_user_count)
+        create(
+          :historical_data,
+          recorded_at: recorded_at + 1.day,
+          active_user_count: 1
+        )
+        create(
+          :historical_data,
+          recorded_at: recorded_at,
+          active_user_count: prior_active_user_count
+        )
+      end
+
       context 'for each plan' do
         before do
           create(:group_member, :guest)
@@ -223,164 +247,155 @@ RSpec.describe License do
         end
       end
 
-      context 'threshold for users overage' do
-        let(:current_active_users_count) { 0 }
-        let(:new_license) { build(:license, data: gitlab_license.export) }
+      context 'when license is a cloud license' do
         let(:gitlab_license) do
           build(
             :gitlab_license,
+            cloud_licensing_enabled: true,
             starts_at: Date.current,
-            restrictions: { active_user_count: 10, previous_user_count: previous_user_count }
+            restrictions: { active_user_count: 10 }
           )
         end
 
-        context 'when current active users count is above the limit set by the license' do
-          before do
-            create_list(:user, current_active_users_count)
-            HistoricalData.track!
+        it { is_expected.to be_valid }
       end
 
-          shared_examples 'current active user count within threshold' do
-            context 'when current active users count is under the threshold' do
-              let(:current_active_users_count) { 10 }
+      context 'when no restriction is set' do
+        let(:restrictions) { {} }
 
-              it 'accepts the license' do
-                expect(new_license).to be_valid
-              end
+        it { is_expected.to be_valid }
       end
 
-            context 'when current active users count is equal to the threshold' do
-              let(:current_active_users_count) { 11 }
+      context 'without historical data' do
+        let(:active_user_count) { 9 }
 
-              it 'accepts the license' do
-                expect(new_license).to be_valid
-              end
-            end
+        before do
+          create_list(:user, billable_users_count)
         end
 
-          context 'when license is from a fresh subscription' do
-            let(:previous_user_count) { nil }
+        context 'with previous user count' do
+          let(:prior_active_user_count) { 0 }
+          let(:restrictions) { { active_user_count: active_user_count, previous_user_count: previous_user_count } }
 
-            include_examples 'current active user count within threshold'
-
-            context 'when current active users count is above the threshold' do
-              let(:current_active_users_count) { 12 }
+          context 'when prior historical max is less than previous user count' do
+            let(:previous_user_count) { 1 }
 
-              it 'does not accept the license' do
-                expect(new_license).not_to be_valid
+            include_examples 'valid daily billable users count compared to limit set by license checks'
+            include_examples 'invalid daily billable users count compared to limit set by license checks'
           end
 
-              context 'when license is a cloud license' do
-                let(:gitlab_license) do
-                  build(
-                    :gitlab_license,
-                    cloud_licensing_enabled: true,
-                    starts_at: Date.current,
-                    restrictions: { active_user_count: 10, previous_user_count: previous_user_count }
-                  )
-                end
+          context 'when prior historical max is equal to previous user count' do
+            let(:previous_user_count) { 0 }
 
-                it 'accepts the license' do
-                  expect(new_license).to be_valid
+            include_examples 'valid daily billable users count compared to limit set by license checks'
+            include_examples 'invalid daily billable users count compared to limit set by license checks'
           end
         end
+
+        context 'without previous user count' do
+          let(:restrictions) { { active_user_count: active_user_count } }
+
+          include_examples 'valid prior historical max compared to limit set by license checks'
         end
       end
 
-          context 'when license is from a renewal' do
-            let(:previous_user_count) { 1 }
+      context 'with historical data in the term of an existing current license' do
+        let(:active_user_count) { 9 }
 
-            include_examples 'current active user count within threshold'
+        before do
+          create_list(:user, billable_users_count)
+          create_historical_data(License.current.expires_at, prior_active_user_count)
+        end
 
-            context 'when current active users count is over the threshold' do
-              let(:current_active_users_count) { 12 }
+        context 'with previous user count' do
+          let(:previous_user_count) { 7 }
+          let(:restrictions) { { active_user_count: active_user_count, previous_user_count: previous_user_count } }
 
-              it 'does not accept the license' do
-                expect(new_license).not_to be_valid
-              end
-            end
-          end
-        end
+          include_examples 'with previous user count checks'
         end
 
-      describe 'Historical active user count' do
-        let(:active_user_count) { described_class.current.daily_billable_users_count + 10 }
-        let(:date)              { described_class.current.starts_at }
-        let!(:historical_data)  { create(:historical_data, recorded_at: date, active_user_count: active_user_count) }
+        context 'without previous user count' do
+          let(:restrictions) { { active_user_count: active_user_count } }
 
-        context 'when there is no active user count restriction' do
-          it { is_expected.to be_valid }
+          include_examples 'valid prior historical max compared to limit set by license checks'
+          include_examples 'invalid prior historical max compared to limit set by license checks'
+        end
       end
 
-        context 'without historical data' do
+      context 'with historical data in the term of the new license (no current license exists)' do
+        let(:active_user_count) { 9 }
+        let(:restrictions) { { active_user_count: active_user_count } }
+
         before do
-            create_list(:user, 2)
+          create_list(:user, billable_users_count)
 
-            gl_license.restrictions = {
-              previous_user_count: 1,
-              active_user_count: described_class.current.daily_billable_users_count - 1
-            }
+          allow(License).to receive(:current).and_return(nil)
+        end
 
-            HistoricalData.delete_all
+        context 'when new license has an expiration date' do
+          before do
+            create_historical_data(license.starts_at - 1.year, prior_active_user_count)
           end
 
-          context 'with previous_user_count and active users above of license limit' do
-            it { is_expected.not_to be_valid }
+          context 'with previous user count' do
+            let(:previous_user_count) { 7 }
+            let(:restrictions) { { active_user_count: active_user_count, previous_user_count: previous_user_count } }
 
-            it 'shows the proper error message' do
-              license.valid?
+            include_examples 'with previous user count checks'
+          end
 
-              error_msg = "This GitLab installation currently has 2 active users, exceeding this license's limit of 1 by 1 user. " \
-                        "Please upload a license for at least 2 users or contact sales at https://about.gitlab.com/sales/"
+          context 'without previous user count' do
+            let(:restrictions) { { active_user_count: active_user_count } }
 
-              expect(license.errors[:base].first).to eq(error_msg)
-            end
+            include_examples 'valid prior historical max compared to limit set by license checks'
+            include_examples 'invalid prior historical max compared to limit set by license checks'
           end
         end
 
-        context 'when the active user count restriction is exceeded' do
-          before do
-            gl_license.restrictions = { active_user_count: active_user_count - 1 }
-          end
+        context 'when new license has no expiration' do
+          let(:expires_at) { nil }
 
-          context 'when the license started' do
-            it { is_expected.not_to be_valid }
+          before do
+            create_historical_data(license.starts_at, prior_active_user_count)
           end
 
-          context 'after the license started' do
-            let(:date) { Date.current }
+          context 'with previous user count' do
+            let(:previous_user_count) { 7 }
+            let(:restrictions) { { active_user_count: active_user_count, previous_user_count: previous_user_count } }
 
-            it { is_expected.to be_valid }
+            include_examples 'with previous user count checks'
           end
 
-          context 'in the year before the license started' do
-            let(:date) { described_class.current.starts_at - 6.months }
+          context 'without previous user count' do
+            let(:restrictions) { { active_user_count: active_user_count } }
 
-            it { is_expected.not_to be_valid }
+            include_examples 'valid prior historical max compared to limit set by license checks'
+            include_examples 'invalid prior historical max compared to limit set by license checks'
           end
-
-          context 'earlier than a year before the license started' do
-            let(:date) { described_class.current.starts_at - 2.years }
-
-            it { is_expected.to be_valid }
         end
       end
 
-        context 'when the active user count restriction is not exceeded' do
+      context 'downgrade' do
+        context 'when more users were added in previous period' do
           before do
-            gl_license.restrictions = { active_user_count: active_user_count + 1 }
+            create(:historical_data, recorded_at: described_class.current.starts_at + 1.month, active_user_count: 15)
+
+            set_restrictions(restricted_user_count: 5, previous_user_count: 10)
           end
 
-          it { is_expected.to be_valid }
+          it 'is invalid without a true-up' do
+            expect(license).not_to be_valid
+          end
         end
 
-        context 'when the active user count is met exactly' do
-          it 'is valid' do
-            active_user_count = 100
-            gl_license.restrictions = { active_user_count: active_user_count }
+        context 'when no users were added in the previous period' do
+          before do
+            create(:historical_data, recorded_at: 6.months.ago, active_user_count: 15)
 
-            expect(license).to be_valid
+            set_restrictions(restricted_user_count: 10, previous_user_count: 15)
           end
+
+          it { is_expected.to be_valid }
         end
       end
     end
@@ -406,30 +421,6 @@ RSpec.describe License do
         it { is_expected.to be_valid }
       end
     end
-
-    describe 'downgrade' do
-      context 'when more users were added in previous period' do
-        before do
-          create(:historical_data, recorded_at: described_class.current.starts_at - 6.months, active_user_count: 15)
-
-          set_restrictions(restricted_user_count: 5, previous_user_count: 10)
-        end
-
-        it 'is invalid without a true-up' do
-          expect(license).not_to be_valid
-        end
-      end
-
-      context 'when no users were added in the previous period' do
-        before do
-          create(:historical_data, recorded_at: 6.months.ago, active_user_count: 15)
-
-          set_restrictions(restricted_user_count: 10, previous_user_count: 15)
-        end
-
-        it { is_expected.to be_valid }
-      end
-    end
   end
 
   describe 'Callbacks' do
@@ -498,36 +489,6 @@ RSpec.describe License do
         end
       end
     end
-
-    describe '#reset_previous', :request_store do
-      let!(:previous_license) do
-        create(
-          :license,
-          data: create(:gitlab_license, starts_at: Date.new(1969, 1, 1), expires_at: Date.new(1969, 12, 31)).export)
-      end
-
-      before do
-        described_class.previous
-
-        expect(Gitlab::SafeRequestStore.read(:previous_license)).to be_present
-      end
-
-      context 'when a license is created' do
-        it 'deletes the previous_license value in Gitlab::SafeRequestStore' do
-          create(:license)
-
-          expect(Gitlab::SafeRequestStore.read(:previous_license)).to be_nil
-        end
-      end
-
-      context 'when a license is destroyed' do
-        it 'deletes the previous_license value in Gitlab::SafeRequestStore' do
-          previous_license.destroy
-
-          expect(Gitlab::SafeRequestStore.read(:previous_license)).to be_nil
-        end
-      end
-    end
   end
 
   describe 'Scopes' do
@@ -689,62 +650,6 @@ RSpec.describe License do
       end
     end
 
-    describe '.previous' do
-      before do
-        described_class.reset_previous
-      end
-
-      context 'when there is no license' do
-        it 'returns nil' do
-          allow(described_class).to receive(:last_hundred).and_return([])
-
-          expect(described_class.previous).to be_nil
-        end
-      end
-
-      context 'when the license is invalid' do
-        it 'returns nil' do
-          license = build(
-            :license,
-            data: build(:gitlab_license, starts_at: Date.new(1969, 1, 1), expires_at: Date.new(1969, 12, 31)).export
-          )
-
-          allow(described_class).to receive(:last_hundred).and_return([license])
-          allow(license).to receive(:valid?).and_return(false)
-
-          expect(described_class.previous).to be_nil
-        end
-      end
-
-      context 'when the license is valid' do
-        context 'when only a current and a future dated license exist' do
-          before do
-            create(:license, data: create(:gitlab_license, starts_at: Date.current + 1.month).export)
-          end
-
-          it 'returns nil' do
-            expect(described_class.previous).to be_nil
-          end
-        end
-
-        context 'when license is not a future dated or the current one' do
-          it 'returns the the previous license' do
-            previous_license = create(
-              :license,
-              data: create(:gitlab_license, starts_at: Date.new(2000, 1, 1), expires_at: Date.new(2000, 12, 31)).export
-            )
-            # create another license since the last uploaded license is considered the current one
-            create(
-              :license,
-              data: create(:gitlab_license, starts_at: Date.new(2001, 1, 1), expires_at: Date.new(2001, 12, 31)).export
-            )
-
-            expect(described_class.previous).to eq(previous_license)
-          end
-        end
-      end
-    end
-
     describe ".block_changes?" do
       before do
         allow(License).to receive(:current).and_return(license)

ee/spec/support/shared_examples/models/license_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'valid daily billable users count compared to limit set by license checks' do
+  context 'when daily billable users count is less than the restricted user count' do
+    let(:billable_users_count) { active_user_count - 5 }
+
+    it { is_expected.to be_valid }
+  end
+
+  context 'when daily billable users count is equal to the restricted user count' do
+    let(:billable_users_count) { active_user_count }
+
+    it { is_expected.to be_valid }
+  end
+
+  context 'when daily billable users count is equal to the restricted user count with threshold' do
+    let(:active_user_count) { 10 }
+    let(:billable_users_count) { 11 }
+
+    it { is_expected.to be_valid }
+  end
+end
+
+RSpec.shared_examples 'invalid daily billable users count compared to limit set by license checks' do
+  context 'when daily billable users count is greater than the restricted user count' do
+    let(:billable_users_count) { active_user_count + 5 }
+
+    it { is_expected.not_to be_valid }
+
+    it 'includes the correct error message' do
+      license.valid?
+
+      overage = billable_users_count - active_user_count
+      error_message = "This GitLab installation currently has #{billable_users_count} active users, " \
+        "exceeding this license's limit of #{active_user_count} by #{overage} users. " \
+        "Please upload a license for at least #{billable_users_count} users"
+
+      expect(license.errors.full_messages.to_sentence).to include(error_message)
+    end
+  end
+end
+
+RSpec.shared_examples 'valid prior historical max compared to limit set by license checks' do
+  context 'when prior historical max is less than the restricted user count' do
+    let(:billable_users_count) { active_user_count }
+    let(:prior_active_user_count) { active_user_count - 1 }
+
+    it { is_expected.to be_valid }
+  end
+
+  context 'when prior historical max is equal to the restricted user count' do
+    let(:billable_users_count) { active_user_count }
+    let(:prior_active_user_count) { active_user_count }
+
+    it { is_expected.to be_valid }
+  end
+
+  context 'when prior historical max is equal to the restricted user count with threshold' do
+    let(:active_user_count) { 10 }
+    let(:billable_users_count) { active_user_count }
+    let(:prior_active_user_count) { 11 }
+
+    it { is_expected.to be_valid }
+  end
+end
+
+RSpec.shared_examples 'invalid prior historical max compared to limit set by license checks' do
+  context 'when prior historical max is greater than the restricted user count' do
+    let(:billable_users_count) { active_user_count }
+    let(:prior_active_user_count) { active_user_count + 1 }
+
+    it { is_expected.not_to be_valid }
+
+    it 'includes the correct error message' do
+      license.valid?
+
+      overage = prior_active_user_count - active_user_count
+      error_message = "During the year before this license started, " \
+        "this GitLab installation had #{prior_active_user_count} active users, " \
+        "exceeding this license's limit of #{active_user_count} by #{overage} user. " \
+        "Please upload a license for at least #{prior_active_user_count} users"
+
+      expect(license.errors.full_messages.to_sentence).to include(error_message)
+    end
+  end
+end
+
+RSpec.shared_examples 'with previous user count checks' do
+  context 'when prior historical max is less than previous user count' do
+    let(:prior_active_user_count) { previous_user_count - 5 }
+
+    include_examples 'valid daily billable users count compared to limit set by license checks'
+    include_examples 'invalid daily billable users count compared to limit set by license checks'
+  end
+
+  context 'when prior historical max is equal to previous user count' do
+    let(:prior_active_user_count) { previous_user_count }
+
+    include_examples 'valid daily billable users count compared to limit set by license checks'
+    include_examples 'invalid daily billable users count compared to limit set by license checks'
+  end
+
+  context 'when prior historical max is greater than previous user count' do
+    include_examples 'valid prior historical max compared to limit set by license checks'
+    include_examples 'invalid prior historical max compared to limit set by license checks'
+  end
+end