Commit 56b52709 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'fix-comment-on-diff-ajax-loading'

parents 406c9124 82571eab
...@@ -6,6 +6,8 @@ v 8.5.0 (unreleased) ...@@ -6,6 +6,8 @@ v 8.5.0 (unreleased)
- New UI for pagination - New UI for pagination
v 8.4.0 v 8.4.0
v 8.4.0 (unreleased)
- Fix diff comments loaded by AJAX to load comment with diff in discussion tab
- Allow LDAP users to change their email if it was not set by the LDAP server - Allow LDAP users to change their email if it was not set by the LDAP server
- Ensure Gravatar host looks like an actual host - Ensure Gravatar host looks like an actual host
- Consider re-assign as a mention from a notification point of view - Consider re-assign as a mention from a notification point of view
......
...@@ -15,6 +15,8 @@ class @Notes ...@@ -15,6 +15,8 @@ class @Notes
@last_fetched_at = last_fetched_at @last_fetched_at = last_fetched_at
@view = view @view = view
@noteable_url = document.URL @noteable_url = document.URL
@notesCountBadge ||= $(".issuable-details").find(".notes-tab .badge")
@initRefresh() @initRefresh()
@setupMainTargetNoteForm() @setupMainTargetNoteForm()
@cleanBinding() @cleanBinding()
...@@ -89,7 +91,7 @@ class @Notes ...@@ -89,7 +91,7 @@ class @Notes
, 15000 , 15000
refresh: -> refresh: ->
unless document.hidden or (@noteable_url != document.URL) if not document.hidden and document.URL.indexOf(@noteable_url) is 0
@getContent() @getContent()
getContent: -> getContent: ->
...@@ -101,7 +103,10 @@ class @Notes ...@@ -101,7 +103,10 @@ class @Notes
notes = data.notes notes = data.notes
@last_fetched_at = data.last_fetched_at @last_fetched_at = data.last_fetched_at
$.each notes, (i, note) => $.each notes, (i, note) =>
@renderNote(note) if note.discussion_with_diff_html?
@renderDiscussionNote(note)
else
@renderNote(note)
### ###
...@@ -116,18 +121,21 @@ class @Notes ...@@ -116,18 +121,21 @@ class @Notes
flash.pinTo('.header-content') flash.pinTo('.header-content')
return return
if note.award
awards_handler.addAwardToEmojiBar(note.note)
awards_handler.scrollToAwards()
# render note if it not present in loaded list # render note if it not present in loaded list
# or skip if rendered # or skip if rendered
if @isNewNote(note) && !note.award else if @isNewNote(note)
@note_ids.push(note.id) @note_ids.push(note.id)
$('ul.main-notes-list').
append(note.html). $('ul.main-notes-list')
syntaxHighlight() .append(note.html)
.syntaxHighlight()
@initTaskList() @initTaskList()
@updateNotesCount(1)
if note.award
awards_handler.addAwardToEmojiBar(note.note)
awards_handler.scrollToAwards()
### ###
Check if note does not exists on page Check if note does not exists on page
...@@ -144,34 +152,39 @@ class @Notes ...@@ -144,34 +152,39 @@ class @Notes
Note: for rendering inline notes use renderDiscussionNote Note: for rendering inline notes use renderDiscussionNote
### ###
renderDiscussionNote: (note) -> renderDiscussionNote: (note) ->
return unless @isNewNote(note)
@note_ids.push(note.id) @note_ids.push(note.id)
form = $("form[rel='" + note.discussion_id + "']") form = $("#new-discussion-note-form-#{note.discussion_id}")
row = form.closest("tr") row = form.closest("tr")
note_html = $(note.html) note_html = $(note.html)
note_html.syntaxHighlight() note_html.syntaxHighlight()
# is this the first note of discussion? # is this the first note of discussion?
if row.is(".js-temp-notes-holder") discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']")
if discussionContainer.length is 0
# insert the note and the reply button after the temp row # insert the note and the reply button after the temp row
row.after note.discussion_html row.after note.discussion_html
# remove the note (will be added again below) # remove the note (will be added again below)
row.next().find(".note").remove() row.next().find(".note").remove()
# Before that, the container didn't exist
discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']")
# Add note to 'Changes' page discussions # Add note to 'Changes' page discussions
$(".notes[rel='" + note.discussion_id + "']").append note_html discussionContainer.append note_html
# Init discussion on 'Discussion' page if it is merge request page # Init discussion on 'Discussion' page if it is merge request page
if $('body').attr('data-page').indexOf('projects:merge_request') == 0 if $('body').attr('data-page').indexOf('projects:merge_request') is 0
discussion_html = $(note.discussion_with_diff_html) $('ul.main-notes-list')
discussion_html.syntaxHighlight() .append(note.discussion_with_diff_html)
$('ul.main-notes-list').append(discussion_html) .syntaxHighlight()
else else
# append new note to all matching discussions # append new note to all matching discussions
$(".notes[rel='" + note.discussion_id + "']").append note_html discussionContainer.append note_html
# cleanup after successfully creating a diff/discussion note @updateNotesCount(1)
@removeDiscussionNoteForm(form)
### ###
Called in response the main target form has been successfully submitted. Called in response the main target form has been successfully submitted.
...@@ -278,6 +291,9 @@ class @Notes ...@@ -278,6 +291,9 @@ class @Notes
addDiscussionNote: (xhr, note, status) => addDiscussionNote: (xhr, note, status) =>
@renderDiscussionNote(note) @renderDiscussionNote(note)
# cleanup after successfully creating a diff/discussion note
@removeDiscussionNoteForm($("#new-discussion-note-form-#{note.discussion_id}"))
### ###
Called in response to the edit note form being submitted Called in response to the edit note form being submitted
...@@ -349,30 +365,32 @@ class @Notes ...@@ -349,30 +365,32 @@ class @Notes
Removes the actual note from view. Removes the actual note from view.
Removes the whole discussion if the last note is being removed. Removes the whole discussion if the last note is being removed.
### ###
removeNote: -> removeNote: (e) =>
note = $(this).closest(".note") noteId = $(e.currentTarget)
note_id = note.attr('id') .closest(".note")
.attr("id")
$('.note[id="' + note_id + '"]').each -> # A same note appears in the "Discussion" and in the "Changes" tab, we have
note = $(this) # to remove all. Using $(".note[id='noteId']") ensure we get all the notes,
# where $("#noteId") would return only one.
$(".note[id='#{noteId}']").each (i, el) =>
note = $(el)
notes = note.closest(".notes") notes = note.closest(".notes")
count = notes.closest(".issuable-details").find(".notes-tab .badge")
# check if this is the last note for this line # check if this is the last note for this line
if notes.find(".note").length is 1 if notes.find(".note").length is 1
# for discussions # "Discussions" tab
notes.closest(".discussion").remove() notes.closest(".timeline-entry").remove()
# for diff lines # "Changes" tab / commit view
notes.closest("tr").remove() notes.closest("tr").remove()
# update notes count
oldNum = parseInt(count.text())
count.text(oldNum - 1)
note.remove() note.remove()
# Decrement the "Discussions" counter only once
@updateNotesCount(-1)
### ###
Called in response to clicking the delete attachment link Called in response to clicking the delete attachment link
...@@ -412,7 +430,7 @@ class @Notes ...@@ -412,7 +430,7 @@ class @Notes
### ###
setupDiscussionNoteForm: (dataHolder, form) => setupDiscussionNoteForm: (dataHolder, form) =>
# setup note target # setup note target
form.attr "rel", dataHolder.data("discussionId") form.attr 'id', "new-discussion-note-form-#{dataHolder.data("discussionId")}"
form.find("#line_type").val dataHolder.data("lineType") 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")
...@@ -542,3 +560,6 @@ class @Notes ...@@ -542,3 +560,6 @@ class @Notes
updateTaskList: -> updateTaskList: ->
$('form', this).submit() $('form', this).submit()
updateNotesCount: (updateCount) ->
@notesCountBadge.text(parseInt(@notesCountBadge.text()) + updateCount)
...@@ -11,11 +11,9 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -11,11 +11,9 @@ class Projects::NotesController < Projects::ApplicationController
notes_json = { notes: [], last_fetched_at: current_fetched_at } notes_json = { notes: [], last_fetched_at: current_fetched_at }
@notes.each do |note| @notes.each do |note|
notes_json[:notes] << { next if note.cross_reference_not_visible_for?(current_user)
id: note.id,
html: note_to_html(note), notes_json[:notes] << note_json(note)
valid: note.valid?
}
end end
render json: notes_json render json: notes_json
...@@ -25,7 +23,7 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -25,7 +23,7 @@ class Projects::NotesController < Projects::ApplicationController
@note = Notes::CreateService.new(project, current_user, note_params).execute @note = Notes::CreateService.new(project, current_user, note_params).execute
respond_to do |format| respond_to do |format|
format.json { render_note_json(@note) } format.json { render json: note_json(@note) }
format.html { redirect_back_or_default } format.html { redirect_back_or_default }
end end
end end
...@@ -34,7 +32,7 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -34,7 +32,7 @@ class Projects::NotesController < Projects::ApplicationController
@note = Notes::UpdateService.new(project, current_user, note_params).execute(note) @note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
respond_to do |format| respond_to do |format|
format.json { render_note_json(@note) } format.json { render json: note_json(@note) }
format.html { redirect_back_or_default } format.html { redirect_back_or_default }
end end
end end
...@@ -99,6 +97,8 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -99,6 +97,8 @@ class Projects::NotesController < Projects::ApplicationController
end end
def note_to_discussion_html(note) def note_to_discussion_html(note)
return unless note.for_diff_line?
if params[:view] == 'parallel' if params[:view] == 'parallel'
template = "projects/notes/_diff_notes_with_reply_parallel" template = "projects/notes/_diff_notes_with_reply_parallel"
locals = locals =
...@@ -131,9 +131,9 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -131,9 +131,9 @@ class Projects::NotesController < Projects::ApplicationController
) )
end end
def render_note_json(note) def note_json(note)
if note.valid? if note.valid?
render json: { {
valid: true, valid: true,
id: note.id, id: note.id,
discussion_id: note.discussion_id, discussion_id: note.discussion_id,
...@@ -144,7 +144,7 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -144,7 +144,7 @@ class Projects::NotesController < Projects::ApplicationController
discussion_with_diff_html: note_to_discussion_with_diff_html(note) discussion_with_diff_html: note_to_discussion_with_diff_html(note)
} }
else else
render json: { {
valid: false, valid: false,
award: note.is_award, award: note.is_award,
errors: note.errors errors: note.errors
...@@ -163,8 +163,6 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -163,8 +163,6 @@ class Projects::NotesController < Projects::ApplicationController
) )
end end
private
def find_current_user_notes def find_current_user_notes
@notes = NotesFinder.new.execute(project, current_user, params) @notes = NotesFinder.new.execute(project, current_user, params)
end end
......
...@@ -7,7 +7,7 @@ ...@@ -7,7 +7,7 @@
%i.fa.fa-comment %i.fa.fa-comment
= notes.count = notes.count
%td.notes_content %td.notes_content
%ul.notes{ rel: note.discussion_id } %ul.notes{ data: { discussion_id: note.discussion_id } }
= render notes = render notes
.discussion-reply-holder .discussion-reply-holder
= link_to_reply_diff(note) = link_to_reply_diff(note)
...@@ -8,7 +8,7 @@ ...@@ -8,7 +8,7 @@
%i.fa.fa-comment %i.fa.fa-comment
= notes_left.count = notes_left.count
%td.notes_content.parallel.old %td.notes_content.parallel.old
%ul.notes{ rel: note1.discussion_id } %ul.notes{ data: { discussion_id: note1.discussion_id } }
= render notes_left = render notes_left
.discussion-reply-holder .discussion-reply-holder
...@@ -23,7 +23,7 @@ ...@@ -23,7 +23,7 @@
%i.fa.fa-comment %i.fa.fa-comment
= notes_right.count = notes_right.count
%td.notes_content.parallel.new %td.notes_content.parallel.new
%ul.notes{ rel: note2.discussion_id } %ul.notes{ data: { discussion_id: note2.discussion_id } }
= render notes_right = render notes_right
.discussion-reply-holder .discussion-reply-holder
...@@ -31,4 +31,3 @@ ...@@ -31,4 +31,3 @@
- else - else
%td.notes_line.new= "" %td.notes_line.new= ""
%td.notes_content.parallel.new= "" %td.notes_content.parallel.new= ""
%li.timeline-entry{ id: dom_id(note), class: [dom_class(note), "note-row-#{note.id}", ('system-note' if note.system)], data: { discussion: note.discussion_id } } %li.timeline-entry{ id: dom_id(note), class: [dom_class(note), "note-row-#{note.id}", ('system-note' if note.system)] }
.timeline-entry-inner .timeline-entry-inner
.timeline-icon .timeline-icon
%a{href: user_path(note.author)} %a{href: user_path(note.author)}
......
...@@ -20,8 +20,7 @@ ...@@ -20,8 +20,7 @@
= render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note = render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
- else - else
.panel.panel-default .panel.panel-default
.notes{ rel: discussion_notes.first.discussion_id } .notes{ data: { discussion_id: discussion_notes.first.discussion_id } }
= render discussion_notes = render discussion_notes
.discussion-reply-holder .discussion-reply-holder
= link_to_reply_diff(discussion_notes.first) = link_to_reply_diff(discussion_notes.first)
...@@ -102,6 +102,15 @@ Feature: Project Merge Requests ...@@ -102,6 +102,15 @@ Feature: Project Merge Requests
And I leave a comment like "Line is wrong" on diff And I leave a comment like "Line is wrong" on diff
And I switch to the merge request's comments tab And I switch to the merge request's comments tab
Then I should see a discussion has started on diff Then I should see a discussion has started on diff
And I should see a badge of "1" next to the discussion link
@javascript
Scenario: I see a new comment on merge request diff from another user in the discussion tab
Given project "Shop" have "Bug NS-05" open merge request with diffs inside
And I visit merge request page "Bug NS-05"
And user "John Doe" leaves a comment like "Line is wrong" on diff
Then I should see a discussion by user "John Doe" has started on diff
And I should see a badge of "1" next to the discussion link
@javascript @javascript
Scenario: I edit a comment on a merge request diff Scenario: I edit a comment on a merge request diff
...@@ -119,9 +128,11 @@ Feature: Project Merge Requests ...@@ -119,9 +128,11 @@ Feature: Project Merge Requests
And I visit merge request page "Bug NS-05" And I visit merge request page "Bug NS-05"
And I click on the Changes tab And I click on the Changes tab
And I leave a comment like "Line is wrong" on diff And I leave a comment like "Line is wrong" on diff
And I should see a badge of "1" next to the discussion link
And I delete the comment "Line is wrong" on diff And I delete the comment "Line is wrong" on diff
And I click on the Discussion tab And I click on the Discussion tab
Then I should not see any discussion Then I should not see any discussion
And I should see a badge of "0" next to the discussion link
@javascript @javascript
Scenario: I comment on a line of a commit in merge request Scenario: I comment on a line of a commit in merge request
......
...@@ -181,6 +181,15 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps ...@@ -181,6 +181,15 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
leave_comment "Line is wrong" leave_comment "Line is wrong"
end end
step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do
mr = MergeRequest.find_by(title: "Bug NS-05")
create(:note_on_merge_request_diff, project: project,
noteable_id: mr.id,
author: user_exists("John Doe"),
line_code: sample_commit.line_code,
note: 'Line is wrong')
end
step 'I leave a comment like "Line is wrong" on diff in commit' do step 'I leave a comment like "Line is wrong" on diff in commit' do
click_diff_line(sample_commit.line_code) click_diff_line(sample_commit.line_code)
leave_comment "Line is wrong" leave_comment "Line is wrong"
...@@ -238,6 +247,22 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps ...@@ -238,6 +247,22 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
end end
end end
step 'I should see a discussion by user "John Doe" has started on diff' do
page.within(".notes .discussion") do
page.should have_content "#{user_exists("John Doe").name} started a discussion"
page.should have_content sample_commit.line_code_path
page.should have_content "Line is wrong"
end
end
step 'I should see a badge of "1" next to the discussion link' do
expect_discussion_badge_to_have_counter("1")
end
step 'I should see a badge of "0" next to the discussion link' do
expect_discussion_badge_to_have_counter("0")
end
step 'I should see a discussion has started on commit diff' do step 'I should see a discussion has started on commit diff' do
page.within(".notes .discussion") do page.within(".notes .discussion") do
page.should have_content "#{current_user.name} started a discussion on commit" page.should have_content "#{current_user.name} started a discussion on commit"
...@@ -452,4 +477,10 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps ...@@ -452,4 +477,10 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
def have_visible_content (text) def have_visible_content (text)
have_css("*", text: text, visible: true) have_css("*", text: text, visible: true)
end end
def expect_discussion_badge_to_have_counter(value)
page.within(".notes-tab .badge") do
page.should have_content value
end
end
end end
...@@ -23,7 +23,7 @@ module SharedDiffNote ...@@ -23,7 +23,7 @@ module SharedDiffNote
page.within(diff_file_selector) do page.within(diff_file_selector) do
click_diff_line(sample_commit.line_code) click_diff_line(sample_commit.line_code)
page.within("form[rel$='#{sample_commit.line_code}']") do page.within("form[id$='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "Typo, please fix" fill_in "note[note]", with: "Typo, please fix"
find(".js-comment-button").trigger("click") find(".js-comment-button").trigger("click")
sleep 0.05 sleep 0.05
...@@ -33,7 +33,7 @@ module SharedDiffNote ...@@ -33,7 +33,7 @@ module SharedDiffNote
step 'I leave a diff comment in a parallel view on the left side like "Old comment"' do 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') click_parallel_diff_line(sample_commit.line_code, 'old')
page.within("#{diff_file_selector} form[rel$='#{sample_commit.line_code}']") do page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "Old comment" fill_in "note[note]", with: "Old comment"
find(".js-comment-button").trigger("click") find(".js-comment-button").trigger("click")
end end
...@@ -41,7 +41,7 @@ module SharedDiffNote ...@@ -41,7 +41,7 @@ module SharedDiffNote
step 'I leave a diff comment in a parallel view on the right side like "New comment"' do 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') click_parallel_diff_line(sample_commit.line_code, 'new')
page.within("#{diff_file_selector} form[rel$='#{sample_commit.line_code}']") do page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "New comment" fill_in "note[note]", with: "New comment"
find(".js-comment-button").trigger("click") find(".js-comment-button").trigger("click")
end end
...@@ -51,7 +51,7 @@ module SharedDiffNote ...@@ -51,7 +51,7 @@ module SharedDiffNote
page.within(diff_file_selector) do page.within(diff_file_selector) do
click_diff_line(sample_commit.line_code) click_diff_line(sample_commit.line_code)
page.within("form[rel$='#{sample_commit.line_code}']") do page.within("form[id$='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "Should fix it :smile:" fill_in "note[note]", with: "Should fix it :smile:"
find('.js-md-preview-button').click find('.js-md-preview-button').click
end end
...@@ -62,7 +62,7 @@ module SharedDiffNote ...@@ -62,7 +62,7 @@ module SharedDiffNote
page.within(diff_file_selector) do page.within(diff_file_selector) do
click_diff_line(sample_commit.del_line_code) click_diff_line(sample_commit.del_line_code)
page.within("form[rel$='#{sample_commit.del_line_code}']") do page.within("form[id$='#{sample_commit.del_line_code}']") do
fill_in "note[note]", with: "DRY this up" fill_in "note[note]", with: "DRY this up"
find('.js-md-preview-button').click find('.js-md-preview-button').click
end end
...@@ -91,7 +91,7 @@ module SharedDiffNote ...@@ -91,7 +91,7 @@ module SharedDiffNote
page.within(diff_file_selector) do page.within(diff_file_selector) do
click_diff_line(sample_commit.line_code) click_diff_line(sample_commit.line_code)
page.within("form[rel$='#{sample_commit.line_code}']") do page.within("form[id$='#{sample_commit.line_code}']") do
fill_in 'note[note]', with: ':smile:' fill_in 'note[note]', with: ':smile:'
click_button('Add Comment') click_button('Add Comment')
end end
......
...@@ -167,7 +167,7 @@ describe 'Comments', feature: true do ...@@ -167,7 +167,7 @@ describe 'Comments', feature: true do
end end
it 'should be removed when canceled' do it 'should be removed when canceled' do
page.within(".diff-file form[rel$='#{line_code}']") do page.within(".diff-file form[id$='#{line_code}']") do
find('.js-close-discussion-note-form').trigger('click') find('.js-close-discussion-note-form').trigger('click')
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