Merge pull request #2304 from gitlabhq/fix_postgres_notes

Split commit_id and noteable_id for Note

.travis.yml

@@ -19,8 +19,7 @@ services:
 before_script:
   - "cp config/database.yml.$DB config/database.yml"
   - "cp config/gitlab.yml.example config/gitlab.yml"
-  - "bundle exec rake db:create RAILS_ENV=test"
-  - "bundle exec rake db:migrate RAILS_ENV=test"
+  - "bundle exec rake db:setup RAILS_ENV=test"
   - "bundle exec rake db:seed_fu RAILS_ENV=test"
   - "sh -e /etc/init.d/xvfb start"
 script: "bundle exec rake travis --trace"

Gemfile

@@ -124,7 +124,7 @@ group :development, :test do
   gem "capybara"
   gem "pry"
   gem "awesome_print"
-  gem "database_cleaner"
+  gem "database_cleaner", ref: "f89c34300e114be99532f14c115b2799a3380ac6", git: "https://github.com/bmabey/database_cleaner.git"
   gem "launchy"
   gem 'factory_girl_rails'
 

Gemfile.lock

@@ -1,3 +1,10 @@
+GIT
+  remote: https://github.com/bmabey/database_cleaner.git
+  revision: f89c34300e114be99532f14c115b2799a3380ac6
+  ref: f89c34300e114be99532f14c115b2799a3380ac6
+  specs:
+    database_cleaner (0.9.1)
+
 GIT
   remote: https://github.com/ctran/annotate_models.git
   revision: be4e26825b521f0b2d86b181e2dff89901aa9b1e
@@ -140,7 +147,6 @@ GEM
     colorize (0.5.8)
     crack (0.3.1)
     daemons (1.1.9)
-    database_cleaner (0.9.1)
     devise (2.1.2)
       bcrypt-ruby (~> 3.0)
       orm_adapter (~> 0.1)
@@ -458,7 +464,7 @@ DEPENDENCIES
   chosen-rails (= 0.9.8)
   coffee-rails (~> 3.2.2)
   colored
-  database_cleaner
+  database_cleaner!
   devise (~> 2.1.0)
   draper (~> 0.18.0)
   email_spec

app/models/merge_request.rb

@@ -204,7 +204,7 @@ class MergeRequest < ActiveRecord::Base
 
   def mr_and_commit_notes
     commit_ids = commits.map(&:id)
-    Note.where("(noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR (noteable_type = 'Commit' AND noteable_id IN (:commit_ids))", mr_id: id, commit_ids: commit_ids)
+    Note.where("(noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR (noteable_type = 'Commit' AND commit_id IN (:commit_ids))", mr_id: id, commit_ids: commit_ids)
   end
 
   # Returns the raw diff for this merge request

app/models/note.rb

@@ -20,7 +20,7 @@ require 'file_size_validator'
 class Note < ActiveRecord::Base
 
   attr_accessible :note, :noteable, :noteable_id, :noteable_type, :project_id,
-                  :attachment, :line_code
+                  :attachment, :line_code, :commit_id
 
   attr_accessor :notify
   attr_accessor :notify_author
@@ -35,10 +35,14 @@ class Note < ActiveRecord::Base
   validates :note, :project, presence: true
   validates :attachment, file_size: { maximum: 10.megabytes.to_i }
 
+  validates :noteable_id, presence: true, if: ->(n) { n.noteable_type.present? && n.noteable_type != 'Commit' }
+  validates :commit_id, presence: true, if: ->(n) { n.noteable_type == 'Commit' }
+
   mount_uploader :attachment, AttachmentUploader
 
   # Scopes
-  scope :common, ->{ where(noteable_id: nil) }
+  scope :for_commits, ->{ where(noteable_type: "Commit") }
+  scope :common, ->{ where(noteable_id: nil, commit_id: nil) }
   scope :today, ->{ where("created_at >= :date", date: Date.today) }
   scope :last_week, ->{ where("created_at  >= :date", date: (Date.today - 7.days)) }
   scope :since, ->(day) { where("created_at  >= :date", date: (day)) }
@@ -66,7 +70,7 @@ class Note < ActiveRecord::Base
   # override to return commits, which are not active record
   def noteable
     if for_commit?
-      project.commit(noteable_id)
+      project.commit(commit_id)
     else
       super
     end

app/models/project.rb

@@ -203,15 +203,15 @@ class Project < ActiveRecord::Base
   end
 
   def build_commit_note(commit)
-    notes.new(noteable_id: commit.id, noteable_type: "Commit")
+    notes.new(commit_id: commit.id, noteable_type: "Commit")
   end
 
   def commit_notes(commit)
-    notes.where(noteable_id: commit.id, noteable_type: "Commit", line_code: nil)
+    notes.where(commit_id: commit.id, noteable_type: "Commit", line_code: nil)
   end
 
   def commit_line_notes(commit)
-    notes.where(noteable_id: commit.id, noteable_type: "Commit").where("line_code IS NOT NULL")
+    notes.where(commit_id: commit.id, noteable_type: "Commit").where("line_code IS NOT NULL")
   end
 
   def public?

app/roles/note_event.rb

@@ -1,6 +1,6 @@
 module NoteEvent
   def note_commit_id
-    target.noteable_id
+    target.commit_id
   end
 
   def note_short_commit_id
@@ -16,7 +16,11 @@ module NoteEvent
   end
 
   def note_target_id
-    target.noteable_id
+    if note_commit?
+      target.commit_id
+    else
+      target.noteable_id.to_s
+    end
   end
 
   def wall_note?

app/views/notes/_common_form.html.haml

@@ -7,6 +7,7 @@
           %div= msg
 
     = f.hidden_field :noteable_id
