Merge branch '271240-convert-to-timestamptz' into 'master'

Change historical_data.date to timestamptz data type

See merge request gitlab-org/gitlab!45893

db/migrate/20201022094845_add_historical_data_recorded_at.rb

+# frozen_string_literal: true
+
+class AddHistoricalDataRecordedAt < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    add_column(:historical_data, :recorded_at, :timestamptz)
+  end
+
+  def down
+    remove_column(:historical_data, :recorded_at)
+  end
+end

db/migrate/20201022094846_update_historical_data_recorded_at.rb

+# frozen_string_literal: true
+
+class UpdateHistoricalDataRecordedAt < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    update_value = Arel.sql("COALESCE(created_at, date + '12:00'::time AT TIME ZONE '#{Time.zone&.tzinfo&.name || "Etc/UTC"}')")
+
+    update_column_in_batches(:historical_data, :recorded_at, update_value) do |table, query|
+      query.where(table[:recorded_at].eq(nil))
+    end
+
+    add_not_null_constraint :historical_data, :recorded_at
+
+    change_column_null :historical_data, :date, true
+  end
+
+  def down
+    change_column_null :historical_data, :date, false
+
+    remove_not_null_constraint :historical_data, :recorded_at
+
+    update_column_in_batches(:historical_data, :recorded_at, nil) do |table, query|
+      query.where(table[:recorded_at].not_eq(nil))
+    end
+  end
+end

db/schema_migrations/20201022094845

+16b402740c6b1dd21908265085e516f63f8858424724ba97f46658e7bd5f7bf2
\ No newline at end of file

db/schema_migrations/20201022094846

+588c5f99d34652bbd5bde86351cbdb8c0455af0c31a440bfb63df02f12fd588f
\ No newline at end of file

db/structure.sql

@@ -12713,10 +12713,12 @@ CREATE TABLE group_wiki_repositories (
 
 CREATE TABLE historical_data (
     id integer NOT NULL,
-    date date NOT NULL,
     active_user_count integer,
     created_at timestamp without time zone,
-    updated_at timestamp without time zone
+    updated_at timestamp without time zone,
+    date date,
+    recorded_at timestamp with time zone,
+    CONSTRAINT check_640e8cf66c CHECK ((recorded_at IS NOT NULL))
 );
 
 CREATE SEQUENCE historical_data_id_seq

ee/app/models/gitlab/seat_link_data.rb

@@ -38,8 +38,8 @@ module Gitlab
 
     def default_max_count(date)
       HistoricalData.max_historical_user_count(
-        from: ::License.current.starts_at,
-        to: date
+        from: ::License.current.starts_at.beginning_of_day,
+        to: date.end_of_day
       )
     end
 

ee/app/models/historical_data.rb

 # frozen_string_literal: true
 
 class HistoricalData < ApplicationRecord
-  validates :date, presence: true
+  validates :recorded_at, presence: true
 
-  # HistoricalData.during((Date.today - 1.year)..Date.today).average(:active_user_count)
-  scope :during, ->(range) { where(date: range) }
-  # HistoricalData.up_until(Date.today - 1.month).average(:active_user_count)
-  scope :up_until, ->(date) { where("date <= :date", date: date) }
+  # HistoricalData.during((Time.current - 1.year)..Time.current).average(:active_user_count)
+  scope :during, ->(range) { where(recorded_at: range) }
+  # HistoricalData.up_until(Time.current - 1.month).average(:active_user_count)
+  scope :up_until, ->(timestamp) { where("recorded_at <= :timestamp", timestamp: timestamp) }
 
   class << self
     def track!
       create!(
-        date:               Date.today,
+        recorded_at:        Time.current,
         active_user_count:  License.load_license&.current_active_users_count
       )
     end
 
     # HistoricalData.at(Date.new(2014, 1, 1)).active_user_count
     def at(date)
-      find_by(date: 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 || Date.today
-      from ||= expires_at - 1.year
-      to   ||= expires_at
+      expires_at = license&.expires_at || Time.current
+      from ||= (expires_at - 1.year).beginning_of_day
+      to   ||= expires_at.end_of_day
 
       HistoricalData.during(from..to).maximum(:active_user_count) || 0
     end
 
     def in_license_term(license)
-      start_date = license.starts_at
-      expiration_date = license.expires_at || Date.current
+      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

ee/app/models/license.rb

@@ -556,8 +556,8 @@ class License < ApplicationRecord
 
   def prior_historical_max
     @prior_historical_max ||= begin
-      from = starts_at - 1.year
-      to   = starts_at
+      from = (starts_at - 1.year).beginning_of_day
+      to   = starts_at.end_of_day
 
       historical_max(from, to)
     end
@@ -586,8 +586,8 @@ class License < ApplicationRecord
 
   def check_trueup
     trueup_qty          = restrictions[:trueup_quantity]
-    trueup_from         = Date.parse(restrictions[:trueup_from]) rescue (starts_at - 1.year)
-    trueup_to           = Date.parse(restrictions[:trueup_to]) rescue starts_at
+    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)
     expected_trueup_qty = if previous_user_count
                             max_historical - previous_user_count

ee/app/services/historical_user_data/csv_service.rb

@@ -22,7 +22,7 @@ module HistoricalUserData
 
     def header_to_value_hash
       {
-        'Date' => -> (historical_datum) { historical_datum.date.to_s(:csv) },
+        'Date' => -> (historical_datum) { historical_datum.recorded_at.to_s(:csv) },
         'Active User Count' => 'active_user_count'
       }
     end

ee/changelogs/unreleased/271240-convert-to-timestamptz.yml

+---
+title: Change historical_data.date from date to timestamptz data type
+merge_request: 45893
+author:
+type: changed

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -293,7 +293,7 @@ RSpec.describe Admin::ApplicationSettingsController do
     end
 
     context 'when an admin user attempts a request' do
-      let_it_be(:yesterday) { Time.current.utc.yesterday.to_date }
+      let_it_be(:yesterday) { Time.current.utc.yesterday }
       let_it_be(:max_count) { 15 }
       let_it_be(:current_count) { 10 }
 
@@ -302,8 +302,8 @@ RSpec.describe Admin::ApplicationSettingsController do
       end
 
       before_all do
-        HistoricalData.create!(date: yesterday - 1.day, active_user_count: max_count)
-        HistoricalData.create!(date: yesterday, active_user_count: current_count)
+        HistoricalData.create!(recorded_at: yesterday - 1.day, active_user_count: max_count)
+        HistoricalData.create!(recorded_at: yesterday, active_user_count: current_count)
       end
 
       before do
@@ -318,7 +318,7 @@ RSpec.describe Admin::ApplicationSettingsController do
         body = response.body
         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}\"</span>")
