Commit 5145e39e authored by Eric Eastwood's avatar Eric Eastwood

Fix linking to line number on parallel diff creating empty discussion

Fix https://gitlab.com/gitlab-org/gitlab-ce/issues/34010
parent 1e42253c
...@@ -291,7 +291,7 @@ import BlobForkSuggestion from './blob/blob_fork_suggestion'; ...@@ -291,7 +291,7 @@ import BlobForkSuggestion from './blob/blob_fork_suggestion';
// Scroll any linked note into view // Scroll any linked note into view
// Similar to `toggler_behavior` in the discussion tab // Similar to `toggler_behavior` in the discussion tab
const hash = window.gl.utils.getLocationHash(); const hash = window.gl.utils.getLocationHash();
const anchor = hash && $container.find(`[id="${hash}"]`); const anchor = hash && $container.find(`.note[id="${hash}"]`);
if (anchor && anchor.length > 0) { if (anchor && anchor.length > 0) {
const notesContent = anchor.closest('.notes_content'); const notesContent = anchor.closest('.notes_content');
const lineType = notesContent.hasClass('new') ? 'new' : 'old'; const lineType = notesContent.hasClass('new') ? 'new' : 'old';
......
---
title: Fix linking to line number on side-by-side diff creating empty discussion box
merge_request:
author:
...@@ -55,20 +55,27 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont ...@@ -55,20 +55,27 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont
render_merge_request(example.description, merge_request) render_merge_request(example.description, merge_request)
end end
it 'merge_requests/changes_tab_with_comments.json' do |example| it 'merge_requests/inline_changes_tab_with_comments.json' do |example|
create(:diff_note_on_merge_request, project: project, author: admin, position: position, noteable: merge_request) create(:diff_note_on_merge_request, project: project, author: admin, position: position, noteable: merge_request)
create(:note_on_merge_request, author: admin, project: project, noteable: merge_request) create(:note_on_merge_request, author: admin, project: project, noteable: merge_request)
render_merge_request(example.description, merge_request, action: :diffs, format: :json) render_merge_request(example.description, merge_request, action: :diffs, format: :json)
end end
it 'merge_requests/parallel_changes_tab_with_comments.json' do |example|
create(:diff_note_on_merge_request, project: project, author: admin, position: position, noteable: merge_request)
create(:note_on_merge_request, author: admin, project: project, noteable: merge_request)
render_merge_request(example.description, merge_request, action: :diffs, format: :json, view: 'parallel')
end
private private
def render_merge_request(fixture_file_name, merge_request, action: :show, format: :html) def render_merge_request(fixture_file_name, merge_request, action: :show, format: :html, view: 'inline')
get action, get action,
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
project_id: project, project_id: project,
id: merge_request.to_param, id: merge_request.to_param,
format: format format: format,
view: view
expect(response).to be_success expect(response).to be_success
store_frontend_fixture(response, fixture_file_name) store_frontend_fixture(response, fixture_file_name)
......
...@@ -15,7 +15,7 @@ describe('Merge request notes', () => { ...@@ -15,7 +15,7 @@ describe('Merge request notes', () => {
gl.utils = gl.utils || {}; gl.utils = gl.utils || {};
const discussionTabFixture = 'merge_requests/diff_comment.html.raw'; const discussionTabFixture = 'merge_requests/diff_comment.html.raw';
const changesTabJsonFixture = 'merge_requests/changes_tab_with_comments.json'; const changesTabJsonFixture = 'merge_requests/inline_changes_tab_with_comments.json';
preloadFixtures(discussionTabFixture, changesTabJsonFixture); preloadFixtures(discussionTabFixture, changesTabJsonFixture);
describe('Discussion tab with diff comments', () => { describe('Discussion tab with diff comments', () => {
......
...@@ -22,7 +22,15 @@ import 'vendor/jquery.scrollTo'; ...@@ -22,7 +22,15 @@ import 'vendor/jquery.scrollTo';
}; };
$.extend(stubLocation, defaults, stubs || {}); $.extend(stubLocation, defaults, stubs || {});
}; };
preloadFixtures('merge_requests/merge_request_with_task_list.html.raw', 'merge_requests/diff_comment.html.raw');
const inlineChangesTabJsonFixture = 'merge_requests/inline_changes_tab_with_comments.json';
const parallelChangesTabJsonFixture = 'merge_requests/parallel_changes_tab_with_comments.json';
preloadFixtures(
'merge_requests/merge_request_with_task_list.html.raw',
'merge_requests/diff_comment.html.raw',
inlineChangesTabJsonFixture,
parallelChangesTabJsonFixture
);
beforeEach(function () { beforeEach(function () {
this.class = new gl.MergeRequestTabs({ stubLocation: stubLocation }); this.class = new gl.MergeRequestTabs({ stubLocation: stubLocation });
...@@ -271,6 +279,19 @@ import 'vendor/jquery.scrollTo'; ...@@ -271,6 +279,19 @@ import 'vendor/jquery.scrollTo';
}); });
describe('loadDiff', function () { describe('loadDiff', function () {
beforeEach(() => {
loadFixtures('merge_requests/diff_comment.html.raw');
spyOn(window.gl.utils, 'getPagePath').and.returnValue('merge_requests');
window.gl.ImageFile = () => {};
window.notes = new Notes('', []);
spyOn(window.notes, 'toggleDiffNote').and.callThrough();
});
afterEach(() => {
delete window.gl.ImageFile;
delete window.notes;
});
it('requires an absolute pathname', function () { it('requires an absolute pathname', function () {
spyOn($, 'ajax').and.callFake(function (options) { spyOn($, 'ajax').and.callFake(function (options) {
expect(options.url).toEqual('/foo/bar/merge_requests/1/diffs.json'); expect(options.url).toEqual('/foo/bar/merge_requests/1/diffs.json');
...@@ -279,45 +300,114 @@ import 'vendor/jquery.scrollTo'; ...@@ -279,45 +300,114 @@ import 'vendor/jquery.scrollTo';
this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); this.class.loadDiff('/foo/bar/merge_requests/1/diffs');
}); });
describe('with note fragment hash', () => { describe('with inline diff', () => {
let noteId;
let noteLineNumId;
beforeEach(() => { beforeEach(() => {
loadFixtures('merge_requests/diff_comment.html.raw'); const diffsResponse = getJSONFixture(inlineChangesTabJsonFixture);
spyOn(window.gl.utils, 'getPagePath').and.returnValue('merge_requests');
window.notes = new Notes('', []); const $html = $(diffsResponse.html);
spyOn(window.notes, 'toggleDiffNote').and.callThrough(); noteId = $html.find('.note').attr('id');
}); noteLineNumId = $html
.find('.note')
.closest('.notes_holder')
.prev('.line_holder')
.find('a[data-linenumber]')
.attr('href')
.replace('#', '');
afterEach(() => { spyOn($, 'ajax').and.callFake(function (options) {
delete window.notes; options.success(diffsResponse);
});
}); });
describe('with note fragment hash', () => {
it('should expand and scroll to linked fragment hash #note_xxx', function () { it('should expand and scroll to linked fragment hash #note_xxx', function () {
const noteId = 'note_1';
spyOn(window.gl.utils, 'getLocationHash').and.returnValue(noteId); spyOn(window.gl.utils, 'getLocationHash').and.returnValue(noteId);
this.class.loadDiff('/foo/bar/merge_requests/1/diffs');
expect(noteId.length).toBeGreaterThan(0);
expect(window.notes.toggleDiffNote).toHaveBeenCalledWith({
target: jasmine.any(Object),
lineType: 'old',
forceShow: true,
});
});
it('should gracefully ignore non-existant fragment hash', function () {
spyOn(window.gl.utils, 'getLocationHash').and.returnValue('note_something-that-does-not-exist');
this.class.loadDiff('/foo/bar/merge_requests/1/diffs');
expect(window.notes.toggleDiffNote).not.toHaveBeenCalled();
});
});
describe('with line number fragment hash', () => {
it('should gracefully ignore line number fragment hash', function () {
spyOn(window.gl.utils, 'getLocationHash').and.returnValue(noteLineNumId);
this.class.loadDiff('/foo/bar/merge_requests/1/diffs');
expect(noteLineNumId.length).toBeGreaterThan(0);
expect(window.notes.toggleDiffNote).not.toHaveBeenCalled();
});
});
});
describe('with parallel diff', () => {
let noteId;
let noteLineNumId;
beforeEach(() => {
const diffsResponse = getJSONFixture(parallelChangesTabJsonFixture);
const $html = $(diffsResponse.html);
noteId = $html.find('.note').attr('id');
noteLineNumId = $html
.find('.note')
.closest('.notes_holder')
.prev('.line_holder')
.find('a[data-linenumber]')
.attr('href')
.replace('#', '');
spyOn($, 'ajax').and.callFake(function (options) { spyOn($, 'ajax').and.callFake(function (options) {
options.success({ html: `<div id="${noteId}">foo</div>` }); options.success(diffsResponse);
});
}); });
describe('with note fragment hash', () => {
it('should expand and scroll to linked fragment hash #note_xxx', function () {
spyOn(window.gl.utils, 'getLocationHash').and.returnValue(noteId);
this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); this.class.loadDiff('/foo/bar/merge_requests/1/diffs');
expect(noteId.length).toBeGreaterThan(0);
expect(window.notes.toggleDiffNote).toHaveBeenCalledWith({ expect(window.notes.toggleDiffNote).toHaveBeenCalledWith({
target: jasmine.any(Object), target: jasmine.any(Object),
lineType: 'old', lineType: 'new',
forceShow: true, forceShow: true,
}); });
}); });
it('should gracefully ignore non-existant fragment hash', function () { it('should gracefully ignore non-existant fragment hash', function () {
spyOn(window.gl.utils, 'getLocationHash').and.returnValue('note_something-that-does-not-exist'); spyOn(window.gl.utils, 'getLocationHash').and.returnValue('note_something-that-does-not-exist');
spyOn($, 'ajax').and.callFake(function (options) { this.class.loadDiff('/foo/bar/merge_requests/1/diffs');
options.success({ html: '' });
expect(window.notes.toggleDiffNote).not.toHaveBeenCalled();
});
}); });
describe('with line number fragment hash', () => {
it('should gracefully ignore line number fragment hash', function () {
spyOn(window.gl.utils, 'getLocationHash').and.returnValue(noteLineNumId);
this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); this.class.loadDiff('/foo/bar/merge_requests/1/diffs');
expect(noteLineNumId.length).toBeGreaterThan(0);
expect(window.notes.toggleDiffNote).not.toHaveBeenCalled(); expect(window.notes.toggleDiffNote).not.toHaveBeenCalled();
}); });
}); });
}); });
}); });
});
}).call(window); }).call(window);
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