From 4a13aa9f34ab4114bc485e1ca8fa0db8daa0394b Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Wed, 17 Aug 2016 12:14:44 -0500
Subject: Store discussion_id on Note for faster discussion lookup.

---
 app/controllers/projects/discussions_controller.rb |  6 +--
 app/helpers/notes_helper.rb                        |  8 ++--
 app/models/diff_note.rb                            | 37 ++++++++++------
 app/models/discussion.rb                           |  4 +-
 app/models/legacy_diff_note.rb                     | 12 +++---
 app/models/merge_request.rb                        | 15 +++++--
 app/models/note.rb                                 | 35 +++++++++++----
 .../20160817154936_add_discussion_ids_to_notes.rb  | 13 ++++++
 db/schema.rb                                       |  6 ++-
 spec/models/diff_note_spec.rb                      | 50 ++++++++++++++++++++++
 spec/models/legacy_diff_note_spec.rb               | 25 +++++++++++
 spec/models/merge_request_spec.rb                  |  2 +-
 spec/models/note_spec.rb                           | 25 +++++++++++
 13 files changed, 191 insertions(+), 47 deletions(-)
 create mode 100644 db/migrate/20160817154936_add_discussion_ids_to_notes.rb

diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb
index 620b41b1011..b2e8733ccb7 100644
--- a/app/controllers/projects/discussions_controller.rb
+++ b/app/controllers/projects/discussions_controller.rb
@@ -5,8 +5,6 @@ class Projects::DiscussionsController < Projects::ApplicationController
   before_action :authorize_resolve_discussion!
 
   def resolve
-    return render_404 unless discussion.resolvable?
-
     discussion.resolve!(current_user)
 
     MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
@@ -18,8 +16,6 @@ class Projects::DiscussionsController < Projects::ApplicationController
   end
 
   def unresolve
-    return render_404 unless discussion.resolvable?
-
     discussion.unresolve!
 
     render json: {
@@ -34,7 +30,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
   end
 
   def discussion
-    @discussion ||= @merge_request.find_discussion(params[:id]) || render_404
+    @discussion ||= @merge_request.find_diff_discussion(params[:id]) || render_404
   end
 
   def authorize_resolve_discussion!
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 6b00ebc98e7..da230f76bae 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -49,7 +49,7 @@ module NotesHelper
     }
 
     if use_legacy_diff_note
-      discussion_id = LegacyDiffNote.build_discussion_id(
+      discussion_id = LegacyDiffNote.discussion_id(
         @comments_target[:noteable_type],
         @comments_target[:noteable_id] || @comments_target[:commit_id],
         line_code
@@ -57,10 +57,10 @@ module NotesHelper
 
       data.merge!(
         note_type: LegacyDiffNote.name,
-        discussion_id: Digest::SHA1.hexdigest(discussion_id)
+        discussion_id: discussion_id
       )
     else
-      discussion_id = DiffNote.build_discussion_id(
+      discussion_id = DiffNote.discussion_id(
         @comments_target[:noteable_type],
         @comments_target[:noteable_id] || @comments_target[:commit_id],
         position
@@ -69,7 +69,7 @@ module NotesHelper
       data.merge!(
         position: position.to_json,
         note_type: DiffNote.name,
-        discussion_id: Digest::SHA1.hexdigest(discussion_id)
+        discussion_id: discussion_id
       )
     end
 
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index cfcdce790e5..1313739f322 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -15,8 +15,9 @@ class DiffNote < Note
   validate :positions_complete
   validate :verify_supported
 
+  after_initialize :ensure_original_discussion_id
   before_validation :set_original_position, :update_position, on: :create
-  before_validation :set_line_code
+  before_validation :set_line_code, :set_original_discussion_id
   after_save :keep_around_commits
 
   class << self
@@ -33,14 +34,6 @@ class DiffNote < Note
     { position: position.to_json }
   end
 
-  def discussion_id
-    @discussion_id ||= Digest::SHA1.hexdigest(self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position))
-  end
-
-  def original_discussion_id
-    @original_discussion_id ||= Digest::SHA1.hexdigest(self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position))
-  end
-
   def position=(new_position)
     if new_position.is_a?(String)
       new_position = JSON.parse(new_position) rescue nil
@@ -106,11 +99,7 @@ class DiffNote < Note
   def discussion
     return unless resolvable?
 
-    discussion_notes = self.noteable.notes.fresh.select { |n| n.discussion_id == self.discussion_id }
-
-    return if discussion_notes.empty?
-
-    Discussion.new(discussion_notes)
+    self.noteable.find_diff_discussion(self.discussion_id)
   end
 
   def to_discussion
@@ -139,6 +128,26 @@ class DiffNote < Note
     self.line_code = self.position.line_code(self.project.repository)
   end
 
+  def ensure_original_discussion_id
+    return unless self.persisted?
+    return if self.original_discussion_id
+
+    set_original_discussion_id
+    update_column(:original_discussion_id, self.original_discussion_id)
+  end
+
+  def set_original_discussion_id
+    self.original_discussion_id = Digest::SHA1.hexdigest(build_original_discussion_id)
+  end
+
+  def build_discussion_id
+    self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position)
+  end
+
+  def build_original_discussion_id
+    self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position)
+  end
+
   def update_position
     return unless supported?
     return if for_commit?
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index b26d2282b00..3fddc084af2 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -57,7 +57,7 @@ class Discussion
   def id
     first_note.discussion_id
   end
