Merge branch '281177_remove_license_params_in_max_historical_user_count' into 'master'

Remove fallback logic

See merge request gitlab-org/gitlab!54221

ee/app/controllers/admin/licenses/usage_exports_controller.rb

@@ -10,11 +10,9 @@ module Admin
       feature_category :utilization
 
       def show
-        historical_data = HistoricalData.in_license_term(license)
-
         respond_to do |format|
           format.csv do
-            csv_data = HistoricalUserData::CsvService.new(historical_data).generate
+            csv_data = HistoricalUserData::CsvService.new(license.historical_data).generate
 
             send_data(csv_data, type: 'text/csv; charset=utf-8', filename: 'license_usage.csv')
           end

ee/app/models/gitlab/seat_link_data.rb

@@ -40,21 +40,15 @@ module Gitlab
       ::License.current.data
     end
 
-    def license_starts_at
-      ::License.current.starts_at.beginning_of_day
-    end
-
     def default_max_count
-      HistoricalData.max_historical_user_count(
-        from: license_starts_at,
-        to: timestamp
-      )
+      ::License.current.historical_max(to: timestamp)
     end
 
     def historical_data
       strong_memoize(:historical_data) do
         to_timestamp = timestamp || Time.current
-        HistoricalData.during(license_starts_at..to_timestamp).order(:recorded_at).last
+
+        ::License.current.historical_data(to: to_timestamp).order(:recorded_at).last
       end
     end
 

ee/app/models/historical_data.rb

@@ -16,21 +16,8 @@ class HistoricalData < ApplicationRecord
       )
     end
 
-    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 ||= starts_at.beginning_of_day
-      to ||= expires_at.end_of_day
-
+    def max_historical_user_count(from:, to:)
       HistoricalData.during(from..to).maximum(:active_user_count) || 0
     end
-
-    def in_license_term(license)
-      start_date = license.starts_at.beginning_of_day
-      expiration_date = license.expires_at&.end_of_day || Time.current
-
-      HistoricalData.during(start_date..expiration_date)
-    end
   end
 end

ee/app/models/license.rb

@@ -502,8 +502,18 @@ class License < ApplicationRecord
     overage(maximum_user_count)
   end
 
+  def historical_data(from: nil, to: nil)
+    from ||= starts_at_for_historical_data
+    to ||= expires_at_for_historical_data
+
+    HistoricalData.during(from..to)
+  end
+
   def historical_max(from: nil, to: nil)
-    HistoricalData.max_historical_user_count(license: self, from: from, to: to)
+    from ||= starts_at_for_historical_data
+    to ||= expires_at_for_historical_data
+
+    HistoricalData.max_historical_user_count(from: from, to: to)
   end
 
   def maximum_user_count
@@ -666,4 +676,12 @@ class License < ApplicationRecord
   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
+
+  def expires_at_for_historical_data
+    (expires_at || Time.current).end_of_day
+  end
 end

ee/lib/ee/gitlab/usage_data.rb

@@ -93,7 +93,7 @@ module EE
             usage_data[:license_md5] = license.md5
             usage_data[:license_id] = license.license_id
             # rubocop: disable UsageData/LargeTable
-            usage_data[:historical_max_users] = ::HistoricalData.max_historical_user_count
+            usage_data[:historical_max_users] = license.historical_max
             # rubocop: enable UsageData/LargeTable
             usage_data[:licensee] = license.licensee
             usage_data[:license_user_count] = license.restricted_user_count

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -372,8 +372,8 @@ RSpec.describe Admin::ApplicationSettingsController do
       end
 
       before_all do
-        HistoricalData.create!(recorded_at: yesterday - 1.day, active_user_count: max_count)
-        HistoricalData.create!(recorded_at: yesterday, active_user_count: current_count)
+        create(:historical_data, recorded_at: yesterday - 1.day, active_user_count: max_count)
+        create(:historical_data, recorded_at: yesterday, active_user_count: current_count)
       end
 
       before do

ee/spec/controllers/admin/licenses/usage_exports_controller_spec.rb

@@ -44,7 +44,9 @@ RSpec.describe Admin::Licenses::UsageExportsController do
       let(:historical_data_relation) { :historical_data_relation }
 
       before do
-        allow(HistoricalData).to receive(:in_license_term).with(License.current).and_return(historical_data_relation)
+        license = build(:license)
+        allow(License).to receive(:current).and_return(license)
+        allow(license).to receive(:historical_data).and_return(historical_data_relation)
         allow(HistoricalUserData::CsvService).to receive(:new).with(historical_data_relation).and_return(csv_service)
       end
 

