Commit b2297a76 authored by Douwe Maan's avatar Douwe Maan Committed by Ruben Davila

Merge branch '21211-comment-on-diff-partially-broken-after-updating-to-8-11' into 'master'

Fix diff comments on legacy MRs

Allow diff commenting on MRs without complete diff refs. (Commenting would work before, but the JSON response would fail, so you'd only see your comment after a refresh.)

Is this really all I needed to do? I feel like there must be more to it 😊

It's a regression!

Closes #21211.

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

See merge request !6029
Conflicts:
	spec/features/merge_requests/diff_notes_spec.rb
parent e0fe6ec6
Please view this file on the master branch, on stable branches it's out of date. Please view this file on the master branch, on stable branches it's out of date.
v 8.11.3 v 8.11.3
v 8.12.0 (unreleased)
- Make push events have equal vertical spacing.
- Add two-factor recovery endpoint to internal API !5510
- Remove vendor prefixes for linear-gradient CSS (ClemMakesApps)
- Add font color contrast to external label in admin area (ClemMakesApps)
- Change logo animation to CSS (ClemMakesApps)
- Change merge_error column from string to text type
- Reduce contributions calendar data payload (ClemMakesApps)
- Add `web_url` field to issue, merge request, and snippet API objects (Ben Boeckel)
- Set path for all JavaScript cookies to honor GitLab's subdirectory setting !5627 (Mike Greiling)
- Add hover color to emoji icon (ClemMakesApps)
- Fix branches page dropdown sort alignment (ClemMakesApps)
- Add white background for no readme container (ClemMakesApps)
- Optimistic locking for Issues and Merge Requests (title and description overriding prevention)
- Add `wiki_page_events` to project hook APIs (Ben Boeckel)
- Remove Gitorious import
- Add Sentry logging to API calls
- Automatically expand hidden discussions when accessed by a permalink !5585 (Mike Greiling)
- Remove unused mixins (ClemMakesApps)
- Fix groups sort dropdown alignment (ClemMakesApps)
- Add horizontal scrolling to all sub-navs on mobile viewports (ClemMakesApps)
- Fix markdown help references (ClemMakesApps)
- Added tests for diff notes
- Add delimiter to project stars and forks count (ClemMakesApps)
- Fix badge count alignment (ClemMakesApps)
- Fix branch title trailing space on hover (ClemMakesApps)
- Award emoji tooltips containing more than 10 usernames are now truncated !4780 (jlogandavison)
- Fix duplicate "me" in award emoji tooltip !5218 (jlogandavison)
- Fix spacing and vertical alignment on build status icon on commits page (ClemMakesApps)
- Update merge_requests.md with a simpler way to check out a merge request. !5944
- Fix button missing type (ClemMakesApps)
- Move to project dropdown with infinite scroll for better performance
- Fix leaking of submit buttons outside the width of a main container !18731 (originally by @pavelloz)
- Load branches asynchronously in Cherry Pick and Revert dialogs.
- Add merge request versions !5467
- Change using size to use count and caching it for number of group members. !5935
- Added 'only_allow_merge_if_build_succeeds' project setting in the API. !5930 (Duck)
- Reduce number of database queries on builds tab
- Capitalize mentioned issue timeline notes (ClemMakesApps)
- Use the default branch for displaying the project icon instead of master !5792 (Hannes Rosenögger)
- Adds response mime type to transaction metric action when it's not HTML
- Fix hover leading space bug in pipeline graph !5980
v 8.11.4 (unreleased)
- Fix broken gitlab:backup:restore because of bad permissions on repo storage !6098 (Dirk Hörner)
v 8.11.4 (unreleased)
- Fix resolving conflicts on forks
v 8.11.4 (unreleased)
- Fix diff commenting on merge requests created prior to 8.10
v 8.11.3 (unreleased)
- Do not enforce using hash with hidden key in CI configuration. !6079
- Allow system info page to handle case where info is unavailable
- Label list shows all issues (opened or closed) with that label
- Don't show resolve conflicts link before MR status is updated - Don't show resolve conflicts link before MR status is updated
- Fix IE11 fork button bug !598 - Fix IE11 fork button bug !598
- Don't prevent viewing the MR when git refs for conflicts can't be found on disk - Don't prevent viewing the MR when git refs for conflicts can't be found on disk
......
...@@ -28,4 +28,8 @@ module NoteOnDiff ...@@ -28,4 +28,8 @@ module NoteOnDiff
def can_be_award_emoji? def can_be_award_emoji?
false false
end end
def to_discussion
Discussion.new([self])
end
end end
...@@ -107,10 +107,6 @@ class DiffNote < Note ...@@ -107,10 +107,6 @@ class DiffNote < Note
self.noteable.find_diff_discussion(self.discussion_id) self.noteable.find_diff_discussion(self.discussion_id)
end end
def to_discussion
Discussion.new([self])
end
private private
def supported? def supported?
......
require 'spec_helper'
feature 'Diff notes', js: true, feature: true do
include WaitForAjax
before do
login_as :admin
@merge_request = create(:merge_request)
@project = @merge_request.source_project
end
context 'merge request diffs' do
let(:comment_button_class) { '.add-diff-note' }
let(:notes_holder_input_class) { 'js-temp-notes-holder' }
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 a parallel view diff file' do
before(:each) do
visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'parallel')
end
context 'with an old line on the left and no line on the right' do
it 'should allow commenting on the left side' do
should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'left')
end
it 'should not allow commenting on the right side' do
should_not_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'right')
end
end
context 'with no line on the left and a new line on the right' do
it 'should not allow commenting on the left side' do
should_not_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'left')
end
it 'should allow commenting on the right side' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'right')
end
end
context 'with an old line on the left and a new line on the right' do
it 'should allow commenting on the left side' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'left')
end
it 'should allow commenting on the right side' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'right')
end
end
context 'with an unchanged line on the left and an unchanged line on the right' do
it 'should allow commenting on the left side' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'left')
end
it 'should allow commenting on the right side' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]', match: :first).find(:xpath, '..'), 'right')
end
end
context 'with a match line' do
it 'should not allow commenting on the left side' do
should_not_allow_commenting(find('.match', match: :first).find(:xpath, '..'), 'left')
end
it 'should not allow commenting on the right side' 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 an inline view diff file' do
before do
visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'inline')
end
context 'with a new line' do
it 'should allow commenting' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
end
end
context 'with an old line' do
it 'should allow commenting' do
should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
end
end
context 'with an unchanged line' do
it 'should allow commenting' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
end
end
context 'with a match line' do
it 'should not allow commenting' 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
context 'when the MR only supports legacy diff notes' do
before do
@merge_request.merge_request_diff.update_attributes(start_commit_sha: nil)
visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, view: 'inline')
end
context 'with a new line' do
it 'should allow commenting' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'))
end
end
context 'with an old line' do
it 'should allow commenting' do
should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]'))
end
end
context 'with an unchanged line' do
it 'should allow commenting' do
should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]'))
end
end
context 'with a match line' do
it 'should not allow commenting' do
should_not_allow_commenting(find('.match', match: :first))
end
end
end
def should_allow_commenting(line_holder, diff_side = nil)
line = get_line_components(line_holder, diff_side)
line[:content].hover
expect(line[:num]).to have_css comment_button_class
comment_on_line(line_holder, line)
assert_comment_persistence(line_holder)
end
def should_not_allow_commenting(line_holder, diff_side = nil)
line = get_line_components(line_holder, diff_side)
line[:content].hover
expect(line[:num]).not_to have_css comment_button_class
end
def get_line_components(line_holder, diff_side = nil)
if diff_side.nil?
get_inline_line_components(line_holder)
else
get_parallel_line_components(line_holder, diff_side)
end
end
def get_inline_line_components(line_holder)
{ content: line_holder.find('.line_content', match: :first), num: line_holder.find('.diff-line-num', match: :first) }
end
def get_parallel_line_components(line_holder, diff_side = nil)
side_index = diff_side == 'left' ? 0 : 1
# Wait for `.line_content`
line_holder.find('.line_content', match: :first)
# Wait for `.diff-line-num`
line_holder.find('.diff-line-num', match: :first)
{ content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] }
end
def comment_on_line(line_holder, line)
line[:num].find(comment_button_class).trigger 'click'
line_holder.find(:xpath, notes_holder_input_xpath)
notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath)
expect(notes_holder_input[:class]).to include(notes_holder_input_class)
notes_holder_input.fill_in 'note[note]', with: test_note_comment
click_button 'Comment'
wait_for_ajax
end
def assert_comment_persistence(line_holder)
expect(line_holder).to have_xpath notes_holder_input_xpath
notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath)
expect(notes_holder_saved[:class]).not_to include(notes_holder_input_class)
expect(notes_holder_saved).to have_content test_note_comment
end
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