Merge branch '32054-rails-should-use-timestamptz-database-type-for-postgresql' into 'master'

Add database helpers 'add_timestamps_with_timezone' and 'timestamps_with_timezone'

Closes #32054

See merge request !11229

4 files changed, 315 insertions, 1 deletions

diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index 3fdafd867da..30aa463faf8 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -7,7 +7,42 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
     )
   end
 
-  before { allow(model).to receive(:puts) }
+  before do
+    allow(model).to receive(:puts)
+  end
+
+  describe '#add_timestamps_with_timezone' do
+    before do
+      allow(model).to receive(:transaction_open?).and_return(false)
+    end
+
+    context 'using PostgreSQL' do
+      before do
+        allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+        allow(model).to receive(:disable_statement_timeout)
+      end
+
+      it 'adds "created_at" and "updated_at" fields with the "datetime_with_timezone" data type' do
+        expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, { null: false })
+        expect(model).to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, { null: false })
+
+        model.add_timestamps_with_timezone(:foo)
+      end
+    end
+
+    context 'using MySQL' do
+      before do
+        allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
+      end
+
+      it 'adds "created_at" and "updated_at" fields with "datetime_with_timezone" data type' do
+        expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, { null: false })
+        expect(model).to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, { null: false })
+
+        model.add_timestamps_with_timezone(:foo)
+      end
+    end
+  end
 
   describe '#add_concurrent_index' do
     context 'outside a transaction' do
diff --git a/spec/rubocop/cop/migration/add_timestamps_spec.rb b/spec/rubocop/cop/migration/add_timestamps_spec.rb
new file mode 100644
index 00000000000..18df62dec3e
--- /dev/null
+++ b/spec/rubocop/cop/migration/add_timestamps_spec.rb
@@ -0,0 +1,90 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/add_timestamps'
+
+describe RuboCop::Cop::Migration::AddTimestamps do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+  let(:migration_with_add_timestamps) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          add_column(:users, :username, :text)
+          add_timestamps(:users)
+        end
+      end
+    )
+  end
+
+  let(:migration_without_add_timestamps) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          add_column(:users, :username, :text)
+        end
+      end
+    )
+  end
+
+  let(:migration_with_add_timestamps_with_timezone) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          add_column(:users, :username, :text)
+          add_timestamps_with_timezone(:users)
+        end
+      end
+    )
+  end
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when the "add_timestamps" method is used' do
+      inspect_source(cop, migration_with_add_timestamps)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([7])
+      end
+    end
+
+    it 'does not register an offense when the "add_timestamps" method is not used' do
+      inspect_source(cop, migration_without_add_timestamps)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(0)
+      end
+    end
+
+    it 'does not register an offense when the "add_timestamps_with_timezone" method is used' do
+      inspect_source(cop, migration_with_add_timestamps_with_timezone)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(0)
+      end
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      inspect_source(cop, migration_with_add_timestamps)
+      inspect_source(cop, migration_without_add_timestamps)
+      inspect_source(cop, migration_with_add_timestamps_with_timezone)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end
diff --git a/spec/rubocop/cop/migration/datetime_spec.rb b/spec/rubocop/cop/migration/datetime_spec.rb
new file mode 100644
index 00000000000..388b086ce6a
--- /dev/null
+++ b/spec/rubocop/cop/migration/datetime_spec.rb
@@ -0,0 +1,90 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/datetime'
+
+describe RuboCop::Cop::Migration::Datetime do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+  let(:migration_with_datetime) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          add_column(:users, :username, :text)
+          add_column(:users, :last_sign_in, :datetime)
+        end
+      end
+    )
+  end
+
+  let(:migration_without_datetime) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          add_column(:users, :username, :text)
+        end
+      end
+    )
+  end
+
+  let(:migration_with_datetime_with_timezone) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          add_column(:users, :username, :text)
+          add_column(:users, :last_sign_in, :datetime_with_timezone)
+        end
+      end
+    )
+  end
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when the ":datetime" data type is used' do
+      inspect_source(cop, migration_with_datetime)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([7])
+      end
+    end
+
+    it 'does not register an offense when the ":datetime" data type is not used' do
+      inspect_source(cop, migration_without_datetime)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(0)
+      end
+    end
+
+    it 'does not register an offense when the ":datetime_with_timezone" data type is used' do
+      inspect_source(cop, migration_with_datetime_with_timezone)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(0)
+      end
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      inspect_source(cop, migration_with_datetime)
+      inspect_source(cop, migration_without_datetime)
+      inspect_source(cop, migration_with_datetime_with_timezone)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end
diff --git a/spec/rubocop/cop/migration/timestamps_spec.rb b/spec/rubocop/cop/migration/timestamps_spec.rb
new file mode 100644
index 00000000000..cdf1423d0c5
--- /dev/null
+++ b/spec/rubocop/cop/migration/timestamps_spec.rb
@@ -0,0 +1,99 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/timestamps'
+
+describe RuboCop::Cop::Migration::Timestamps do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+  let(:migration_with_timestamps) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          create_table :users do |t|
+            t.string :username, null: false
+            t.timestamps null: true
+            t.string :password
+          end
+        end
+      end
+    )
+  end
+
+  let(:migration_without_timestamps) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          create_table :users do |t|
+            t.string :username, null: false
+            t.string :password
+          end
+        end
+      end
+    )
+  end
+
+  let(:migration_with_timestamps_with_timezone) do
+    %q(
+      class Users < ActiveRecord::Migration
+        DOWNTIME = false
+
+        def change
+          create_table :users do |t|
+            t.string :username, null: false
+            t.timestamps_with_timezone null: true
+            t.string :password
+          end
+        end
+      end
+    )
+  end
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when the "timestamps" method is used' do
+      inspect_source(cop, migration_with_timestamps)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([8])
+      end
+    end
+
+    it 'does not register an offense when the "timestamps" method is not used' do
+      inspect_source(cop, migration_without_timestamps)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(0)
+      end
+    end
+
+    it 'does not register an offense when the "timestamps_with_timezone" method is used' do
+      inspect_source(cop, migration_with_timestamps_with_timezone)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(0)
+      end
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      inspect_source(cop, migration_with_timestamps)
+      inspect_source(cop, migration_without_timestamps)
+      inspect_source(cop, migration_with_timestamps_with_timezone)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end