diff options
| -rw-r--r-- | CHANGELOG | 2 | ||||
| -rw-r--r-- | app/assets/javascripts/notes.js.coffee | 45 | ||||
| -rw-r--r-- | app/controllers/projects/notes_controller.rb | 17 | ||||
| -rw-r--r-- | app/helpers/notes_helper.rb | 10 | ||||
| -rw-r--r-- | app/views/projects/commit/show.html.haml | 2 | ||||
| -rw-r--r-- | app/views/projects/diffs/_parallel_view.html.haml | 6 | ||||
| -rw-r--r-- | app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml | 32 | ||||
| -rw-r--r-- | app/views/projects/notes/_form.html.haml | 2 | ||||
| -rw-r--r-- | app/views/projects/notes/_notes_with_form.html.haml | 4 | ||||
| -rw-r--r-- | features/project/commits/diff_comments.feature | 14 | ||||
| -rw-r--r-- | features/steps/project/commits/commits.rb | 9 | ||||
| -rw-r--r-- | features/steps/shared/diff_note.rb | 40 |
12 files changed, 139 insertions, 44 deletions
diff --git a/CHANGELOG b/CHANGELOG index e881fa42a20..86de9314d80 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,8 @@ Please view this file on the master branch, on stable branches it's out of date. 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 - Rename "Design" profile settings page to "Preferences". - Allow users to customize their default Dashboard page. diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee index 21656f59149..1c05a2b9fe8 100644 --- a/app/assets/javascripts/notes.js.coffee +++ b/app/assets/javascripts/notes.js.coffee @@ -8,11 +8,12 @@ class @Notes @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 = gon.relative_url_root + @notes_url if gon.relative_url_root? @note_ids = note_ids @last_fetched_at = last_fetched_at + @view = view @noteable_url = document.URL @initRefresh() @setupMainTargetNoteForm() @@ -131,6 +132,8 @@ class @Notes isNewNote: (note) -> $.inArray(note.id, @note_ids) == -1 + isParallelView: -> + @view == 'parallel' ### Render note in discussion area. @@ -391,6 +394,7 @@ class @Notes setupDiscussionNoteForm: (dataHolder, form) => # setup note target 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_line_code").val dataHolder.data("lineCode") form.find("#note_noteable_type").val dataHolder.data("noteableType") @@ -411,19 +415,40 @@ class @Notes form = $(".js-new-note-form") row = $(link).closest("tr") nextRow = row.next() - - # does it already have notes? - if nextRow.is(".notes_holder") - replyButton = nextRow.find(".js-discussion-reply-button") - if replyButton.length > 0 - $.proxy(@replyToDiscussionNote, replyButton).call() + hasNotes = nextRow.is(".notes_holder") + addForm = false + targetContent = ".notes_content" + rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"></td></tr>" + + # 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 # 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>" - form.clone().appendTo row.next().find(".notes_content") + row.after rowCssToAdd + addForm = true + + if addForm + newForm = form.clone() + newForm.appendTo row.next().find(targetContent) # show the form - @setupDiscussionNoteForm $(link), row.next().find("form") + @setupDiscussionNoteForm $(link), newForm ### Called in response to "cancel" on a diff note form. diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 496b85cb46d..f3e521adb69 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -77,11 +77,24 @@ class Projects::NotesController < Projects::ApplicationController end 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( - "projects/notes/_diff_notes_with_reply", + template, layout: false, formats: [:html], - locals: { notes: [note] } + locals: locals ) end diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 271b53aa2b6..a7c1fa0b071 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -47,7 +47,7 @@ module NotesHelper }.to_json 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( @comments_target[:noteable_type], @comments_target[:noteable_id] || @comments_target[:commit_id], @@ -59,7 +59,8 @@ module NotesHelper noteable_id: @comments_target[:noteable_id], commit_id: @comments_target[:commit_id], 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', @@ -69,7 +70,7 @@ module NotesHelper end end - def link_to_reply_diff(note) + def link_to_reply_diff(note, line_type = nil) return unless current_user data = { @@ -77,7 +78,8 @@ module NotesHelper noteable_id: note.noteable_id, commit_id: note.commit_id, 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', diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index fc91f71e8d2..60b112e67d4 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -1,4 +1,4 @@ - page_title "#{@commit.title} (#{@commit.short_id})", "Commits" = render "commit_box" = render "projects/diffs/diffs", diffs: @diffs, project: @project -= render "projects/notes/notes_with_form" += render "projects/notes/notes_with_form", view: params[:view] diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 75f3a80f0d7..cb41dd852d3 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -18,6 +18,8 @@ - elsif type_left == 'old' || type_left.nil? %td.old_line{id: line_code_left, class: "#{type_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 - if type_right == 'new' @@ -29,12 +31,14 @@ %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 + - 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 - if @reply_allowed - comments_left, comments_right = organize_comments(type_left, type_right, line_code_left, line_code_right) - 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? .file-mode-changed diff --git a/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml b/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml index 789f3e19fd2..c6726cbafa3 100644 --- a/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml +++ b/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml @@ -1,34 +1,34 @@ -- note1 = notes1.present? ? notes1.first : nil -- note2 = notes2.present? ? notes2.first : nil +- note1 = notes_left.present? ? notes_left.first : nil +- note2 = notes_right.present? ? notes_right.first : nil %tr.notes_holder - if note1 - %td.notes_line + %td.notes_line.old %span.btn.disabled %i.fa.fa-comment - = notes1.count - %td.notes_content.parallel + = notes_left.count + %td.notes_content.parallel.old %ul.notes{ rel: note1.discussion_id } - = render notes1 + = render notes_left .discussion-reply-holder - = link_to_reply_diff(note1) + = link_to_reply_diff(note1, 'old') - else - %td= "" - %td= "" + %td.notes_line.old= "" + %td.notes_content.parallel.old= "" - if note2 - %td.notes_line + %td.notes_line.new %span.btn.disabled %i.fa.fa-comment - = notes2.count - %td.notes_content.parallel + = notes_right.count + %td.notes_content.parallel.new %ul.notes{ rel: note2.discussion_id } - = render notes2 + = render notes_right .discussion-reply-holder - = link_to_reply_diff(note2) + = link_to_reply_diff(note2, 'new') - else - %td= "" - %td= "" + %td.notes_line.new= "" + %td.notes_content.parallel.new= "" diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index f28b3e9b508..3fb044d736e 100644 --- a/app/views/projects/notes/_form.html.haml +++ b/app/views/projects/notes/_form.html.haml @@ -1,4 +1,6 @@ = 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) = f.hidden_field :commit_id = f.hidden_field :line_code diff --git a/app/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml index 813e37276bd..a202e74a892 100644 --- a/app/views/projects/notes/_notes_with_form.html.haml +++ b/app/views/projects/notes/_notes_with_form.html.haml @@ -4,7 +4,7 @@ .js-main-target-form - if can? current_user, :write_note, @project - = render "projects/notes/form" + = render "projects/notes/form", view: params[:view] :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]}") diff --git a/features/project/commits/diff_comments.feature b/features/project/commits/diff_comments.feature index 56b9a13678d..4a2b870e082 100644 --- a/features/project/commits/diff_comments.feature +++ b/features/project/commits/diff_comments.feature @@ -77,3 +77,17 @@ Feature: Project Commits Diff Comments And I submit the diff comment Then I should not see the diff comment form 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" diff --git a/features/steps/project/commits/commits.rb b/features/steps/project/commits/commits.rb index 4b19e3beed4..e6330ec457e 100644 --- a/features/steps/project/commits/commits.rb +++ b/features/steps/project/commits/commits.rb @@ -2,6 +2,7 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps include SharedAuthentication include SharedProject include SharedPaths + include SharedDiffNote include RepoHelpers step 'I see project commits' do @@ -88,14 +89,6 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps expect(links[1]['href']).to match %r{blob/#{sample_image_commit.new_blob_id}} 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 expect(page).to have_content "Inline" end diff --git a/features/steps/shared/diff_note.rb b/features/steps/shared/diff_note.rb index a716ca5837c..c4f89ca31c9 100644 --- a/features/steps/shared/diff_note.rb +++ b/features/steps/shared/diff_note.rb @@ -28,6 +28,22 @@ module SharedDiffNote 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 click_diff_line(sample_commit.line_code) page.within("#{diff_file_selector} form[rel$='#{sample_commit.line_code}']") do @@ -102,6 +118,18 @@ module SharedDiffNote 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 page.within(diff_file_selector) do expect(page).to have_button('Reply') @@ -157,6 +185,14 @@ module SharedDiffNote 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 ".diff-file:nth-of-type(1)" end @@ -164,4 +200,8 @@ module SharedDiffNote def click_diff_line(code) find("button[data-line-code='#{code}']").click end + + def click_parallel_diff_line(code, line_type) + find("button[data-line-code='#{code}'][data-line-type='#{line_type}']").trigger('click') + end end |