ee/spec/lib/ee/gitlab/usage_data_spec.rb

@@ -183,7 +183,7 @@ RSpec.describe Gitlab::UsageData do
 
       expect(subject[:license_md5]).to eq(Digest::MD5.hexdigest(license.data))
       expect(subject[:license_id]).to eq(license.license_id)
-      expect(subject[:historical_max_users]).to eq(::HistoricalData.max_historical_user_count)
+      expect(subject[:historical_max_users]).to eq(license.historical_max)
       expect(subject[:licensee]).to eq(license.licensee)
       expect(subject[:license_user_count]).to eq(license.restricted_user_count)
       expect(subject[:license_starts_at]).to eq(license.starts_at)

ee/spec/models/gitlab/seat_link_data_spec.rb

@@ -27,10 +27,10 @@ RSpec.describe Gitlab::SeatLinkData do
     let_it_be(:today_active_count) { 20 }
 
     before_all do
-      HistoricalData.create!(recorded_at: license_start_date, active_user_count: 10)
-      HistoricalData.create!(recorded_at: license_start_date + 1.day, active_user_count: max_before_today)
-      HistoricalData.create!(recorded_at: utc_time - 1.day, active_user_count: yesterday_active_count)
-      HistoricalData.create!(recorded_at: utc_time, active_user_count: today_active_count)
+      create(:historical_data, recorded_at: license_start_date, active_user_count: 10)
+      create(:historical_data, recorded_at: license_start_date + 1.day, active_user_count: max_before_today)
+      create(:historical_data, recorded_at: utc_time - 1.day, active_user_count: yesterday_active_count)
+      create(:historical_data, recorded_at: utc_time, active_user_count: today_active_count)
     end
 
     around do |example|

ee/spec/models/historical_data_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe HistoricalData do
   before do
     (1..12).each do |i|
-      described_class.create!(recorded_at: Date.new(2014, i, 1), active_user_count: i * 100)
+      create(:historical_data, recorded_at: Date.new(2014, i, 1), active_user_count: i * 100)
     end
   end
 
@@ -40,156 +40,41 @@ 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) }
+    subject(:max_historical_user_count) { described_class.max_historical_user_count(from: from, to: to) }
 
-    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 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
+    let(:from) { (Date.current - 1.month).beginning_of_day }
+    let(:to) { (Date.current + 1.month).end_of_day }
 
-    context 'using parameters' do
-      let!(:license) do
-        create(
-          :license,
-          starts_at: Date.new(2014, 1, 1),
-          expires_at: Date.new(2014, 12, 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
-
-      it 'returns max user count for the time range' do
-        from = Date.new(2014, 6, 1)
-        to = Date.new(2014, 9, 1)
-
-        expect(described_class.max_historical_user_count(from: from, to: to)).to eq(900)
-      end
-    end
-
-    context 'with different plans' do
-      using RSpec::Parameterized::TableSyntax
-
-      before do
-        create(:group_member, :guest)
-        create(:group_member, :reporter)
-
-        described_class.track!
-      end
-
-      where(:gl_plan, :expected_count) do
-        ::License::STARTER_PLAN  | 2
-        ::License::PREMIUM_PLAN  | 2
-        ::License::ULTIMATE_PLAN | 1
-      end
-
-      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)
-        end
-      end
-    end
-
-    context 'with data outside of the license period' do
-      context 'with stats before the license period' do
-        before do
-          described_class.create!(recorded_at: current_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
+    context 'with data outside of the given period' do
+      context 'with stats before the given period' do
         before do
-          described_class.create!(recorded_at: current_license.expires_at.in(2.days), active_user_count: 10)
+          create(:historical_data, recorded_at: from - 2.days, active_user_count: 10)
         end
 
-        it 'ignore those records' do
-          expect(described_class.max_historical_user_count).to eq(0)
+        it 'ignores those records' do
+          expect(max_historical_user_count).to eq(0)
         end
       end
 
-      context 'with stats inside license period' do
+      context 'with stats after the given period' do
         before do
-          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)
+          create(:historical_data, recorded_at: to + 2.days, active_user_count: 10)
         end
 
-        it 'returns max value for active_user_count' do
-          expect(described_class.max_historical_user_count).to eq(15)
+        it 'ignores those records' do
+          expect(max_historical_user_count).to eq(0)
         end
       end
     end
-  end
-
-  describe '.in_license_term' do
-    let_it_be(:now) { DateTime.new(2014, 12, 15) }
-    let_it_be(:license) do
-      create_current_license(
-        starts_at: Date.new(2014, 7, 1),
-        expires_at: Date.new(2014, 12, 31)
-      )
-    end
-
-    before_all do
-      described_class.create!(recorded_at: license.starts_at - 1.day, active_user_count: 1)
-      described_class.create!(recorded_at: license.expires_at + 1.day, active_user_count: 2)
-      described_class.create!(recorded_at: now - 1.year - 1.day, active_user_count: 3)
-      described_class.create!(recorded_at: now + 1.day, active_user_count: 4)
-    end
 
-    around do |example|
-      travel_to(now) { example.run }
-    end
-
-    context 'with a license that has a start and end date' do
-      it 'returns correct number of records within the license range' do
-        expect(described_class.in_license_term(license).count).to eq(7)
-      end
-    end
-
-    context 'with a license that has no end date' do
-      let_it_be(:license) do
-        create_current_license(
-          starts_at: Date.new(2014, 7, 1),
-          expires_at: nil
-        )
+    context 'with data inside of the given period' do
+      before do
+        create(:historical_data, recorded_at: from + 2.days, active_user_count: 10)
+        create(:historical_data, recorded_at: from + 5.days, active_user_count: 15)
       end
 
-      it 'returns correct number of records within the past year' do
-        expect(described_class.in_license_term(license).count).to eq(6)
+      it 'returns max value for active_user_count' do
+        expect(max_historical_user_count).to eq(15)
       end
     end
   end

ee/spec/models/license_spec.rb

@@ -122,7 +122,7 @@ RSpec.describe License do
     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)  { HistoricalData.create!(recorded_at: date, active_user_count: active_user_count) }
