Merge branch '271343-send-latest-historical-data' into 'master'

Send latest historical data as seat link data

See merge request gitlab-org/gitlab!47614

ee/app/models/gitlab/seat_link_data.rb

@@ -2,7 +2,9 @@
 
 module Gitlab
   class SeatLinkData
-    attr_reader :date, :key, :max_users, :active_users
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :timestamp, :key, :max_users, :active_users
 
     delegate :to_json, to: :data
 
@@ -10,41 +12,54 @@ module Gitlab
     # are preferable, like for SyncSeatLinkWorker, to determine seat link data, and in others,
     # like for SyncSeatLinkRequestWorker, the params are passed because the values from when
     # the job was enqueued are necessary.
-    def initialize(date: default_date, key: default_key, max_users: nil, active_users: nil)
-      @date = date
+    def initialize(timestamp: nil, key: default_key, max_users: nil, active_users: nil)
+      @timestamp = timestamp || historical_data&.recorded_at
       @key = key
-      @max_users = max_users || default_max_count(@date)
-      @active_users = active_users || default_active_count(@date)
+      @max_users = max_users || default_max_count
+      @active_users = active_users || default_active_count
+    end
+
+    # Returns true if historical data exists between license start and the given date.
+    def historical_data_exists?
+      historical_data.present?
     end
 
     private
 
     def data
       {
-        date: date.to_s,
+        timestamp: timestamp&.iso8601,
+        date: timestamp&.to_date&.to_s,
         license_key: key,
         max_historical_user_count: max_users,
         active_users: active_users
       }
     end
 
-    def default_date
-      Time.current.utc.yesterday.to_date
-    end
-
     def default_key
       ::License.current.data
     end
 
-    def default_max_count(date)
+    def license_starts_at
+      ::License.current.starts_at.beginning_of_day
+    end
+
+    def default_max_count
       HistoricalData.max_historical_user_count(
-        from: ::License.current.starts_at.beginning_of_day,
-        to: date.end_of_day
+        from: license_starts_at,
+        to: timestamp
       )
     end
 
-    def default_active_count(date)
-      HistoricalData.at(date)&.active_user_count
+    def historical_data
+      strong_memoize(:historical_data) do
+        to_timestamp = timestamp || Time.current
+        HistoricalData.during(license_starts_at..to_timestamp).order(:recorded_at).last
+      end
+    end
+
+    def default_active_count
+      historical_data&.active_user_count
     end
   end
 end

ee/app/models/historical_data.rb

@@ -16,11 +16,6 @@ class HistoricalData < ApplicationRecord
       )
     end
 
-    # HistoricalData.at(Date.new(2014, 1, 1)).active_user_count
-    def at(date)
-      find_by(recorded_at: date.all_day)
-    end
-
     def max_historical_user_count(license: nil, from: nil, to: nil)
       license ||= License.current
       expires_at = license&.expires_at || Time.current

ee/app/workers/sync_seat_link_request_worker.rb

@@ -15,12 +15,12 @@ class SyncSeatLinkRequestWorker
 
   RequestError = Class.new(StandardError)
 
-  def perform(date, license_key, max_historical_user_count, active_users)
+  def perform(timestamp, license_key, max_historical_user_count, active_users)
     response = Gitlab::HTTP.post(
       URI_PATH,
       base_uri: EE::SUBSCRIPTIONS_URL,
       headers: request_headers,
-      body: request_body(date, license_key, max_historical_user_count, active_users)
+      body: request_body(timestamp, license_key, max_historical_user_count, active_users)
     )
 
     raise RequestError, request_error_message(response) unless response.success?
@@ -28,9 +28,9 @@ class SyncSeatLinkRequestWorker
 
   private
 
