diff options
author | Douwe Maan <douwe@selenight.nl> | 2016-08-17 20:14:44 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2016-08-17 20:16:46 +0300 |
commit | 4a13aa9f34ab4114bc485e1ca8fa0db8daa0394b (patch) | |
tree | 17554a901009603f52be08914636495b06db2e68 | |
parent | f3acf9fd248a16665a114bf6cce761e9277c2d5b (diff) |
Store discussion_id on Note for faster discussion lookup.
-rw-r--r-- | app/controllers/projects/discussions_controller.rb | 6 | ||||
-rw-r--r-- | app/helpers/notes_helper.rb | 8 | ||||
-rw-r--r-- | app/models/diff_note.rb | 37 | ||||
-rw-r--r-- | app/models/discussion.rb | 4 | ||||
-rw-r--r-- | app/models/legacy_diff_note.rb | 12 | ||||
-rw-r--r-- | app/models/merge_request.rb | 15 | ||||
-rw-r--r-- | app/models/note.rb | 35 | ||||
-rw-r--r-- | db/migrate/20160817154936_add_discussion_ids_to_notes.rb | 13 | ||||
-rw-r--r-- | db/schema.rb | 6 | ||||
-rw-r--r-- | spec/models/diff_note_spec.rb | 50 | ||||
-rw-r--r-- | spec/models/legacy_diff_note_spec.rb | 25 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/note_spec.rb | 25 |
13 files changed, 191 insertions, 47 deletions
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 |