Commit f8fabfcc authored by Douwe Maan's avatar Douwe Maan

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

parent 185fd98f
...@@ -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 }