summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--CHANGELOG2
-rw-r--r--app/assets/javascripts/notes.js.coffee87
-rw-r--r--app/controllers/projects/notes_controller.rb22
-rw-r--r--app/views/projects/notes/_diff_notes_with_reply.html.haml2
-rw-r--r--app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml5
-rw-r--r--app/views/projects/notes/_note.html.haml2
-rw-r--r--app/views/projects/notes/discussions/_commit.html.haml3
-rw-r--r--features/project/merge_requests.feature11
-rw-r--r--features/steps/project/merge_requests.rb31
-rw-r--r--features/steps/shared/diff_note.rb12
-rw-r--r--spec/features/notes_on_merge_requests_spec.rb2
11 files changed, 120 insertions, 59 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 7c14a922c88..a8fee206890 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -6,6 +6,8 @@ v 8.5.0 (unreleased)
- New UI for pagination
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
- Ensure Gravatar host looks like an actual host
- Consider re-assign as a mention from a notification point of view
diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee
index 2bfc5cb2d9c..53d72be66e3 100644
--- a/app/assets/javascripts/notes.js.coffee
+++ b/app/assets/javascripts/notes.js.coffee
@@ -15,6 +15,8 @@ class @Notes
@last_fetched_at = last_fetched_at
@view = view
@noteable_url = document.URL
+ @notesCountBadge ||= $(".issuable-details").find(".notes-tab .badge")
+
@initRefresh()
@setupMainTargetNoteForm()
@cleanBinding()
@@ -89,7 +91,7 @@ class @Notes
, 15000
refresh: ->
- unless document.hidden or (@noteable_url != document.URL)
+ if not document.hidden and document.URL.indexOf(@noteable_url) is 0
@getContent()
getContent: ->
@@ -101,7 +103,10 @@ class @Notes
notes = data.notes
@last_fetched_at = data.last_fetched_at
$.each notes, (i, note) =>
- @renderNote(note)
+ if note.discussion_with_diff_html?
+ @renderDiscussionNote(note)
+ else
+ @renderNote(note)
###
@@ -116,18 +121,21 @@ class @Notes
flash.pinTo('.header-content')
return
+ if note.award
+ awards_handler.addAwardToEmojiBar(note.note)
+ awards_handler.scrollToAwards()
+
# render note if it not present in loaded list
# or skip if rendered
- if @isNewNote(note) && !note.award
+ else if @isNewNote(note)
@note_ids.push(note.id)
- $('ul.main-notes-list').
- append(note.html).
- syntaxHighlight()
+
+ $('ul.main-notes-list')
+ .append(note.html)
+ .syntaxHighlight()
@initTaskList()
+ @updateNotesCount(1)
- if note.award
- awards_handler.addAwardToEmojiBar(note.note)
- awards_handler.scrollToAwards()
###
Check if note does not exists on page
@@ -144,34 +152,39 @@ class @Notes
Note: for rendering inline notes use renderDiscussionNote
###
renderDiscussionNote: (note) ->
+ return unless @isNewNote(note)
+
@note_ids.push(note.id)
- form = $("form[rel='" + note.discussion_id + "']")
+ form = $("#new-discussion-note-form-#{note.discussion_id}")
row = form.closest("tr")
note_html = $(note.html)
note_html.syntaxHighlight()
# 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
row.after note.discussion_html
# remove the note (will be added again below)
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
- $(".notes[rel='" + note.discussion_id + "']").append note_html
+ discussionContainer.append note_html
# Init discussion on 'Discussion' page if it is merge request page
- if $('body').attr('data-page').indexOf('projects:merge_request') == 0
- discussion_html = $(note.discussion_with_diff_html)
- discussion_html.syntaxHighlight()
- $('ul.main-notes-list').append(discussion_html)
+ if $('body').attr('data-page').indexOf('projects:merge_request') is 0
+ $('ul.main-notes-list')
+ .append(note.discussion_with_diff_html)
+ .syntaxHighlight()
else
# 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
- @removeDiscussionNoteForm(form)
+ @updateNotesCount(1)
###
Called in response the main target form has been successfully submitted.
@@ -278,6 +291,9 @@ class @Notes
addDiscussionNote: (xhr, note, status) =>
@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
@@ -349,30 +365,32 @@ class @Notes
Removes the actual note from view.
Removes the whole discussion if the last note is being removed.
###
- removeNote: ->
- note = $(this).closest(".note")
- note_id = note.attr('id')
+ removeNote: (e) =>
+ noteId = $(e.currentTarget)
+ .closest(".note")
+ .attr("id")
- $('.note[id="' + note_id + '"]').each ->
- note = $(this)
+ # A same note appears in the "Discussion" and in the "Changes" tab, we have
+ # 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")
- count = notes.closest(".issuable-details").find(".notes-tab .badge")
# check if this is the last note for this line
if notes.find(".note").length is 1
- # for discussions
- notes.closest(".discussion").remove()
+ # "Discussions" tab
+ notes.closest(".timeline-entry").remove()
- # for diff lines
+ # "Changes" tab / commit view
notes.closest("tr").remove()
- # update notes count
- oldNum = parseInt(count.text())
- count.text(oldNum - 1)
-
note.remove()
+ # Decrement the "Discussions" counter only once
+ @updateNotesCount(-1)
+
###
Called in response to clicking the delete attachment link
@@ -412,7 +430,7 @@ class @Notes
###
setupDiscussionNoteForm: (dataHolder, form) =>
# 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("#note_commit_id").val dataHolder.data("commitId")
form.find("#note_line_code").val dataHolder.data("lineCode")
@@ -542,3 +560,6 @@ class @Notes
updateTaskList: ->
$('form', this).submit()
+
+ updateNotesCount: (updateCount) ->
+ @notesCountBadge.text(parseInt(@notesCountBadge.text()) + updateCount)
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 6f1e186d408..4a2599dda37 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -11,11 +11,9 @@ class Projects::NotesController < Projects::ApplicationController
notes_json = { notes: [], last_fetched_at: current_fetched_at }
@notes.each do |note|
- notes_json[:notes] << {
- id: note.id,
- html: note_to_html(note),
- valid: note.valid?
- }
+ next if note.cross_reference_not_visible_for?(current_user)
+
+ notes_json[:notes] << note_json(note)
end
render json: notes_json
@@ -25,7 +23,7 @@ class Projects::NotesController < Projects::ApplicationController
@note = Notes::CreateService.new(project, current_user, note_params).execute
respond_to do |format|
- format.json { render_note_json(@note) }
+ format.json { render json: note_json(@note) }
format.html { redirect_back_or_default }
end
end
@@ -34,7 +32,7 @@ class Projects::NotesController < Projects::ApplicationController
@note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
respond_to do |format|
- format.json { render_note_json(@note) }
+ format.json { render json: note_json(@note) }
format.html { redirect_back_or_default }
end
end
@@ -99,6 +97,8 @@ class Projects::NotesController < Projects::ApplicationController
end
def note_to_discussion_html(note)
+ return unless note.for_diff_line?
+
if params[:view] == 'parallel'
template = "projects/notes/_diff_notes_with_reply_parallel"
locals =
@@ -131,9 +131,9 @@ class Projects::NotesController < Projects::ApplicationController
)
end
- def render_note_json(note)
+ def note_json(note)
if note.valid?
- render json: {
+ {
valid: true,
id: note.id,
discussion_id: note.discussion_id,
@@ -144,7 +144,7 @@ class Projects::NotesController < Projects::ApplicationController
discussion_with_diff_html: note_to_discussion_with_diff_html(note)
}
else
- render json: {
+ {
valid: false,
award: note.is_award,
errors: note.errors
@@ -163,8 +163,6 @@ class Projects::NotesController < Projects::ApplicationController
)
end
- private
-
def find_current_user_notes
@notes = NotesFinder.new.execute(project, current_user, params)
end
diff --git a/app/views/projects/notes/_diff_notes_with_reply.html.haml b/app/views/projects/notes/_diff_notes_with_reply.html.haml
index c731baf0a65..11f9859a90f 100644
--- a/app/views/projects/notes/_diff_notes_with_reply.html.haml
+++ b/app/views/projects/notes/_diff_notes_with_reply.html.haml
@@ -7,7 +7,7 @@
%i.fa.fa-comment
= notes.count
%td.notes_content
- %ul.notes{ rel: note.discussion_id }
+ %ul.notes{ data: { discussion_id: note.discussion_id } }
= render notes
.discussion-reply-holder
= link_to_reply_diff(note)
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 c6726cbafa3..bb761ed2f94 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
@@ -8,7 +8,7 @@
%i.fa.fa-comment
= notes_left.count
%td.notes_content.parallel.old
- %ul.notes{ rel: note1.discussion_id }
+ %ul.notes{ data: { discussion_id: note1.discussion_id } }
= render notes_left
.discussion-reply-holder
@@ -23,7 +23,7 @@
%i.fa.fa-comment
= notes_right.count
%td.notes_content.parallel.new
- %ul.notes{ rel: note2.discussion_id }
+ %ul.notes{ data: { discussion_id: note2.discussion_id } }
= render notes_right
.discussion-reply-holder
@@ -31,4 +31,3 @@
- else
%td.notes_line.new= ""
%td.notes_content.parallel.new= ""
-
diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml
index 922535e5c4a..e858c412836 100644
--- a/app/views/projects/notes/_note.html.haml
+++ b/app/views/projects/notes/_note.html.haml
@@ -1,4 +1,4 @@
-%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-icon
%a{href: user_path(note.author)}
diff --git a/app/views/projects/notes/discussions/_commit.html.haml b/app/views/projects/notes/discussions/_commit.html.haml
index 6903fad4a0a..3da2f2060b8 100644
--- a/app/views/projects/notes/discussions/_commit.html.haml
+++ b/app/views/projects/notes/discussions/_commit.html.haml
@@ -20,8 +20,7 @@
= render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
- else
.panel.panel-default
- .notes{ rel: discussion_notes.first.discussion_id }
+ .notes{ data: { discussion_id: discussion_notes.first.discussion_id } }
= render discussion_notes
.discussion-reply-holder
= link_to_reply_diff(discussion_notes.first)
-
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index f1629a26f10..4f780aa680f 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -102,6 +102,15 @@ Feature: Project Merge Requests
And I leave a comment like "Line is wrong" on diff
And I switch to the merge request's comments tab
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
Scenario: I edit a comment on a merge request diff
@@ -119,9 +128,11 @@ Feature: Project Merge Requests
And I visit merge request page "Bug NS-05"
And I click on the Changes tab
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 click on the Discussion tab
Then I should not see any discussion
+ And I should see a badge of "0" next to the discussion link
@javascript
Scenario: I comment on a line of a commit in merge request
diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index 8af635689e0..337893e6209 100644
--- a/features/steps/project/merge_requests.rb
+++ b/features/steps/project/merge_requests.rb
@@ -181,6 +181,15 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
leave_comment "Line is wrong"
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
click_diff_line(sample_commit.line_code)
leave_comment "Line is wrong"
@@ -238,6 +247,22 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
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
page.within(".notes .discussion") do
page.should have_content "#{current_user.name} started a discussion on commit"
@@ -452,4 +477,10 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
def have_visible_content (text)
have_css("*", text: text, visible: true)
end
+
+ def expect_discussion_badge_to_have_counter(value)
+ page.within(".notes-tab .badge") do
+ page.should have_content value
+ end
+ end
end
diff --git a/features/steps/shared/diff_note.rb b/features/steps/shared/diff_note.rb
index c6a0ae2ba38..06e69441894 100644
--- a/features/steps/shared/diff_note.rb
+++ b/features/steps/shared/diff_note.rb
@@ -23,7 +23,7 @@ module SharedDiffNote
page.within(diff_file_selector) do
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"
find(".js-comment-button").trigger("click")
sleep 0.05
@@ -33,7 +33,7 @@ module SharedDiffNote
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
+ page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "Old comment"
find(".js-comment-button").trigger("click")
end
@@ -41,7 +41,7 @@ module SharedDiffNote
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
+ page.within("#{diff_file_selector} form[id$='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "New comment"
find(".js-comment-button").trigger("click")
end
@@ -51,7 +51,7 @@ module SharedDiffNote
page.within(diff_file_selector) do
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:"
find('.js-md-preview-button').click
end
@@ -62,7 +62,7 @@ module SharedDiffNote
page.within(diff_file_selector) do
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"
find('.js-md-preview-button').click
end
@@ -91,7 +91,7 @@ module SharedDiffNote
page.within(diff_file_selector) do
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:'
click_button('Add Comment')
end
diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb
index f0fc6916c4d..1a360cd1ebc 100644
--- a/spec/features/notes_on_merge_requests_spec.rb
+++ b/spec/features/notes_on_merge_requests_spec.rb
@@ -167,7 +167,7 @@ describe 'Comments', feature: true do
end
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')
end
View Lorry Controller Status