Commit 21e4f053 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'dm-link-discussion-to-outdated-diff' into 'master'

Allow commenting on older versions of the diff and comparisons between diff versions

See merge request !10388
parents c1e2da92 f8fabfcc
...@@ -425,12 +425,6 @@ ...@@ -425,12 +425,6 @@
float: right; float: right;
} }
.diffs {
.content-block {
border-bottom: none;
}
}
.files-changed { .files-changed {
border-bottom: none; border-bottom: none;
} }
......
...@@ -511,7 +511,6 @@ ...@@ -511,7 +511,6 @@
.mr-version-controls { .mr-version-controls {
background: $gray-light; background: $gray-light;
border-bottom: 1px solid $border-color;
color: $gl-text-color; color: $gl-text-color;
.mr-version-menus-container { .mr-version-menus-container {
......
...@@ -120,7 +120,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -120,7 +120,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
define_diff_comment_vars define_diff_comment_vars
else else
build_merge_request build_merge_request
@diffs = @merge_request.diffs(diff_options) @compare = @merge_request
@diffs = @compare.diffs(diff_options)
@diff_notes_disabled = true @diff_notes_disabled = true
end end
...@@ -584,12 +585,14 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -584,12 +585,14 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
end end
@diffs = @compare =
if @start_sha if @start_sha
@merge_request_diff.compare_with(@start_sha).diffs(diff_options) @merge_request_diff.compare_with(@start_sha)
else else
@merge_request_diff.diffs(diff_options) @merge_request_diff
end end
@diffs = @compare.diffs(diff_options)
end end
def define_diff_comment_vars def define_diff_comment_vars
...@@ -598,11 +601,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -598,11 +601,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
noteable_id: @merge_request.id 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? @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)) @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
end end
......
...@@ -60,20 +60,16 @@ module NotesHelper ...@@ -60,20 +60,16 @@ module NotesHelper
note.project.team.human_max_access(note.author_id) note.project.team.human_max_access(note.author_id)
end end
def discussion_diff_path(discussion) def discussion_path(discussion)
if discussion.for_merge_request? && discussion.diff_discussion? if discussion.for_merge_request?
if discussion.active? return unless discussion.diff_discussion?
# Without a diff ID, the link always points to the latest diff version
diff_id = nil version_params = discussion.merge_request_version_params
elsif merge_request_diff = discussion.latest_merge_request_diff return unless version_params
diff_id = merge_request_diff.id
else path_params = version_params.merge(anchor: discussion.line_code)
# If the discussion is not active, and we cannot find the latest
# merge request diff for this discussion, we return no path at all. diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, path_params)
return
end
diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, diff_id: diff_id, anchor: discussion.line_code)
elsif discussion.for_commit? elsif discussion.for_commit?
anchor = discussion.line_code if discussion.diff_discussion? anchor = discussion.line_code if discussion.diff_discussion?
......
...@@ -11,6 +11,7 @@ module DiscussionOnDiff ...@@ -11,6 +11,7 @@ module DiscussionOnDiff
:diff_line, :diff_line,
:for_line?, :for_line?,
:active?, :active?,
:created_at_diff?,
to: :first_note to: :first_note
......
...@@ -30,6 +30,10 @@ module NoteOnDiff ...@@ -30,6 +30,10 @@ module NoteOnDiff
raise NotImplementedError raise NotImplementedError
end end
def created_at_diff?(diff_refs)
false
end
private private
def noteable_diff_refs def noteable_diff_refs
......
...@@ -10,7 +10,6 @@ class DiffDiscussion < Discussion ...@@ -10,7 +10,6 @@ class DiffDiscussion < Discussion
delegate :position, delegate :position,
:original_position, :original_position,
:latest_merge_request_diff,
to: :first_note to: :first_note
...@@ -18,6 +17,25 @@ class DiffDiscussion < Discussion ...@@ -18,6 +17,25 @@ class DiffDiscussion < Discussion
false false
end 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 def reply_attributes
super.merge( super.merge(
original_position: original_position.to_json, original_position: original_position.to_json,
......
...@@ -65,10 +65,11 @@ class DiffNote < Note ...@@ -65,10 +65,11 @@ class DiffNote < Note
self.position.diff_refs == diff_refs self.position.diff_refs == diff_refs
end end
def latest_merge_request_diff def created_at_diff?(diff_refs)
return unless for_merge_request? 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 end
private private
......
...@@ -9,14 +9,14 @@ class LegacyDiffDiscussion < Discussion ...@@ -9,14 +9,14 @@ class LegacyDiffDiscussion < Discussion
memoized_values << :active memoized_values << :active
def legacy_diff_discussion?
true
end
def self.note_class def self.note_class
LegacyDiffNote LegacyDiffNote
end end
def legacy_diff_discussion?
true
end
def active?(*args) def active?(*args)
return @active if @active.present? return @active if @active.present?
...@@ -27,6 +27,16 @@ class LegacyDiffDiscussion < Discussion ...@@ -27,6 +27,16 @@ class LegacyDiffDiscussion < Discussion
!active? !active?
end end
def merge_request_version_params
return unless for_merge_request?
if active?
{}
else
nil
end
end
def reply_attributes def reply_attributes
super.merge(line_code: line_code) super.merge(line_code: line_code)
end end
......
...@@ -374,12 +374,18 @@ class MergeRequest < ActiveRecord::Base ...@@ -374,12 +374,18 @@ class MergeRequest < ActiveRecord::Base
merge_request_diff(true) merge_request_diff(true)
end end
def merge_request_diff_for(diff_refs) def merge_request_diff_for(diff_refs_or_sha)
@merge_request_diffs_by_diff_refs ||= Hash.new do |h, diff_refs| @merge_request_diffs_by_diff_refs_or_sha ||= Hash.new do |h, diff_refs_or_sha|
h[diff_refs] = merge_request_diffs.viewable.select_without_diff.find_by_diff_refs(diff_refs) diffs = merge_request_diffs.viewable.select_without_diff
end h[diff_refs_or_sha] =
if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs)
@merge_request_diffs_by_diff_refs[diff_refs] 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 end
def reload_diff_if_branch_changed def reload_diff_if_branch_changed
......
...@@ -115,11 +115,19 @@ class Note < ActiveRecord::Base ...@@ -115,11 +115,19 @@ class Note < ActiveRecord::Base
end end
def grouped_diff_discussions(diff_refs = nil) def grouped_diff_discussions(diff_refs = nil)
diff_notes. groups = {}
fresh.
discussions. diff_notes.fresh.discussions.each do |discussion|
select { |n| n.active?(diff_refs) }. if discussion.active?(diff_refs)
group_by(&:line_code) 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 end
def count_for_collection(ids, type) def count_for_collection(ids, type)
...@@ -141,10 +149,6 @@ class Note < ActiveRecord::Base ...@@ -141,10 +149,6 @@ class Note < ActiveRecord::Base
true true
end end
def latest_merge_request_diff
nil
end
def max_attachment_size def max_attachment_size
current_application_settings.max_attachment_size.megabytes.to_i current_application_settings.max_attachment_size.megabytes.to_i
end end
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
.diff-file.file-holder .diff-file.file-holder
.js-file-title.file-title .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 .diff-content.code.js-syntax-highlight
%table %table
......
...@@ -20,7 +20,7 @@ ...@@ -20,7 +20,7 @@
= discussion.author.to_reference = discussion.author.to_reference
started a discussion started a discussion
- url = discussion_diff_path(discussion) - url = discussion_path(discussion)
- if discussion.for_commit? && @noteable != discussion.noteable - if discussion.for_commit? && @noteable != discussion.noteable
on on
- commit = discussion.noteable - commit = discussion.noteable
......
...@@ -35,6 +35,6 @@ ...@@ -35,6 +35,6 @@
- else - else
= diff_line_content(line.text) = diff_line_content(line.text)
- if line_discussions - if line_discussions&.any?
- discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?)) - discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?))
= render "discussions/diff_discussion", discussions: line_discussions, expanded: discussion_expanded = render "discussions/diff_discussion", discussions: line_discussions, expanded: discussion_expanded
...@@ -35,7 +35,7 @@ ...@@ -35,7 +35,7 @@
%span.dropdown.inline.mr-version-compare-dropdown %span.dropdown.inline.mr-version-compare-dropdown
%a.btn.btn-default.dropdown-toggle{ data: {toggle: :dropdown} } %a.btn.btn-default.dropdown-toggle{ data: {toggle: :dropdown} }
%span %span
- if @start_sha - if @start_version
version #{version_index(@start_version)} version #{version_index(@start_version)}
- else - else
#{@merge_request.target_branch} #{@merge_request.target_branch}
...@@ -59,7 +59,7 @@ ...@@ -59,7 +59,7 @@
%small %small
= time_ago_with_tooltip(merge_request_diff.created_at) = time_ago_with_tooltip(merge_request_diff.created_at)
%li %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 %strong
#{@merge_request.target_branch} (base) #{@merge_request.target_branch} (base)
.monospace= short_sha(@merge_request_diff.base_commit_sha) .monospace= short_sha(@merge_request_diff.base_commit_sha)
...@@ -75,13 +75,15 @@ ...@@ -75,13 +75,15 @@
= succeed '.' do = succeed '.' do
%code= @merge_request.target_branch %code= @merge_request.target_branch
- if @diff_notes_disabled - if @start_version || !@merge_request_diff.latest?
.comments-disabled-notif.content-block .comments-disabled-notif.content-block
= icon('info-circle') = icon('info-circle')
- if @start_sha Not all comments are displayed because you're
Comments are disabled because you're comparing two versions of this merge request. - if @start_version
comparing two versions
- else - 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 .pull-right
= link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm' = link_to 'Show latest version', diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm'
---
title: Allow commenting on older versions of the diff and comparisons between diff versions
merge_request:
author:
...@@ -24,7 +24,12 @@ feature 'Merge Request versions', js: true, feature: true do ...@@ -24,7 +24,12 @@ feature 'Merge Request versions', js: true, feature: true do
before do before do
page.within '.mr-version-dropdown' do page.within '.mr-version-dropdown' do
find('.btn-default').click 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
end end
...@@ -36,8 +41,8 @@ feature 'Merge Request versions', js: true, feature: true do ...@@ -36,8 +41,8 @@ feature 'Merge Request versions', js: true, feature: true do
expect(page).to have_content '5 changed files' expect(page).to have_content '5 changed files'
end end
it 'show the message about disabled comment creation' do it 'show the message about comments' do
expect(page).to have_content 'comment creation is disabled' expect(page).to have_content 'Not all comments are displayed'
end end
it 'shows comments that were last relevant at that version' do it 'shows comments that were last relevant at that version' do
...@@ -52,15 +57,41 @@ feature 'Merge Request versions', js: true, feature: true 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.position = outdated_diff_note.original_position
outdated_diff_note.save! outdated_diff_note.save!
visit current_url
expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']") expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']")
end 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 end
describe 'compare with older version' do describe 'compare with older version' do
before do before do
page.within '.mr-version-compare-dropdown' do page.within '.mr-version-compare-dropdown' do
find('.btn-default').click 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
end end
...@@ -80,8 +111,43 @@ feature 'Merge Request versions', js: true, feature: true do ...@@ -80,8 +111,43 @@ feature 'Merge Request versions', js: true, feature: true do
end end
end end
it 'show the message about disabled comments' do it 'show the message about comments' do
expect(page).to have_content 'Comments are disabled' 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 end
it 'show diff between new and old version' do it 'show diff between new and old version' do
......
require "spec_helper" require "spec_helper"
describe NotesHelper do describe NotesHelper do
include RepoHelpers
let(:owner) { create(:owner) } let(:owner) { create(:owner) }
let(:group) { create(:group) } let(:group) { create(:group) }
let(:project) { create(:empty_project, namespace: group) } let(:project) { create(:empty_project, namespace: group) }
...@@ -36,4 +38,141 @@ describe NotesHelper do ...@@ -36,4 +38,141 @@ describe NotesHelper do
expect(helper.note_max_access_for_user(other_note)).to eq('Reporter') expect(helper.note_max_access_for_user(other_note)).to eq('Reporter')
end end
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 end
require 'spec_helper' require 'spec_helper'
describe DiffDiscussion, model: true do 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) } subject { described_class.new([diff_note]) }
let(:merge_request) { first_note.noteable }
let(:project) { first_note.project } let(:project) { create(:project) }
let(:second_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
let(:third_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) } let(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
describe '#reply_attributes' do describe '#reply_attributes' do
it 'includes position and original_position' do it 'includes position and original_position' do
attributes = subject.reply_attributes attributes = subject.reply_attributes
expect(attributes[:position]).to eq(first_note.position.to_json) expect(attributes[:position]).to eq(diff_note.position.to_json)
expect(attributes[:original_position]).to eq(first_note.original_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 end
end end
...@@ -155,23 +155,6 @@ describe DiffNote, models: true do ...@@ -155,23 +155,6 @@ describe DiffNote, models: true do
end end
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 "creation" do
describe "updating of position" do describe "updating of position" do
context "when noteable is a commit" do context "when noteable is a commit" do
...@@ -256,4 +239,39 @@ describe DiffNote, models: true do ...@@ -256,4 +239,39 @@ describe DiffNote, models: true do
end end
end 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 end
...@@ -8,4 +8,26 @@ describe LegacyDiffDiscussion, models: true do ...@@ -8,4 +8,26 @@ describe LegacyDiffDiscussion, models: true do
expect(subject.reply_attributes[:line_code]).to eq(subject.line_code) expect(subject.reply_attributes[:line_code]).to eq(subject.line_code)
end end
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 end
...@@ -1554,4 +1554,23 @@ describe MergeRequest, models: true do ...@@ -1554,4 +1554,23 @@ describe MergeRequest, models: true do
expect(subject.has_no_commits?).to be_truthy expect(subject.has_no_commits?).to be_truthy
end end
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 end
...@@ -272,9 +272,9 @@ describe Note, models: true do ...@@ -272,9 +272,9 @@ describe Note, models: true do
Gitlab::Diff::Position.new( Gitlab::Diff::Position.new(
old_path: "files/ruby/popen.rb", old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb", new_path: "files/ruby/popen.rb",
old_line: 16, old_line: nil,
new_line: 22, new_line: 13,
diff_refs: merge_request.diff_refs diff_refs: project.commit(sample_commit.id).diff_refs
) )
end end
...@@ -288,26 +288,78 @@ describe Note, models: true do ...@@ -288,26 +288,78 @@ describe Note, models: true do
) )
end 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 it "includes active discussions" do
discussions = subject.values.flatten discussions = subject.values.flatten
expect(discussions.count).to eq(2) expect(discussions.count).to eq(2)
expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id]) 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.all?(&:active?)).to be true
expect(discussions.first.notes).to eq([active_diff_note1, active_diff_note2]) expect(discussions.first.notes).to eq([active_diff_note1, active_diff_note2])
expect(discussions.last.notes).to eq([active_diff_note3]) expect(discussions.last.notes).to eq([active_diff_note3])
end end
it "doesn't include outdated discussions" do it "doesn't include outdated discussions" do
expect(subject.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id) 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 end
it "groups the discussions by line code" do context 'diff discussions for older diff refs' do
expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id) subject { merge_request.notes.grouped_diff_discussions(diff_refs) }
expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment