Commit 02591b04 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'disable-commenting-on-unrelatable-diff-line' into 'master'

Disable commenting on unrelatable diff line

## What does this MR do?

Fixes a bug where users can comment on diff lines that can't be commented on and attached to.

This is the case for unfolded lines and lines that appear as snippets in the discussion tab.

**!5864 should be merged before this MR.**

## Are there points in the code the reviewer needs to double check?

😕 

## Why was this MR needed?

Comments were getting lost on the discussion feed, unable to find their related diff.

## What are the relevant issue numbers?

Closes #20633.

## Screenshots (if relevant)

![diffs-comment-fix](/uploads/f902b687fda75492e38397f9705283e0/diffs-comment-fix.mp4)

## Does this MR meet the acceptance criteria?

- [ ] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] API support added
- Tests
  - [ ] Added for this feature/bug
  - [ ] All builds are passing
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5681
parents aa6fe141 6692bca4
...@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -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) v 8.12.0 (unreleased)
- Change merge_error column from string to text type - Change merge_error column from string to text type
- Optimistic locking for Issues and Merge Requests (title and description overriding prevention) - Optimistic locking for Issues and Merge Requests (title and description overriding prevention)
- Added tests for diff notes
v 8.11.1 (unreleased) v 8.11.1 (unreleased)
- Fix file links on project page when default view is Files !5933 - Fix file links on project page when default view is Files !5933
......
...@@ -39,12 +39,13 @@ ...@@ -39,12 +39,13 @@
FilesCommentButton.prototype.render = function(e) { FilesCommentButton.prototype.render = function(e) {
var $currentTarget, buttonParentElement, lineContentElement, textFileElement; var $currentTarget, buttonParentElement, lineContentElement, textFileElement;
$currentTarget = $(e.currentTarget); $currentTarget = $(e.currentTarget);
buttonParentElement = this.getButtonParent($currentTarget); buttonParentElement = this.getButtonParent($currentTarget);
if (!this.shouldRender(e, buttonParentElement)) { if (!this.validateButtonParent(buttonParentElement)) return;
return;
}
textFileElement = this.getTextFileElement($currentTarget);
lineContentElement = this.getLineContent($currentTarget); lineContentElement = this.getLineContent($currentTarget);
if (!this.validateLineContent(lineContentElement)) return;
textFileElement = this.getTextFileElement($currentTarget);
buttonParentElement.append(this.buildButton({ buttonParentElement.append(this.buildButton({
noteableType: textFileElement.attr('data-noteable-type'), noteableType: textFileElement.attr('data-noteable-type'),
noteableID: textFileElement.attr('data-noteable-id'), noteableID: textFileElement.attr('data-noteable-id'),
...@@ -119,10 +120,14 @@ ...@@ -119,10 +120,14 @@
return newButtonParent.is(this.getButtonParent($(e.currentTarget))); 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; 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; return FilesCommentButton;
})(); })();
......
...@@ -15,7 +15,7 @@ feature 'Diff notes', js: true, feature: true do ...@@ -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(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' }
let(:test_note_comment) { 'this is a test note!' } 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 before(:each) do
visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'parallel') visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'parallel')
end end
...@@ -69,9 +69,28 @@ feature 'Diff notes', js: true, feature: true do ...@@ -69,9 +69,28 @@ feature 'Diff notes', js: true, feature: true do
should_not_allow_commenting(find('.match', match: :first).find(:xpath, '..'), 'right') should_not_allow_commenting(find('.match', match: :first).find(:xpath, '..'), 'right')
end end
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 end
context 'when hovering over the inline view diff file' do context 'when hovering over an inline view diff file' do
before do before do
visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'inline') visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'inline')
end end
...@@ -99,6 +118,33 @@ feature 'Diff notes', js: true, feature: true do ...@@ -99,6 +118,33 @@ feature 'Diff notes', js: true, feature: true do
should_not_allow_commenting(find('.match', match: :first)) should_not_allow_commenting(find('.match', match: :first))
end end
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 end
def should_allow_commenting(line_holder, diff_side = nil) def should_allow_commenting(line_holder, diff_side = nil)
......
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