-  
+
   alias_method :to_param, :id
 
   def diff_discussion?
@@ -93,7 +93,7 @@ class Discussion
     return false unless resolvable?
 
     current_user == self.noteable.author ||
-      current_user.can?(:push_code, self.project)
+      current_user.can?(:resolve_note, self.project)
   end
 
   def resolve!(current_user)
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index f15553d706e..8e26cbe9835 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -8,8 +8,8 @@ class LegacyDiffNote < Note
   before_create :set_diff
 
   class << self
-    def build_discussion_id(noteable_type, noteable_id, line_code, active = true)
-      [super(noteable_type, noteable_id), line_code, active].join("-")
+    def build_discussion_id(noteable_type, noteable_id, line_code)
+      [super(noteable_type, noteable_id), line_code].join("-")
     end
   end
 
@@ -21,10 +21,6 @@ class LegacyDiffNote < Note
     { line_code: line_code }
   end
 
-  def discussion_id
-    @discussion_id ||= Digest::SHA1.hexdigest(self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code))
-  end
-
   def project_repository
     if RequestStore.active?
       RequestStore.fetch("project:#{project_id}:repository") { self.project.repository }
@@ -119,4 +115,8 @@ class LegacyDiffNote < Note
     diffs = noteable.raw_diffs(Commit.max_diff_options)
     diffs.find { |d| d.new_path == self.diff.new_path }
   end
+
+  def build_discussion_id
+    self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code)
+  end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 5bb48117851..31badb54188 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -425,16 +425,23 @@ class MergeRequest < ActiveRecord::Base
       discussions
   end
 
-  def find_discussion(discussion_id)
-    discussions.find { |d| d.id == discussion_id }
+  def diff_discussions
+    @diff_discussions ||= self.notes.diff_notes.discussions
+  end
+
+  def find_diff_discussion(discussion_id)
+    notes = self.notes.diff_notes.where(discussion_id: discussion_id).fresh.to_a
+    return if notes.empty?
+
+    Discussion.new(notes)
   end
 
   def discussions_resolvable?
-    discussions.any?(&:resolvable?)
+    diff_discussions.any?(&:resolvable?)
   end
 
   def discussions_resolved?
-    discussions_resolvable? && discussions.none?(&:to_be_resolved?)
+    discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?)
   end
 
   def hook_attrs
diff --git a/app/models/note.rb b/app/models/note.rb
index 732b2ff9bf7..fb0d0ebc396 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -70,7 +70,9 @@ class Note < ActiveRecord::Base
              project: [:project_members, { group: [:group_members] }])
   end
 
+  after_initialize :ensure_discussion_id
   before_validation :nullify_blank_type, :nullify_blank_line_code
+  before_validation :set_discussion_id
   after_save :keep_around_commit
 
   class << self
@@ -82,6 +84,10 @@ class Note < ActiveRecord::Base
       [:discussion, noteable_type.try(:underscore), noteable_id].join("-")
     end
 
+    def self.discussion_id(*args)
+      Digest::SHA1.hexdigest(build_discussion_id(*args))
+    end
+
     def discussions
       Discussion.for_notes(all)
     end
@@ -142,15 +148,6 @@ class Note < ActiveRecord::Base
     resolvable? && !resolved?
   end
 
-  def discussion_id
-    @discussion_id ||=
-      if for_merge_request?
-        Digest::SHA1.hexdigest([:discussion, :note, id].join("-"))
-      else
-        Digest::SHA1.hexdigest(self.class.build_discussion_id(noteable_type, noteable_id || commit_id))
-      end
-  end
-
   def max_attachment_size
     current_application_settings.max_attachment_size.megabytes.to_i
   end
@@ -256,4 +253,24 @@ class Note < ActiveRecord::Base
   def nullify_blank_line_code
     self.line_code = nil if self.line_code.blank?
   end
+
+  def ensure_discussion_id
+    return unless self.persisted?
+    return if self.discussion_id
+
+    set_discussion_id
+    update_column(:discussion_id, self.discussion_id)
+  end
+
+  def set_discussion_id
+    self.discussion_id = Digest::SHA1.hexdigest(build_discussion_id)
+  end
+
+  def build_discussion_id
+    if for_merge_request?
+      [:discussion, :note, id].join("-")
+    else
+      self.class.build_discussion_id(noteable_type, noteable_id || commit_id)
+    end
+  end
 end
