Merge branch 'moveable-comment-pins' into 'master'

Design view: moveable design note pins

See merge request gitlab-org/gitlab!24934

doc/user/project/issues/design_management.md

@@ -144,8 +144,10 @@ which you can start a new discussion:
 
 ![Starting a new discussion on design](img/adding_note_to_design_1.png)
 
-From GitLab 12.8 on, when you are starting a new discussion, you can adjust the badge's position by
-dragging it around the image.
+[Introduced](https://gitlab.com/gitlab-org/gitlab/issues/34353) in [GitLab Premium](https://about.gitlab.com/pricing/) 12.8,
+you can adjust the badge's position by dragging it around the image. This is useful
+for when your design layout has changed between revisions, or if you need to move an
+existing badge to add a new one in its place.
 
 Different discussions have different badge numbers:
 

ee/app/assets/javascripts/design_management/components/design_overlay.vue

@@ -41,7 +41,7 @@ export default {
       };
     },
     isMovingCurrentComment() {
-      return Boolean(this.movingNoteStartPosition);
+      return Boolean(this.movingNoteStartPosition && !this.movingNoteStartPosition.noteId);
     },
     currentCommentPositionStyle() {
       return this.isMovingCurrentComment && this.movingNoteNewPosition
@@ -84,13 +84,23 @@ export default {
         deltaY,
       };
     },
+    isMovingNote(noteId) {
+      const movingNoteId = this.movingNoteStartPosition?.noteId;
+      return Boolean(movingNoteId && movingNoteId === noteId);
+    },
+    canMoveNote(note) {
+      const { userPermissions } = note;
+      const { adminNote } = userPermissions || {};
+
+      return Boolean(adminNote);
+    },
     isPositionInOverlay(position) {
       const { top, left } = this.getNoteRelativePosition(position);
       const { height, width } = this.dimensions;
 
       return top >= 0 && top <= height && left >= 0 && left <= width;
     },
