Merge branch 'fix-design-managemnt-comment-highlight' into 'master'

Ensure design comment is highlighted when in URL hash

Closes #225987

See merge request gitlab-org/gitlab!40477

app/assets/javascripts/design_management/components/design_notes/design_discussion.vue

@@ -62,22 +62,20 @@ export default {
     activeDiscussion: {
       query: activeDiscussionQuery,
       result({ data }) {
-        const discussionId = data.activeDiscussion.id;
         if (this.discussion.resolved && !this.resolvedDiscussionsExpanded) {
           return;
         }
-        // We watch any changes to the active discussion from the design pins and scroll to this discussion if it exists
-        // We don't want scrollIntoView to be triggered from the discussion click itself
-        if (
-          discussionId &&
-          data.activeDiscussion.source === ACTIVE_DISCUSSION_SOURCE_TYPES.pin &&
-          discussionId === this.discussion.notes[0].id
-        ) {
+
+        this.$nextTick(() => {
+          // We watch any changes to the active discussion from the design pins and scroll to this discussion if it exists.
+          // We don't want scrollIntoView to be triggered from the discussion click itself.
+          if (this.$el && this.shouldScrollToDiscussion(data.activeDiscussion)) {
             this.$el.scrollIntoView({
               behavior: 'smooth',
               inline: 'start',
             });
           }
+        });
       },
     },
   },
@@ -136,6 +134,18 @@ export default {
     isFormVisible() {
       return this.isFormRendered && this.discussionWithOpenForm === this.discussion.id;
     },
+    shouldScrollToDiscussion(activeDiscussion) {
+      const ALLOWED_ACTIVE_DISCUSSION_SOURCES = [
+        ACTIVE_DISCUSSION_SOURCE_TYPES.pin,
+        ACTIVE_DISCUSSION_SOURCE_TYPES.url,
+      ];
+      const { id: activeDiscussionId, source: activeDiscussionSource } = activeDiscussion;
+
+      return (
+        ALLOWED_ACTIVE_DISCUSSION_SOURCES.includes(activeDiscussionSource) &&
+        activeDiscussionId === this.discussion.notes[0].id
+      );
+    },
   },
   methods: {
     addDiscussionComment(

app/assets/javascripts/design_management/components/design_notes/design_note.vue

@@ -60,9 +60,11 @@ export default {
     },
   },
   mounted() {
+    this.$nextTick(() => {
       if (this.isNoteLinked) {
         this.$el.scrollIntoView({ behavior: 'smooth', inline: 'start' });
       }
+    });
   },
   methods: {
     hideForm() {

app/assets/javascripts/design_management/constants.js

@@ -11,6 +11,7 @@ export const VALID_DATA_TRANSFER_TYPE = 'Files';
 export const ACTIVE_DISCUSSION_SOURCE_TYPES = {
   pin: 'pin',
   discussion: 'discussion',
+  url: 'url',
 };
 
 export const DESIGN_DETAIL_LAYOUT_CLASSLIST = ['design-detail-layout', 'overflow-hidden', 'm-0'];

app/assets/javascripts/design_management/pages/design/index.vue

@@ -19,6 +19,8 @@ import {
   extractDiscussions,
   extractDesign,
   updateImageDiffNoteOptimisticResponse,
+  toDiffNoteGid,
+  extractDesignNoteId,
 } from '../../utils/design_management_utils';
 import {
   updateStoreAfterAddImageDiffNote,
@@ -145,8 +147,11 @@ export default {
   mounted() {
     Mousetrap.bind('esc', this.closeDesign);
     this.trackEvent();
-    // We need to reset the active discussion when opening a new design
-    this.updateActiveDiscussion();
+
+    // Set active discussion immediately.
+    // This will ensure that, if a note is specified in the URL hash,
+    // the browser will scroll to, and highlight, the note in the UI
+    this.updateActiveDiscussionFromUrl();
   },
   beforeDestroy() {
     Mousetrap.unbind('esc', this.closeDesign);
@@ -266,15 +271,20 @@ export default {
         this.isLatestVersion,
       );
     },
-    updateActiveDiscussion(id) {
+    updateActiveDiscussion(id, source = ACTIVE_DISCUSSION_SOURCE_TYPES.discussion) {
       this.$apollo.mutate({
         mutation: updateActiveDiscussionMutation,
         variables: {
           id,
-          source: ACTIVE_DISCUSSION_SOURCE_TYPES.discussion,
+          source,
         },
       });
     },
+    updateActiveDiscussionFromUrl() {
+      const noteId = extractDesignNoteId(this.$route.hash);
+      const diffNoteGid = noteId ? toDiffNoteGid(noteId) : undefined;
+      return this.updateActiveDiscussion(diffNoteGid, ACTIVE_DISCUSSION_SOURCE_TYPES.url);
+    },
     toggleResolvedComments() {
       this.resolvedDiscussionsExpanded = !this.resolvedDiscussionsExpanded;
     },

app/assets/javascripts/design_management/utils/design_management_utils.js

@@ -34,6 +34,17 @@ export const extractDesigns = data => data.project.issue.designCollection.design
 
 export const extractDesign = data => (extractDesigns(data) || [])[0];
 
+export const toDiffNoteGid = noteId => `gid://gitlab/DiffNote/${noteId}`;
+
+/**
+ * Return the note ID from a URL hash parameter
+ * @param {String} urlHash URL hash, including `#` prefix
+ */
+export const extractDesignNoteId = urlHash => {
+  const [, noteId] = urlHash.match('#note_([0-9]+$)') || [];
+  return noteId || null;
+};
+
 /**
  * Generates optimistic response for a design upload mutation
  * @param {Array<File>} files

changelogs/unreleased/fix-design-managemnt-comment-highlight.yml

+---
+title: Ensure design comment is highlighted when comment is in URL
+merge_request: 40477
+author:
+type: fixed

spec/frontend/design_management/components/design_notes/design_note_spec.js

@@ -91,8 +91,10 @@ describe('Design note component', () => {
       note,
     });
 
+    return wrapper.vm.$nextTick().then(() => {
       expect(scrollIntoViewMock).toHaveBeenCalled();
     });
+  });
 
   it('should not render edit icon when user does not have a permission', () => {
     createComponent({

spec/frontend/design_management/pages/design/__snapshots__/index_spec.js.snap

@@ -57,7 +57,7 @@ exports[`Design management design index page renders design index 1`] = `
      
     <design-discussion-stub
       data-testid="unresolved-discussion"
-      designid="test"
+      designid="design-id"
       discussion="[object Object]"
       discussionwithopenform=""
       markdownpreviewpath="/project-path/preview_markdown?target_type=Issue"
@@ -105,7 +105,7 @@ exports[`Design management design index page renders design index 1`] = `
     >
       <design-discussion-stub
         data-testid="resolved-discussion"
-        designid="test"
+        designid="design-id"
         discussion="[object Object]"
         discussionwithopenform=""
         markdownpreviewpath="/project-path/preview_markdown?target_type=Issue"

spec/frontend/design_management/pages/design/index_spec.js

@@ -7,18 +7,19 @@ import DesignIndex from '~/design_management/pages/design/index.vue';
 import DesignSidebar from '~/design_management/components/design_sidebar.vue';
 import DesignPresentation from '~/design_management/components/design_presentation.vue';
 import createImageDiffNoteMutation from '~/design_management/graphql/mutations/create_image_diff_note.mutation.graphql';
-import design from '../../mock_data/design';
-import mockResponseWithDesigns from '../../mock_data/designs';
-import mockResponseNoDesigns from '../../mock_data/no_designs';
-import mockAllVersions from '../../mock_data/all_versions';
+import updateActiveDiscussion from '~/design_management/graphql/mutations/update_active_discussion.mutation.graphql';
 import {
   DESIGN_NOT_FOUND_ERROR,
   DESIGN_VERSION_NOT_EXIST_ERROR,
 } from '~/design_management/utils/error_messages';
-import { DESIGNS_ROUTE_NAME } from '~/design_management/router/constants';
+import { DESIGNS_ROUTE_NAME, DESIGN_ROUTE_NAME } from '~/design_management/router/constants';
 import createRouter from '~/design_management/router';
 import * as utils from '~/design_management/utils/design_management_utils';
 import { DESIGN_DETAIL_LAYOUT_CLASSLIST } from '~/design_management/constants';
+import design from '../../mock_data/design';
+import mockResponseWithDesigns from '../../mock_data/designs';
+import mockResponseNoDesigns from '../../mock_data/no_designs';
+import mockAllVersions from '../../mock_data/all_versions';
 
 jest.mock('~/flash');
 jest.mock('mousetrap', () => ({
@@ -34,6 +35,12 @@ const DesignReplyForm = {
     focusInput,
   },
 };
+const mockDesignNoDiscussions = {
+  ...design,
+  discussions: {
+    nodes: [],
+  },
+};
 
 const localVue = createLocalVue();
 localVue.use(VueRouter);
@@ -75,7 +82,7 @@ describe('Design management design index page', () => {
   const findSidebar = () => wrapper.find(DesignSidebar);
   const findDesignPresentation = () => wrapper.find(DesignPresentation);
 
-  function createComponent(loading = false, data = {}) {
+  function createComponent({ loading = false } = {}, { data = {}, intialRouteOptions = {} } = {}) {
     const $apollo = {
       queries: {
         design: {
@@ -87,6 +94,8 @@ describe('Design management design index page', () => {
 
     router = createRouter();
 
+    router.push({ name: DESIGN_ROUTE_NAME, params: { id: design.id }, ...intialRouteOptions });
+
     wrapper = shallowMount(DesignIndex, {
       propsData: { id: '1' },
       mocks: { $apollo },
@@ -126,29 +135,28 @@ describe('Design management design index page', () => {
         },
       };
       jest.spyOn(utils, 'getPageLayoutElement').mockReturnValue(mockEl);
-      createComponent(true);
+      createComponent({ loading: true });
 
-      wrapper.vm.$router.push('/designs/test');
       expect(mockEl.classList.add).toHaveBeenCalledTimes(1);
       expect(mockEl.classList.add).toHaveBeenCalledWith(...DESIGN_DETAIL_LAYOUT_CLASSLIST);
     });
   });
 
   it('sets loading state', () => {
-    createComponent(true);
+    createComponent({ loading: true });
 
     expect(wrapper.element).toMatchSnapshot();
   });
 
   it('renders design index', () => {
-    createComponent(false, { design });
+    createComponent({ loading: false }, { data: { design } });
 
     expect(wrapper.element).toMatchSnapshot();
     expect(wrapper.find(GlAlert).exists()).toBe(false);
   });
 
   it('passes correct props to sidebar component', () => {
-    createComponent(false, { design });
+    createComponent({ loading: false }, { data: { design } });
 
     expect(findSidebar().props()).toEqual({
       design,
@@ -158,14 +166,14 @@ describe('Design management design index page', () => {
   });
 
   it('opens a new discussion form', () => {
-    createComponent(false, {
-      design: {
-        ...design,
-        discussions: {
-          nodes: [],
+    createComponent(
+      { loading: false },
+      {
+        data: {
+          design,
         },
       },
-    });
+    );
 
     findDesignPresentation().vm.$emit('openCommentForm', { x: 0, y: 0 });
 
@@ -175,15 +183,15 @@ describe('Design management design index page', () => {
   });
 
   it('keeps new discussion form focused', () => {
-    createComponent(false, {
-      design: {
-        ...design,
-        discussions: {
-          nodes: [],
+    createComponent(
+      { loading: false },
+      {
+        data: {
+          design,
+          annotationCoordinates,
         },
       },
-      annotationCoordinates,
-    });
+    );
 
     findDesignPresentation().vm.$emit('openCommentForm', { x: 10, y: 10 });
 
@@ -191,16 +199,16 @@ describe('Design management design index page', () => {
   });
 
   it('sends a mutation on submitting form and closes form', () => {
-    createComponent(false, {
-      design: {
-        ...design,
-        discussions: {
-          nodes: [],
-        },
-      },
+    createComponent(
+      { loading: false },
+      {
+        data: {
+          design,
           annotationCoordinates,
           comment: newComment,
-    });
+        },
+      },
+    );
 
     findDiscussionForm().vm.$emit('submitForm');
     expect(mutate).toHaveBeenCalledWith(createDiscussionMutationVariables);
@@ -216,16 +224,16 @@ describe('Design management design index page', () => {
   });
 
   it('closes the form and clears the comment on canceling form', () => {
-    createComponent(false, {
-      design: {
-        ...design,
-        discussions: {
-          nodes: [],
-        },
-      },
+    createComponent(
+      { loading: false },
+      {
+        data: {
+          design,
           annotationCoordinates,
           comment: newComment,
-    });
+        },
+      },
+    );
 
     findDiscussionForm().vm.$emit('cancelForm');
 
@@ -238,15 +246,15 @@ describe('Design management design index page', () => {
 
   describe('with error', () => {
     beforeEach(() => {
-      createComponent(false, {
-        design: {
-          ...design,
-          discussions: {
-            nodes: [],
+      createComponent(
+        { loading: false },
+        {
+          data: {
+            design: mockDesignNoDiscussions,
+            errorMessage: 'woops',
           },
         },
-        errorMessage: 'woops',
-      });
+      );
     });
 
     it('GlAlert is rendered in correct position with correct content', () => {
@@ -257,7 +265,7 @@ describe('Design management design index page', () => {
   describe('onDesignQueryResult', () => {
     describe('with no designs', () => {
       it('redirects to /designs', () => {
-        createComponent(true);
+        createComponent({ loading: true });
         router.push = jest.fn();
 
         wrapper.vm.onDesignQueryResult({ data: mockResponseNoDesigns, loading: false });
@@ -272,7 +280,7 @@ describe('Design management design index page', () => {
 
     describe('when no design exists for given version', () => {
       it('redirects to /designs', () => {
-        createComponent(true);
+        createComponent({ loading: true });
         wrapper.setData({
           allVersions: mockAllVersions,
         });
@@ -291,4 +299,24 @@ describe('Design management design index page', () => {
       });
     });
   });
+
+  describe('when hash present in current route', () => {
+    it('calls updateActiveDiscussion mutation', () => {
+      createComponent(
+        { loading: false },
+        {
+          data: {
+            design,
+          },
+          intialRouteOptions: { hash: '#note_123' },
+        },
+      );
+
+      expect(mutate).toHaveBeenCalledTimes(1);
+      expect(mutate).toHaveBeenCalledWith({
+        mutation: updateActiveDiscussion,
+        variables: { id: 'gid://gitlab/DiffNote/123', source: 'url' },
+      });
+    });
+  });
 });

spec/frontend/design_management/pages/index_spec.js

@@ -579,7 +579,9 @@ describe('Design management index page', () => {
       });
       createComponent(true);
 
+      return wrapper.vm.$nextTick().then(() => {
         expect(scrollIntoViewMock).toHaveBeenCalled();
       });
     });
+  });
 });

spec/frontend/design_management/utils/design_management_utils_spec.js

@@ -6,6 +6,7 @@ import {
   updateImageDiffNoteOptimisticResponse,
   isValidDesignFile,
   extractDesign,
+  extractDesignNoteId,
 } from '~/design_management/utils/design_management_utils';
 import mockResponseNoDesigns from '../mock_data/no_designs';
 import mockResponseWithDesigns from '../mock_data/designs';
@@ -171,3 +172,19 @@ describe('extractDesign', () => {
     });
   });
 });
+
+describe('extractDesignNoteId', () => {
+  it.each`
+    hash            | expectedNoteId
+    ${'#note_0'}    | ${'0'}
+    ${'#note_1'}    | ${'1'}
+    ${'#note_23'}   | ${'23'}
+    ${'#note_456'}  | ${'456'}
+    ${'note_1'}     | ${null}
+    ${'#note_'}     | ${null}
+    ${'#note_asd'}  | ${null}
+    ${'#note_1asd'} | ${null}
+  `('returns $expectedNoteId when hash is $hash', ({ hash, expectedNoteId }) => {
+    expect(extractDesignNoteId(hash)).toBe(expectedNoteId);
+  });
+});