Merge branch '12241-current-active-users-historic-max' into 'master'

Resolve "Current active users > historic max"

Closes #12241

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

ee/app/models/historical_data.rb

@@ -21,10 +21,13 @@ class HistoricalData < ApplicationRecord
       find_by(date: date)
     end
 
-    def max_historical_user_count
-      exp_date = License.current&.expires_at || Date.today
+    def max_historical_user_count(license: nil, from: nil, to: nil)
+      license ||= License.current
+      expires_at = license&.expires_at || Date.today
+      from ||= expires_at - 1.year
+      to   ||= expires_at
 
-      HistoricalData.during(exp_date.ago(1.year)..exp_date).maximum(:active_user_count) || 0
+      HistoricalData.during(from..to).maximum(:active_user_count) || 0
     end
   end
 end

ee/app/models/license.rb

@@ -406,10 +406,7 @@ class License < ApplicationRecord
   end
 
   def historical_max(from = nil, to = nil)
-    from ||= starts_at - 1.year
-    to   ||= starts_at
-
-    HistoricalData.during(from..to).maximum(:active_user_count) || 0
+    HistoricalData.max_historical_user_count(license: self, from: from, to: to)
   end
 
   def historical_max_with_default_period
@@ -439,22 +436,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
 
-  def empty_historical_max?
-    historical_max == 0
+  def prior_historical_max
+    @prior_historical_max ||= begin
+      from = starts_at - 1.year
+      to   = starts_at
+
+      historical_max(from, to)
+    end
   end
 
   def check_users_limit
     return unless restricted_user_count
 
-    if previous_user_count && (historical_max <= previous_user_count)
+    if previous_user_count && (prior_historical_max <= previous_user_count)
       return if restricted_user_count >= current_active_users_count
     else
-      return if restricted_user_count >= historical_max
+      return if restricted_user_count >= prior_historical_max
     end
 
-    user_count = empty_historical_max? ? current_active_users_count : historical_max
+    user_count = prior_historical_max.zero? ? current_active_users_count : prior_historical_max
 
-    add_limit_error(current_period: empty_historical_max?, user_count: user_count)
+    add_limit_error(current_period: prior_historical_max.zero?, user_count: user_count)
   end
 
   def check_trueup

ee/changelogs/unreleased/12241-current-active-users-historic-max.yml

+---
+title: Show correct historic max user count for a license
+merge_request: 15107
+author:
+type: fixed

ee/spec/models/historical_data_spec.rb

@@ -39,75 +39,98 @@ describe HistoricalData do
     end
   end
 
-  describe ".max_historical_user_count" do
-    before do
-      (1..3).each do |i|
-        described_class.create!(date: Time.now - i.days, active_user_count: i * 100)
+  describe '.max_historical_user_count' do
+    context 'with multiple historical data points for the current license' do
+      before do
+        (1..3).each do |i|
+          described_class.create!(date: Time.now - i.days, active_user_count: i * 100)
+        end
       end
-    end
 
-    it "returns max user count for the past year" do
-      expect(described_class.max_historical_user_count).to eq(300)
+      it 'returns max user count for the past year' do
+        expect(described_class.max_historical_user_count).to eq(300)
+      end
     end
-  end
-
-  describe '.max_historical_user_count with different plans' do
-    using RSpec::Parameterized::TableSyntax
 
-    before do
-      create(:group_member, :guest)
-      create(:group_member, :reporter)
-      create(:license, plan: plan)
-
-      described_class.track!
-    end
+    context 'using parameters' do
+      let!(:license) do
+        create(
+          :license,
+          starts_at: Date.new(2014, 1, 1),
+          expires_at: Date.new(2014, 12, 1)
+        )
+      end
 
-    where(:gl_plan, :expected_count) do
-      ::License::STARTER_PLAN  | 2
-      ::License::PREMIUM_PLAN  | 2
-      ::License::ULTIMATE_PLAN | 1
-    end
+      it 'returns max user count for the given license' do
+        expect(described_class.max_historical_user_count(license: license)).to eq(1200)
+      end
 
-    with_them do
-      let(:plan) { gl_plan }
+      it 'returns max user count for the time range' do
+        from = Date.new(2014, 6, 1)
+        to = Date.new(2014, 9, 1)
 
-      it 'does not count guest users' do
-        expect(described_class.max_historical_user_count).to eq(expected_count)
+        expect(described_class.max_historical_user_count(from: from, to: to)).to eq(900)
       end
     end
-  end
 
-  describe '.max_historical_user_count with data outside of the license period' do
-    let!(:license) { create(:license) }
+    context 'with different plans' do
+      using RSpec::Parameterized::TableSyntax
 
-    context 'with stats before the license period' do
       before do
-        described_class.create!(date: license.starts_at.ago(2.days), active_user_count: 10)
-      end
+        create(:group_member, :guest)
+        create(:group_member, :reporter)
+        create(:license, plan: plan)
 
-      it 'ignore those records' do
-        expect(described_class.max_historical_user_count).to eq(0)
+        described_class.track!
       end
-    end
 
-    context 'with stats after the license period' do
-      before do
-        described_class.create!(date: license.expires_at.in(2.days), active_user_count: 10)
+      where(:gl_plan, :expected_count) do
+        ::License::STARTER_PLAN  | 2
+        ::License::PREMIUM_PLAN  | 2
+        ::License::ULTIMATE_PLAN | 1
       end
 
-      it 'ignore those records' do
-        expect(described_class.max_historical_user_count).to eq(0)
+      with_them do
+        let(:plan) { gl_plan }
+
+        it 'does not count guest users' do
+          expect(described_class.max_historical_user_count).to eq(expected_count)
+        end
       end
     end
 
-    context 'with stats inside license period' do
-      before do
-        described_class.create!(date: license.starts_at.in(2.days), active_user_count: 10)
-        described_class.create!(date: license.starts_at.in(5.days), active_user_count: 15)
+    context 'with data outside of the license period' do
+      let!(:license) { create(:license) }
+
+      context 'with stats before the license period' do
+        before do
+          described_class.create!(date: license.starts_at.ago(2.days), active_user_count: 10)
+        end
+
+        it 'ignore those records' do
+          expect(described_class.max_historical_user_count).to eq(0)
+        end
+      end
+
+      context 'with stats after the license period' do
+        before do
+          described_class.create!(date: license.expires_at.in(2.days), active_user_count: 10)
+        end
+
+        it 'ignore those records' do
+          expect(described_class.max_historical_user_count).to eq(0)
+        end
       end
 
-      it 'returns max value for active_user_count' do
-        expect(described_class.max_historical_user_count).to eq(15)
+      context 'with stats inside license period' do
+        before do
+          described_class.create!(date: license.starts_at.in(2.days), active_user_count: 10)
+          described_class.create!(date: license.starts_at.in(5.days), active_user_count: 15)
+        end
+
+        it 'returns max value for active_user_count' do
+          expect(described_class.max_historical_user_count).to eq(15)
+        end
       end
     end
   end

ee/spec/models/license_spec.rb

@@ -83,7 +83,7 @@ describe License do
             active_user_count: User.active.count - 1
           }
 
-          allow(license).to receive(:historical_max).and_return(0)
+          HistoricalData.delete_all
         end
 
         context 'with previous_user_count and active users above of license limit' do