-    onOverlayMousemove(e) {
+    onNewNoteMove(e) {
       if (!this.isMovingCurrentComment) return;
 
       const { deltaX, deltaY } = this.getMovingNotePositionDelta(e);
@@ -111,17 +121,76 @@ export default {
 
       this.movingNoteNewPosition = movingNoteNewPosition;
     },
-    onNewNoteMousedown({ clientX, clientY }) {
-      this.movingNoteStartPosition = {
-        clientX,
-        clientY,
+    onExistingNoteMove(e) {
+      const note = this.notes.find(({ id }) => id === this.movingNoteStartPosition.noteId);
+      if (!note) return;
+
+      const { position } = note;
+      const { width, height } = position;
+      const widthRatio = this.dimensions.width / width;
+      const heightRatio = this.dimensions.height / height;
+
+      const { deltaX, deltaY } = this.getMovingNotePositionDelta(e);
+      const x = position.x * widthRatio + deltaX;
+      const y = position.y * heightRatio + deltaY;
+
+      const movingNoteNewPosition = {
+        x,
+        y,
+        width: this.dimensions.width,
+        height: this.dimensions.height,
       };
+
+      if (!this.isPositionInOverlay(movingNoteNewPosition)) {
+        this.onExistingNoteMouseup();
+        return;
+      }
+
+      this.movingNoteNewPosition = movingNoteNewPosition;
     },
     onNewNoteMouseup() {
       if (!this.movingNoteNewPosition) return;
 
       const { x, y } = this.movingNoteNewPosition;
       this.setNewNoteCoordinates({ x, y });
+    },
+    onExistingNoteMouseup() {
+      if (!this.movingNoteStartPosition || !this.movingNoteNewPosition) return;
+
+      const { x, y } = this.movingNoteNewPosition;
+      this.$emit('moveNote', {
+        noteId: this.movingNoteStartPosition.noteId,
+        discussionId: this.movingNoteStartPosition.discussionId,
+        coordinates: { x, y },
+      });
+    },
+    onNoteMousedown({ clientX, clientY }, note) {
+      if (note && !this.canMoveNote(note)) return;
+
+      this.movingNoteStartPosition = {
+        noteId: note?.id,
+        discussionId: note?.discussion.id,
+        clientX,
+        clientY,
+      };
+    },
+    onOverlayMousemove(e) {
+      if (!this.movingNoteStartPosition) return;
+
+      if (this.isMovingCurrentComment) {
+        this.onNewNoteMove(e);
+      } else {
+        this.onExistingNoteMove(e);
+      }
+    },
+    onNoteMouseup() {
+      if (!this.movingNoteStartPosition) return;
+
+      if (this.isMovingCurrentComment) {
+        this.onNewNoteMouseup();
+      } else {
+        this.onExistingNoteMouseup();
+      }
 
       this.movingNoteStartPosition = null;
       this.movingNoteNewPosition = null;
@@ -135,7 +204,7 @@ export default {
     class="position-absolute image-diff-overlay frame"
     :style="overlayStyle"
     @mousemove="onOverlayMousemove"
-    @mouseleave="onNewNoteMouseup"
+    @mouseleave="onNoteMouseup"
   >
     <button
       type="button"
@@ -147,14 +216,21 @@ export default {
       v-for="(note, index) in notes"
       :key="note.id"
       :label="`${index + 1}`"
-      :position="getNotePositionStyle(note.position)"
+      :repositioning="isMovingNote(note.id)"
+      :position="
+        isMovingNote(note.id) && movingNoteNewPosition
+          ? getNotePositionStyle(movingNoteNewPosition)
+          : getNotePositionStyle(note.position)
+      "
+      @mousedown="onNoteMousedown($event, note)"
+      @mouseup="onNoteMouseup"
     />
     <design-note-pin
       v-if="currentCommentForm"
       :position="currentCommentPositionStyle"
       :repositioning="isMovingCurrentComment"
-      @mousedown="onNewNoteMousedown"
-      @mouseup="onNewNoteMouseup"
+      @mousedown="onNoteMousedown"
+      @mouseup="onNoteMouseup"
     />
   </div>
 </template>

ee/app/assets/javascripts/design_management/components/design_presentation.vue

@@ -202,6 +202,10 @@ export default {
       this.currentAnnotationPosition = this.getAnnotationPositon(coordinates);
       this.$emit('openCommentForm', this.currentAnnotationPosition);
     },
+    moveNote({ noteId, discussionId, coordinates }) {
+      const position = this.getAnnotationPositon(coordinates);
+      this.$emit('moveNote', { noteId, discussionId, position });
+    },
   },
 };
 </script>
@@ -226,6 +230,7 @@ export default {
         :notes="discussionStartingNotes"
         :current-comment-form="currentCommentForm"
         @openCommentForm="openCommentForm"
+        @moveNote="moveNote"
       />
     </div>
   </div>

ee/app/assets/javascripts/design_management/graphql/fragments/designNote.fragment.graphql

 #import "./diffRefs.fragment.graphql"
 #import "~/graphql_shared/fragments/author.fragment.graphql"
+#import "./notePermissions.fragment.graphql"
 
 fragment DesignNote on Note {
   id
@@ -18,4 +19,10 @@ fragment DesignNote on Note {
     height
     width
   }
+  userPermissions {
+    ...DesignNotePermissions
+  }
+  discussion {
+    id
+  }
 }

ee/app/assets/javascripts/design_management/graphql/fragments/notePermissions.fragment.graphql

+fragment DesignNotePermissions on NotePermissions {
+  adminNote
+}
\ No newline at end of file

ee/app/assets/javascripts/design_management/graphql/mutations/updateImageDiffNote.mutation.graphql

+#import "../fragments/designNote.fragment.graphql"
+
+mutation updateImageDiffNote($input: UpdateImageDiffNoteInput!) {
+  updateImageDiffNote(input: $input) {
+    errors
+    note {
+      ...DesignNote
+    }
+  }
+}

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

@@ -15,14 +15,21 @@ import DesignPresentation from '../../components/design_presentation.vue';
 import getDesignQuery from '../../graphql/queries/getDesign.query.graphql';
 import appDataQuery from '../../graphql/queries/appData.query.graphql';
 import createImageDiffNoteMutation from '../../graphql/mutations/createImageDiffNote.mutation.graphql';
+import updateImageDiffNoteMutation from '../../graphql/mutations/updateImageDiffNote.mutation.graphql';
 import {
   extractDiscussions,
   extractDesign,
   extractParticipants,
+  updateImageDiffNoteOptimisticResponse,
 } from '../../utils/design_management_utils';
-import { updateStoreAfterAddImageDiffNote } from '../../utils/cache_update';
+import {
+  updateStoreAfterAddImageDiffNote,
+  updateStoreAfterUpdateImageDiffNote,
+} from '../../utils/cache_update';
 import {
   ADD_DISCUSSION_COMMENT_ERROR,
+  ADD_IMAGE_DIFF_NOTE_ERROR,
+  UPDATE_IMAGE_DIFF_NOTE_ERROR,
   DESIGN_NOT_FOUND_ERROR,
   DESIGN_NOT_EXIST_ERROR,
   designDeletionError,
@@ -165,19 +172,62 @@ export default {
         this.designVariables,
       );
     },
+    updateImageDiffNoteInStore(
+      store,
+      {
+        data: { updateImageDiffNote },
+      },
+    ) {
+      return updateStoreAfterUpdateImageDiffNote(
+        store,
+        updateImageDiffNote,
+        getDesignQuery,
+        this.designVariables,
+      );
+    },
+    onMoveNote({ noteId, discussionId, position }) {
+      const discussion = this.discussions.find(({ id }) => id === discussionId);
+      const note = discussion.notes.find(
+        ({ discussion: noteDiscussion }) => noteDiscussion.id === discussionId,
+      );
+
+      const mutationPayload = {
+        optimisticResponse: updateImageDiffNoteOptimisticResponse(note, {
+          position,
+        }),
+        variables: {
+          input: {
+            id: noteId,
+            position,
+          },
+        },
+        mutation: updateImageDiffNoteMutation,
+        update: this.updateImageDiffNoteInStore,
+      };
+
+      return this.$apollo.mutate(mutationPayload).catch(e => this.onUpdateImageDiffNoteError(e));
+    },
     onQueryError(message) {
       // because we redirect user to /designs (the issue page),
       // we want to create these flashes on the issue page
       createFlash(message);
       this.$router.push({ name: this.$options.DESIGNS_ROUTE_NAME });
     },
-    onDiffNoteError(e) {
-      this.errorMessage = ADD_DISCUSSION_COMMENT_ERROR;
+    onError(message, e) {
+      this.errorMessage = message;
       throw e;
     },
+    onCreateImageDiffNoteError(e) {
+      this.onError(ADD_IMAGE_DIFF_NOTE_ERROR, e);
+    },
+    onDesignDiscussionError(e) {
+      this.onError(ADD_DISCUSSION_COMMENT_ERROR, e);
+    },
+    onUpdateImageDiffNoteError(e) {
+      this.onError(UPDATE_IMAGE_DIFF_NOTE_ERROR, e);
+    },
     onDesignDeleteError(e) {
-      this.errorMessage = designDeletionError({ singular: true });
-      throw e;
+      this.onError(designDeletionError({ singular: true }), e);
     },
     openCommentForm(annotationCoordinates) {
       this.annotationCoordinates = annotationCoordinates;
@@ -239,6 +289,7 @@ export default {
           :is-annotating="isAnnotating"
           :scale="scale"
           @openCommentForm="openCommentForm"
+          @moveNote="onMoveNote"
         />
 
         <div class="design-scaler-wrapper position-absolute mb-4 d-flex-center">
@@ -264,7 +315,7 @@ export default {
             :noteable-id="design.id"
             :discussion-index="index + 1"
             :markdown-preview-path="markdownPreviewPath"
-            @error="onDiffNoteError"
+            @error="onDesignDiscussionError"
           />
           <apollo-mutation
             v-if="annotationCoordinates"
@@ -275,7 +326,7 @@ export default {
             }"
             :update="addImageDiffNoteToStore"
             @done="closeCommentForm"
-            @error="onDiffNoteError"
+            @error="onCreateImageDiffNoteError"
           >
             <design-reply-form
               v-model="comment"

ee/app/assets/javascripts/design_management/utils/cache_update.js

@@ -2,6 +2,7 @@ import createFlash from '~/flash';
 import { extractCurrentDiscussion, extractDesign } from './design_management_utils';
 import {
   ADD_IMAGE_DIFF_NOTE_ERROR,
+  UPDATE_IMAGE_DIFF_NOTE_ERROR,
   ADD_DISCUSSION_COMMENT_ERROR,
   UPLOAD_DESIGN_ERROR,
   designDeletionError,
@@ -148,6 +149,43 @@ const addImageDiffNoteToStore = (store, createImageDiffNote, query, variables) =
   });
 };
 
+const updateImageDiffNoteInStore = (store, updateImageDiffNote, query, variables) => {
+  const data = store.readQuery({
+    query,
+    variables,
+  });
+
+  const design = extractDesign(data);
+  const discussion = extractCurrentDiscussion(
+    design.discussions,
+    updateImageDiffNote.note.discussion.id,
+  );
+
+  discussion.node = {
+    ...discussion.node,
+    notes: {
+      ...discussion.node.notes,
+      edges: [
+        // the first note is original discussion, and includes the pin `position`
+        {
+          __typename: 'NoteEdge',
+          node: updateImageDiffNote.note,
+        },
+        ...discussion.node.notes.edges.slice(1),
+      ],
+    },
+  };
+
+  store.writeQuery({
+    query,
+    variables,
+    data: {
+      ...data,
+      design,
+    },
+  });
+};
+
 const addNewDesignToStore = (store, designManagementUpload, query) => {
   const data = store.readQuery(query);
 
@@ -244,6 +282,14 @@ export const updateStoreAfterAddImageDiffNote = (store, data, query, queryVariab
   }
 };
 
+export const updateStoreAfterUpdateImageDiffNote = (store, data, query, queryVariables) => {
+  if (hasErrors(data)) {
+    onError(data, UPDATE_IMAGE_DIFF_NOTE_ERROR);
+  } else {
+    updateImageDiffNoteInStore(store, data, query, queryVariables);
+  }
+};
+
 export const updateStoreAfterUploadDesign = (store, data, query) => {
   if (hasErrors(data)) {
     onError(data, UPLOAD_DESIGN_ERROR);

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

@@ -89,6 +89,27 @@ export const designUploadOptimisticResponse = files => {
   };
 };
 
+/**
+ * Generates optimistic response for a design upload mutation
+ * @param {Array<File>} files
+ */
+export const updateImageDiffNoteOptimisticResponse = (note, { position }) => ({
+  // False positive i18n lint: https://gitlab.com/gitlab-org/frontend/eslint-plugin-i18n/issues/26
+  // eslint-disable-next-line @gitlab/i18n/no-non-i18n-strings
+  __typename: 'Mutation',
+  updateImageDiffNote: {
+    __typename: 'UpdateImageDiffNotePayload',
+    note: {
+      ...note,
+      position: {
+        ...note.position,
+        ...position,
+      },
+    },
+    errors: [],
+  },
+});
+
 const normalizeAuthor = author => ({
   ...author,
   web_url: author.webUrl,

ee/app/assets/javascripts/design_management/utils/error_messages.js

@@ -8,6 +8,10 @@ export const ADD_IMAGE_DIFF_NOTE_ERROR = s__(
   'DesignManagement|Could not create new discussion. Please try again.',
 );
 
+export const UPDATE_IMAGE_DIFF_NOTE_ERROR = s__(
+  'DesignManagement|Could not update discussion. Please try again.',
+);
+
 export const UPLOAD_DESIGN_ERROR = s__(
   'DesignManagement|Error uploading a new design. Please try again.',
 );

ee/changelogs/unreleased/moveable-comment-pins.yml

+---
+title: Moveable design note pins
+merge_request: 24934
+author:
+type: added

ee/spec/frontend/design_management/components/design_overlay_spec.js

 import { mount } from '@vue/test-utils';
 import DesignOverlay from 'ee/design_management/components/design_overlay.vue';
+import notes from '../mock_data/notes';
 
 describe('Design overlay component', () => {
   let wrapper;
 
-  const notes = [
-    {
-      position: {
-        height: 100,
-        width: 100,
-        x: 10,
-        y: 15,
-      },
-    },
-    {
-      position: {
-        height: 50,
-        width: 50,
-        x: 25,
-        y: 25,
-      },
-    },
-  ];
-
   const mockDimensions = { width: 100, height: 100 };
+  const mockNoteNotAuthorised = {
+    id: 'note-not-authorised',
+    discussion: { id: 'discussion-not-authorised' },
+    position: {
+      x: 1,
+      y: 80,
+      ...mockDimensions,
+    },
+    userPermissions: {},
+  };
 
   const findOverlay = () => wrapper.find('.image-diff-overlay');
   const findAllNotes = () => wrapper.findAll('.js-image-badge');
@@ -66,10 +58,10 @@ describe('Design overlay component', () => {
       x: 10,
       y: 10,
     };
+
     wrapper
       .find('.image-diff-overlay-add-comment')
       .trigger('click', { offsetX: newCoordinates.x, offsetY: newCoordinates.y });
-
     return wrapper.vm.$nextTick().then(() => {
       expect(wrapper.emitted('openCommentForm')).toEqual([
         [{ x: newCoordinates.x, y: newCoordinates.y }],
@@ -117,6 +109,78 @@ describe('Design overlay component', () => {
     });
   });
 
+  describe('when moving notes', () => {
+    it('should update badge style when note is being moved', () => {
+      createComponent({
+        notes,
+      });
+
+      const { position } = notes[0];
+
+      return clickAndDragBadge(
+        findFirstBadge(),
+        { x: position.x, y: position.y },
+        { x: 20, y: 20 },
+      ).then(() => {
+        expect(findFirstBadge().attributes().style).toBe('left: 20px; top: 20px; cursor: move;');
+      });
+    });
+
+    it('should emit `moveNote` event when note-moving action ends', () => {
+      createComponent({ notes });
+      const note = notes[0];
+      const { position } = note;
+      const newCoordinates = { x: 20, y: 20 };
+
+      wrapper.setData({
+        movingNoteNewPosition: {
+          ...position,
+          ...newCoordinates,
+        },
+        movingNoteStartPosition: {
+          noteId: notes[0].id,
+          discussionId: notes[0].discussion.id,
+          ...position,
+        },
+      });
+
+      const badge = findFirstBadge();
+      return clickAndDragBadge(badge, { x: position.x, y: position.y }, newCoordinates)
+        .then(() => {
+          badge.trigger('mouseup');
+          return wrapper.vm.$nextTick();
+        })
+        .then(() => {
+          expect(wrapper.emitted('moveNote')).toEqual([
+            [
+              {
+                noteId: notes[0].id,
+                discussionId: notes[0].discussion.id,
+                coordinates: newCoordinates,
+              },
+            ],
+          ]);
+        });
+    });
+
+    it('should do nothing if [adminNote] permission is not present', () => {
+      createComponent({
+        dimensions: mockDimensions,
+        notes: [mockNoteNotAuthorised],
+      });
+
+      const badge = findAllNotes().at(0);
+      return clickAndDragBadge(
+        badge,
+        { x: mockNoteNotAuthorised.x, y: mockNoteNotAuthorised.y },
+        { x: 20, y: 20 },
+      ).then(() => {
+        expect(wrapper.vm.movingNoteStartPosition).toBeNull();
+        expect(findFirstBadge().attributes().style).toBe('left: 1px; top: 80px;');
+      });
+    });
+  });
+
   describe('with a new form', () => {
     it('should render a new comment badge', () => {
       createComponent({
@@ -255,4 +319,25 @@ describe('Design overlay component', () => {
       expect(wrapper.vm.getNoteRelativePosition(position)).toEqual({ left: 25, top: 25 });
     });
   });
+
+  describe('canMoveNote', () => {
+    it.each`
+      adminNotePermission | canMoveNoteResult
+      ${true}             | ${true}
+      ${false}            | ${false}
+      ${undefined}        | ${false}
+    `(
+      'returns [$canMoveNoteResult] when [adminNote permission] is [$adminNotePermission]',
+      ({ adminNotePermission, canMoveNoteResult }) => {
+        createComponent();
+
+        const note = {
+          userPermissions: {
+            adminNote: adminNotePermission,
+          },
+        };
+        expect(wrapper.vm.canMoveNote(note)).toBe(canMoveNoteResult);
+      },
+    );
+  });
 });

ee/spec/frontend/design_management/mock_data/notes.js

+export default [
+  {
+    id: 'note-id-1',
+    position: {
+      height: 100,
+      width: 100,
+      x: 10,
+      y: 15,
+    },
+    userPermissions: {
+      adminNote: true,
+    },
+    discussion: {
+      id: 'discussion-id-1',
+    },
+  },
+  {
+    id: 'note-id-2',
+    position: {
+      height: 50,
+      width: 50,
+      x: 25,
+      y: 25,
+    },
+    userPermissions: {
+      adminNote: true,
+    },
+    discussion: {
+      id: 'discussion-id-2',
+    },
+  },
+];

ee/spec/frontend/design_management/utils/cache_update_spec.js

@@ -4,12 +4,14 @@ import {
   updateStoreAfterAddDiscussionComment,
   updateStoreAfterAddImageDiffNote,
   updateStoreAfterUploadDesign,
+  updateStoreAfterUpdateImageDiffNote,
 } from 'ee/design_management/utils/cache_update';
 import {
   designDeletionError,
   ADD_DISCUSSION_COMMENT_ERROR,
   ADD_IMAGE_DIFF_NOTE_ERROR,
   UPLOAD_DESIGN_ERROR,
+  UPDATE_IMAGE_DIFF_NOTE_ERROR,
 } from 'ee/design_management/utils/error_messages';
 import design from '../mock_data/design';
 import createFlash from '~/flash';
@@ -32,6 +34,7 @@ describe('Design Management cache update', () => {
       ${'updateStoreAfterAddDiscussionComment'} | ${updateStoreAfterAddDiscussionComment} | ${ADD_DISCUSSION_COMMENT_ERROR}            | ${[]}
       ${'updateStoreAfterAddImageDiffNote'}     | ${updateStoreAfterAddImageDiffNote}     | ${ADD_IMAGE_DIFF_NOTE_ERROR}               | ${[]}
       ${'updateStoreAfterUploadDesign'}         | ${updateStoreAfterUploadDesign}         | ${UPLOAD_DESIGN_ERROR}                     | ${[]}
+      ${'updateStoreAfterUpdateImageDiffNote'}  | ${updateStoreAfterUpdateImageDiffNote}  | ${UPDATE_IMAGE_DIFF_NOTE_ERROR}            | ${[]}
     `('$fnName handles errors in response', ({ subject, extraArgs, errorMessage }) => {
       expect(createFlash).not.toHaveBeenCalled();
       expect(() => subject(mockStore, { errors: mockErrors }, {}, ...extraArgs)).toThrow();

ee/spec/frontend/design_management/utils/design_management_utils_spec.js

@@ -4,6 +4,7 @@ import {
   extractDiscussions,
   findVersionId,
   designUploadOptimisticResponse,
+  updateImageDiffNoteOptimisticResponse,
 } from 'ee/design_management/utils/design_management_utils';
 
 describe('extractCurrentDiscussion', () => {
@@ -72,7 +73,7 @@ describe('version parser', () => {
 });
 
 describe('optimistic responses', () => {
-  it('correctly generated for design upload', () => {
+  it('correctly generated for designManagementUpload', () => {
     jest.spyOn(underscore, 'uniqueId').mockImplementation(() => 1);
     const expectedResponse = {
       __typename: 'Mutation',
@@ -104,4 +105,32 @@ describe('optimistic responses', () => {
     };
     expect(designUploadOptimisticResponse([{ name: 'test' }])).toEqual(expectedResponse);
   });
+
+  it('correctly generated for updateImageDiffNoteOptimisticResponse', () => {
+    const mockNote = {
+      id: 'test-note-id',
+    };
+
+    const mockPosition = {
+      x: 10,
+      y: 10,
+      width: 10,
+      height: 10,
+    };
+
+    const expectedResponse = {
+      __typename: 'Mutation',
+      updateImageDiffNote: {
+        __typename: 'UpdateImageDiffNotePayload',
+        note: {
+          ...mockNote,
+          position: mockPosition,
+        },
+        errors: [],
+      },
+    };
+    expect(updateImageDiffNoteOptimisticResponse(mockNote, { position: mockPosition })).toEqual(
+      expectedResponse,
+    );
+  });
 });

locale/gitlab.pot

@@ -6540,6 +6540,9 @@ msgstr ""
 msgid "DesignManagement|Could not create new discussion. Please try again."
 msgstr ""
 
+msgid "DesignManagement|Could not update discussion. Please try again."
+msgstr ""
+
 msgid "DesignManagement|Delete"
 msgstr ""