-  def request_body(date, license_key, max_historical_user_count, active_users)
+  def request_body(timestamp, license_key, max_historical_user_count, active_users)
     Gitlab::SeatLinkData.new(
-      date: date,
+      timestamp: Time.zone.parse(timestamp),
       key: license_key,
       max_users: max_historical_user_count,
       active_users: active_users

ee/app/workers/sync_seat_link_worker.rb

@@ -16,7 +16,7 @@ class SyncSeatLinkWorker # rubocop:disable Scalability/IdempotentWorker
     return unless should_sync_seats?
 
     SyncSeatLinkRequestWorker.perform_async(
-      seat_link_data.date.to_s,
+      seat_link_data.timestamp.iso8601,
       seat_link_data.key,
       seat_link_data.max_users,
       seat_link_data.active_users
@@ -36,6 +36,7 @@ class SyncSeatLinkWorker # rubocop:disable Scalability/IdempotentWorker
       License.current &&
       !License.current.trial? &&
       License.current.expires_at && # Skip sync if license has no expiration
-      seat_link_data.date.between?(License.current.starts_at, License.current.expires_at + 14.days)
+      seat_link_data.historical_data_exists? && # Skip sync if there is no historical data
+      seat_link_data.timestamp.between?(License.current.starts_at.beginning_of_day, License.current.expires_at.end_of_day + 14.days)
   end
 end

ee/changelogs/unreleased/271343-send-latest-historical-data.yml

+---
+title: Modify seat link API to send latest historical data instead of previous day's
+  data
+merge_request: 47614
+author:
+type: changed

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -312,6 +312,7 @@ RSpec.describe Admin::ApplicationSettingsController do
         expect(body).to start_with('<span id="LC1" class="line" lang="json">')
         expect(body).to include('<span class="nl">"license_key"</span>')
         expect(body).to include("<span class=\"s2\">\"#{yesterday.to_date}\"</span>")
+        expect(body).to include("<span class=\"s2\">\"#{yesterday.iso8601}\"</span>")
         expect(body).to include("<span class=\"mi\">#{max_count}</span>")
         expect(body).to include("<span class=\"mi\">#{current_count}</span>")
       end

ee/spec/features/admin/admin_settings_spec.rb

@@ -257,7 +257,7 @@ RSpec.describe 'Admin updates EE-only settings' do
 
     it 'loads seat link payload on click', :js do
       page.within('#js-seat-link-settings') do
-        expected_payload_content = /(?=.*"date")(?=.*"license_key")(?=.*"max_historical_user_count")(?=.*"active_users")/m
+        expected_payload_content = /(?=.*"date")(?=.*"timestamp")(?=.*"license_key")(?=.*"max_historical_user_count")(?=.*"active_users")/m
 
         expect(page).not_to have_content expected_payload_content
 

ee/spec/models/gitlab/seat_link_data_spec.rb

@@ -5,22 +5,21 @@ require 'spec_helper'
 RSpec.describe Gitlab::SeatLinkData do
   subject do
     described_class.new(
-      date: date,
+      timestamp: timestamp,
       key: key,
       max_users: max_users,
       active_users: active_users
     )
   end
 
-  let_it_be(:date) { '2020-03-22'.to_date }
+  let_it_be(:timestamp) { Time.iso8601('2020-03-22T06:09:18Z') }
   let_it_be(:key) { 'key' }
   let_it_be(:max_users) { 11 }
   let_it_be(:active_users) { 5 }
 
   describe '#initialize' do
     let_it_be(:utc_time) { Time.utc(2020, 3, 12, 12, 00) }
-    let_it_be(:utc_date) { utc_time.to_date }
-    let_it_be(:license_start_date) { utc_date - 1.month }
+    let_it_be(:license_start_date) { utc_time.to_date - 1.month }
     let_it_be(:current_license) { create_current_license(starts_at: license_start_date)}
 
     let_it_be(:max_before_today) { 15 }
@@ -30,8 +29,8 @@ RSpec.describe Gitlab::SeatLinkData do
     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_date - 1.day, active_user_count: yesterday_active_count)
-      HistoricalData.create!(recorded_at: utc_date, active_user_count: today_active_count)
+      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)
     end
 
     around do |example|
@@ -43,10 +42,10 @@ RSpec.describe Gitlab::SeatLinkData do
 
       it 'returns object with default attributes set' do
         expect(subject).to have_attributes(
-          date: eq(utc_date - 1.day),
+          timestamp: eq(utc_time),
           key: eq(current_license.data),
-          max_users: eq(max_before_today),
-          active_users: eq(yesterday_active_count)
+          max_users: eq(today_active_count),
+          active_users: eq(today_active_count)
         )
       end
     end
