Commit 4a13aa9f authored by Douwe Maan's avatar Douwe Maan

Store discussion_id on Note for faster discussion lookup.

parent f3acf9fd
......@@ -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!
......
......@@ -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
......
......@@ -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?
......
......@@ -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)
......
......@@ -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
......@@ -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
......
......@@ -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
# 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
......@@ -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"
......@@ -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
......
......@@ -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
......@@ -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
......@@ -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
......
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment