Allow commenting on older versions of the diff and comparisons between diff versionsdm-link-discussion-to-outdated-diff

7 files changed, 431 insertions, 48 deletions

diff --git a/spec/features/merge_requests/versions_spec.rb b/spec/features/merge_requests/versions_spec.rb
index 7a2da623c58..2b5b803946c 100644
--- a/spec/features/merge_requests/versions_spec.rb
+++ b/spec/features/merge_requests/versions_spec.rb
@@ -24,7 +24,12 @@ feature 'Merge Request versions', js: true, feature: true do
     before do
       page.within '.mr-version-dropdown' do
         find('.btn-default').click
-        find(:link, 'version 1').trigger('click')
+        click_link 'version 1'
+      end
+
+      # Wait for the page to load
+      page.within '.mr-version-dropdown' do
+        expect(page).to have_content 'version 1'
       end
     end
 
@@ -36,8 +41,8 @@ feature 'Merge Request versions', js: true, feature: true do
       expect(page).to have_content '5 changed files'
     end
 
-    it 'show the message about disabled comment creation' do
-      expect(page).to have_content 'comment creation is disabled'
+    it 'show the message about comments' do
+      expect(page).to have_content 'Not all comments are displayed'
     end
 
     it 'shows comments that were last relevant at that version' do
@@ -52,15 +57,41 @@ feature 'Merge Request versions', js: true, feature: true do
       outdated_diff_note.position = outdated_diff_note.original_position
       outdated_diff_note.save!
 
+      visit current_url
+
       expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']")
     end
+
+    it 'allows commenting' do
+      diff_file_selector = ".diff-file[id='7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44']"
+      line_code = '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_2_2'
+
+      page.within(diff_file_selector) do
+        find(".line_holder[id='#{line_code}'] td:nth-of-type(1)").trigger 'mouseover'
+        find(".line_holder[id='#{line_code}'] button").trigger 'click'
+
+        page.within("form[data-line-code='#{line_code}']") do
+          fill_in "note[note]", with: "Typo, please fix"
+          find(".js-comment-button").click
+        end
+
+        wait_for_ajax
+
+        expect(page).to have_content("Typo, please fix")
+      end
+    end
   end
 
   describe 'compare with older version' do
     before do
       page.within '.mr-version-compare-dropdown' do
         find('.btn-default').click
-        find(:link, 'version 1').trigger('click')
+        click_link 'version 1'
+      end
+
+      # Wait for the page to load
+      page.within '.mr-version-compare-dropdown' do
+        expect(page).to have_content 'version 1'
       end
     end
 
@@ -80,8 +111,43 @@ feature 'Merge Request versions', js: true, feature: true do
       end
     end
 
-    it 'show the message about disabled comments' do
-      expect(page).to have_content 'Comments are disabled'
+    it 'show the message about comments' do
+      expect(page).to have_content 'Not all comments are displayed'
+    end
+
+    it 'shows comments that were last relevant at that version' do
+      position = Gitlab::Diff::Position.new(
+        old_path: ".gitmodules",
+        new_path: ".gitmodules",
+        old_line: 4,
+        new_line: 4,
+        diff_refs: merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs
+      )
+      outdated_diff_note = create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position)
+
+      visit current_url
+      wait_for_ajax
+
+      expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']")
+    end
+
+    it 'allows commenting' do
+      diff_file_selector = ".diff-file[id='7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44']"
+      line_code = '7445606fbf8f3683cd42bdc54b05d7a0bc2dfc44_4_4'
+
+      page.within(diff_file_selector) do
+        find(".line_holder[id='#{line_code}'] td:nth-of-type(1)").trigger 'mouseover'
+        find(".line_holder[id='#{line_code}'] button").trigger 'click'
+
+        page.within("form[data-line-code='#{line_code}']") do
+          fill_in "note[note]", with: "Typo, please fix"
+          find(".js-comment-button").click
+        end
+
+        wait_for_ajax
+
+        expect(page).to have_content("Typo, please fix")
+      end
     end
 
     it 'show diff between new and old version' do
diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb
index a427de32c4c..6c990f94175 100644
--- a/spec/helpers/notes_helper_spec.rb
+++ b/spec/helpers/notes_helper_spec.rb
@@ -1,6 +1,8 @@
 require "spec_helper"
 
 describe NotesHelper do
+  include RepoHelpers
+
   let(:owner) { create(:owner) }
   let(:group) { create(:group) }
   let(:project) { create(:empty_project, namespace: group) }
@@ -36,4 +38,141 @@ describe NotesHelper do
       expect(helper.note_max_access_for_user(other_note)).to eq('Reporter')
     end
   end
+
+  describe '#discussion_path' do
+    let(:project) { create(:project) }
+
+    context 'for a merge request discusion' do
+      let(:merge_request) { create(:merge_request, source_project: project, target_project: project, importing: true) }
+      let!(:merge_request_diff1) { merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
+      let!(:merge_request_diff2) { merge_request.merge_request_diffs.create(head_commit_sha: nil) }
+      let!(:merge_request_diff3) { merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
+
+      context 'for a diff discussion' do
+        context 'when the discussion is active' do
+          let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
+
+          it 'returns the diff path with the line code' do
+            expect(helper.discussion_path(discussion)).to eq(diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, anchor: discussion.line_code))
+          end
+        end
+
+        context 'when the discussion is on an older merge request version' do
+          let(:position) do
+            Gitlab::Diff::Position.new(
+              old_path: ".gitmodules",
+              new_path: ".gitmodules",
+              old_line: nil,
+              new_line: 4,
+              diff_refs: merge_request_diff1.diff_refs
+            )
+          end
+
+          let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, position: position) }
+          let(:discussion) { diff_note.to_discussion }
+
+          before do
+            diff_note.position = diff_note.original_position
+            diff_note.save!
+          end
+
+          it 'returns the diff version path with the line code' do
+            expect(helper.discussion_path(discussion)).to eq(diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, diff_id: merge_request_diff1, anchor: discussion.line_code))
+          end
+        end
+
+        context 'when the discussion is on a comparison between merge request versions' do
+          let(:position) do
+            Gitlab::Diff::Position.new(
+              old_path: ".gitmodules",
+              new_path: ".gitmodules",
+              old_line: 4,
+              new_line: 4,
+              diff_refs: merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs
+            )
+          end
+
+          let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, position: position).to_discussion }
+
+          it 'returns the diff version comparison path with the line code' do
+            expect(helper.discussion_path(discussion)).to eq(diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, diff_id: merge_request_diff3, start_sha: merge_request_diff1.head_commit_sha, anchor: discussion.line_code))
+          end
+        end
+
+        context 'when the discussion does not have a merge request version' do
+          let(:outdated_diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, diff_refs: project.commit(sample_commit.id).diff_refs) }
+          let(:discussion) { outdated_diff_note.to_discussion }
+
+          before do
+            outdated_diff_note.position = outdated_diff_note.original_position
+            outdated_diff_note.save!
+          end
+
+          it 'returns nil' do
+            expect(helper.discussion_path(discussion)).to be_nil
+          end
+        end
+      end
+
+      context 'for a legacy diff discussion' do
+        let(:discussion) { create(:legacy_diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
+
+        context 'when the discussion is active' do
+          before do
+            allow(discussion).to receive(:active?).and_return(true)
+          end
+
+          it 'returns the diff path with the line code' do
+            expect(helper.discussion_path(discussion)).to eq(diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, anchor: discussion.line_code))
+          end
+        end
+
+        context 'when the discussion is outdated' do
+          before do
+            allow(discussion).to receive(:active?).and_return(false)
+          end
+
+          it 'returns nil' do
+            expect(helper.discussion_path(discussion)).to be_nil
+          end
+        end
+      end
+
+      context 'for a non-diff discussion' do
+        let(:discussion) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
+
+        it 'returns nil' do
+          expect(helper.discussion_path(discussion)).to be_nil
+        end
+      end
+    end
+
+    context 'for a commit discussion' do
+      let(:commit) { discussion.noteable }
+
+      context 'for a diff discussion' do
+        let(:discussion) { create(:diff_note_on_commit, project: project).to_discussion }
+
+        it 'returns the commit path with the line code' do
+          expect(helper.discussion_path(discussion)).to eq(namespace_project_commit_path(project.namespace, project, commit, anchor: discussion.line_code))
+        end
+      end
+
+      context 'for a legacy diff discussion' do
+        let(:discussion) { create(:legacy_diff_note_on_commit, project: project).to_discussion }
+
+        it 'returns the commit path with the line code' do
+          expect(helper.discussion_path(discussion)).to eq(namespace_project_commit_path(project.namespace, project, commit, anchor: discussion.line_code))
+        end
+      end
+
+      context 'for a non-diff discussion' do
+        let(:discussion) { create(:discussion_note_on_commit, project: project).to_discussion }
+
+        it 'returns the commit path' do
+          expect(helper.discussion_path(discussion)).to eq(namespace_project_commit_path(project.namespace, project, commit))
+        end
+      end
+    end
+  end
 end
diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb
index 48e7c0a822c..81f338745b1 100644
--- a/spec/models/diff_discussion_spec.rb
+++ b/spec/models/diff_discussion_spec.rb
@@ -1,19 +1,86 @@
 require 'spec_helper'
 
 describe DiffDiscussion, model: true do
-  subject { described_class.new([first_note, second_note, third_note]) }
+  include RepoHelpers
 
-  let(:first_note) { create(:diff_note_on_merge_request) }
-  let(:merge_request) { first_note.noteable }
-  let(:project) { first_note.project }
-  let(:second_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
-  let(:third_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
+  subject { described_class.new([diff_note]) }
+
+  let(:project) { create(:project) }
+  let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+  let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
 
   describe '#reply_attributes' do
     it 'includes position and original_position' do
       attributes = subject.reply_attributes
-      expect(attributes[:position]).to eq(first_note.position.to_json)
-      expect(attributes[:original_position]).to eq(first_note.original_position.to_json)
+      expect(attributes[:position]).to eq(diff_note.position.to_json)
+      expect(attributes[:original_position]).to eq(diff_note.original_position.to_json)
+    end
+  end
+
+  describe '#merge_request_version_params' do
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project, importing: true) }
+    let!(:merge_request_diff1) { merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
+    let!(:merge_request_diff2) { merge_request.merge_request_diffs.create(head_commit_sha: nil) }
+    let!(:merge_request_diff3) { merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
+
+    context 'when the discussion is active' do
+      it 'returns an empty hash, which will end up showing the latest version' do
+        expect(subject.merge_request_version_params).to eq({})
+      end
+    end
+
+    context 'when the discussion is on an older merge request version' do
+      let(:position) do
+        Gitlab::Diff::Position.new(
+          old_path: ".gitmodules",
+          new_path: ".gitmodules",
+          old_line: nil,
+          new_line: 4,
+          diff_refs: merge_request_diff1.diff_refs
+        )
+      end
+
+      let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, position: position) }
+
+      before do
+        diff_note.position = diff_note.original_position
+        diff_note.save!
+      end
+
+      it 'returns the diff ID for the version to show' do
+        expect(diff_id: merge_request_diff1.id)
+      end
+    end
+
+    context 'when the discussion is on a comparison between merge request versions' do
+      let(:position) do
+        Gitlab::Diff::Position.new(
+          old_path: ".gitmodules",
+          new_path: ".gitmodules",
+          old_line: 4,
+          new_line: 4,
+          diff_refs: merge_request_diff3.compare_with(merge_request_diff1.head_commit_sha).diff_refs
+        )
+      end
+
+      let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, position: position) }
+
+      it 'returns the diff ID and start sha of the versions to compare' do
+        expect(subject.merge_request_version_params).to eq(diff_id: merge_request_diff3.id, start_sha: merge_request_diff1.head_commit_sha)
+      end
+    end
+
+    context 'when the discussion does not have a merge request version' do
+      let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, diff_refs: project.commit(sample_commit.id).diff_refs) }
+
+      before do
+        diff_note.position = diff_note.original_position
+        diff_note.save!
+      end
+
+      it 'returns nil' do
+        expect(subject.merge_request_version_params).to be_nil
+      end
     end
   end
 end
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index f32b6b99b3d..ab4c51a87b0 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -155,23 +155,6 @@ describe DiffNote, models: true do
     end
   end
 
