Merge branch '317-edit-comment' into 'main'

feat(mr review): editing comments on MR diffs

See merge request gitlab-org/gitlab-vscode-extension!197

README.md

@@ -250,7 +250,7 @@ You can use [GitLab Slash Commands](https://docs.gitlab.com/ee/integration/slash
 
 GitLab Workflow allows you to review changes and comments on merge requests directly inside the editor. You can navigate to merge requests on the left hand sidebar and expand any relevant merge requests to see both the description and files changed.
 
-When navigating changed files you can review comments that have been made on the diff.
+When navigating changed files you can review discussions that have been made on the diff. You can resolve and unresolve these discussions and also delete and edit individual comments.
 
 ![_diff-comments_.gif](https://gitlab.com/gitlab-org/gitlab-vscode-extension/raw/main/src/assets/_diff-comments.gif)
 

package.json

@@ -142,6 +142,22 @@
         "title": "Delete comment",
         "category": "GitLab",
         "icon": "$(trash)"
+      },
+      {
+        "command": "gl.startEditingComment",
+        "title": "Edit Comment",
+        "category": "GitLab",
+        "icon": "$(edit)"
+      },
+      {
+        "command": "gl.cancelEditingComment",
+        "title": "Cancel",
+        "category": "GitLab"
+      },
+      {
+        "command": "gl.submitCommentEdit",
+        "title": "Save comment",
+        "category": "GitLab"
       }
     ],
     "menus": {
@@ -157,6 +173,18 @@
         {
           "command": "gl.deleteComment",
           "when": "false"
+        },
+        {
+          "command": "gl.startEditingComment",
+          "when": "false"
+        },
+        {
+          "command": "gl.cancelEditingComment",
+          "when": "false"
+        },
+        {
+          "command": "gl.submitCommentEdit",
+          "when": "false"
         }
       ],
       "view/title": [
@@ -168,9 +196,24 @@
       ],
       "comments/comment/title": [
         {
-          "command": "gl.deleteComment",
+          "command": "gl.startEditingComment",
           "group": "inline@1",
           "when": "comment =~ /canAdmin/"
+        },
+        {
+          "command": "gl.deleteComment",
+          "group": "inline@2",
+          "when": "comment =~ /canAdmin/"
+        }
+      ],
+      "comments/comment/context": [
+        {
+          "command": "gl.submitCommentEdit",
+          "group": "inline@1"
+        },
+        {
+          "command": "gl.cancelEditingComment",
+          "group": "inline@2"
         }
       ],
       "comments/commentThread/title": [

src/__mocks__/vscode.js

@@ -30,7 +30,7 @@ module.exports = {
   extensions: {
     getExtension: jest.fn(),
   },
-  CommentMode: { Preview: 1 },
+  CommentMode: { Editing: 0, Preview: 1 },
   StatusBarAlignment: { Left: 0 },
   CommentThreadCollapsibleState: { Collapsed: 0, Expanded: 1 },
   Position: function Position(line, character) {

src/command_names.ts

@@ -27,6 +27,9 @@ export const USER_COMMANDS = {
   RESOLVE_THREAD: 'gl.resolveThread',
   UNRESOLVE_THREAD: 'gl.unresolveThread',
   DELETE_COMMENT: 'gl.deleteComment',
+  START_EDITING_COMMENT: 'gl.startEditingComment',
+  CANCEL_EDITING_COMMENT: 'gl.cancelEditingComment',
+  SUBMIT_COMMENT_EDIT: 'gl.submitCommentEdit',
 };
 
 /*

src/commands/mr_discussion_commands.ts

@@ -26,3 +26,15 @@ export const deleteComment = async (comment: GitLabComment): Promise<void> => {
   }
   return comment.thread.deleteComment(comment);
 };
+
+export const editComment = (comment: GitLabComment): void => {
+  comment.thread.startEdit(comment);
+};
+
+export const cancelEdit = (comment: GitLabComment): void => {
+  comment.thread.cancelEdit(comment);
+};
+
+export const submitEdit = async (comment: GitLabComment): Promise<void> => {
+  return comment.thread.submitEdit(comment);
+};

src/data_providers/items/mr_item_model.ts

@@ -71,7 +71,7 @@ export class MrItemModel extends ItemModel {
       return GitLabCommentThread.createThread({
         commentController,
         workspaceFolder: this.workspace.uri,
-        gitlabProjectId: this.mr.project_id,
+        mr: this.mr,
         discussion,
         gitlabService,
       });

src/extension.js

@@ -18,7 +18,13 @@ const { REVIEW_URI_SCHEME } = require('./constants');
 const { USER_COMMANDS, PROGRAMMATIC_COMMANDS } = require('./command_names');
 const { CiCompletionProvider } = require('./completion/ci_completion_provider');
 const { GitExtensionWrapper } = require('./git/git_extension_wrapper');
-const { toggleResolved, deleteComment } = require('./commands/mr_discussion_commands');
+const {
+  toggleResolved,
+  deleteComment,
+  editComment: startEdit,
+  cancelEdit,
+  submitEdit,
+} = require('./commands/mr_discussion_commands');
 
 vscode.gitLabWorkflow = {
   sidebarDataProviders: [],
@@ -73,6 +79,9 @@ const registerCommands = (context, outputChannel) => {
     [USER_COMMANDS.RESOLVE_THREAD]: toggleResolved,
     [USER_COMMANDS.UNRESOLVE_THREAD]: toggleResolved,
     [USER_COMMANDS.DELETE_COMMENT]: deleteComment,
+    [USER_COMMANDS.START_EDITING_COMMENT]: startEdit,
+    [USER_COMMANDS.CANCEL_EDITING_COMMENT]: cancelEdit,
+    [USER_COMMANDS.SUBMIT_COMMENT_EDIT]: submitEdit,
     [PROGRAMMATIC_COMMANDS.NO_IMAGE_REVIEW]: () =>
       vscode.window.showInformationMessage("GitLab MR review doesn't support images yet."),
   };

src/gitlab/gitlab_new_service.ts

@@ -141,6 +141,10 @@ interface GetDiscussionsOptions {
   endCursor?: string;
 }
 
+interface RestNote {
+  body: string;
+}
+
 function isLabelEvent(note: Note): note is RestLabelEvent {
   return (note as RestLabelEvent).label !== undefined;
 }
@@ -309,6 +313,14 @@ const deleteNoteMutation = gql`
   }
 `;
 
+const updateNoteBodyMutation = gql`
+  mutation UpdateNoteBody($noteId: NoteID!, $body: String) {
+    updateNote(input: { id: $noteId, body: $body }) {
+      errors
+    }
+  }
+`;
+
 const getProjectPath = (issuable: RestIssuable) => issuable.references.full.split(/[#!]/)[0];
 const isMr = (issuable: RestIssuable) => Boolean(issuable.sha);
 const getIssuableGqlId = (issuable: RestIssuable) =>
@@ -550,4 +562,52 @@ export class GitLabNewService {
       );
     }
   }
+
+  /**
+   * This method is used only as a replacement of optimistic locking when updating a note.
+   * We request the latest note to validate that it hasn't changed since we last saw it.
+   */
+  private async getMrNote(mr: RestIssuable, noteId: number): Promise<RestNote> {
+    const noteUrl = `${this.instanceUrl}/api/v4/projects/${mr.project_id}/merge_requests/${mr.iid}/notes/${noteId}`;
+    const result = await crossFetch(noteUrl, this.fetchOptions);
+    if (!result.ok) {
+      throw new FetchError(`Fetching the latest note from ${noteUrl} failed`, result);
+    }
+    return result.json();
+  }
+
+  async updateNoteBody(
+    noteGqlId: string,
+    body: string,
+    originalBody: string,
+    mr: RestIssuable,
+  ): Promise<void> {
+    const latestNote = await this.getMrNote(mr, getRestIdFromGraphQLId(noteGqlId));
+    // This check is the best workaround we can do in the lack of optimistic locking
+    // Issue to make this check in the GitLab instance: https://gitlab.com/gitlab-org/gitlab/-/issues/323808
+    if (latestNote.body !== originalBody) {
+      throw new UserFriendlyError(
+        `This comment changed after you last viewed it, and can't be edited.
+        Your new comment is NOT lost. To retrieve it, edit the comment again and copy your comment text,
+        then update the original comment by opening the sidebar and running the
+        "GitLab: Refresh sidebar" command.`,
+        new Error(
+          `You last saw:\n"${originalBody}"\nbut the latest version is:\n"${latestNote.body}"`,
+        ),
+      );
+    }
+    try {
+      await this.client.request<void>(updateNoteBodyMutation, {
+        noteId: noteGqlId,
+        body,
+      });
+    } catch (e) {
+      throw new UserFriendlyError(
+        `Couldn't update the comment when calling the API.
+        Your draft hasn't been lost. To see it, edit the comment.
+        For more information, review the extension logs.`,
+        e,
+      );
+    }
+  }
 }

src/review/gitlab_comment.ts

@@ -2,11 +2,18 @@ import * as vscode from 'vscode';
 import { GqlTextDiffNote } from '../gitlab/gitlab_new_service';
 import { GitLabCommentThread } from './gitlab_comment_thread';
 
+interface CommentOptions {
+  mode?: vscode.CommentMode;
+  body?: string;
+  note?: GqlTextDiffNote;
+}
+
 export class GitLabComment implements vscode.Comment {
   protected constructor(
     readonly gqlNote: GqlTextDiffNote,
     public mode: vscode.CommentMode,
     readonly thread: GitLabCommentThread,
+    public body: string,
   ) {}
 
   get id(): string {
@@ -25,11 +32,33 @@ export class GitLabComment implements vscode.Comment {
     };
   }
 
-  get body(): string {
-    return this.gqlNote.body;
+  resetBody(): GitLabComment {
+    return this.copyWith({ body: this.gqlNote.body });
+  }
+
+  markBodyAsSubmitted(): GitLabComment {
+    return this.copyWith({
+      note: {
+        ...this.gqlNote,
+        body: this.body, // this synchronizes the API response with the latest body
+      },
+    });
+  }
+
+  withMode(mode: vscode.CommentMode): GitLabComment {
+    return this.copyWith({ mode });
+  }
+
+  private copyWith({ mode, body, note }: CommentOptions) {
+    return new GitLabComment(
+      note ?? this.gqlNote,
+      mode ?? this.mode,
+      this.thread,
+      body ?? this.body,
+    );
   }
 
   static fromGqlNote(gqlNote: GqlTextDiffNote, thread: GitLabCommentThread): GitLabComment {
-    return new GitLabComment(gqlNote, vscode.CommentMode.Preview, thread);
+    return new GitLabComment(gqlNote, vscode.CommentMode.Preview, thread, gqlNote.body);
   }
 }

src/review/gitlab_comment_thread.test.ts

@@ -10,12 +10,26 @@ import {
   GqlTextDiffDiscussion,
   GqlTextDiffNote,
 } from '../gitlab/gitlab_new_service';
+import { mr } from '../test_utils/entities';
 
 describe('GitLabCommentThread', () => {
   let gitlabCommentThread: GitLabCommentThread;
   let vsCommentThread: vscode.CommentThread;
   let gitlabService: GitLabNewService;
 
+  const twoNotes = [
+    {
+      ...(noteOnDiff as GqlTextDiffNote),
+      id: 'gid://gitlab/DiffNote/1',
+      body: 'first body',
+    },
+    {
+      ...(noteOnDiff as GqlTextDiffNote),
+      id: 'gid://gitlab/DiffNote/2',
+      body: 'second body',
+    },
+  ];
+
   const createGqlTextDiffDiscussion: (...notes: GqlTextDiffNote[]) => GqlTextDiffDiscussion = (
     ...notes
   ) => {
@@ -48,11 +62,12 @@ describe('GitLabCommentThread', () => {
     gitlabService = ({
       setResolved: jest.fn(),
       deleteNote: jest.fn(),
+      updateNoteBody: jest.fn(),
     } as unknown) as GitLabNewService;
     gitlabCommentThread = GitLabCommentThread.createThread({
       commentController: fakeCommentController,
       workspaceFolder: '/workspaceFolder',
-      gitlabProjectId: 12345,
+      mr,
       discussion,
       gitlabService,
     });
@@ -127,23 +142,12 @@ describe('GitLabCommentThread', () => {
     expect(vsCommentThread.comments.length).toBe(1);
     const [comment] = vsCommentThread.comments;
     expect(comment).toBeInstanceOf(GitLabComment);
-    expect((comment as GitLabComment).body).toBe(noteOnDiff.body);
+    expect(comment.body).toBe(noteOnDiff.body);
   });
 
   describe('deleting comments', () => {
     it('deletes a comment', async () => {
-      createGitLabCommentThread(
-        createGqlTextDiffDiscussion(
-          {
-            ...(noteOnDiff as GqlTextDiffNote),
-            id: 'gid://gitlab/DiffNote/1',
-          },
-          {
-            ...(noteOnDiff as GqlTextDiffNote),
-            id: 'gid://gitlab/DiffNote/2',
-          },
-        ),
-      );
+      createGitLabCommentThread(createGqlTextDiffDiscussion(...twoNotes));
       (gitlabService.deleteNote as jest.Mock).mockResolvedValue(undefined);
 
       await gitlabCommentThread.deleteComment(vsCommentThread.comments[0] as GitLabComment);
@@ -174,4 +178,70 @@ describe('GitLabCommentThread', () => {
       expect(vsCommentThread.comments.length).toBe(1);
     });
   });
+
+  describe('editing comments', () => {
+    beforeEach(() => {
+      createGitLabCommentThread(createGqlTextDiffDiscussion(...twoNotes));
+    });
+
+    it('starts editing comment', () => {
+      gitlabCommentThread.startEdit(vsCommentThread.comments[0] as GitLabComment);
+
+      expect(vsCommentThread.comments[0].mode).toBe(vscode.CommentMode.Editing);
+      expect(vsCommentThread.comments[1].mode).toBe(vscode.CommentMode.Preview);
+    });
+
+    it('replaces the original comments array when editing a comment', () => {
+      const originalCommentArray = vsCommentThread.comments;
+
+      gitlabCommentThread.startEdit(vsCommentThread.comments[0] as GitLabComment);
+
+      // this is important because the real vscode.CommentThread implementation listens
+      // on `set comments()` and updates the visual representation when the array reference changes
+      expect(vsCommentThread.comments).not.toBe(originalCommentArray);
+    });
+
+    it('stops editing the comment and resets the comment body', () => {
+      gitlabCommentThread.startEdit(vsCommentThread.comments[0] as GitLabComment);
+      vsCommentThread.comments[0].body = 'new body'; // vs code updates the edited text in place
+
+      gitlabCommentThread.cancelEdit(vsCommentThread.comments[0] as GitLabComment);
+
+      expect(vsCommentThread.comments[0].mode).toBe(vscode.CommentMode.Preview);
+      expect(vsCommentThread.comments[0].body).toBe('first body');
+      expect(vsCommentThread.comments[1].mode).toBe(vscode.CommentMode.Preview);
+    });
+  });
+
+  describe('updating comments', () => {
+    beforeEach(() => {
+      createGitLabCommentThread(createGqlTextDiffDiscussion(...twoNotes));
+      gitlabCommentThread.startEdit(vsCommentThread.comments[0] as GitLabComment);
+      vsCommentThread.comments[0].body = 'updated body';
+    });
+
+    it('submits updated comment', async () => {
+      (gitlabService.updateNoteBody as jest.Mock).mockResolvedValue(undefined);
+
+      await gitlabCommentThread.submitEdit(vsCommentThread.comments[0] as GitLabComment);
+
+      expect(vsCommentThread.comments[0].mode).toBe(vscode.CommentMode.Preview);
+      expect(vsCommentThread.comments[0].body).toBe('updated body');
+      expect((vsCommentThread.comments[0] as GitLabComment).gqlNote.body).toBe('updated body');
+    });
+
+    it("doesn't update the underlying GraphQL note body if API update fails", async () => {
+      const error = new Error();
+
+      (gitlabService.updateNoteBody as jest.Mock).mockRejectedValue(error);
+
+      await expect(
+        gitlabCommentThread.submitEdit(vsCommentThread.comments[0] as GitLabComment),
+      ).rejects.toBe(error);
+
+      expect(vsCommentThread.comments[0].mode).toBe(vscode.CommentMode.Editing);
+      expect(vsCommentThread.comments[0].body).toBe('updated body');
+      expect((vsCommentThread.comments[0] as GitLabComment).gqlNote.body).toBe('first body');
+    });
+  });
 });

src/review/gitlab_comment_thread.ts

@@ -33,7 +33,7 @@ const uriFromPosition = (
 interface CreateThreadOptions {
   commentController: vscode.CommentController;
   workspaceFolder: string;
-  gitlabProjectId: number;
+  mr: RestIssuable;
   discussion: GqlTextDiffDiscussion;
   gitlabService: GitLabNewService;
 }
@@ -45,6 +45,7 @@ export class GitLabCommentThread {
     private vsThread: vscode.CommentThread,
     private gqlDiscussion: GqlTextDiffDiscussion,
     private gitlabService: GitLabNewService,
+    private mr: RestIssuable,
   ) {
     this.vsThread.collapsibleState = vscode.CommentThreadCollapsibleState.Expanded;
     this.vsThread.canReply = false;
@@ -75,6 +76,35 @@ export class GitLabCommentThread {
     }
   }
 
+  startEdit(comment: GitLabComment): void {
+    this.changeOneComment(comment.id, c => c.withMode(vscode.CommentMode.Editing));
+  }
+
+  cancelEdit(comment: GitLabComment): void {
+    this.changeOneComment(comment.id, c => c.withMode(vscode.CommentMode.Preview).resetBody());
+  }
+
+  async submitEdit(comment: GitLabComment): Promise<void> {
+    await this.gitlabService.updateNoteBody(
+      comment.id,
+      comment.body,
+      comment.gqlNote.body, // this is what we think is the latest version stored in API
+      this.mr,
+    );
+    this.changeOneComment(comment.id, c =>
+      c.markBodyAsSubmitted().withMode(vscode.CommentMode.Preview),
+    );
+  }
+
+  private changeOneComment(id: string, changeFn: (c: GitLabComment) => GitLabComment): void {
+    this.vsThread.comments = this.vsThread.comments.map(c => {
+      if (c instanceof GitLabComment && c.id === id) {
+        return changeFn(c);
+      }
+      return c;
+    });
+  }
+
   private updateThreadContext() {
     // when user doesn't have permission to resolve the discussion we don't show the
     // resolve/unresolve buttons at all (`context` stays `undefined`) because otherwise
@@ -91,19 +121,19 @@ export class GitLabCommentThread {
   static createThread({
     commentController,
     workspaceFolder,
-    gitlabProjectId,
+    mr,
     discussion,
     gitlabService,
   }: CreateThreadOptions): GitLabCommentThread {
     const { position } = discussion.notes.nodes[0];
     const vsThread = commentController.createCommentThread(
-      uriFromPosition(position, workspaceFolder, gitlabProjectId),
+      uriFromPosition(position, workspaceFolder, mr.project_id),
       commentRangeFromPosition(position),
       // the comments need to know about the thread, so we first
       // create empty thread to be able to create comments
       [],
     );
-    const glThread = new GitLabCommentThread(vsThread, discussion, gitlabService);
+    const glThread = new GitLabCommentThread(vsThread, discussion, gitlabService, mr);
     vsThread.comments = discussion.notes.nodes.map(note =>
       GitLabComment.fromGqlNote(note, glThread),
     );

test/integration/fixtures/rest/diff_note.json

+{
+  "id": 469379582,
+  "type": "DiffNote",
+  "body": "This is the core improvement. `NoteBody` sends the `note.body` to our `/api/v4/markdown` endpoint to render HTML. For labels, we can easily render the HTML ourselves, saving all the API requests and complexity.",
+  "attachment": null,
+  "author": {
+    "id": 3457201,
+    "name": "Tomas Vik",
+    "username": "viktomas",
+    "state": "active",
+    "avatar_url": "https://secure.gravatar.com/avatar/6042a9152ada74d9fb6a0cdce895337e?s=80&d=identicon",
+    "web_url": "https://gitlab.com/viktomas"
+  },
+  "created_at": "2021-03-08T14:19:18.296Z",
+  "updated_at": "2021-03-08T14:57:27.990Z",
+  "system": false,
+  "noteable_id": 87196674,
+  "noteable_type": "MergeRequest",
+  "commit_id": null,
+  "position": {
+    "base_sha": "5e6dffa282c5129aa67cd227a0429be21bfdaf80",
+    "start_sha": "5e6dffa282c5129aa67cd227a0429be21bfdaf80",
+    "head_sha": "a2616e0fea06c8b5188e4064a76d623afbf97b0c",
+    "old_path": "test.js",
+    "new_path": "test.ts",
+    "position_type": "text",
+    "old_line": null,
+    "new_line": 19,
+    "line_range": {
+      "start": {
+        "line_code": "5f7b6aabace88f74752a2b616628e855914e4bc7_19_19",
+        "type": "new",
+        "old_line": null,
+        "new_line": 19
+      },
+      "end": {
+        "line_code": "5f7b6aabace88f74752a2b616628e855914e4bc7_19_19",
+        "type": "new",
+        "old_line": null,
+        "new_line": 19
+      }
+    }
+  },
+  "resolvable": true,
+  "resolved": false,
+  "resolved_by": null,
+  "resolved_at": null,
+  "confidential": false,
+  "noteable_iid": 5,
+  "commands_changes": {}
+}

test/integration/mr_review.test.js

@@ -6,9 +6,11 @@ const { graphql } = require('msw');
 const IssuableDataProvider = require('../../src/data_providers/issuable').DataProvider;
 const { MrItemModel } = require('../../src/data_providers/items/mr_item_model');
 const { tokenService } = require('../../src/services/token_service');
+const { submitEdit } = require('../../src/commands/mr_discussion_commands');
 const openMergeRequestResponse = require('./fixtures/rest/open_mr.json');
 const versionsResponse = require('./fixtures/rest/versions.json');
 const versionResponse = require('./fixtures/rest/mr_version.json');
+const diffNote = require('./fixtures/rest/diff_note.json');
 const { projectWithMrDiscussions, noteOnDiff } = require('./fixtures/graphql/discussions');
 const {
   getServer,
@@ -28,6 +30,7 @@ describe('MR Review', () => {
   before(async () => {
     server = getServer([
       createJsonEndpoint('/projects/278964/merge_requests/33824/versions', versionsResponse),
+      createJsonEndpoint('/projects/278964/merge_requests/33824/notes/469379582', diffNote),
       createJsonEndpoint(
         '/projects/278964/merge_requests/33824/versions/127919672',
         versionResponse,
@@ -116,6 +119,19 @@ describe('MR Review', () => {
       assert.strictEqual(range.start.line, noteOnDiff.position.oldLine - 1);
       assert.strictEqual(comments[0].body, noteOnDiff.body);
     });
+
+    it('editing comment fails if the comment body has changed on the GitLab instance', async () => {
+      await dataProvider.getChildren(mrItemModel);
+      const [firstComment] = thread.comments;
+      firstComment.gqlNote.body =
+        'this body simulates that our version of the note body is out of sync with the GitLab instance';
+      firstComment.body = 'user wants to change the body to this';
+
+      await assert.rejects(
+        submitEdit(firstComment),
+        /This comment changed after you last viewed it, and can't be edited/,
+      );
+    });
   });
 
   describe('clicking on a changed file', () => {