Merge branch '26908-add-foreign-keys-to-timelogs' into 'master'

Use normal associations instead of polymorphic

Closes #26908

See merge request !8769

app/models/concerns/time_trackable.rb

@@ -18,7 +18,7 @@ module TimeTrackable
     validates :time_estimate, numericality: { message: 'has an invalid format' }, allow_nil: false
     validate  :check_negative_time_spent
 
-    has_many :timelogs, as: :trackable, dependent: :destroy
+    has_many :timelogs, dependent: :destroy
   end
 
   def spend_time(options)

app/models/timelog.rb

 class Timelog < ActiveRecord::Base
   validates :time_spent, :user, presence: true
+  validate :issuable_id_is_present
 
-  belongs_to :trackable, polymorphic: true
+  belongs_to :issue
+  belongs_to :merge_request
   belongs_to :user
+
+  def issuable
+    issue || merge_request
+  end
+
+  private
+
+  def issuable_id_is_present
+    if issue_id && merge_request_id
+      errors.add(:base, 'Only Issue ID or Merge Request ID is required')
+    elsif issuable.nil?
+      errors.add(:base, 'Issue or Merge Request ID is required')
+    end
+  end
 end

changelogs/unreleased/26908-make-timelogs-use-foreign-keys

+---
+title: Refactor Timelogs structure to use foreign keys.
+merge_request: 8769
+author:

db/migrate/20170124174637_add_foreign_keys_to_timelogs.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddForeignKeysToTimelogs < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  disable_ddl_transaction!
+
+  def up
+    change_table :timelogs do |t|
+      t.column :issue_id, :integer
+      t.column :merge_request_id, :integer
+    end
+
+    add_concurrent_index :timelogs, :issue_id
+    add_concurrent_index :timelogs, :merge_request_id
+
+    if Gitlab::Database.postgresql?
+      execute <<-EOF
+        ALTER TABLE timelogs ADD CONSTRAINT "fk_timelogs_issues_issue_id" FOREIGN KEY (issue_id) REFERENCES "issues" (id) ON DELETE CASCADE NOT VALID;
+        ALTER TABLE timelogs ADD CONSTRAINT "fk_timelogs_merge_requests_merge_request_id" FOREIGN KEY (merge_request_id) REFERENCES "merge_requests" (id) ON DELETE CASCADE NOT VALID;
+      EOF
+    else
+      execute "ALTER TABLE timelogs ADD CONSTRAINT fk_timelogs_issues_issue_id FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE;"
+      execute "ALTER TABLE timelogs ADD CONSTRAINT fk_timelogs_merge_requests_merge_request_id FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE;"
+    end
+
+    Timelog.where(trackable_type: 'Issue').update_all("issue_id = trackable_id")
+    Timelog.where(trackable_type: 'MergeRequest').update_all("merge_request_id = trackable_id")
+  end
+
+  def down
+    Timelog.where('issue_id IS NOT NULL').update_all("trackable_id = issue_id, trackable_type = 'Issue'")
+    Timelog.where('merge_request_id IS NOT NULL').update_all("trackable_id = merge_request_id, trackable_type = 'MergeRequest'")
+
+    remove_columns :timelogs, :issue_id, :merge_request_id
+  end
+end

db/post_migrate/20170206101007_remove_trackable_columns_from_timelogs.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveTrackableColumnsFromTimelogs < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    remove_columns :timelogs, :trackable_id, :trackable_type
+  end
+end

db/post_migrate/20170206101030_validate_foreign_keys_on_timelogs.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ValidateForeignKeysOnTimelogs < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  disable_ddl_transaction!
+
+  def up
+    if Gitlab::Database.postgresql?
+      execute <<-EOF
+        ALTER TABLE timelogs VALIDATE CONSTRAINT "fk_timelogs_issues_issue_id";
+        ALTER TABLE timelogs VALIDATE CONSTRAINT "fk_timelogs_merge_requests_merge_request_id";
+      EOF
+    end
+  end
+
+  def down
+    # noop
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170206071414) do
+ActiveRecord::Schema.define(version: 20170206101030) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1151,14 +1151,15 @@ ActiveRecord::Schema.define(version: 20170206071414) do
 
   create_table "timelogs", force: :cascade do |t|
     t.integer "time_spent", null: false
-    t.integer "trackable_id"
-    t.string "trackable_type"
     t.integer "user_id"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.integer "issue_id"
+    t.integer "merge_request_id"
   end
 
-  add_index "timelogs", ["trackable_type", "trackable_id"], name: "index_timelogs_on_trackable_type_and_trackable_id", using: :btree
+  add_index "timelogs", ["issue_id"], name: "index_timelogs_on_issue_id", using: :btree
+  add_index "timelogs", ["merge_request_id"], name: "index_timelogs_on_merge_request_id", using: :btree
   add_index "timelogs", ["user_id"], name: "index_timelogs_on_user_id", using: :btree
 
   create_table "todos", force: :cascade do |t|
@@ -1340,6 +1341,8 @@ ActiveRecord::Schema.define(version: 20170206071414) do
   add_foreign_key "protected_branch_merge_access_levels", "protected_branches"
   add_foreign_key "protected_branch_push_access_levels", "protected_branches"
   add_foreign_key "subscriptions", "projects", on_delete: :cascade
+  add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade
+  add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
 end

spec/factories/timelogs.rb

@@ -4,6 +4,6 @@ FactoryGirl.define do
   factory :timelog do
     time_spent 3600
     user
-    association :trackable, factory: :issue
+    issue
   end
 end

spec/lib/gitlab/import_export/all_models.yml

@@ -203,5 +203,6 @@ award_emoji:
 priorities:
 - label
 timelogs:
-- trackable
+- issue
+- merge_request
 - user

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -350,8 +350,8 @@ LabelPriority:
 Timelog:
 - id
 - time_spent
-- trackable_id
-- trackable_type
+- merge_request_id
+- issue_id
 - user_id
 - created_at
 - updated_at

spec/models/timelog_spec.rb

@@ -2,9 +2,37 @@ require 'rails_helper'
 
 RSpec.describe Timelog, type: :model do
   subject { build(:timelog) }
+  let(:issue) { create(:issue) }
+  let(:merge_request) { create(:merge_request) }
 
   it { is_expected.to be_valid }
 
   it { is_expected.to validate_presence_of(:time_spent) }
   it { is_expected.to validate_presence_of(:user) }
+
+  describe 'Issuable validation' do
+    it 'is invalid if issue_id and merge_request_id are missing' do
+      subject.attributes = { issue: nil, merge_request: nil }
+
+      expect(subject).to be_invalid
+    end
+
+    it 'is invalid if issue_id and merge_request_id are set' do
+      subject.attributes = { issue: issue, merge_request: merge_request }
+
+      expect(subject).to be_invalid
+    end
+
+    it 'is valid if only issue_id is set' do
+      subject.attributes = { issue: issue, merge_request: nil }
+
+      expect(subject).to be_valid
+    end
+
+    it 'is valid if only merge_request_id is set' do
+      subject.attributes = { merge_request: merge_request, issue: nil }
+
+      expect(subject).to be_valid
+    end
+  end
 end