Merge branch '281177_use_previous_license_term_for_historical_max_data' into 'master'

Use previous license term for seat overage

See merge request gitlab-org/gitlab!53878

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

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

ee/app/models/historical_data.rb

@@ -18,9 +18,10 @@ class HistoricalData < ApplicationRecord
 
     def max_historical_user_count(license: nil, from: nil, to: nil)
       license ||= License.current
+      starts_at = license&.starts_at || Time.current - 1.year
       expires_at = license&.expires_at || Time.current
-      from ||= (expires_at - 1.year).beginning_of_day
-      to   ||= expires_at.end_of_day
+      from ||= starts_at.beginning_of_day
+      to ||= expires_at.end_of_day
 
       HistoricalData.during(from..to).maximum(:active_user_count) || 0
     end

ee/app/models/license.rb

@@ -242,6 +242,7 @@ class License < ApplicationRecord
   after_create :reset_current
   after_destroy :reset_current
   after_commit :reset_future_dated, on: [:create, :destroy]
+  after_commit :reset_previous, on: [:create, :destroy]
 
   scope :recent, -> { reorder(id: :desc) }
   scope :last_hundred, -> { recent.limit(100) }
@@ -304,6 +305,14 @@ class License < ApplicationRecord
       future_dated.present?
     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
@@ -335,6 +344,10 @@ 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
@@ -489,12 +502,12 @@ class License < ApplicationRecord
     overage(maximum_user_count)
   end
 
-  def historical_max(from = nil, to = nil)
+  def historical_max(from: nil, to: nil)
     HistoricalData.max_historical_user_count(license: self, from: from, to: to)
   end
 
   def maximum_user_count
-    [historical_max(starts_at), daily_billable_users_count].max
+    [historical_max(from: starts_at), daily_billable_users_count].max
   end
 
   def update_trial_setting
@@ -562,6 +575,10 @@ class License < ApplicationRecord
     self.class.reset_future_dated
   end
 
+  def reset_previous
+    self.class.reset_previous
+  end
+
   def reset_license
     @license = nil
   end
@@ -574,10 +591,7 @@ class License < ApplicationRecord
 
   def prior_historical_max
     @prior_historical_max ||= begin
-      from = (starts_at - 1.year).beginning_of_day
-      to   = starts_at.end_of_day
-
-      historical_max(from, to)
+      historical_max(from: previous_started_at, to: previous_expired_at)
     end
   end
 
@@ -604,9 +618,9 @@ class License < ApplicationRecord
 
   def check_trueup
     trueup_qty          = restrictions[:trueup_quantity]
-    trueup_from         = Date.parse(restrictions[:trueup_from]).beginning_of_day rescue (starts_at - 1.year).beginning_of_day
-    trueup_to           = Date.parse(restrictions[:trueup_to]).end_of_day rescue starts_at.end_of_day
-    max_historical      = historical_max(trueup_from, trueup_to)
+    trueup_from         = Date.parse(restrictions[:trueup_from]).beginning_of_day rescue previous_started_at
+    trueup_to           = Date.parse(restrictions[:trueup_to]).end_of_day rescue previous_expired_at
+    max_historical      = historical_max(from: trueup_from, to: trueup_to)
     expected_trueup_qty = if previous_user_count
                             max_historical - previous_user_count
                           else
@@ -644,4 +658,12 @@ 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
 end

ee/changelogs/unreleased/281177_use_previous_license_term_for_historical_max_data.yml

+---
+title: Use previous license term for seat overage
+merge_request: 53878
+author:
+type: changed

ee/spec/models/historical_data_spec.rb

@@ -40,16 +40,35 @@ RSpec.describe HistoricalData do
   end
 
   describe '.max_historical_user_count' do
+    let(:current_license) { create(:license, starts_at: Date.current - 1.month, expires_at: Date.current + 1.month) }
+
+    before do
+      # stub current license to cover a shorter period (one month ago until a date in the future) than the one
+      # set for the whole test suite (1970-01-01 to a date in the future)
+      allow(License).to receive(:load_license).and_return(current_license)
+      allow(License).to receive(:current).and_return(current_license)
+    end
+
     context 'with multiple historical data points for the current license' do
       before do
         (1..3).each do |i|
           described_class.create!(recorded_at: Time.current - i.days, active_user_count: i * 100)
         end
+
+        described_class.create!(recorded_at: Time.current - 1.year, active_user_count: 400)
       end
 
-      it 'returns max user count for the past year' do
+      it 'returns max user count for the duration of the current license' do
         expect(described_class.max_historical_user_count).to eq(300)
       end
+
+      context 'when there is no current license' do
+        let(:current_license) { nil }
+
+        it 'returns max user count for the past year as a fallback' do
+          expect(described_class.max_historical_user_count).to eq(400)
+        end
+      end
     end
 
     context 'using parameters' do
@@ -79,7 +98,6 @@ RSpec.describe HistoricalData do
       before do
         create(:group_member, :guest)
         create(:group_member, :reporter)
-        create(:license, plan: plan)
 
         described_class.track!
       end
@@ -92,6 +110,9 @@ RSpec.describe HistoricalData do
 
       with_them do
         let(:plan) { gl_plan }
+        let(:current_license) do
+          create(:license, plan: plan, starts_at: Date.current - 1.month, expires_at: Date.current + 1.month)
+        end
 
         it 'does not count guest users' do
           expect(described_class.max_historical_user_count).to eq(expected_count)
@@ -100,11 +121,9 @@ RSpec.describe HistoricalData do
     end
 
     context 'with data outside of the license period' do
-      let!(:license) { create(:license, starts_at: Date.current - 1.month) }
-
       context 'with stats before the license period' do
         before do
-          described_class.create!(recorded_at: license.starts_at.ago(2.days), active_user_count: 10)
+          described_class.create!(recorded_at: current_license.starts_at.ago(2.days), active_user_count: 10)
         end
 
         it 'ignore those records' do
@@ -114,7 +133,7 @@ RSpec.describe HistoricalData do
 
       context 'with stats after the license period' do
         before do
-          described_class.create!(recorded_at: license.expires_at.in(2.days), active_user_count: 10)
+          described_class.create!(recorded_at: current_license.expires_at.in(2.days), active_user_count: 10)
         end
 
         it 'ignore those records' do
@@ -124,8 +143,8 @@ RSpec.describe HistoricalData do
 
       context 'with stats inside license period' do
         before do
-          described_class.create!(recorded_at: license.starts_at.in(2.days), active_user_count: 10)
-          described_class.create!(recorded_at: license.starts_at.in(5.days), active_user_count: 15)
+          described_class.create!(recorded_at: current_license.starts_at.in(2.days), active_user_count: 10)
+          described_class.create!(recorded_at: current_license.starts_at.in(5.days), active_user_count: 15)
         end
 
         it 'returns max value for active_user_count' do

ee/spec/models/license_spec.rb

@@ -71,7 +71,7 @@ RSpec.describe License do
         let(:new_license) do
           gl_license = build(
             :gitlab_license,
-            starts_at: Date.today,
+            starts_at: Date.current,
             restrictions: { active_user_count: 10, previous_user_count: previous_user_count }
           )
 
@@ -353,6 +353,36 @@ 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 "Class methods" do
@@ -559,6 +589,62 @@ 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)