@@ -54,7 +53,7 @@ RSpec.describe Gitlab::SeatLinkData do
     context 'when passing params' do
       it 'returns object with given attributes set' do
         expect(subject).to have_attributes(
-          date: eq(date),
+          timestamp: eq(timestamp),
           key: eq(key),
           max_users: eq(max_users),
           active_users: eq(active_users)
@@ -62,14 +61,14 @@ RSpec.describe Gitlab::SeatLinkData do
       end
 
       context 'when passing date param only' do
-        subject { described_class.new(date: utc_date) }
+        subject { described_class.new(timestamp: utc_time - 1.day) }
 
         it 'returns object with attributes set using given date' do
           expect(subject).to have_attributes(
-            date: eq(utc_date),
+            timestamp: eq(utc_time - 1.day),
             key: eq(current_license.data),
-            max_users: eq(today_active_count),
-            active_users: eq(today_active_count)
+            max_users: eq(max_before_today),
+            active_users: eq(yesterday_active_count)
           )
         end
       end
@@ -82,7 +81,8 @@ RSpec.describe Gitlab::SeatLinkData do
     it 'returns payload data as a JSON string' do
       expect(subject.to_json).to eq(
         {
-          date: date.to_s,
+          timestamp: timestamp.iso8601,
+          date: timestamp.to_date.iso8601,
           license_key: key,
           max_historical_user_count: max_users,
           active_users: active_users
@@ -90,4 +90,25 @@ RSpec.describe Gitlab::SeatLinkData do
       )
     end
   end
+
+  describe '#historical_data_exists?' do
+    let_it_be(:license) { create_current_license(starts_at: Date.current - 7.days) }
+
+    it 'returns false if no historical data exists' do
+      expect(described_class.new.historical_data_exists?).to be(false)
+    end
+
+    it 'returns false if no historical data exists within [license start date, seat_link_data.date]' do
+      create(:historical_data, recorded_at: Time.current - 8.days)
+      create(:historical_data, recorded_at: Time.current)
+
+      expect(described_class.new(timestamp: Time.current - 1.day).historical_data_exists?).to be(false)
+    end
+
+    it 'returns true if historical data exists within [license start date, seat_link_data.date]' do
+      create(:historical_data, recorded_at: Time.current - 2.days)
+
+      expect(described_class.new(timestamp: Time.current - 1.day).historical_data_exists?).to be(true)
+    end
+  end
 end

ee/spec/models/historical_data_spec.rb

@@ -21,12 +21,6 @@ RSpec.describe HistoricalData do
     end
   end
 
-  describe ".at" do
-    it "returns the historical data at the specified date" do
-      expect(described_class.at(Date.new(2014, 8, 1)).active_user_count).to eq(800)
-    end
-  end
-
   describe ".track!" do
     before do
       allow(User).to receive(:billable).and_return([1, 2, 3, 4, 5])

ee/spec/workers/sync_seat_link_request_worker_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe SyncSeatLinkRequestWorker, type: :worker do
   describe '#perform' do
     subject do
-      described_class.new.perform('2020-01-01', '123', 5, 4)
+      described_class.new.perform('2020-01-01T01:20:12+02:00', '123', 5, 4)
     end
 
     let(:seat_link_url) { [EE::SUBSCRIPTIONS_URL, '/api/v1/seat_links'].join }
@@ -18,6 +18,29 @@ RSpec.describe SyncSeatLinkRequestWorker, type: :worker do
       expect(WebMock).to have_requested(:post, seat_link_url).with(
         headers: { 'Content-Type' => 'application/json' },
         body: {
+          timestamp: '2019-12-31T23:20:12Z',
+          date: '2019-12-31',
+          license_key: '123',
+          max_historical_user_count: 5,
+          active_users: 4
+        }.to_json
+      )
+    end
+
+    context 'with old date format string' do
+      subject do
+        described_class.new.perform('2020-01-01', '123', 5, 4)
+      end
+
+      it 'makes an HTTP POST request with passed params' do
+        stub_request(:post, seat_link_url).to_return(status: 200)
+
+        subject
+
+        expect(WebMock).to have_requested(:post, seat_link_url).with(
+          headers: { 'Content-Type' => 'application/json' },
+          body: {
+            timestamp: '2020-01-01T00:00:00Z',
             date: '2020-01-01',
             license_key: '123',
             max_historical_user_count: 5,
@@ -25,6 +48,7 @@ RSpec.describe SyncSeatLinkRequestWorker, type: :worker do
           }.to_json
         )
       end
+    end
 
     shared_examples 'unsuccessful request' do
       context 'when the request is not successful' do

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-11'.to_date, active_user_count: 100)
-        HistoricalData.create!(recorded_at: '2020-02-12'.to_date, active_user_count: 10)
-        HistoricalData.create!(recorded_at: '2020-02-13'.to_date, active_user_count: 15)
+        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)
 
-        HistoricalData.create!(recorded_at: '2020-03-11'.to_date, active_user_count: 10)
-        HistoricalData.create!(recorded_at: '2020-03-12'.to_date, active_user_count: 20)
-        HistoricalData.create!(recorded_at: '2020-03-15'.to_date, active_user_count: 25)
+        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)
         allow(SyncSeatLinkRequestWorker).to receive(:perform_async).and_return(true)
       end
 