+        expect(body).to include("<span class=\"s2\">\"#{yesterday.to_date}\"</span>")
         expect(body).to include("<span class=\"mi\">#{max_count}</span>")
         expect(body).to include("<span class=\"mi\">#{current_count}</span>")
       end

ee/spec/factories/historical_data.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :historical_data do
-    date { Date.today }
+    recorded_at { Time.current }
     active_user_count { 1 }
   end
 end

ee/spec/models/gitlab/seat_link_data_spec.rb

@@ -28,10 +28,10 @@ RSpec.describe Gitlab::SeatLinkData do
     let_it_be(:today_active_count) { 20 }
 
     before_all do
-      HistoricalData.create!(date: license_start_date, active_user_count: 10)
-      HistoricalData.create!(date: license_start_date + 1.day, active_user_count: max_before_today)
-      HistoricalData.create!(date: utc_date - 1.day, active_user_count: yesterday_active_count)
-      HistoricalData.create!(date: utc_date, active_user_count: today_active_count)
+      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)
     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!(date: Date.new(2014, i, 1), active_user_count: i * 100)
+      described_class.create!(recorded_at: Date.new(2014, i, 1), active_user_count: i * 100)
     end
   end
 
@@ -33,11 +33,15 @@ RSpec.describe HistoricalData do
     end
 
     it "creates a new historical data record" do
-      described_class.track!
+      freeze_time do
+        described_class.track!
 
-      data = described_class.last
-      expect(data.date).to eq(Date.today)
-      expect(data.active_user_count).to eq(5)
+        data = described_class.last
+        # Database time has microsecond precision, while Ruby time has nanosecond precision,
+        # which is why we need the be_within matcher even though we're freezing time.
+        expect(data.recorded_at).to be_within(1e-6.seconds).of(Time.current)
+        expect(data.active_user_count).to eq(5)
+      end
     end
   end
 
@@ -45,7 +49,7 @@ RSpec.describe HistoricalData do
     context 'with multiple historical data points for the current license' do
       before do
         (1..3).each do |i|
-          described_class.create!(date: Time.current - i.days, active_user_count: i * 100)
+          described_class.create!(recorded_at: Time.current - i.days, active_user_count: i * 100)
         end
       end
 
@@ -106,7 +110,7 @@ RSpec.describe HistoricalData do
 
       context 'with stats before the license period' do
         before do
