Automatic merge of gitlab-org/gitlab master

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.$el.scrollIntoView({
-            behavior: 'smooth',
-            inline: 'start',
-          });
-        }
+
+        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() {
-    if (this.isNoteLinked) {
-      this.$el.scrollIntoView({ behavior: 'smooth', inline: 'start' });
-    }
+    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

doc/ci/quick_start/README.md

@@ -11,9 +11,9 @@ GitLab offers a [continuous integration](https://about.gitlab.com/stages-devops-
 [pipeline](../pipelines/index.md), you must:
 
 - Add a [`.gitlab-ci.yml` file](#creating-a-gitlab-ciyml-file) to your repository's root directory.
-- Ensure your project is configured to use a [Runner](#configuring-a-runner).
+- Ensure your project is configured to use a [runner](#configuring-a-runner).
 
-The `.gitlab-ci.yml` file tells the GitLab Runner what to do. A simple pipeline commonly has
+The `.gitlab-ci.yml` file tells the runner what to do. A simple pipeline commonly has
 three [stages](../yaml/README.md#stages):
 
 - `build`
@@ -57,7 +57,7 @@ The `.gitlab-ci.yml` file is where you configure what CI does with your project.
 It lives in the root of your repository.
 
 On any push to your repository, GitLab will look for the `.gitlab-ci.yml`
-file and start jobs on _Runners_ according to the contents of the file,
+file and start jobs on _runners_ according to the contents of the file,
 for that commit.
 
 Because `.gitlab-ci.yml` is in the repository and is version controlled, old
@@ -109,7 +109,7 @@ The `.gitlab-ci.yml` file defines sets of jobs with constraints of how and when
 they should be run. The jobs are defined as top-level elements with a name (in
 our case `rspec` and `rubocop`) and always have to contain the `script` keyword.
 Jobs are used to create jobs, which are then picked by
-[Runners](../runners/README.md) and executed within the environment of the Runner.
+[runners](../runners/README.md) and executed within the environment of the runner.
 
 What is important is that each job is run independently from each other.
 
@@ -148,59 +148,54 @@ Clicking on it you will be directed to the jobs page for that specific commit.
 ![Single commit jobs page](img/single_commit_status_pending.png)
 
 Notice that there is a pending job which is named after what we wrote in
-`.gitlab-ci.yml`. "stuck" indicates that there is no Runner configured
+`.gitlab-ci.yml`. "stuck" indicates that there is no runner configured
 yet for this job.
 
-The next step is to configure a Runner so that it picks the pending jobs.
+The next step is to configure a runner so that it picks the pending jobs.
 
-## Configuring a Runner
+## Configuring a runner
 
-In GitLab, Runners run the jobs that you define in `.gitlab-ci.yml`. A Runner
-can be a virtual machine, a VPS, a bare-metal machine, a Docker container or
-even a cluster of containers. GitLab and the Runners communicate through an API,
-so the only requirement is that the Runner's machine has network access to the
+In GitLab, runners run the jobs that you define in `.gitlab-ci.yml`. A runner
+can be a virtual machine, a VPS, a bare-metal machine, a Docker container, or
+even a cluster of containers. GitLab and the runner communicate through an API,
+so the only requirement is that the runner's machine has network access to the
 GitLab server.
 
-A Runner can be specific to a certain project or serve multiple projects in
-GitLab. If it serves all projects it's called a _Shared Runner_.
+A runner can be specific to a certain project or serve multiple projects in
+GitLab. If it serves all projects, it's called a _shared runner_.
 
-Find more information about different Runners in the
-[Runners](../runners/README.md) documentation.
+Find more information about runners in the
+[runner](../runners/README.md) documentation.
 
-You can find whether any Runners are assigned to your project by going to
-**Settings ➔ CI/CD**. Setting up a Runner is easy and straightforward. The
-official Runner supported by GitLab is written in Go and its documentation
-can be found at <https://docs.gitlab.com/runner/>.
+The official runner supported by GitLab is written in Go.
+View [the documentation](https://docs.gitlab.com/runner/).
 
-In order to have a functional Runner you need to follow two steps:
+For a runner to be available in GitLab, you must:
 
-1. [Install it](https://docs.gitlab.com/runner/install/)
-1. [Configure it](https://docs.gitlab.com/runner/configuration/)
+1. [Install GitLab Runner](https://docs.gitlab.com/runner/install/).
+1. [Register a runner for your group or project](https://docs.gitlab.com/runner/register/).
 
-Follow the links above to set up your own Runner or use a Shared Runner as
-described in the next section.
-
-Once the Runner has been set up, you should see it on the Runners page of your
-project, following **Settings ➔ CI/CD**.
+When a runner is available, you can view it by
+clicking **Settings > CI/CD** and expanding **Runners**.
 
 ![Activated runners](img/runners_activated.png)
 
-### Shared Runners
+### Shared runners
 
-If you use [GitLab.com](https://gitlab.com/) you can use the **Shared Runners**
-provided by GitLab Inc.
+If you use [GitLab.com](https://gitlab.com/), you can use the **shared runners**
+provided by GitLab.
 
 These are special virtual machines that run on GitLab's infrastructure and can
 build any project.
 
-To enable the **Shared Runners** you have to go to your project's
-**Settings ➔ CI/CD** and click **Enable shared runners**.
+To enable shared runners, go to your project's or group's
+**Settings > CI/CD** and click **Enable shared runners**.
 
-[Read more on Shared Runners](../runners/README.md).
+[Read more about shared runners](../runners/README.md#shared-runners).
 
-## Seeing the status of your pipeline and jobs
+## Viewing the status of your pipeline and jobs
 
-After configuring the Runner successfully, you should see the status of your
+After configuring the runner successfully, you should see the status of your
 last commit change from _pending_ to either _running_, _success_ or _failed_.
 
 You can view all pipelines by going to the **Pipelines** page in your project.

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

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

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,
         },
       },
-      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,
         },
       },
-      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);
 
-      expect(scrollIntoViewMock).toHaveBeenCalled();
+      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);
+  });
+});