+    = f.hidden_field :commit_id
     = f.hidden_field :noteable_type
     = f.text_area :note, size: 255, class: 'note-text js-gfm-input'
     #preview-note.preview_note.hide

app/views/notes/_per_line_form.html.haml

@@ -11,6 +11,7 @@
                   %div= msg
 
             = f.hidden_field :noteable_id
+            = f.hidden_field :commit_id
             = f.hidden_field :noteable_type
             = f.hidden_field :line_code
             = f.text_area :note,  size: 255, class: 'line-note-text js-gfm-input'

config/database.yml.postgresql

@@ -9,6 +9,7 @@ production:
   username: postgres
   password:
   # host: localhost
+  # port: 5432 
   # socket: /tmp/postgresql.sock
 
 #

db/migrate/20121218164840_move_noteable_commit_to_own_field.rb

@@ -0,0 +1,20 @@
+class MoveNoteableCommitToOwnField < ActiveRecord::Migration
+  def up
+    add_column :notes, :commit_id, :string, null: true
+    add_column :notes, :new_noteable_id, :integer, null: true
+    Note.where(noteable_type: 'Commit').update_all('commit_id = noteable_id')
+
+    if ActiveRecord::Base.connection.adapter_name == 'PostgreSQL'
+      Note.where("noteable_type != 'Commit'").update_all('new_noteable_id = CAST (noteable_id AS INTEGER)')
+    else
+      Note.where("noteable_type != 'Commit'").update_all('new_noteable_id = noteable_id')
+    end
+
+    remove_column :notes, :noteable_id
+    rename_column :notes, :new_noteable_id, :noteable_id
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/migrate/20121219095402_indices_for_notes.rb

@@ -0,0 +1,6 @@
+class IndicesForNotes < ActiveRecord::Migration
+  def change
+    add_index :notes, :commit_id
+    add_index :notes, [:project_id, :noteable_type]
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20121205201726) do
+ActiveRecord::Schema.define(:version => 20121219095402) do
 
   create_table "events", :force => true do |t|
     t.string   "target_type"
@@ -124,7 +124,6 @@ ActiveRecord::Schema.define(:version => 20121205201726) do
 
   create_table "notes", :force => true do |t|
     t.text     "note"
-    t.string   "noteable_id"
     t.string   "noteable_type"
     t.integer  "author_id"
     t.datetime "created_at",    :null => false
@@ -132,11 +131,14 @@ ActiveRecord::Schema.define(:version => 20121205201726) do
     t.integer  "project_id"
     t.string   "attachment"
     t.string   "line_code"
+    t.string   "commit_id"
+    t.integer  "noteable_id"
   end
 
+  add_index "notes", ["commit_id"], :name => "index_notes_on_commit_id"
   add_index "notes", ["created_at"], :name => "index_notes_on_created_at"
-  add_index "notes", ["noteable_id"], :name => "index_notes_on_noteable_id"
   add_index "notes", ["noteable_type"], :name => "index_notes_on_noteable_type"
+  add_index "notes", ["project_id", "noteable_type"], :name => "index_notes_on_project_id_and_noteable_type"
   add_index "notes", ["project_id"], :name => "index_notes_on_project_id"
 
   create_table "projects", :force => true do |t|

features/support/env.rb

@@ -36,8 +36,6 @@ Spinach.hooks.before_scenario do
   Gitlab.config.stub(git_base_path: Rails.root.join('tmp', 'test-git-base-path'))
   FileUtils.rm_rf Gitlab.config.git_base_path
   FileUtils.mkdir_p Gitlab.config.git_base_path
-
-  DatabaseCleaner.start
 end
 
 Spinach.hooks.after_scenario do

spec/models/merge_request_spec.rb

@@ -42,7 +42,7 @@ describe MergeRequest do
 
     before do
       merge_request.stub(:commits) { [merge_request.project.commit] }
-      create(:note, noteable: merge_request.commits.first)
+      create(:note, commit_id: merge_request.commits.first.id, noteable_type: 'Commit')
       create(:note, noteable: merge_request)
     end
 

spec/models/note_spec.rb

@@ -81,18 +81,18 @@ describe Note do
   describe "Commit notes" do
     before do
       @note = create(:note,
-                     noteable_id: commit.id,
+                     commit_id: commit.id,
                      noteable_type: "Commit")
     end
 
     it "should be accessible through #noteable" do
-      @note.noteable_id.should == commit.id
+      @note.commit_id.should == commit.id
       @note.noteable.should be_a(Commit)
       @note.noteable.should == commit
     end
 
     it "should save a valid note" do
-      @note.noteable_id.should == commit.id
+      @note.commit_id.should == commit.id
       @note.noteable == commit
     end
 
@@ -104,13 +104,13 @@ describe Note do
   describe "Pre-line commit notes" do
     before do
       @note = create(:note,
-                     noteable_id: commit.id,
+                     commit_id: commit.id,
                      noteable_type: "Commit",
                      line_code: "0_16_1")
     end
 
     it "should save a valid note" do
-      @note.noteable_id.should == commit.id
+      @note.commit_id.should == commit.id
       @note.noteable.id.should == commit.id
     end
 

spec/requests/issues_spec.rb

@@ -91,13 +91,13 @@ describe "Issues" do
                title: title)
       end
 
-      issue = Issue.first # with title 'foobar'
-      issue.milestone = create(:milestone, project: project)
-      issue.assignee = nil
-      issue.save
+      @issue = Issue.first # with title 'foobar'
+      @issue.milestone = create(:milestone, project: project)
+      @issue.assignee = nil
+      @issue.save
     end
 
-    let(:issue) { Issue.first }
+    let(:issue) { @issue }
 
     it "should allow filtering by issues with no specified milestone" do
       visit project_issues_path(project, milestone_id: '0')