diff --git a/db/migrate/20160817154936_add_discussion_ids_to_notes.rb b/db/migrate/20160817154936_add_discussion_ids_to_notes.rb
new file mode 100644
index 00000000000..61facce665a
--- /dev/null
+++ b/db/migrate/20160817154936_add_discussion_ids_to_notes.rb
@@ -0,0 +1,13 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddDiscussionIdsToNotes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :notes, :discussion_id, :string
+    add_column :notes, :original_discussion_id, :string
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 8d8555528b8..0bc1c967af3 100644
--- a/db/schema.rb
+++ b/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: 20160810142633) do
+ActiveRecord::Schema.define(version: 20160817154936) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -663,7 +663,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do
     t.string   "line_code"
     t.string   "commit_id"
     t.integer  "noteable_id"
-    t.boolean  "system",            default: false, null: false
+    t.boolean  "system",                 default: false, null: false
     t.text     "st_diff"
     t.integer  "updated_by_id"
     t.string   "type"
@@ -671,6 +671,8 @@ ActiveRecord::Schema.define(version: 20160810142633) do
     t.text     "original_position"
     t.datetime "resolved_at"
     t.integer  "resolved_by_id"
+    t.string   "discussion_id"
+    t.string   "original_discussion_id"
   end
 
   add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 6b8e6577cfb..6a640474cfe 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -434,4 +434,54 @@ describe DiffNote, models: true do
       end
     end
   end
+
+  describe "#discussion_id" do
+    let(:note) { create(:diff_note_on_merge_request) }
+
+    context "when it is newly created" do
+      it "has a discussion id" do
+        expect(note.discussion_id).not_to be_nil
+        expect(note.discussion_id).to match(/\A\h{40}\z/)
+      end
+    end
+
+    context "when it didn't store a discussion id before" do
+      before do
+        note.update_column(:discussion_id, nil)
+      end
+
+      it "has a discussion id" do
+        # The discussion_id is set in `after_initialize`, so `reload` won't work
+        reloaded_note = Note.find(note.id)
+
+        expect(reloaded_note.discussion_id).not_to be_nil
+        expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/)
+      end
+    end
+  end
+
+  describe "#original_discussion_id" do
+    let(:note) { create(:diff_note_on_merge_request) }
+
+    context "when it is newly created" do
+      it "has a discussion id" do
+        expect(note.original_discussion_id).not_to be_nil
+        expect(note.original_discussion_id).to match(/\A\h{40}\z/)
+      end
+    end
+
+    context "when it didn't store a discussion id before" do
+      before do
+        note.update_column(:original_discussion_id, nil)
+      end
+
+      it "has a discussion id" do
+        # The original_discussion_id is set in `after_initialize`, so `reload` won't work
+        reloaded_note = Note.find(note.id)
+
+        expect(reloaded_note.original_discussion_id).not_to be_nil
+        expect(reloaded_note.original_discussion_id).to match(/\A\h{40}\z/)
+      end
+    end
+  end
 end
diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb
index 2cfd26419ca..81517a18b74 100644
--- a/spec/models/legacy_diff_note_spec.rb
+++ b/spec/models/legacy_diff_note_spec.rb
@@ -73,4 +73,29 @@ describe LegacyDiffNote, models: true do
       end
     end
   end
+
+  describe "#discussion_id" do
+    let(:note) { create(:note) }
+
+    context "when it is newly created" do
+      it "has a discussion id" do
+        expect(note.discussion_id).not_to be_nil
+        expect(note.discussion_id).to match(/\A\h{40}\z/)
+      end
+    end
+
+    context "when it didn't store a discussion id before" do
+      before do
+        note.update_column(:discussion_id, nil)
+      end
+
+      it "has a discussion id" do
+        # The discussion_id is set in `after_initialize`, so `reload` won't work
+        reloaded_note = Note.find(note.id)
+
+        expect(reloaded_note.discussion_id).not_to be_nil
+        expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/)
+      end
+    end
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index b9bec76fdbd..00c528c8cde 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -763,7 +763,7 @@ describe MergeRequest, models: true do
     let(:third_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) }
 
     before do
-      allow(subject).to receive(:discussions).and_return([first_discussion, second_discussion, third_discussion])
+      allow(subject).to receive(:diff_discussions).and_return([first_discussion, second_discussion, third_discussion])
     end
 
     describe "#discussions_resolvable?" do
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 9f55fd1d53c..ef2747046b9 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -321,4 +321,29 @@ describe Note, models: true do
       expect(subject[active_diff_note3.line_code].id).to eq(active_diff_note3.discussion_id)
     end
   end
+
+  describe "#discussion_id" do
+    let(:note) { create(:note) }
+
+    context "when it is newly created" do
+      it "has a discussion id" do
+        expect(note.discussion_id).not_to be_nil
+        expect(note.discussion_id).to match(/\A\h{40}\z/)
+      end
+    end
+
+    context "when it didn't store a discussion id before" do
+      before do
+        note.update_column(:discussion_id, nil)
+      end
+
+      it "has a discussion id" do
+        # The discussion_id is set in `after_initialize`, so `reload` won't work
+        reloaded_note = Note.find(note.id)
+
+        expect(reloaded_note.discussion_id).not_to be_nil
+        expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/)
+      end
+    end
+  end
 end
-- 
cgit v1.2.1