Commit a7932fe2 authored by Stan Hu's avatar Stan Hu

Support commenting on a diff in side-by-side view

Closes https://github.com/gitlabhq/gitlabhq/issues/9283
parent 903132bc
Please view this file on the master branch, on stable branches it's out of date. Please view this file on the master branch, on stable branches it's out of date.
v 7.13.0 (unreleased) v 7.13.0 (unreleased)
- Support commenting on diffs in side-by-side mode (Stan Hu)
- Fix JavaScript error when clicking on the comment button on a diff line that has a comment already (Stan Hu)
- Remove project visibility icons from dashboard projects list - Remove project visibility icons from dashboard projects list
- Rename "Design" profile settings page to "Preferences". - Rename "Design" profile settings page to "Preferences".
- Allow users to customize their default Dashboard page. - Allow users to customize their default Dashboard page.
......
...@@ -8,11 +8,12 @@ ...@@ -8,11 +8,12 @@
class @Notes class @Notes
@interval: null @interval: null
constructor: (notes_url, note_ids, last_fetched_at) -> constructor: (notes_url, note_ids, last_fetched_at, view) ->
@notes_url = notes_url @notes_url = notes_url
@notes_url = gon.relative_url_root + @notes_url if gon.relative_url_root? @notes_url = gon.relative_url_root + @notes_url if gon.relative_url_root?
@note_ids = note_ids @note_ids = note_ids
@last_fetched_at = last_fetched_at @last_fetched_at = last_fetched_at
@view = view
@noteable_url = document.URL @noteable_url = document.URL
@initRefresh() @initRefresh()
@setupMainTargetNoteForm() @setupMainTargetNoteForm()
...@@ -131,6 +132,8 @@ class @Notes ...@@ -131,6 +132,8 @@ class @Notes
isNewNote: (note) -> isNewNote: (note) ->
$.inArray(note.id, @note_ids) == -1 $.inArray(note.id, @note_ids) == -1
isParallelView: ->
@view == 'parallel'
### ###
Render note in discussion area. Render note in discussion area.
...@@ -391,6 +394,7 @@ class @Notes ...@@ -391,6 +394,7 @@ class @Notes
setupDiscussionNoteForm: (dataHolder, form) => setupDiscussionNoteForm: (dataHolder, form) =>
# setup note target # setup note target
form.attr "rel", dataHolder.data("discussionId") form.attr "rel", dataHolder.data("discussionId")
form.find("#line_type").val dataHolder.data("lineType")
form.find("#note_commit_id").val dataHolder.data("commitId") form.find("#note_commit_id").val dataHolder.data("commitId")
form.find("#note_line_code").val dataHolder.data("lineCode") form.find("#note_line_code").val dataHolder.data("lineCode")
form.find("#note_noteable_type").val dataHolder.data("noteableType") form.find("#note_noteable_type").val dataHolder.data("noteableType")
...@@ -411,19 +415,40 @@ class @Notes ...@@ -411,19 +415,40 @@ class @Notes
form = $(".js-new-note-form") form = $(".js-new-note-form")
row = $(link).closest("tr") row = $(link).closest("tr")
nextRow = row.next() nextRow = row.next()
hasNotes = nextRow.is(".notes_holder")
# does it already have notes? addForm = false
if nextRow.is(".notes_holder") targetContent = ".notes_content"
replyButton = nextRow.find(".js-discussion-reply-button") rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"></td></tr>"
if replyButton.length > 0
$.proxy(@replyToDiscussionNote, replyButton).call() # In parallel view, look inside the correct left/right pane
if @isParallelView()
lineType = $(link).data("lineType")
targetContent += "." + lineType
rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\"></td><td class=\"notes_content parallel old\"></td><td class=\"notes_line\"></td><td class=\"notes_content parallel new\"></td></tr>"
if hasNotes
notesContent = nextRow.find(targetContent)
if notesContent.length
replyButton = notesContent.find(".js-discussion-reply-button:visible")
if replyButton.length
e.target = replyButton[0]
$.proxy(@replyToDiscussionNote, replyButton[0], e).call()
else
# In parallel view, the form may not be present in one of the panes
noteForm = notesContent.find(".js-discussion-note-form")
if noteForm.length == 0
addForm = true
else else
# add a notes row and insert the form # add a notes row and insert the form
row.after "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"></td></tr>" row.after rowCssToAdd
form.clone().appendTo row.next().find(".notes_content") addForm = true
if addForm
newForm = form.clone()
newForm.appendTo row.next().find(targetContent)
# show the form # show the form
@setupDiscussionNoteForm $(link), row.next().find("form") @setupDiscussionNoteForm $(link), newForm
### ###
Called in response to "cancel" on a diff note form. Called in response to "cancel" on a diff note form.
......
...@@ -77,11 +77,24 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -77,11 +77,24 @@ class Projects::NotesController < Projects::ApplicationController
end end
def note_to_discussion_html(note) def note_to_discussion_html(note)
if params[:view] == 'parallel'
template = "projects/notes/_diff_notes_with_reply_parallel"
locals =
if params[:line_type] == 'old'
{ notes_left: [note], notes_right: [] }
else
{ notes_left: [], notes_right: [note] }
end
else
template = "projects/notes/_diff_notes_with_reply"
locals = { notes: [note] }
end
render_to_string( render_to_string(
"projects/notes/_diff_notes_with_reply", template,
layout: false, layout: false,
formats: [:html], formats: [:html],
locals: { notes: [note] } locals: locals
) )
end end
......
...@@ -47,7 +47,7 @@ module NotesHelper ...@@ -47,7 +47,7 @@ module NotesHelper
}.to_json }.to_json
end end
def link_to_new_diff_note(line_code) def link_to_new_diff_note(line_code, line_type = nil)
discussion_id = Note.build_discussion_id( discussion_id = Note.build_discussion_id(
@comments_target[:noteable_type], @comments_target[:noteable_type],
@comments_target[:noteable_id] || @comments_target[:commit_id], @comments_target[:noteable_id] || @comments_target[:commit_id],
...@@ -59,7 +59,8 @@ module NotesHelper ...@@ -59,7 +59,8 @@ module NotesHelper
noteable_id: @comments_target[:noteable_id], noteable_id: @comments_target[:noteable_id],
commit_id: @comments_target[:commit_id], commit_id: @comments_target[:commit_id],
line_code: line_code, line_code: line_code,
discussion_id: discussion_id discussion_id: discussion_id,
line_type: line_type
} }
button_tag(class: 'btn add-diff-note js-add-diff-note-button', button_tag(class: 'btn add-diff-note js-add-diff-note-button',
...@@ -69,7 +70,7 @@ module NotesHelper ...@@ -69,7 +70,7 @@ module NotesHelper
end end
end end
def link_to_reply_diff(note) def link_to_reply_diff(note, line_type = nil)
return unless current_user return unless current_user
data = { data = {
...@@ -77,7 +78,8 @@ module NotesHelper ...@@ -77,7 +78,8 @@ module NotesHelper
noteable_id: note.noteable_id, noteable_id: note.noteable_id,
commit_id: note.commit_id, commit_id: note.commit_id,
line_code: note.line_code, line_code: note.line_code,
discussion_id: note.discussion_id discussion_id: note.discussion_id,
line_type: line_type
} }
button_tag class: 'btn reply-btn js-discussion-reply-button', button_tag class: 'btn reply-btn js-discussion-reply-button',
......
- page_title "#{@commit.title} (#{@commit.short_id})", "Commits" - page_title "#{@commit.title} (#{@commit.short_id})", "Commits"
= render "commit_box" = render "commit_box"
= render "projects/diffs/diffs", diffs: @diffs, project: @project = render "projects/diffs/diffs", diffs: @diffs, project: @project
= render "projects/notes/notes_with_form" = render "projects/notes/notes_with_form", view: params[:view]
...@@ -18,6 +18,8 @@ ...@@ -18,6 +18,8 @@
- elsif type_left == 'old' || type_left.nil? - elsif type_left == 'old' || type_left.nil?
%td.old_line{id: line_code_left, class: "#{type_left}"} %td.old_line{id: line_code_left, class: "#{type_left}"}
= link_to raw(line_number_left), "##{line_code_left}", id: line_code_left = link_to raw(line_number_left), "##{line_code_left}", id: line_code_left
- if @comments_allowed && can?(current_user, :write_note, @project)
= link_to_new_diff_note(line_code_left, 'old')
%td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= raw line_content_left %td.line_content{class: "parallel noteable_line #{type_left} #{line_code_left}", "line_code" => line_code_left }= raw line_content_left
- if type_right == 'new' - if type_right == 'new'
...@@ -29,12 +31,14 @@ ...@@ -29,12 +31,14 @@
%td.new_line{id: new_line_code, class: "#{new_line_class}", data: { linenumber: line_number_right }} %td.new_line{id: new_line_code, class: "#{new_line_class}", data: { linenumber: line_number_right }}
= link_to raw(line_number_right), "##{new_line_code}", id: new_line_code = link_to raw(line_number_right), "##{new_line_code}", id: new_line_code
- if @comments_allowed && can?(current_user, :write_note, @project)
= link_to_new_diff_note(line_code_right, 'new')
%td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= raw line_content_right %td.line_content.parallel{class: "noteable_line #{new_line_class} #{new_line_code}", "line_code" => new_line_code}= raw line_content_right
- if @reply_allowed - if @reply_allowed
- comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right)
- if comments_left.present? || comments_right.present? - if comments_left.present? || comments_right.present?
= render "projects/notes/diff_notes_with_reply_parallel", notes1: comments_left, notes2: comments_right = render "projects/notes/diff_notes_with_reply_parallel", notes_left: comments_left, notes_right: comments_right
- if diff_file.diff.diff.blank? && diff_file.mode_changed? - if diff_file.diff.diff.blank? && diff_file.mode_changed?
.file-mode-changed .file-mode-changed
......
- note1 = notes1.present? ? notes1.first : nil - note1 = notes_left.present? ? notes_left.first : nil
- note2 = notes2.present? ? notes2.first : nil - note2 = notes_right.present? ? notes_right.first : nil
%tr.notes_holder %tr.notes_holder
- if note1 - if note1
%td.notes_line %td.notes_line.old
%span.btn.disabled %span.btn.disabled
%i.fa.fa-comment %i.fa.fa-comment
= notes1.count = notes_left.count
%td.notes_content.parallel %td.notes_content.parallel.old
%ul.notes{ rel: note1.discussion_id } %ul.notes{ rel: note1.discussion_id }
= render notes1 = render notes_left
.discussion-reply-holder .discussion-reply-holder
= link_to_reply_diff(note1) = link_to_reply_diff(note1, 'old')
- else - else
%td= "" %td.notes_line.old= ""
%td= "" %td.notes_content.parallel.old= ""
- if note2 - if note2
%td.notes_line %td.notes_line.new
%span.btn.disabled %span.btn.disabled
%i.fa.fa-comment %i.fa.fa-comment
= notes2.count = notes_right.count
%td.notes_content.parallel %td.notes_content.parallel.new
%ul.notes{ rel: note2.discussion_id } %ul.notes{ rel: note2.discussion_id }
= render notes2 = render notes_right
.discussion-reply-holder .discussion-reply-holder
= link_to_reply_diff(note2) = link_to_reply_diff(note2, 'new')
- else - else
%td= "" %td.notes_line.new= ""
%td= "" %td.notes_content.parallel.new= ""
= form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new_note js-new-note-form common-note-form gfm-form" }, authenticity_token: true do |f| = form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new_note js-new-note-form common-note-form gfm-form" }, authenticity_token: true do |f|
= hidden_field_tag :view, params[:view]
= hidden_field_tag :line_type
= note_target_fields(@note) = note_target_fields(@note)
= f.hidden_field :commit_id = f.hidden_field :commit_id
= f.hidden_field :line_code = f.hidden_field :line_code
......
...@@ -4,7 +4,7 @@ ...@@ -4,7 +4,7 @@
.js-main-target-form .js-main-target-form
- if can? current_user, :write_note, @project - if can? current_user, :write_note, @project
= render "projects/notes/form" = render "projects/notes/form", view: params[:view]
:javascript :javascript
new Notes("#{namespace_project_notes_path(namespace_id: @project.namespace, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i}) new Notes("#{namespace_project_notes_path(namespace_id: @project.namespace, target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i}, "#{params[:view]}")
...@@ -77,3 +77,17 @@ Feature: Project Commits Diff Comments ...@@ -77,3 +77,17 @@ Feature: Project Commits Diff Comments
And I submit the diff comment And I submit the diff comment
Then I should not see the diff comment form Then I should not see the diff comment form
And I should see a discussion reply button And I should see a discussion reply button
@javascript
Scenario: I can add a comment on a side-by-side commit diff (left side)
Given I open a diff comment form
And I click side-by-side diff button
When I leave a diff comment in a parallel view on the left side like "Old comment"
Then I should see a diff comment on the left side saying "Old comment"
@javascript
Scenario: I can add a comment on a side-by-side commit diff (right side)
Given I open a diff comment form
And I click side-by-side diff button
When I leave a diff comment in a parallel view on the right side like "New comment"
Then I should see a diff comment on the right side saying "New comment"
...@@ -2,6 +2,7 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps ...@@ -2,6 +2,7 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps
include SharedAuthentication include SharedAuthentication
include SharedProject include SharedProject
include SharedPaths include SharedPaths
include SharedDiffNote
include RepoHelpers include RepoHelpers
step 'I see project commits' do step 'I see project commits' do
...@@ -88,14 +89,6 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps ...@@ -88,14 +89,6 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps
expect(links[1]['href']).to match %r{blob/#{sample_image_commit.new_blob_id}} expect(links[1]['href']).to match %r{blob/#{sample_image_commit.new_blob_id}}
end end
step 'I click side-by-side diff button' do
click_link "Side-by-side"
end
step 'I see side-by-side diff button' do
expect(page).to have_content "Side-by-side"
end
step 'I see inline diff button' do step 'I see inline diff button' do
expect(page).to have_content "Inline" expect(page).to have_content "Inline"
end end
......
...@@ -28,6 +28,22 @@ module SharedDiffNote ...@@ -28,6 +28,22 @@ module SharedDiffNote
end end
end end
step 'I leave a diff comment in a parallel view on the left side like "Old comment"' do
click_parallel_diff_line(sample_commit.line_code, 'old')
page.within("#{diff_file_selector} form[rel$='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "Old comment"
find(".js-comment-button").trigger("click")
end
end
step 'I leave a diff comment in a parallel view on the right side like "New comment"' do
click_parallel_diff_line(sample_commit.line_code, 'new')
page.within("#{diff_file_selector} form[rel$='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "New comment"
find(".js-comment-button").trigger("click")
end
end
step 'I preview a diff comment text like "Should fix it :smile:"' do step 'I preview a diff comment text like "Should fix it :smile:"' do
click_diff_line(sample_commit.line_code) click_diff_line(sample_commit.line_code)
page.within("#{diff_file_selector} form[rel$='#{sample_commit.line_code}']") do page.within("#{diff_file_selector} form[rel$='#{sample_commit.line_code}']") do
...@@ -102,6 +118,18 @@ module SharedDiffNote ...@@ -102,6 +118,18 @@ module SharedDiffNote
end end
end end
step 'I should see a diff comment on the left side saying "Old comment"' do
page.within("#{diff_file_selector} .notes_content.parallel.old") do
expect(page).to have_content("Old comment")
end
end
step 'I should see a diff comment on the right side saying "New comment"' do
page.within("#{diff_file_selector} .notes_content.parallel.new") do
expect(page).to have_content("New comment")
end
end
step 'I should see a discussion reply button' do step 'I should see a discussion reply button' do
page.within(diff_file_selector) do page.within(diff_file_selector) do
expect(page).to have_button('Reply') expect(page).to have_button('Reply')
...@@ -157,6 +185,14 @@ module SharedDiffNote ...@@ -157,6 +185,14 @@ module SharedDiffNote
end end
end end
step 'I click side-by-side diff button' do
click_link "Side-by-side"
end
step 'I see side-by-side diff button' do
expect(page).to have_content "Side-by-side"
end
def diff_file_selector def diff_file_selector
".diff-file:nth-of-type(1)" ".diff-file:nth-of-type(1)"
end end
...@@ -164,4 +200,8 @@ module SharedDiffNote ...@@ -164,4 +200,8 @@ module SharedDiffNote
def click_diff_line(code) def click_diff_line(code)
find("button[data-line-code='#{code}']").click find("button[data-line-code='#{code}']").click
end end
def click_parallel_diff_line(code, line_type)
find("button[data-line-code='#{code}'][data-line-type='#{line_type}']").trigger('click')
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