Merge branch '32596-add-rubocop-to-enforce-datetime-with-timezone' into 'master'

Enforce use of datetime_with_timezone on create_table migrations

Closes #32596

See merge request gitlab-org/gitlab!28690

db/migrate/20171230123729_init_schema.rb

@@ -6,6 +6,7 @@
 # rubocop:disable Migration/AddConcurrentForeignKey
 # rubocop:disable Style/WordArray
 # rubocop:disable Migration/AddLimitToStringColumns
+# rubocop:disable Migration/Datetime
 
 class InitSchema < ActiveRecord::Migration[4.2]
   DOWNTIME = false

db/migrate/20180116193854_create_lfs_file_locks.rb

@@ -9,7 +9,7 @@ class CreateLfsFileLocks < ActiveRecord::Migration[4.2]
     create_table :lfs_file_locks do |t|
       t.references :project, null: false, foreign_key: { on_delete: :cascade }
       t.references :user, null: false, index: true, foreign_key: { on_delete: :cascade }
-      t.datetime :created_at, null: false
+      t.datetime :created_at, null: false # rubocop:disable Migration/Datetime
       t.string :path, limit: 511
     end
 

db/migrate/20180503131624_create_remote_mirrors.rb

@@ -14,9 +14,9 @@ class CreateRemoteMirrors < ActiveRecord::Migration[4.2]
       t.string :url
       t.boolean :enabled, default: true
       t.string :update_status
-      t.datetime :last_update_at
-      t.datetime :last_successful_update_at
-      t.datetime :last_update_started_at
+      t.datetime :last_update_at # rubocop:disable Migration/Datetime
+      t.datetime :last_successful_update_at # rubocop:disable Migration/Datetime
+      t.datetime :last_update_started_at # rubocop:disable Migration/Datetime
       t.string :last_error
       t.boolean :only_protected_branches, default: false, null: false
       t.string :remote_name

db/migrate/20200215225103_drop_forked_project_links_table.rb

 # frozen_string_literal: true
 
+# rubocop:disable Migration/Datetime
+
 class DropForkedProjectLinksTable < ActiveRecord::Migration[6.0]
   include Gitlab::Database::MigrationHelpers
 

ee/db/geo/migrate/20170206203234_create_project_registry.rb

@@ -2,10 +2,10 @@ class CreateProjectRegistry < ActiveRecord::Migration[4.2]
   def change
     create_table :project_registry do |t|
       t.integer  :project_id, null: false
-      t.datetime :last_repository_synced_at
-      t.datetime :last_repository_successful_sync_at
+      t.datetime :last_repository_synced_at # rubocop:disable Migration/Datetime
+      t.datetime :last_repository_successful_sync_at # rubocop:disable Migration/Datetime
 
-      t.datetime :created_at, null: false
+      t.datetime :created_at, null: false # rubocop:disable Migration/Datetime
     end
   end
 end

ee/db/geo/migrate/20170223033541_create_file_registry.rb

@@ -6,7 +6,7 @@ class CreateFileRegistry < ActiveRecord::Migration[4.2]
       t.integer :bytes
       t.string  :sha256 # rubocop:disable Migration/AddLimitToStringColumns
 
-      t.datetime :created_at, null: false
+      t.datetime :created_at, null: false # rubocop:disable Migration/Datetime
     end
 
     add_index :file_registry, :file_type

ee/db/geo/migrate/20190612211021_add_container_repository_registry.rb

@@ -9,9 +9,9 @@ class AddContainerRepositoryRegistry < ActiveRecord::Migration[5.0]
       t.string :state # rubocop:disable Migration/AddLimitToStringColumns
       t.integer :retry_count, default: 0
       t.string :last_sync_failure # rubocop:disable Migration/AddLimitToStringColumns
-      t.datetime :retry_at
-      t.datetime :last_synced_at
-      t.datetime :created_at, null: false
+      t.datetime :retry_at # rubocop:disable Migration/Datetime
+      t.datetime :last_synced_at # rubocop:disable Migration/Datetime
+      t.datetime :created_at, null: false # rubocop:disable Migration/Datetime
 
       t.index :container_repository_id, name: :index_container_repository_registry_on_repository_id, using: :btree
       t.index :retry_at, name: :index_container_repository_registry_on_retry_at, using: :btree

ee/db/geo/migrate/20190923111102_add_design_registry.rb

@@ -16,9 +16,9 @@ class AddDesignRegistry < ActiveRecord::Migration[5.2]
       t.string :last_sync_failure # rubocop:disable Migration/AddLimitToStringColumns
       t.boolean :force_to_redownload
       t.boolean :missing_on_primary
-      t.datetime :retry_at
-      t.datetime :last_synced_at
-      t.datetime :created_at, null: false
+      t.datetime :retry_at # rubocop:disable Migration/Datetime
+      t.datetime :last_synced_at # rubocop:disable Migration/Datetime
+      t.datetime :created_at, null: false # rubocop:disable Migration/Datetime
 
       t.index :project_id, name: :index_design_registry_on_project_id, using: :btree
       t.index :retry_at, name: :index_design_registry_on_retry_at, using: :btree

rubocop/cop/migration/datetime.rb

@@ -14,7 +14,7 @@ module RuboCop
           return unless in_migration?(node)
 
           node.each_descendant(:send) do |send_node|
-            method_name = node.children[1]
+            method_name = send_node.children[1]
 
             if method_name == :datetime || method_name == :timestamp
               add_offense(send_node, location: :selector, message: format(MSG, method_name))

spec/rubocop/cop/migration/datetime_spec.rb

@@ -12,9 +12,69 @@ describe RuboCop::Cop::Migration::Datetime do
 
   subject(:cop) { described_class.new }
 
