From 789bae241cab6fdab00fb8ef632e7e1c9dd34025 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 9 Mar 2017 08:53:47 +0000 Subject: Fixed bugs with diff comment avatars The comment button on commit view was way out to the left side because the element that renders the diff avatars was rendering when it shouldnt be When commenting on a discussion on the discussions tab it would try to render the avatar & in some cases work correctly even though it shouldnt be possible for this to happen. Changed the if statement to take this into account Closes #29237, #29238, #29243 --- app/views/projects/diffs/_line.html.haml | 5 +++-- app/views/projects/diffs/_parallel_view.html.haml | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index ed279cfe168..b40d9cb928b 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -5,6 +5,7 @@ - line_code = diff_file.line_code(line) - if discussions && !line.meta? - discussion = discussions[line_code] + - show_discussion_avatars = discussion && discussion.resolvable? && !plain %tr.line_holder{ class: type, id: (line_code unless plain) } - case type - when 'match' @@ -14,13 +15,13 @@ %td.new_line.diff-line-num %td.line_content.match= line.text - else - %td.old_line.diff-line-num.js-avatar-container{ class: type, data: { linenumber: line.old_pos } } + %td.old_line.diff-line-num{ class: [type, ("js-avatar-container" if show_discussion_avatars)], data: { linenumber: line.old_pos } } - link_text = type == "new" ? " " : line.old_pos - if plain = link_text - else %a{ href: "##{line_code}", data: { linenumber: link_text } } - - if discussion && !plain + - if show_discussion_avatars %diff-note-avatars{ "discussion-id" => discussion.id } %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } } - link_text = type == "old" ? " " : line.new_pos diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 6448748113b..e7758c8bdfa 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -20,7 +20,7 @@ - left_position = diff_file.position(left) %td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } } %a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } } - - if discussion_left + - if discussion_left && discussion_left.resolvable? %diff-note-avatars{ "discussion-id" => discussion_left.id } %td.line_content.parallel.noteable_line{ class: left.type, data: diff_view_line_data(left_line_code, left_position, 'old') }= diff_line_content(left.text) - else @@ -39,7 +39,7 @@ - right_position = diff_file.position(right) %td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } } %a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } } - - if discussion_right + - if discussion_right && discussion_right.resolvable? %diff-note-avatars{ "discussion-id" => discussion_right.id } %td.line_content.parallel.noteable_line{ class: right.type, data: diff_view_line_data(right_line_code, right_position, 'new') }= diff_line_content(right.text) - else -- cgit v1.2.1 From 2edd42cafc821422076ec3aeaaaebd8f81b7aa4f Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 9 Mar 2017 08:59:29 +0000 Subject: Check was in wrong part, the avatar container class should always be added on lines in the changes tab --- app/views/projects/diffs/_line.html.haml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index b40d9cb928b..62135d3ae32 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -5,7 +5,6 @@ - line_code = diff_file.line_code(line) - if discussions && !line.meta? - discussion = discussions[line_code] - - show_discussion_avatars = discussion && discussion.resolvable? && !plain %tr.line_holder{ class: type, id: (line_code unless plain) } - case type - when 'match' @@ -15,13 +14,13 @@ %td.new_line.diff-line-num %td.line_content.match= line.text - else - %td.old_line.diff-line-num{ class: [type, ("js-avatar-container" if show_discussion_avatars)], data: { linenumber: line.old_pos } } + %td.old_line.diff-line-num{ class: [type, ("js-avatar-container" if !plain)], data: { linenumber: line.old_pos } } - link_text = type == "new" ? " " : line.old_pos - if plain = link_text - else %a{ href: "##{line_code}", data: { linenumber: link_text } } - - if show_discussion_avatars + - if discussion && discussion.resolvable? && !plain %diff-note-avatars{ "discussion-id" => discussion.id } %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } } - link_text = type == "old" ? " " : line.new_pos -- cgit v1.2.1 From aa9aa11de549fc2486b4d37ebfad1b3bc63b07d1 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 9 Mar 2017 09:47:01 +0000 Subject: Added regression tests --- .../merge_requests/diff_notes_avatars_spec.rb | 50 ++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb index 7df102067d6..a6c72b0b3ac 100644 --- a/spec/features/merge_requests/diff_notes_avatars_spec.rb +++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb @@ -23,6 +23,56 @@ feature 'Diff note avatars', feature: true, js: true do login_as user end + context 'discussion tab' do + before do + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + end + + it 'does not show avatars on discussion tab' do + expect(page).not_to have_selector('.js-avatar-container') + expect(page).not_to have_selector('.diff-comment-avatar-holders') + end + + it 'does not render avatars after commening on discussion tab' do + click_button 'Reply...' + + page.within('.js-discussion-note-form') do + find('.note-textarea').native.send_keys('Test comment') + + click_button 'Comment' + end + + expect(page).to have_content('Test comment') + expect(page).not_to have_selector('.js-avatar-container') + expect(page).not_to have_selector('.diff-comment-avatar-holders') + end + end + + context 'commit view' do + before do + visit namespace_project_commit_path(project.namespace, project, merge_request.commits.first.id) + end + + it 'does not render avatar after commenting' do + first('.diff-line-num').trigger('mouseover') + find('.js-add-diff-note-button').click + + page.within('.js-discussion-note-form') do + find('.note-textarea').native.send_keys('test comment') + + click_button 'Comment' + + wait_for_ajax + end + + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + + expect(page).to have_content('test comment') + expect(page).not_to have_selector('.js-avatar-container') + expect(page).not_to have_selector('.diff-comment-avatar-holders') + end + end + %w(inline parallel).each do |view| context "#{view} view" do before do -- cgit v1.2.1