+      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
@@ -303,7 +303,7 @@ RSpec.describe License do
     describe 'downgrade' do
       context 'when more users were added in previous period' do
         before do
-          HistoricalData.create!(recorded_at: described_class.current.starts_at - 6.months, active_user_count: 15)
+          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
@@ -315,7 +315,7 @@ RSpec.describe License do
 
       context 'when no users were added in the previous period' do
         before do
-          HistoricalData.create!(recorded_at: 6.months.ago, active_user_count: 15)
+          create(:historical_data, recorded_at: 6.months.ago, active_user_count: 15)
 
           set_restrictions(restricted_user_count: 10, previous_user_count: 15)
         end
@@ -964,6 +964,150 @@ RSpec.describe License do
     end
   end
 
+  describe '#historical_data' do
+    subject(:historical_data_count) { license.historical_data.count }
+
+    let_it_be(:now) { DateTime.new(2014, 12, 15) }
+    let_it_be(:license) { create(:license, starts_at: Date.new(2014, 7, 1), expires_at: Date.new(2014, 12, 31)) }
+
+    before_all do
+      (1..12).each do |i|
+        create(:historical_data, recorded_at: Date.new(2014, i, 1), active_user_count: i * 100)
+      end
+
+      create(:historical_data, recorded_at: license.starts_at - 1.day, active_user_count: 1)
+      create(:historical_data, recorded_at: license.expires_at + 1.day, active_user_count: 2)
+      create(:historical_data, recorded_at: now - 1.year - 1.day, active_user_count: 3)
+      create(:historical_data, recorded_at: now + 1.day, active_user_count: 4)
+    end
+
+    around do |example|
+      travel_to(now) { example.run }
+    end
+
+    context 'with using parameters' do
+      it 'returns correct number of records within the given range' do
+        from = Date.new(2014, 8, 1)
+        to = Date.new(2014, 11, 30)
+
+        expect(license.historical_data(from: from, to: to).count).to eq(4)
+      end
+    end
+
+    context 'with a license that has a start and end date' do
+      it 'returns correct number of records within the license range' do
+        expect(historical_data_count).to eq(7)
+      end
+    end
+
+    context 'with a license that has no start date' do
+      let_it_be(:license) { create(:license, starts_at: nil, expires_at: Date.new(2014, 12, 31)) }
+
+      it 'returns correct number of records starting a year ago to license\s expiration date' do
+        expect(historical_data_count).to eq(14)
+      end
+    end
+
+    context 'with a license that has no end date' do
+      let_it_be(:license) { create(:license, starts_at: Date.new(2014, 7, 1), expires_at: nil) }
+
+      it 'returns correct number of records from the license\'s start date to today' do
+        expect(historical_data_count).to eq(6)
+      end
+    end
+  end
+
+  describe '#historical_max' do
+    subject(:historical_max) { license.historical_max }
+
+    let(:license) { create(:license, starts_at: Date.current - 1.month, expires_at: Date.current + 1.month) }
+
+    context 'when using parameters' do
+      before do
+        (1..12).each do |i|
+          create(:historical_data, recorded_at: Date.new(2014, i, 1), active_user_count: i * 100)
+        end
+      end
+
+      it 'returns max user count for the given time range' do
+        from = Date.new(2014, 6, 1)
+        to = Date.new(2014, 9, 1)
+
+        expect(license.historical_max(from: from, to: to)).to eq(900)
+      end
+    end
+
+    context 'with different plans for the license' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:gl_plan, :expected_count) do
+        ::License::STARTER_PLAN  | 2
+        ::License::PREMIUM_PLAN  | 2
+        ::License::ULTIMATE_PLAN | 1
+      end
+
+      with_them do
+        let(:plan) { gl_plan }
+        let(:license) do
+          create(:license, plan: plan, starts_at: Date.current - 1.month, expires_at: Date.current + 1.month)
+        end
+
+        before do
+          license
+
+          create(:group_member, :guest)
+          create(:group_member, :reporter)
+
+          HistoricalData.track!
+        end
+
+        it 'does not count guest users' do
+          expect(historical_max).to eq(expected_count)
+        end
+      end
+    end
+
+    context 'with data inside and outside of the license period' do
+      before do
+        create(:historical_data, recorded_at: license.starts_at.ago(2.days), active_user_count: 20)
+        create(:historical_data, recorded_at: license.starts_at.in(2.days), active_user_count: 10)
+        create(:historical_data, recorded_at: license.starts_at.in(5.days), active_user_count: 15)
+        create(:historical_data, recorded_at: license.expires_at.in(2.days), active_user_count: 25)
+      end
+
+      it 'returns max value for active_user_count for within the license period only' do
+        expect(historical_max).to eq(15)
+      end
+    end
+
+    context 'when license has no start date' do
+      let(:license) { create(:license, starts_at: nil, expires_at: Date.current + 1.month) }
+
+      before do
+        create(:historical_data, recorded_at: Date.yesterday.ago(1.year), active_user_count: 15)
+        create(:historical_data, recorded_at: Date.current.ago(1.year), active_user_count: 12)
+        create(:historical_data, recorded_at: license.expires_at.ago(2.days), active_user_count: 10)
+      end
+
+      it 'returns max value for active_user_count from up to a year ago' do
+        expect(historical_max).to eq(12)
+      end
+    end
+
+    context 'when license has no expiration date' do
+      let(:license) { create(:license, starts_at: Date.current.ago(1.month), expires_at: nil) }
+
+      before do
+        create(:historical_data, recorded_at: license.starts_at.in(2.days), active_user_count: 10)
+        create(:historical_data, recorded_at: Date.tomorrow, active_user_count: 15)
+      end
+
+      it 'returns max value for active_user_count until today' do
+        expect(historical_max).to eq(10)
+      end
+    end
+  end
+
   describe '#maximum_user_count' do
     let(:now) { Date.current }
 

