Merge branch '34010-fix-linking-to-parallel-diff-line-number-creating-gray-box' into 'master'

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';

changelogs/unreleased/34010-fix-linking-to-parallel-diff-line-number-creating-gray-box.yml

+---
+title: Fix linking to line number on side-by-side diff creating empty discussion box
+merge_request:
+author:

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)

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', () => {

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: `<div id="${noteId}">foo</div>` });
+        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();
+          });
         });
       });
     });