Send latest historical data as seat link data

- Currently, seat link data sends the previous day's historical data.
  This appears to have been done because historical data used to contain
  date, not timestamp. So, to avoid problems like seemingly future
  dates being received from GitLab instances in timezones ahead of UTC,
  the seat link data sends the previous day's historical data. Now that
  the historical data contains a timestamp with timezone, we can send
  the latest historical data instead.

- Format the timestamp using the iso8601 format when transmitting.

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

@@ -319,6 +319,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(License).to receive(:current_active_users).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