diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/assets/javascripts/files_comment_button.js | 15 | ||||
| -rw-r--r-- | spec/features/merge_requests/diff_notes_spec.rb | 50 |
3 files changed, 59 insertions, 7 deletions
diff --git a/CHANGELOG b/CHANGELOG index 4ed592b580f..ef38d3e29f5 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.12.0 (unreleased) - Change merge_error column from string to text type - Optimistic locking for Issues and Merge Requests (title and description overriding prevention) + - Added tests for diff notes v 8.11.1 (unreleased) - Fix file links on project page when default view is Files !5933 diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js index b2e49b71fec..3fb3b1a8b51 100644 --- a/app/assets/javascripts/files_comment_button.js +++ b/app/assets/javascripts/files_comment_button.js @@ -39,12 +39,13 @@ FilesCommentButton.prototype.render = function(e) { var $currentTarget, buttonParentElement, lineContentElement, textFileElement; $currentTarget = $(e.currentTarget); + buttonParentElement = this.getButtonParent($currentTarget); - if (!this.shouldRender(e, buttonParentElement)) { - return; - } - textFileElement = this.getTextFileElement($currentTarget); + if (!this.validateButtonParent(buttonParentElement)) return; lineContentElement = this.getLineContent($currentTarget); + if (!this.validateLineContent(lineContentElement)) return; + + textFileElement = this.getTextFileElement($currentTarget); buttonParentElement.append(this.buildButton({ noteableType: textFileElement.attr('data-noteable-type'), noteableID: textFileElement.attr('data-noteable-id'), @@ -119,10 +120,14 @@ return newButtonParent.is(this.getButtonParent($(e.currentTarget))); }; - FilesCommentButton.prototype.shouldRender = function(e, buttonParentElement) { + FilesCommentButton.prototype.validateButtonParent = function(buttonParentElement) { return !buttonParentElement.hasClass(EMPTY_CELL_CLASS) && !buttonParentElement.hasClass(UNFOLDABLE_LINE_CLASS) && $(COMMENT_BUTTON_CLASS, buttonParentElement).length === 0; }; + FilesCommentButton.prototype.validateLineContent = function(lineContentElement) { + return lineContentElement.attr('data-discussion-id') && lineContentElement.attr('data-discussion-id') !== ''; + }; + return FilesCommentButton; })(); diff --git a/spec/features/merge_requests/diff_notes_spec.rb b/spec/features/merge_requests/diff_notes_spec.rb index b8f82d06e19..a818679a874 100644 --- a/spec/features/merge_requests/diff_notes_spec.rb +++ b/spec/features/merge_requests/diff_notes_spec.rb @@ -15,7 +15,7 @@ feature 'Diff notes', js: true, feature: true do let(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' } let(:test_note_comment) { 'this is a test note!' } - context 'when hovering over the parallel view diff file' do + context 'when hovering over a parallel view diff file' do before(:each) do visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'parallel') end @@ -69,9 +69,28 @@ feature 'Diff notes', js: true, feature: true do should_not_allow_commenting(find('.match', match: :first).find(:xpath, '..'), 'right') end end + + context 'with an unfolded line' do + before(:each) do + find('.js-unfold', match: :first).click + wait_for_ajax + end + + # The first `.js-unfold` unfolds upwards, therefore the first + # `.line_holder` will be an unfolded line. + let(:line_holder) { first('.line_holder[id="1"]') } + + it 'should not allow commenting on the left side' do + should_not_allow_commenting(line_holder, 'left') + end + + it 'should not allow commenting on the right side' do + should_not_allow_commenting(line_holder, 'right') + end + end end - context 'when hovering over the inline view diff file' do + context 'when hovering over an inline view diff file' do before do visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'inline') end @@ -99,6 +118,33 @@ feature 'Diff notes', js: true, feature: true do should_not_allow_commenting(find('.match', match: :first)) end end + + context 'with an unfolded line' do + before(:each) do + find('.js-unfold', match: :first).click + wait_for_ajax + end + + # The first `.js-unfold` unfolds upwards, therefore the first + # `.line_holder` will be an unfolded line. + let(:line_holder) { first('.line_holder[id="1"]') } + + it 'should not allow commenting' do + should_not_allow_commenting line_holder + end + end + + context 'when hovering over a diff discussion' do + before do + visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'inline') + should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) + visit namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + end + + it 'should not allow commenting' do + should_not_allow_commenting(find('.line_holder', match: :first)) + end + end end def should_allow_commenting(line_holder, diff_side = nil) |