-  describe '#latest_merge_request_diff' do
-    context 'when active' do
-      it 'returns the current merge request diff' do
-        expect(subject.latest_merge_request_diff).to eq(merge_request.merge_request_diff)
-      end
-    end
-
-    context 'when outdated' do
-      let!(:old_merge_request_diff) { merge_request.merge_request_diff }
-      let!(:new_merge_request_diff) { merge_request.merge_request_diffs.create(diff_refs: commit.diff_refs) }
-
-      it 'returns the latest merge request diff that this diff note applied to' do
-        expect(subject.latest_merge_request_diff).to eq(old_merge_request_diff)
-      end
-    end
-  end
-
   describe "creation" do
     describe "updating of position" do
       context "when noteable is a commit" do
@@ -256,4 +239,39 @@ describe DiffNote, models: true do
       end
     end
   end
+
+  describe '#created_at_diff?' do
+    let(:diff_refs) { project.commit(sample_commit.id).diff_refs }
+    let(:position) do
+      Gitlab::Diff::Position.new(
+        old_path: "files/ruby/popen.rb",
+        new_path: "files/ruby/popen.rb",
+        old_line: nil,
+        new_line: 14,
+        diff_refs: diff_refs
+      )
+    end
+
+    context "when noteable is a commit" do
+      subject { build(:diff_note_on_commit, project: project, position: position) }
+
+      it "returns true" do
+        expect(subject.created_at_diff?(diff_refs)).to be true
+      end
+    end
+
+    context "when noteable is a merge request" do
+      context "when the diff refs match the original one of the diff note" do
+        it "returns true" do
+          expect(subject.created_at_diff?(diff_refs)).to be true
+        end
+      end
+
+      context "when the diff refs don't match the original one of the diff note" do
+        it "returns false" do
+          expect(subject.created_at_diff?(merge_request.diff_refs)).to be false
+        end
+      end
+    end
+  end
 end
