summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/assets/stylesheets/pages/diff.scss6
-rw-r--r--app/assets/stylesheets/pages/merge_requests.scss1
-rwxr-xr-xapp/controllers/projects/merge_requests_controller.rb15
-rw-r--r--app/helpers/notes_helper.rb24
-rw-r--r--app/models/concerns/discussion_on_diff.rb1
-rw-r--r--app/models/concerns/note_on_diff.rb4
-rw-r--r--app/models/diff_discussion.rb20
-rw-r--r--app/models/diff_note.rb7
-rw-r--r--app/models/legacy_diff_discussion.rb18
-rw-r--r--app/models/merge_request.rb18
-rw-r--r--app/models/note.rb22
-rw-r--r--app/views/discussions/_diff_with_notes.html.haml2
-rw-r--r--app/views/discussions/_discussion.html.haml2
-rw-r--r--app/views/projects/diffs/_line.html.haml2
-rw-r--r--app/views/projects/merge_requests/show/_versions.html.haml14
-rw-r--r--changelogs/unreleased/dm-comment-on-diff-versions.yml4
-rw-r--r--spec/features/merge_requests/versions_spec.rb78
-rw-r--r--spec/helpers/notes_helper_spec.rb139
-rw-r--r--spec/models/diff_discussion_spec.rb83
-rw-r--r--spec/models/diff_note_spec.rb52
-rw-r--r--spec/models/legacy_diff_discussion_spec.rb22
-rw-r--r--spec/models/merge_request_spec.rb19
-rw-r--r--spec/models/note_spec.rb86
23 files changed, 532 insertions, 107 deletions
diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss
index 1b4694377b3..feefaad8a15 100644
--- a/app/assets/stylesheets/pages/diff.scss
+++ b/app/assets/stylesheets/pages/diff.scss
@@ -425,12 +425,6 @@
float: right;
}
-.diffs {
- .content-block {
- border-bottom: none;
- }
-}
-
.files-changed {
border-bottom: none;
}
diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index 6a419384a34..bca62b7fc31 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -511,7 +511,6 @@
.mr-version-controls {
background: $gray-light;
- border-bottom: 1px solid $border-color;
color: $gl-text-color;
.mr-version-menus-container {
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 09dc8b38229..a63b7ff0bed 100755
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -120,7 +120,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
define_diff_comment_vars
else
build_merge_request
- @diffs = @merge_request.diffs(diff_options)
+ @compare = @merge_request
+ @diffs = @compare.diffs(diff_options)
@diff_notes_disabled = true
end
@@ -584,12 +585,14 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
end
- @diffs =
+ @compare =
if @start_sha
- @merge_request_diff.compare_with(@start_sha).diffs(diff_options)
+ @merge_request_diff.compare_with(@start_sha)
else
- @merge_request_diff.diffs(diff_options)
+ @merge_request_diff
end
+
+ @diffs = @compare.diffs(diff_options)
end
def define_diff_comment_vars
@@ -598,11 +601,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
noteable_id: @merge_request.id
}
- @diff_notes_disabled = !@merge_request_diff.latest? || @start_sha
+ @diff_notes_disabled = false
@use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
- @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@merge_request_diff.diff_refs)
+ @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@compare.diff_refs)
@notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
end
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index eab0738a368..08180883eb9 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -60,20 +60,16 @@ module NotesHelper
note.project.team.human_max_access(note.author_id)
end
- def discussion_diff_path(discussion)
- if discussion.for_merge_request? && discussion.diff_discussion?
- if discussion.active?
- # Without a diff ID, the link always points to the latest diff version
- diff_id = nil
- elsif merge_request_diff = discussion.latest_merge_request_diff
- diff_id = merge_request_diff.id
- else
- # If the discussion is not active, and we cannot find the latest
- # merge request diff for this discussion, we return no path at all.
- return
- end
-
- diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, diff_id: diff_id, anchor: discussion.line_code)
+ def discussion_path(discussion)
+ if discussion.for_merge_request?
+ return unless discussion.diff_discussion?
+
+ version_params = discussion.merge_request_version_params
+ return unless version_params
+
+ path_params = version_params.merge(anchor: discussion.line_code)
+
+ diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, path_params)
elsif discussion.for_commit?
anchor = discussion.line_code if discussion.diff_discussion?
diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index 8ee42875670..a7bdf5587b2 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -11,6 +11,7 @@ module DiscussionOnDiff
:diff_line,
:for_line?,
:active?,
+ :created_at_diff?,
to: :first_note
diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb
index 6c27dd5aa5c..6359f7596b1 100644
--- a/app/models/concerns/note_on_diff.rb
+++ b/app/models/concerns/note_on_diff.rb
@@ -30,6 +30,10 @@ module NoteOnDiff
raise NotImplementedError
end
+ def created_at_diff?(diff_refs)
+ false
+ end
+
private
def noteable_diff_refs
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index 6a6466b493b..d627fbe327f 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -10,7 +10,6 @@ class DiffDiscussion < Discussion
delegate :position,
:original_position,
- :latest_merge_request_diff,
to: :first_note
@@ -18,6 +17,25 @@ class DiffDiscussion < Discussion
false
end
+ def merge_request_version_params
+ return unless for_merge_request?
+
+ if active?
+ {}
+ else
+ diff_refs = position.diff_refs
+
+ if diff = noteable.merge_request_diff_for(diff_refs)
+ { diff_id: diff.id }
+ elsif diff = noteable.merge_request_diff_for(diff_refs.head_sha)
+ {
+ diff_id: diff.id,
+ start_sha: diff_refs.start_sha
+ }
+ end
+ end
+ end
+
def reply_attributes
super.merge(
original_position: original_position.to_json,
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index abe4518d62a..76c59199afd 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -65,10 +65,11 @@ class DiffNote < Note
self.position.diff_refs == diff_refs
end
- def latest_merge_request_diff
- return unless for_merge_request?
+ def created_at_diff?(diff_refs)
+ return false unless supported?
+ return true if for_commit?
- self.noteable.merge_request_diff_for(self.position.diff_refs)
+ self.original_position.diff_refs == diff_refs
end
private
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index e617ce36f56..3c1d34db5fa 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -9,14 +9,14 @@ class LegacyDiffDiscussion < Discussion
memoized_values << :active
- def legacy_diff_discussion?
- true
- end
-
def self.note_class
LegacyDiffNote
end
+ def legacy_diff_discussion?
+ true
+ end
+
def active?(*args)
return @active if @active.present?
@@ -27,6 +27,16 @@ class LegacyDiffDiscussion < Discussion
!active?
end
+ def merge_request_version_params
+ return unless for_merge_request?
+
+ if active?
+ {}
+ else
+ nil
+ end
+ end
+
def reply_attributes
super.merge(line_code: line_code)
end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 365fa4f1e70..12c5481cd6d 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -374,12 +374,18 @@ class MergeRequest < ActiveRecord::Base
merge_request_diff(true)
end
- def merge_request_diff_for(diff_refs)
- @merge_request_diffs_by_diff_refs ||= Hash.new do |h, diff_refs|
- h[diff_refs] = merge_request_diffs.viewable.select_without_diff.find_by_diff_refs(diff_refs)
- end
-
- @merge_request_diffs_by_diff_refs[diff_refs]
+ def merge_request_diff_for(diff_refs_or_sha)
+ @merge_request_diffs_by_diff_refs_or_sha ||= Hash.new do |h, diff_refs_or_sha|
+ diffs = merge_request_diffs.viewable.select_without_diff
+ h[diff_refs_or_sha] =
+ if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs)
+ diffs.find_by_diff_refs(diff_refs_or_sha)
+ else
+ diffs.find_by(head_commit_sha: diff_refs_or_sha)
+ end
+ end
+
+ @merge_request_diffs_by_diff_refs_or_sha[diff_refs_or_sha]
end
def reload_diff_if_branch_changed
diff --git a/app/models/note.rb b/app/models/note.rb
index e720bfba030..b06985b4a6f 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -115,11 +115,19 @@ class Note < ActiveRecord::Base
end
def grouped_diff_discussions(diff_refs = nil)
- diff_notes.
- fresh.
- discussions.
- select { |n| n.active?(diff_refs) }.
- group_by(&:line_code)
+ groups = {}
+
+ diff_notes.fresh.discussions.each do |discussion|
+ if discussion.active?(diff_refs)
+ discussions = groups[discussion.line_code] ||= []
+ elsif diff_refs && discussion.created_at_diff?(diff_refs)
+ discussions = groups[discussion.original_line_code] ||= []
+ end
+
+ discussions << discussion if discussions
+ end
+
+ groups
end
def count_for_collection(ids, type)
@@ -141,10 +149,6 @@ class Note < ActiveRecord::Base
true
end
- def latest_merge_request_diff
- nil
- end
-
def max_attachment_size
current_application_settings.max_attachment_size.megabytes.to_i
end
diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml
index 549364761e6..78c5b0c1dda 100644
--- a/app/views/discussions/_diff_with_notes.html.haml
+++ b/app/views/discussions/_diff_with_notes.html.haml
@@ -3,7 +3,7 @@
.diff-file.file-holder
.js-file-title.file-title
- = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: discussion.project, url: discussion_diff_path(discussion)
+ = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: discussion.project, url: discussion_path(discussion)
.diff-content.code.js-syntax-highlight
%table
diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml
index 8440fb3d785..38e85168f40 100644
--- a/app/views/discussions/_discussion.html.haml
+++ b/app/views/discussions/_discussion.html.haml
@@ -20,7 +20,7 @@
= discussion.author.to_reference
started a discussion
- - url = discussion_diff_path(discussion)
+ - url = discussion_path(discussion)
- if discussion.for_commit? && @noteable != discussion.noteable
on
- commit = discussion.noteable
diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml
index 3e426ee9e7d..7439b8a66f7 100644
--- a/app/views/projects/diffs/_line.html.haml
+++ b/app/views/projects/diffs/_line.html.haml
@@ -35,6 +35,6 @@
- else
= diff_line_content(line.text)
-- if line_discussions
+- if line_discussions&.any?
- discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?))
= render "discussions/diff_discussion", discussions: line_discussions, expanded: discussion_expanded
diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml
index 547be78992e..11b0c55be0b 100644
--- a/app/views/projects/merge_requests/show/_versions.html.haml
+++ b/app/views/projects/merge_requests/show/_versions.html.haml
@@ -35,7 +35,7 @@
%span.dropdown.inline.mr-version-compare-dropdown
%a.btn.btn-default.dropdown-toggle{ data: {toggle: :dropdown} }
%span
- - if @start_sha
+ - if @start_version
version #{version_index(@start_version)}
- else
#{@merge_request.target_branch}
@@ -59,7 +59,7 @@
%small
= time_ago_with_tooltip(merge_request_diff.created_at)
%li
- = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless @start_sha) do
+ = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless @start_version) do
%strong
#{@merge_request.target_branch} (base)
.monospace= short_sha(@merge_request_diff.base_commit_sha)
@@ -75,13 +75,15 @@
= succeed '.' do
%code= @merge_request.target_branch
- - if @diff_notes_disabled
+ - if @start_version || !@merge_request_diff.latest?
.comments-disabled-notif.content-block
= icon('info-circle')
- - if @start_sha
- Comments are disabled because you're comparing two versions of this merge request.
+ Not all comments are displayed because you're
+ - if @start_version
+ comparing two versions
- else
- Discussions on this version of the merge request are displayed but comment creation is disabled.
+ viewing an old version
+ of this merge request.
.pull-right
= link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'
diff --git a/changelogs/unreleased/dm-comment-on-diff-versions.yml b/changelogs/unreleased/dm-comment-on-diff-versions.yml
new file mode 100644
index 00000000000..af299713ad3
--- /dev/null
+++ b/changelogs/unreleased/dm-comment-on-diff-versions.yml
@@ -0,0 +1,4 @@
+---
+title: Allow commenting on older versions of the diff and comparisons between diff versions
+merge_request:
+author:
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