-          described_class.create!(date: license.starts_at.ago(2.days), active_user_count: 10)
+          described_class.create!(recorded_at: license.starts_at.ago(2.days), active_user_count: 10)
         end
 
         it 'ignore those records' do
@@ -116,7 +120,7 @@ RSpec.describe HistoricalData do
 
       context 'with stats after the license period' do
         before do
-          described_class.create!(date: license.expires_at.in(2.days), active_user_count: 10)
+          described_class.create!(recorded_at: license.expires_at.in(2.days), active_user_count: 10)
         end
 
         it 'ignore those records' do
@@ -126,8 +130,8 @@ RSpec.describe HistoricalData do
 
       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)
+          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)
         end
 
         it 'returns max value for active_user_count' do
@@ -147,10 +151,10 @@ RSpec.describe HistoricalData do
     end
 
     before_all do
-      described_class.create!(date: license.starts_at - 1.day, active_user_count: 1)
-      described_class.create!(date: license.expires_at + 1.day, active_user_count: 2)
-      described_class.create!(date: now - 1.year - 1.day, active_user_count: 3)
-      described_class.create!(date: now + 1.day, active_user_count: 4)
+      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|

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_active_users.count + 10 }
       let(:date)              { described_class.current.starts_at }
-      let!(:historical_data)  { HistoricalData.create!(date: date, active_user_count: active_user_count) }
+      let!(:historical_data)  { HistoricalData.create!(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!(date: described_class.current.starts_at - 6.months, active_user_count: 15)
+          HistoricalData.create!(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!(date: 6.months.ago, active_user_count: 15)
+          HistoricalData.create!(recorded_at: 6.months.ago, active_user_count: 15)
 
           set_restrictions(restricted_user_count: 10, previous_user_count: 15)
         end

ee/spec/services/historical_user_data/csv_service_spec.rb

@@ -89,10 +89,10 @@ RSpec.describe HistoricalUserData::CsvService do
 
   context 'User Count Table' do
     let_it_be(:historical_datum) do
-      create(:historical_data, date: license_start_date, active_user_count: 1)
+      create(:historical_data, recorded_at: license_start_date, active_user_count: 1)
     end
     let_it_be(:historical_datum2) do
-      create(:historical_data, date: license_start_date + 1.day, active_user_count: 2)
+      create(:historical_data, recorded_at: license_start_date + 1.day, active_user_count: 2)
     end
 
     it 'shows the header for the user counts table' do
@@ -101,11 +101,11 @@ RSpec.describe HistoricalUserData::CsvService do
 
     it 'includes proper values for each column type', :aggregate_failures do
       expect(csv[8]).to contain_exactly(
-        historical_datum.date.to_s(:db),
+        historical_datum.recorded_at.to_s(:db),
         historical_datum.active_user_count.to_s
       )
       expect(csv[9]).to contain_exactly(
-        historical_datum2.date.to_s(:db),
+        historical_datum2.recorded_at.to_s(:db),
         historical_datum2.active_user_count.to_s
       )
     end

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!(date: '2020-02-11'.to_date, active_user_count: 100)
-        HistoricalData.create!(date: '2020-02-12'.to_date, active_user_count: 10)
-        HistoricalData.create!(date: '2020-02-13'.to_date, active_user_count: 15)
+        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!(date: '2020-03-11'.to_date, active_user_count: 10)
-        HistoricalData.create!(date: '2020-03-12'.to_date, active_user_count: 20)
-        HistoricalData.create!(date: '2020-03-15'.to_date, active_user_count: 25)
+        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)
         allow(SyncSeatLinkRequestWorker).to receive(:perform_async).and_return(true)
       end
 

spec/migrations/update_historical_data_recorded_at_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'migrate', '20201022094846_update_historical_data_recorded_at.rb')
+
+RSpec.describe UpdateHistoricalDataRecordedAt do
+  let(:historical_data_table) { table(:historical_data) }
+
+  it 'reversibly populates recorded_at from created_at or date' do
+    row1 = historical_data_table.create!(
+      date: Date.current - 1.day,
+      created_at: Time.current - 1.day
+    )
+
+    row2 = historical_data_table.create!(date: Date.current - 2.days)
+    row2.update!(created_at: nil)
+
+    reversible_migration do |migration|
+      migration.before -> {
+        expect(row1.reload.recorded_at).to eq(nil)
+        expect(row2.reload.recorded_at).to eq(nil)
+      }
+
+      migration.after -> {
+        expect(row1.reload.recorded_at).to eq(row1.created_at)
+        expect(row2.reload.recorded_at).to eq(row2.date.in_time_zone(Time.zone).change(hour: 12))
+      }
+    end
+  end
+end