diff --git a/spec/models/legacy_diff_discussion_spec.rb b/spec/models/legacy_diff_discussion_spec.rb
index 153e757a0ef..6eb4a2aaf39 100644
--- a/spec/models/legacy_diff_discussion_spec.rb
+++ b/spec/models/legacy_diff_discussion_spec.rb
@@ -8,4 +8,26 @@ describe LegacyDiffDiscussion, models: true do
       expect(subject.reply_attributes[:line_code]).to eq(subject.line_code)
     end
   end
+
+  describe '#merge_request_version_params' do
+    context 'when the discussion is active' do
+      before do
+        allow(subject).to receive(:active?).and_return(true)
+      end
+
+      it 'returns an empty hash, which will end up showing the latest version' do
+        expect(subject.merge_request_version_params).to eq({})
+      end
+    end
+
+    context 'when the discussion is outdated' do
+      before do
+        allow(subject).to receive(:active?).and_return(false)
+      end
+
+      it 'returns nil' do
+        expect(subject.merge_request_version_params).to be_nil
+      end
+    end
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index be08b96641a..8b72125dd5d 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1554,4 +1554,23 @@ describe MergeRequest, models: true do
       expect(subject.has_no_commits?).to be_truthy
     end
   end
+
+  describe '#merge_request_diff_for' do
+    subject { create(:merge_request, importing: true) }
+    let!(:merge_request_diff1) { subject.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') }
+    let!(:merge_request_diff2) { subject.merge_request_diffs.create(head_commit_sha: nil) }
+    let!(:merge_request_diff3) { subject.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
+
+    context 'with diff refs' do
+      it 'returns the diffs' do
+        expect(subject.merge_request_diff_for(merge_request_diff1.diff_refs)).to eq(merge_request_diff1)
+      end
+    end
+
+    context 'with a commit SHA' do
+      it 'returns the diffs' do
+        expect(subject.merge_request_diff_for(merge_request_diff3.head_commit_sha)).to eq(merge_request_diff3)
+      end
+    end
+  end
 end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 557ea97b008..7a01cef9b4b 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -272,9 +272,9 @@ describe Note, models: true do
       Gitlab::Diff::Position.new(
         old_path: "files/ruby/popen.rb",
         new_path: "files/ruby/popen.rb",
-        old_line: 16,
-        new_line: 22,
-        diff_refs: merge_request.diff_refs
+        old_line: nil,
+        new_line: 13,
+        diff_refs: project.commit(sample_commit.id).diff_refs
       )
     end
 
@@ -288,26 +288,78 @@ describe Note, models: true do
       )
     end
 
-    subject { merge_request.notes.grouped_diff_discussions }
+    context 'active diff discussions' do
+      subject { merge_request.notes.grouped_diff_discussions }
 
-    it "includes active discussions" do
-      discussions = subject.values.flatten
+      it "includes active discussions" do
+        discussions = subject.values.flatten
 
-      expect(discussions.count).to eq(2)
-      expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id])
-      expect(discussions.all?(&:active?)).to be true
+        expect(discussions.count).to eq(2)
+        expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id])
+        expect(discussions.all?(&:active?)).to be true
 