ee/spec/workers/sync_seat_link_worker_spec.rb

@@ -11,13 +11,13 @@ RSpec.describe SyncSeatLinkWorker, type: :worker do
         # Setting the date as 12th March 2020 12:00 UTC for tests and creating new license
         create_current_license(starts_at: '2020-02-12'.to_date)
 
-        HistoricalData.create!(recorded_at: '2020-02-11T00:00:00Z', active_user_count: 100)
-        HistoricalData.create!(recorded_at: '2020-02-12T00:00:00Z', active_user_count: 10)
-        HistoricalData.create!(recorded_at: '2020-02-13T00:00:00Z', active_user_count: 15)
+        create(:historical_data, recorded_at: '2020-02-11T00:00:00Z', active_user_count: 100)
+        create(:historical_data, recorded_at: '2020-02-12T00:00:00Z', active_user_count: 10)
+        create(:historical_data, recorded_at: '2020-02-13T00:00:00Z', active_user_count: 15)
 
-        HistoricalData.create!(recorded_at: '2020-03-11T00:00:00Z', active_user_count: 10)
-        HistoricalData.create!(recorded_at: '2020-03-12T00:00:00Z', active_user_count: 12)
-        HistoricalData.create!(recorded_at: '2020-03-15T00:00:00Z', active_user_count: 25)
+        create(:historical_data, recorded_at: '2020-03-11T00:00:00Z', active_user_count: 10)
+        create(:historical_data, recorded_at: '2020-03-12T00:00:00Z', active_user_count: 12)
+        create(:historical_data, recorded_at: '2020-03-15T00:00:00Z', active_user_count: 25)
         allow(SyncSeatLinkRequestWorker).to receive(:perform_async).and_return(true)
       end