-  let(:migration_with_datetime) do
+  let(:create_table_migration_with_datetime) do
     %q(
-      class Users < ActiveRecord::Migration[4.2]
+      class Users < ActiveRecord::Migration[6.0]
+        DOWNTIME = false
+
+        def change
+          create_table :users do |t|
+            t.string :username, null: false
+            t.datetime :last_sign_in
+          end
+        end
+      end
+    )
+  end
+
+  let(:create_table_migration_with_timestamp) do
+    %q(
+      class Users < ActiveRecord::Migration[6.0]
+        DOWNTIME = false
+
+        def change
+          create_table :users do |t|
+            t.string :username, null: false
+            t.timestamp :last_sign_in
+          end
+        end
+      end
+    )
+  end
+
+  let(:create_table_migration_without_datetime) do
+    %q(
+      class Users < ActiveRecord::Migration[6.0]
+        DOWNTIME = false
+
+        def change
+          create_table :users do |t|
+            t.string :username, null: false
+            t.string :password
+          end
+        end
+      end
+    )
+  end
+
+  let(:create_table_migration_with_datetime_with_timezone) do
+    %q(
+      class Users < ActiveRecord::Migration[6.0]
+        DOWNTIME = false
+
+        def change
+          create_table :users do |t|
+            t.string :username, null: false
+            t.datetime_with_timezone :last_sign_in
+          end
+        end
+      end
+    )
+  end
+
+  let(:add_column_migration_with_datetime) do
+    %q(
+      class Users < ActiveRecord::Migration[6.0]
         DOWNTIME = false
 
         def change
@@ -25,9 +85,9 @@ describe RuboCop::Cop::Migration::Datetime do
     )
   end
 
-  let(:migration_with_timestamp) do
+  let(:add_column_migration_with_timestamp) do
     %q(
-      class Users < ActiveRecord::Migration[4.2]
+      class Users < ActiveRecord::Migration[6.0]
         DOWNTIME = false
 
         def change
@@ -38,9 +98,9 @@ describe RuboCop::Cop::Migration::Datetime do
     )
   end
 
-  let(:migration_without_datetime) do
+  let(:add_column_migration_without_datetime) do
     %q(
-      class Users < ActiveRecord::Migration[4.2]
+      class Users < ActiveRecord::Migration[6.0]
         DOWNTIME = false
 
         def change
@@ -50,9 +110,9 @@ describe RuboCop::Cop::Migration::Datetime do
     )
   end
 
-  let(:migration_with_datetime_with_timezone) do
+  let(:add_column_migration_with_datetime_with_timezone) do
     %q(
-      class Users < ActiveRecord::Migration[4.2]
+      class Users < ActiveRecord::Migration[6.0]
         DOWNTIME = false
 
         def change
@@ -68,18 +128,54 @@ describe RuboCop::Cop::Migration::Datetime 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(migration_with_datetime)
+    it 'registers an offense when the ":datetime" data type is used on create_table' do
+      inspect_source(create_table_migration_with_datetime)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([8])
+        expect(cop.offenses.first.message).to include('`datetime`')
+      end
+    end
+
+    it 'registers an offense when the ":timestamp" data type is used on create_table' do
+      inspect_source(create_table_migration_with_timestamp)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([8])
+        expect(cop.offenses.first.message).to include('timestamp')
+      end
+    end
+
+    it 'does not register an offense when the ":datetime" data type is not used on create_table' do
+      inspect_source(create_table_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 on create_table' do
+      inspect_source(create_table_migration_with_datetime_with_timezone)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(0)
+      end
+    end
+
+    it 'registers an offense when the ":datetime" data type is used on add_column' do
+      inspect_source(add_column_migration_with_datetime)
 
       aggregate_failures do
         expect(cop.offenses.size).to eq(1)
         expect(cop.offenses.map(&:line)).to eq([7])
-        expect(cop.offenses.first.message).to include('datetime')
+        expect(cop.offenses.first.message).to include('`datetime`')
       end
     end
 
-    it 'registers an offense when the ":timestamp" data type is used' do
-      inspect_source(migration_with_timestamp)
+    it 'registers an offense when the ":timestamp" data type is used on add_column' do
+      inspect_source(add_column_migration_with_timestamp)
 
       aggregate_failures do
         expect(cop.offenses.size).to eq(1)
@@ -88,16 +184,16 @@ describe RuboCop::Cop::Migration::Datetime do
       end
     end
 
-    it 'does not register an offense when the ":datetime" data type is not used' do
-      inspect_source(migration_without_datetime)
+    it 'does not register an offense when the ":datetime" data type is not used on add_column' do
+      inspect_source(add_column_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(migration_with_datetime_with_timezone)
+    it 'does not register an offense when the ":datetime_with_timezone" data type is used on add_column' do
+      inspect_source(add_column_migration_with_datetime_with_timezone)
 
       aggregate_failures do
         expect(cop.offenses.size).to eq(0)
@@ -107,10 +203,10 @@ describe RuboCop::Cop::Migration::Datetime do
 
   context 'outside of migration' do
     it 'registers no offense' do
-      inspect_source(migration_with_datetime)
-      inspect_source(migration_with_timestamp)
-      inspect_source(migration_without_datetime)
-      inspect_source(migration_with_datetime_with_timezone)
+      inspect_source(add_column_migration_with_datetime)
+      inspect_source(add_column_migration_with_timestamp)
+      inspect_source(add_column_migration_without_datetime)
+      inspect_source(add_column_migration_with_datetime_with_timezone)
 
       expect(cop.offenses.size).to eq(0)
     end