-      expect(discussions.first.notes).to eq([active_diff_note1, active_diff_note2])
-      expect(discussions.last.notes).to eq([active_diff_note3])
-    end
+        expect(discussions.first.notes).to eq([active_diff_note1, active_diff_note2])
+        expect(discussions.last.notes).to eq([active_diff_note3])
+      end
 
-    it "doesn't include outdated discussions" do
-      expect(subject.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
+      it "doesn't include outdated discussions" do
+        expect(subject.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
+      end
+
+      it "groups the discussions by line code" do
+        expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
+        expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
+      end
     end
 
-    it "groups the discussions by line code" do
-      expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
-      expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
+    context 'diff discussions for older diff refs' do
+      subject { merge_request.notes.grouped_diff_discussions(diff_refs) }
+
+      context 'for diff refs a discussion was created at' do
+        let(:diff_refs) { active_position2.diff_refs }
+
+        it "includes discussions that were created then" do
+          discussions = subject.values.flatten
+
+          expect(discussions.count).to eq(1)
+
+          discussion = discussions.first
+
+          expect(discussion.id).to eq(active_diff_note3.discussion_id)
+          expect(discussion.active?).to be true
+          expect(discussion.active?(diff_refs)).to be false
+          expect(discussion.created_at_diff?(diff_refs)).to be true
+
+          expect(discussion.notes).to eq([active_diff_note3])
+        end
+
+        it "groups the discussions by original line code" do
+          expect(subject[active_diff_note3.original_line_code].first.id).to eq(active_diff_note3.discussion_id)
+        end
+      end
+
+      context 'for diff refs a discussion was last active at' do
+        let(:diff_refs) { outdated_position.diff_refs }
+
+        it "includes discussions that were last active" do
+          discussions = subject.values.flatten
+
+          expect(discussions.count).to eq(1)
+
+          discussion = discussions.first
+
+          expect(discussion.id).to eq(outdated_diff_note1.discussion_id)
+          expect(discussion.active?).to be false
+          expect(discussion.active?(diff_refs)).to be true
+          expect(discussion.created_at_diff?(diff_refs)).to be true
+
+          expect(discussion.notes).to eq([outdated_diff_note1, outdated_diff_note2])
+        end
+
+        it "groups the discussions by line code" do
+          expect(subject[outdated_diff_note1.line_code].first.id).to eq(outdated_diff_note1.discussion_id)
+        end
+      end
     end
   end