@@ -27,10 +27,10 @@ RSpec.describe SyncSeatLinkWorker, type: :worker do
 
           expect(SyncSeatLinkRequestWorker).to have_received(:perform_async)
             .with(
-              '2020-03-11',
+              '2020-03-12T00:00:00Z',
               License.current.data,
               15,
-              10
+              12
             )
         end
       end
@@ -46,12 +46,13 @@ RSpec.describe SyncSeatLinkWorker, type: :worker do
 
             subject.perform
 
+            # Time.iso8601('2020-03-12T13:00:00+13:00') == Time.iso8601('2020-03-12T00:00:00Z')
             expect(SyncSeatLinkRequestWorker).to have_received(:perform_async)
               .with(
-                '2020-03-11',
+                '2020-03-12T13:00:00+13:00',
                 License.current.data,
                 15,
-                10
+                12
               )
           end
         end
@@ -67,12 +68,13 @@ RSpec.describe SyncSeatLinkWorker, type: :worker do
 
               subject.perform
 
+              # Time.iso8601('2020-03-11T18:00:00-06:00') == Time.iso8601('2020-03-12T00:00:00Z')
               expect(SyncSeatLinkRequestWorker).to have_received(:perform_async)
                 .with(
-                  '2020-03-11',
+                  '2020-03-11T18:00:00-06:00',
                   License.current.data,
                   15,
-                  10
+                  12
                 )
             end
           end
@@ -88,6 +90,9 @@ RSpec.describe SyncSeatLinkWorker, type: :worker do
       end
     end
 
+    context 'license checks' do
+      let_it_be(:historical_data) { create(:historical_data) }
+
       context 'when license is missing' do
         before do
           License.current.destroy!
@@ -117,14 +122,14 @@ RSpec.describe SyncSeatLinkWorker, type: :worker do
           create_current_license(expires_at: expiration_date)
         end
 
-      context 'the license expired over 15 days ago' do
-        let(:expiration_date) { Time.now.utc.to_date - 16.days }
+        context 'the license expired over 14 days ago' do
+          let(:expiration_date) { Time.zone.now.utc.to_date - 15.days }
 
           include_examples 'no seat link sync'
         end
 
-      context 'the license expired less than or equal to 15 days ago' do
-        let(:expiration_date) { Time.now.utc.to_date - 15.days }
+        context 'the license expired less than or equal to 14 days ago' do
+          let(:expiration_date) { Time.zone.now.utc.to_date - 14.days }
 
           it 'executes the SyncSeatLinkRequestWorker' do
             expect(SyncSeatLinkRequestWorker).to receive(:perform_async).and_return(true)
@@ -133,13 +138,20 @@ RSpec.describe SyncSeatLinkWorker, type: :worker do
           end
         end
       end
+    end
 
     context 'when seat link has been disabled' do
       before do
+        create(:historical_data)
+
         allow(Gitlab::CurrentSettings).to receive(:seat_link_enabled?).and_return(false)
       end
 
       include_examples 'no seat link sync'
     end
+
+    context 'when no historical data exists' do
+      include_examples 'no seat link sync'
+    end
   end
 end