diff options
25 files changed, 47 insertions, 183 deletions
diff --git a/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js index fc2f20e3bcb..eb76b7d15fd 100644 --- a/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js +++ b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js @@ -42,10 +42,14 @@ import Vue from 'vue'; } }, created() { - this.discussion = CommentsStore.state[this.discussionId]; + if (this.discussionId) { + this.discussion = CommentsStore.state[this.discussionId]; + } }, mounted: function () { - const $textarea = $(`#new-discussion-note-form-${this.discussionId} .note-textarea`); + if (!this.discussionId) return; + + const $textarea = $(`.js-discussion-note-form[data-discussion-id=${this.discussionId}] .note-textarea`); this.textareaIsEmpty = $textarea.val() === ''; $textarea.on('input.comment-and-resolve-btn', () => { @@ -53,7 +57,9 @@ import Vue from 'vue'; }); }, destroyed: function () { - $(`#new-discussion-note-form-${this.discussionId} .note-textarea`).off('input.comment-and-resolve-btn'); + if (!this.discussionId) return; + + $(`.js-discussion-note-form[data-discussion-id=${this.discussionId}] .note-textarea`).off('input.comment-and-resolve-btn'); } }); diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js index 7f91bdd38ab..59d6508fc02 100644 --- a/app/assets/javascripts/files_comment_button.js +++ b/app/assets/javascripts/files_comment_button.js @@ -131,7 +131,7 @@ window.FilesCommentButton = (function() { }; FilesCommentButton.prototype.validateLineContent = function(lineContentElement) { - return lineContentElement.attr('data-discussion-id') && lineContentElement.attr('data-discussion-id') !== ''; + return lineContentElement.attr('data-note-type') && lineContentElement.attr('data-note-type') !== ''; }; return FilesCommentButton; diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 08bbc15f6b9..57335c77e40 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -272,10 +272,10 @@ require('./task_list'); Note: for rendering inline notes use renderDiscussionNote */ - Notes.prototype.renderNote = function(note) { + Notes.prototype.renderNote = function(note, $form) { var $notesList; if (note.discussion_html != null) { - return this.renderDiscussionNote(note); + return this.renderDiscussionNote(note, $form); } if (!note.valid) { @@ -317,16 +317,13 @@ require('./task_list'); Note: for rendering inline notes use renderDiscussionNote */ - Notes.prototype.renderDiscussionNote = function(note) { + Notes.prototype.renderDiscussionNote = function(note, $form) { var discussionContainer, form, note_html, row, lineType, diffAvatarContainer; if (!this.isNewNote(note)) { return; } this.note_ids.push(note.id); - form = $(".js-discussion-note-form[data-discussion-id='" + note.discussion_id + "']"); - if (form.length === 0) { - form = $(".js-discussion-note-form[data-original-discussion-id='" + note.original_discussion_id + "']"); - } + form = $form || $(".js-discussion-note-form[data-discussion-id='" + note.discussion_id + "']"); row = form.closest("tr"); lineType = this.isParallelView() ? form.find('#line_type').val() : 'old'; diffAvatarContainer = row.prevAll('.line_holder').first().find('.js-avatar-container.' + lineType + '_line'); @@ -334,8 +331,8 @@ require('./task_list'); note_html.renderGFM(); // is this the first note of discussion? discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']"); - if (discussionContainer.length === 0) { - discussionContainer = $(".notes[data-original-discussion-id='" + note.original_discussion_id + "']"); + if (!discussionContainer.length) { + discussionContainer = form.closest('.discussion').find('.notes'); } if (discussionContainer.length === 0) { if (!this.isParallelView() || row.hasClass('js-temp-notes-holder')) { @@ -525,7 +522,7 @@ require('./task_list'); } } - this.renderNote(note); + this.renderNote(note, $form); // cleanup after successfully creating a diff/discussion note this.removeDiscussionNoteForm($form); }; @@ -749,13 +746,13 @@ require('./task_list'); // setup note target var discussionID = dataHolder.data("discussionId"); - form.attr('id', "new-discussion-note-form-" + discussionID); - form.attr("data-discussion-id", discussionID); - form.attr("data-original-discussion-id", dataHolder.data("originalDiscussionId") || discussionID); - form.attr("data-line-code", dataHolder.data("lineCode")); + if (discussionID) { + form.attr("data-discussion-id", discussionID); + form.find("#in_reply_to_discussion_id").val(discussionID); + } + form.attr("data-line-code", dataHolder.data("lineCode")); form.find("#line_type").val(dataHolder.data("lineType")); - form.find("#in_reply_to_discussion_id").val(dataHolder.data("originalDiscussionId")); form.find("#note_noteable_type").val(dataHolder.data("noteableType")); form.find("#note_noteable_id").val(dataHolder.data("noteableId")); @@ -775,8 +772,7 @@ require('./task_list'); if (typeof gl.diffNotesCompileComponents !== 'undefined') { var $commentBtn = form.find('comment-and-resolve-btn'); - $commentBtn - .attr(':discussion-id', "'" + dataHolder.data('discussionId') + "'"); + $commentBtn.attr(':discussion-id', "'" + discussionID + "'"); gl.diffNotesCompileComponents(); } @@ -784,7 +780,7 @@ require('./task_list'); form.find(".js-note-text").focus(); form .find('.js-comment-resolve-button') - .attr('data-discussion-id', dataHolder.data('discussionId')); + .attr('data-discussion-id', discussionID); form .removeClass('js-main-target-form') .addClass("discussion-form js-discussion-note-form"); diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 8f82021a464..fb9195bb08d 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -173,12 +173,7 @@ class Projects::NotesController < Projects::ApplicationController discussion_resolvable: discussion.resolvable?, diff_discussion_html: diff_discussion_html(discussion), - discussion_html: discussion_html(discussion), - - # Since the `discussion_id` can change, for example when new commits are pushed into an MR, - # the never-changing `original_discussion_id` is used as a fallback to the find the relevant - # discussion container to add this note to. - original_discussion_id: note.original_discussion_id + discussion_html: discussion_html(discussion) ) end else diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index e2fa9905e86..61805021b39 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -53,23 +53,19 @@ module NotesHelper } if use_legacy_diff_note - new_note = LegacyDiffNote.new(@new_diff_note_attrs.merge(line_code: line_code)) + data[:note_type] = LegacyDiffNote.name else - new_note = DiffNote.new(@new_diff_note_attrs.merge(position: position)) - + data[:note_type] = DiffNote.name data[:position] = position.to_json end - data.merge( - note_type: new_note.type, - discussion_id: new_note.discussion_class.discussion_id(new_note) - ) + data end def link_to_reply_discussion(discussion, line_type = nil) return unless current_user - data = { discussion_id: discussion.id, original_discussion_id: discussion.original_id, line_type: line_type } + data = { discussion_id: discussion.id, line_type: line_type } data[:line_code] = discussion.line_code if discussion.respond_to?(:line_code) button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button', diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index 7900af6aaac..d378152eb56 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -3,7 +3,7 @@ module Noteable notes end - delegate :find_discussion, :find_original_discussion, to: :discussion_notes + delegate :find_discussion, to: :discussion_notes def discussions @discussions ||= discussion_notes diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index e349f743fdd..8acb70eb7cb 100644 --- a/app/models/diff_discussion.rb +++ b/app/models/diff_discussion.rb @@ -6,14 +6,6 @@ class DiffDiscussion < Discussion to: :first_note - def self.build_discussion_id(note) - [*super(note), *note.position.key] - end - - def self.build_original_discussion_id(note) - [*Discussion.build_discussion_id(note), *note.original_position.key] - end - def legacy_diff_discussion? false end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 38477528279..6029fc42e9c 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -16,9 +16,6 @@ class DiffNote < Note before_validation :set_original_position, :update_position, on: :create before_validation :set_line_code - # We need to do this again, because it's already in `Note`, but is affected by - # `update_position` and needs to run after that. - before_validation :set_discussion_id, if: :position_changed? after_save :keep_around_commits def discussion_class(*) diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 27ed0480e6d..782db4044ed 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -34,24 +34,13 @@ class Discussion nil end - def self.build_discussion_id(note) + def self.build_discussion_id_base(note) noteable_id = note.noteable_id || note.commit_id [:discussion, note.noteable_type.try(:underscore), noteable_id] end - def self.original_discussion_id(note) - original_discussion_id = build_original_discussion_id(note) - if original_discussion_id - Digest::SHA1.hexdigest(original_discussion_id.join("-")) - else - note.discussion_id - end - end - - # Optionally build a separate original discussion ID that will never change, - # if the main discussion ID _can_ change, like in the case of DiffDiscussion. - def self.build_original_discussion_id(note) - nil + def self.build_discussion_id(note) + [*build_discussion_id_base(note), SecureRandom.hex] end def initialize(notes, noteable = nil) @@ -80,10 +69,6 @@ class Discussion alias_method :to_param, :id - def original_id - first_note.original_discussion_id - end - def diff_discussion? false end @@ -109,7 +94,7 @@ class Discussion end def reply_attributes - first_note.slice(:type, :noteable_type, :noteable_id, :commit_id) + first_note.slice(:type, :noteable_type, :noteable_id, :commit_id, :discussion_id) end private diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb index 585b8527883..ebcf60beaf3 100644 --- a/app/models/individual_note_discussion.rb +++ b/app/models/individual_note_discussion.rb @@ -1,8 +1,4 @@ class IndividualNoteDiscussion < Discussion - def self.build_discussion_id(note) - [*super(note), SecureRandom.hex] - end - # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote` def potentially_resolvable? false diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb index 53fe9fbab06..36612a28ba1 100644 --- a/app/models/legacy_diff_discussion.rb +++ b/app/models/legacy_diff_discussion.rb @@ -1,10 +1,6 @@ class LegacyDiffDiscussion < Discussion include DiscussionOnDiff - def self.build_discussion_id(note) - [*super(note), note.line_code] - end - def legacy_diff_discussion? true end diff --git a/app/models/note.rb b/app/models/note.rb index eef868a05d6..3db1656ba57 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -52,7 +52,7 @@ class Note < ActiveRecord::Base validates :noteable_id, presence: true, unless: [:for_commit?, :importing?] validates :commit_id, presence: true, if: :for_commit? validates :author, presence: true - validates :discussion_id, :original_discussion_id, presence: true, format: { with: /\A\h{40}\z/ } + validates :discussion_id, presence: true, format: { with: /\A\h{40}\z/ } validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note| unless note.noteable.try(:project) == note.project @@ -84,9 +84,9 @@ class Note < ActiveRecord::Base project: [:project_members, { group: [:group_members] }]) end - after_initialize :ensure_discussion_id, :ensure_original_discussion_id + after_initialize :ensure_discussion_id before_validation :nullify_blank_type, :nullify_blank_line_code - before_validation :set_discussion_id, :set_original_discussion_id, on: :create + before_validation :set_discussion_id, on: :create after_save :keep_around_commit, unless: :for_personal_snippet? after_save :expire_etag_cache @@ -99,13 +99,6 @@ class Note < ActiveRecord::Base Discussion.build_collection(fresh, noteable) end - def find_original_discussion(discussion_id) - note = find_by(original_discussion_id: discussion_id) - return unless note - - note.to_discussion - end - def find_discussion(discussion_id) notes = where(discussion_id: discussion_id).fresh.to_a return if notes.empty? @@ -309,20 +302,6 @@ class Note < ActiveRecord::Base self.discussion_id ||= discussion_class.discussion_id(self) end - def ensure_original_discussion_id - return unless self.persisted? - # Needed in case the SELECT statement doesn't ask for `original_discussion_id` - return unless self.has_attribute?(:original_discussion_id) - 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 = discussion_class.original_discussion_id(self) - end - def expire_etag_cache return unless for_issue? diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb index 0019064e25c..ecbfd97699f 100644 --- a/app/models/out_of_context_discussion.rb +++ b/app/models/out_of_context_discussion.rb @@ -2,7 +2,7 @@ class OutOfContextDiscussion < Discussion # To make sure all out-of-context notes are displayed in one discussion, # we override the discussion ID to be a newly generated but consistent ID. def self.override_discussion_id(note) - discussion_id(note) + Digest::SHA1.hexdigest(build_discussion_id_base(note).join("-")) end # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote` diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index 81fc2ddac77..7d65b2b7993 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -46,7 +46,7 @@ class SentNotification < ActiveRecord::Base end def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {}) - attrs[:in_reply_to_discussion_id] = note.original_discussion_id + attrs[:in_reply_to_discussion_id] = note.discussion_id record(note.noteable, recipient_id, reply_key, attrs) end diff --git a/app/models/simple_discussion.rb b/app/models/simple_discussion.rb index a5ef5c565e7..41c679daf2d 100644 --- a/app/models/simple_discussion.rb +++ b/app/models/simple_discussion.rb @@ -1,9 +1,2 @@ class SimpleDiscussion < Discussion - def self.build_discussion_id(note) - [*super(note), SecureRandom.hex] - end - - def reply_attributes - super.merge(discussion_id: self.id) - end end diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb index eddfcf5f391..4619ba552d7 100644 --- a/app/services/notes/build_service.rb +++ b/app/services/notes/build_service.rb @@ -5,9 +5,7 @@ module Notes new_discussion = params.delete(:new_discussion) if project && in_reply_to_discussion_id.present? - discussion = - project.notes.find_original_discussion(in_reply_to_discussion_id) || - project.notes.find_discussion(in_reply_to_discussion_id) + discussion = project.notes.find_discussion(in_reply_to_discussion_id) unless discussion note = Note.new diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml index 38112f67881..e04958817e4 100644 --- a/app/views/discussions/_discussion.html.haml +++ b/app/views/discussions/_discussion.html.haml @@ -5,7 +5,7 @@ = link_to user_path(discussion.author) do = image_tag avatar_icon(discussion.author), class: "avatar s40" .timeline-content - .discussion.js-toggle-container{ class: discussion.id, data: { discussion_id: discussion.id } } + .discussion.js-toggle-container{ data: { discussion_id: discussion.id } } .discussion-header .discussion-actions %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button" } diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml index d7b2abf1269..ad778612f92 100644 --- a/app/views/discussions/_notes.html.haml +++ b/app/views/discussions/_notes.html.haml @@ -1,4 +1,4 @@ -%ul.notes{ data: { discussion_id: discussion.id, original_discussion_id: discussion.original_id } } +%ul.notes{ data: { discussion_id: discussion.id } } = render partial: "projects/notes/note", collection: discussion.notes, as: :note - if current_user diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index fc728123c97..fa567dd9da4 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -42,7 +42,7 @@ module Gitlab def encode_with(coder) coder['attributes'] = self.to_h end - + def key @key ||= [base_sha, start_sha, head_sha, Digest::SHA1.hexdigest(old_path || ""), Digest::SHA1.hexdigest(new_path || ""), old_line, new_line] end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 1ad16a9b57d..4c3c7d8cce2 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -55,7 +55,6 @@ Note: - resolved_at - resolved_by_id - discussion_id -- original_discussion_id LabelLink: - id - label_id diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 533d38c0229..fb80b74b226 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -239,29 +239,4 @@ describe DiffNote, models: true do 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/note_spec.rb b/spec/models/note_spec.rb index 67b4fed5f8f..3cdabb2875f 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -245,20 +245,6 @@ describe Note, models: true do end end - describe '.find_original_discussion' do - let!(:note) { create(:discussion_note_on_merge_request) } - let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note) } - let(:merge_request) { note.noteable } - - it 'returns a discussion with one note' do - discussion = merge_request.notes.find_original_discussion(note.original_discussion_id) - - expect(discussion).not_to be_nil - expect(discussion.notes.count).to be(1) - expect(discussion.first_note.original_discussion_id).to eq(note.original_discussion_id) - end - end - describe '.find_discussion' do let!(:note) { create(:discussion_note_on_merge_request) } let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note) } @@ -499,31 +485,6 @@ describe Note, models: true do 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 - describe '#to_discussion' do subject { create(:discussion_note_on_merge_request) } let!(:note2) { create(:discussion_note_on_merge_request, project: subject.project, noteable: subject.noteable, in_reply_to: subject) } diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb index e482cafa831..6b7eef388be 100644 --- a/spec/models/sent_notification_spec.rb +++ b/spec/models/sent_notification_spec.rb @@ -12,7 +12,7 @@ describe SentNotification, model: true do end context "when the project doesn't match the discussion project" do - let(:discussion_id) { create(:note).original_discussion_id } + let(:discussion_id) { create(:note).discussion_id } subject { build(:sent_notification, in_reply_to_discussion_id: discussion_id) } it "is invalid" do @@ -23,7 +23,7 @@ describe SentNotification, model: true do context "when the noteable project and discussion project match" do let(:project) { create(:project) } let(:issue) { create(:issue, project: project) } - let(:discussion_id) { create(:note, project: project, noteable: issue).original_discussion_id } + let(:discussion_id) { create(:note, project: project, noteable: issue).discussion_id } subject { build(:sent_notification, project: project, noteable: issue, in_reply_to_discussion_id: discussion_id) } it "is valid" do diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb index 464b24cb447..f9dd5541b10 100644 --- a/spec/services/notes/build_service_spec.rb +++ b/spec/services/notes/build_service_spec.rb @@ -9,7 +9,7 @@ describe Notes::BuildService, services: true do context 'when in_reply_to_discussion_id is specified' do context 'when a note with that original discussion ID exists' do it 'sets the note up to be in reply to that note' do - new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.original_discussion_id).execute + new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute expect(new_note).to be_valid expect(new_note.in_reply_to?(note)).to be_truthy end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index c4e00fcf080..617c8eaf3d5 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -439,7 +439,7 @@ describe NotificationService, services: true do notification.new_note(note) - expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.original_discussion_id) + expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.discussion_id) end end end |