Merge branch '32424-fix-linking-to-unresolved-expanded-diff-note' into 'master'

Fix linking to unresolved/expanded diff note

Closes #32424

See merge request !11458

app/assets/javascripts/merge_request_tabs.js

@@ -288,7 +288,11 @@ import BlobForkSuggestion from './blob/blob_fork_suggestion';
           if (anchor) {
             const notesContent = anchor.closest('.notes_content');
             const lineType = notesContent.hasClass('new') ? 'new' : 'old';
-            notes.addDiffNote(anchor, lineType, false);
+            notes.toggleDiffNote({
+              target: anchor,
+              lineType,
+              forceShow: true,
+            });
             anchor[0].scrollIntoView();
             // We have multiple elements on the page with `#note_xxx`
             // (discussion and diff tabs) and `:target` only applies to the first

app/assets/javascripts/notes.js

@@ -860,10 +860,19 @@ const normalizeNewlines = function(str) {
       e.preventDefault();
       const $link = $(e.currentTarget || e.target);
       const showReplyInput = !$link.hasClass('js-diff-comment-avatar');
-      this.addDiffNote($link, $link.data('lineType'), showReplyInput);
+      this.toggleDiffNote({
+        target: $link,
+        lineType: $link.data('lineType'),
+        showReplyInput
+      });
     };
-    Notes.prototype.addDiffNote = function(target, lineType, showReplyInput) {
+    Notes.prototype.toggleDiffNote = function({
+      target,
+      lineType,
+      forceShow,
+      showReplyInput = false,
+    }) {
       var $link, addForm, hasNotes, newForm, noteForm, replyButton, row, rowCssToAdd, targetContent, isDiffCommentAvatar;
       $link = $(target);
       row = $link.closest("tr");
@@ -908,12 +917,12 @@ const normalizeNewlines = function(str) {
         notesContent = targetRow.find(notesContentSelector);
         addForm = true;
       } else {
-        targetRow.show();
-        notesContent.toggle(!notesContent.is(':visible'));
-        if (!targetRow.find('.content:not(:empty)').is(':visible')) {
-          targetRow.hide();
-        }
+        const isCurrentlyShown = targetRow.find('.content:not(:empty)').is(':visible');
+        const isForced = forceShow === true || forceShow === false;
+        const showNow = forceShow === true || (!isCurrentlyShown && !isForced);
+        targetRow.toggle(showNow);
+        notesContent.toggle(showNow);
       }
       if (addForm) {

spec/features/merge_requests/diffs_spec.rb

@@ -20,6 +20,34 @@ feature 'Diffs URL', js: true, feature: true do
     end
   end
+  context 'when linking to note' do
+    describe 'with unresolved note' do
+      let(:note) { create :diff_note_on_merge_request, project: project, noteable: merge_request }
+      let(:fragment) { "#note_#{note.id}" }
+
+      before do
+        visit "#{diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)}#{fragment}"
+      end
+
+      it 'shows expanded note' do
+        expect(page).to have_selector(fragment, visible: true)
+      end
+    end
+
+    describe 'with resolved note' do
+      let(:note) { create :diff_note_on_merge_request, :resolved, project: project, noteable: merge_request }
+      let(:fragment) { "#note_#{note.id}" }
+
+      before do
+        visit "#{diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)}#{fragment}"
+      end
+
+      it 'shows expanded note' do
+        expect(page).to have_selector(fragment, visible: true)
+      end
+    end
+  end
+
   context 'when merge request has overflow' do
     it 'displays warning' do
       allow(Commit).to receive(:max_diff_options).and_return(max_files: 3)