From 5145e39ef7efe7618a2f78a63e13bc630be251a9 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 21 Jun 2017 01:36:08 -0500 Subject: Fix linking to line number on parallel diff creating empty discussion Fix https://gitlab.com/gitlab-org/gitlab-ce/issues/34010 --- app/assets/javascripts/merge_request_tabs.js | 2 +- ...parallel-diff-line-number-creating-gray-box.yml | 4 + spec/javascripts/fixtures/merge_requests.rb | 13 +- spec/javascripts/merge_request_notes_spec.js | 2 +- spec/javascripts/merge_request_tabs_spec.js | 138 +++++++++++++++++---- 5 files changed, 130 insertions(+), 29 deletions(-) create mode 100644 changelogs/unreleased/34010-fix-linking-to-parallel-diff-line-number-creating-gray-box.yml diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 7bb2236017e..d2c64182248 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -291,7 +291,7 @@ import BlobForkSuggestion from './blob/blob_fork_suggestion'; // Scroll any linked note into view // Similar to `toggler_behavior` in the discussion tab 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) { const notesContent = anchor.closest('.notes_content'); const lineType = notesContent.hasClass('new') ? 'new' : 'old'; diff --git a/changelogs/unreleased/34010-fix-linking-to-parallel-diff-line-number-creating-gray-box.yml b/changelogs/unreleased/34010-fix-linking-to-parallel-diff-line-number-creating-gray-box.yml new file mode 100644 index 00000000000..705d44f8b10 --- /dev/null +++ b/changelogs/unreleased/34010-fix-linking-to-parallel-diff-line-number-creating-gray-box.yml @@ -0,0 +1,4 @@ +--- +title: Fix linking to line number on side-by-side diff creating empty discussion box +merge_request: +author: diff --git a/spec/javascripts/fixtures/merge_requests.rb b/spec/javascripts/fixtures/merge_requests.rb index 0715f4d5f6b..daaddd8f390 100644 --- a/spec/javascripts/fixtures/merge_requests.rb +++ b/spec/javascripts/fixtures/merge_requests.rb @@ -55,20 +55,27 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont render_merge_request(example.description, merge_request) 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(:note_on_merge_request, author: admin, project: project, noteable: merge_request) render_merge_request(example.description, merge_request, action: :diffs, format: :json) 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 - 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, namespace_id: project.namespace.to_param, project_id: project, id: merge_request.to_param, - format: format + format: format, + view: view expect(response).to be_success store_frontend_fixture(response, fixture_file_name) diff --git a/spec/javascripts/merge_request_notes_spec.js b/spec/javascripts/merge_request_notes_spec.js index b6d0ce02c4f..9e9eb17d439 100644 --- a/spec/javascripts/merge_request_notes_spec.js +++ b/spec/javascripts/merge_request_notes_spec.js @@ -15,7 +15,7 @@ describe('Merge request notes', () => { gl.utils = gl.utils || {}; 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); describe('Discussion tab with diff comments', () => { diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index 9916d2c1e21..bb6b5d852d3 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -22,7 +22,15 @@ import 'vendor/jquery.scrollTo'; }; $.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 () { this.class = new gl.MergeRequestTabs({ stubLocation: stubLocation }); @@ -271,6 +279,19 @@ import 'vendor/jquery.scrollTo'; }); 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 () { spyOn($, 'ajax').and.callFake(function (options) { expect(options.url).toEqual('/foo/bar/merge_requests/1/diffs.json'); @@ -279,43 +300,112 @@ import 'vendor/jquery.scrollTo'; this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); }); - describe('with note fragment hash', () => { + describe('with inline diff', () => { + let noteId; + let noteLineNumId; + beforeEach(() => { - loadFixtures('merge_requests/diff_comment.html.raw'); - spyOn(window.gl.utils, 'getPagePath').and.returnValue('merge_requests'); - window.notes = new Notes('', []); - spyOn(window.notes, 'toggleDiffNote').and.callThrough(); - }); + const diffsResponse = getJSONFixture(inlineChangesTabJsonFixture); + + 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('#', ''); - afterEach(() => { - delete window.notes; + spyOn($, 'ajax').and.callFake(function (options) { + options.success(diffsResponse); + }); }); - 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($, 'ajax').and.callFake(function (options) { - options.success({ html: `
foo
` }); + 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'); + + expect(noteId.length).toBeGreaterThan(0); + expect(window.notes.toggleDiffNote).toHaveBeenCalledWith({ + target: jasmine.any(Object), + lineType: 'old', + forceShow: true, + }); }); - this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); + 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).toHaveBeenCalledWith({ - target: jasmine.any(Object), - lineType: 'old', - forceShow: true, + expect(window.notes.toggleDiffNote).not.toHaveBeenCalled(); }); }); - it('should gracefully ignore non-existant fragment hash', function () { - spyOn(window.gl.utils, 'getLocationHash').and.returnValue('note_something-that-does-not-exist'); + 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) { - options.success({ html: '' }); + 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(window.notes.toggleDiffNote).not.toHaveBeenCalled(); + expect(noteId.length).toBeGreaterThan(0); + expect(window.notes.toggleDiffNote).toHaveBeenCalledWith({ + target: jasmine.any(Object), + lineType: 'new', + 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(); + }); }); }); }); -- cgit v1.2.3