Don't use original_discussion_id

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');
     }
   });
 

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;

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");

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

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',

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

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

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(*)

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

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

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

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?
 

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`

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

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

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

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" }

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

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

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

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

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) }

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

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

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