Merge branch '339-respond-in-diff-thread' into 'main'

refactor: prepare the code for thread replies

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

src/data_providers/items/mr_item_model.ts

@@ -64,7 +64,6 @@ export class MrItemModel extends ItemModel {
 
     const discussions = await gitlabService.getDiscussions({
       issuable: this.mr,
-      includePosition: true,
     });
     const discussionsOnDiff = discussions.filter(isTextDiffDiscussion);
     const threads = discussionsOnDiff.map(discussion => {

src/gitlab/gitlab_new_service.ts

@@ -36,6 +36,13 @@ interface GqlSnippetProject {
   snippets: Node<GqlSnippet>;
 }
 
+interface CreateNoteResult {
+  createNote: {
+    errors: unknown[];
+    note: GqlNote | null;
+  };
+}
+
 export interface GqlSnippet {
   id: string;
   projectId: string;
@@ -88,6 +95,7 @@ export type GqlTextPosition = GqlOldPosition | GqlNewPosition;
 interface GqlNotePermissions {
   resolveNote: boolean;
   adminNote: boolean;
+  createNote: boolean;
 }
 
 interface GqlGenericNote<T extends GqlBasePosition | null> {
@@ -101,21 +109,25 @@ interface GqlGenericNote<T extends GqlBasePosition | null> {
   position: T;
 }
 
-interface GqlGenericDiscussion<T extends GqlBasePosition | null> {
+interface GqlGenericDiscussion<T extends GqlNote> {
   replyId: string;
   createdAt: string;
   resolved: boolean;
   resolvable: boolean;
-  notes: Node<GqlGenericNote<T>>;
+  notes: Node<T>;
 }
 
 export type GqlTextDiffNote = GqlGenericNote<GqlTextPosition>;
-export type GqlTextDiffDiscussion = GqlGenericDiscussion<GqlTextPosition>;
+type GqlImageNote = GqlGenericNote<GqlImagePosition>;
+export type GqlOverviewNote = GqlGenericNote<null>;
+export type GqlNote = GqlTextDiffNote | GqlImageNote | GqlOverviewNote;
 
 export type GqlDiscussion =
-  | GqlGenericDiscussion<GqlTextPosition>
-  | GqlGenericDiscussion<GqlImagePosition>
-  | GqlGenericDiscussion<null>;
+  | GqlGenericDiscussion<GqlTextDiffNote>
+  | GqlGenericDiscussion<GqlImageNote>
+  | GqlGenericDiscussion<GqlOverviewNote>;
+
+export type GqlTextDiffDiscussion = GqlGenericDiscussion<GqlTextDiffNote>;
 
 interface GqlDiscussionsProject {
   mergeRequest?: {
@@ -137,7 +149,6 @@ type Note = GqlDiscussion | RestLabelEvent;
 
 interface GetDiscussionsOptions {
   issuable: RestIssuable;
-  includePosition?: boolean;
   endCursor?: string;
 }
 
@@ -233,8 +244,31 @@ const positionFragment = gql`
   }
 `;
 
-const createDiscussionsFragment = (includePosition: boolean) => gql`
-${includePosition ? positionFragment : ''}
+const noteDetailsFragment = gql`
+  ${positionFragment}
+  fragment noteDetails on Note {
+    id
+    createdAt
+    system
+    author {
+      avatarUrl
+      name
+      username
+      webUrl
+    }
+    body
+    bodyHtml
+    userPermissions {
+      resolveNote
+      adminNote
+      createNote
+    }
+    ...position
+  }
+`;
+
+const discussionsFragment = gql`
+  ${noteDetailsFragment}
   fragment discussions on DiscussionConnection {
     pageInfo {
       hasNextPage
@@ -251,30 +285,15 @@ ${includePosition ? positionFragment : ''}
           endCursor
         }
         nodes {
-          id
-          createdAt
-          system
-          author {
-            avatarUrl
-            name
-            username
-            webUrl
-          }
-          body
-          bodyHtml
-          userPermissions {
-            resolveNote
-            adminNote
-          }
-          ${includePosition ? `...position` : ''}
+          ...noteDetails
         }
       }
     }
   }
 `;
 
-const constructGetDiscussionsQuery = (isMr: boolean, includePosition = false) => gql`
-  ${createDiscussionsFragment(includePosition)}
+const constructGetDiscussionsQuery = (isMr: boolean) => gql`
+  ${discussionsFragment}
   query Get${
     isMr ? 'Mr' : 'Issue'
   }Discussions($projectPath: ID!, $iid: String!, $afterCursor: String) {
@@ -298,9 +317,13 @@ const discussionSetResolved = gql`
 `;
 
 const createNoteMutation = gql`
+  ${noteDetailsFragment}
   mutation CreateNote($issuableId: NoteableID!, $body: String!, $replyId: DiscussionID) {
     createNote(input: { noteableId: $issuableId, body: $body, discussionId: $replyId }) {
       errors
+      note {
+        ...noteDetails
+      }
     }
   }
 `;
@@ -472,13 +495,9 @@ export class GitLabNewService {
     } as GqlDiscussion;
   }
 
-  async getDiscussions({
-    issuable,
-    includePosition = false,
-    endCursor,
-  }: GetDiscussionsOptions): Promise<GqlDiscussion[]> {
+  async getDiscussions({ issuable, endCursor }: GetDiscussionsOptions): Promise<GqlDiscussion[]> {
     const projectPath = getProjectPath(issuable);
-    const query = constructGetDiscussionsQuery(isMr(issuable), includePosition);
+    const query = constructGetDiscussionsQuery(isMr(issuable));
     const result = await this.client.request<GqlProjectResult<GqlDiscussionsProject>>(query, {
       projectPath,
       iid: String(issuable.iid),
@@ -492,7 +511,6 @@ export class GitLabNewService {
       assert(discussions.pageInfo.endCursor);
       const remainingPages = await this.getDiscussions({
         issuable,
-        includePosition,
         endCursor: discussions.pageInfo.endCursor,
       });
       return [...discussions.nodes, ...remainingPages];
@@ -541,12 +559,21 @@ export class GitLabNewService {
     return combinedEvents;
   }
 
-  async createNote(issuable: RestIssuable, body: string, replyId?: string): Promise<void> {
-    await this.client.request<void>(createNoteMutation, {
+  async createNote(issuable: RestIssuable, body: string, replyId?: string): Promise<GqlNote> {
+    const result = await this.client.request<CreateNoteResult>(createNoteMutation, {
       issuableId: getIssuableGqlId(issuable),
       body,
       replyId,
     });
+    if (result.createNote.errors.length > 0) {
+      throw new UserFriendlyError(
+        `Couldn't create the comment when calling the API.
+        For more information, review the extension logs.`,
+        new Error(result.createNote.errors.join(',')),
+      );
+    }
+    assert(result.createNote.note);
+    return result.createNote.note;
   }
 
   async deleteNote(noteId: string): Promise<void> {

src/review/gitlab_comment_thread.test.ts

@@ -81,10 +81,6 @@ describe('GitLabCommentThread', () => {
     expect(vsCommentThread.collapsibleState).toBe(vscode.CommentThreadCollapsibleState.Expanded);
   });
 
-  it('sets canReply on the VS thread', () => {
-    expect(vsCommentThread.canReply).toBe(false);
-  });
-
   it('takes position from the first note', () => {
     expect(vsCommentThread.range.start.line).toBe(noteOnDiff.position.oldLine - 1); // vs code numbers lines from 0
   });
@@ -95,6 +91,33 @@ describe('GitLabCommentThread', () => {
     expect(vsCommentThread.uri.query).toMatch(baseSha);
   });
 
+  describe('allowing replies to the thread', () => {
+    const createNoteAndSetCreatePermissions = (createNote: boolean): GqlTextDiffNote => ({
+      ...(noteOnDiff as GqlTextDiffNote),
+      userPermissions: {
+        ...noteOnDiff.userPermissions,
+        createNote,
+      },
+    });
+
+    it('disallows replies if the first note has createNote permissions', () => {
+      const note = createNoteAndSetCreatePermissions(true);
+
+      createGitLabCommentThread(createGqlTextDiffDiscussion(note));
+
+      // TODO: allow replies when finishing #339
+      expect(vsCommentThread.canReply).toBe(false);
+    });
+
+    it('disallows replies if the first note does not have createNote permissions', () => {
+      const note = createNoteAndSetCreatePermissions(false);
+
+      createGitLabCommentThread(createGqlTextDiffDiscussion(note));
+
+      expect(vsCommentThread.canReply).toBe(false);
+    });
+  });
+
   describe('resolving discussions', () => {
     it('sets context to unresolved', () => {
       expect(vsCommentThread.contextValue).toBe('unresolved');

src/review/gitlab_comment_thread.ts

@@ -3,11 +3,18 @@ import * as assert from 'assert';
 import {
   GitLabNewService,
   GqlTextDiffDiscussion,
+  GqlTextDiffNote,
   GqlTextPosition,
 } from '../gitlab/gitlab_new_service';
 import { GitLabComment } from './gitlab_comment';
 import { toReviewUri } from './review_uri';
 
+const firstNoteFrom = (discussion: GqlTextDiffDiscussion): GqlTextDiffNote => {
+  const note = discussion.notes.nodes[0];
+  assert(note, 'discussion should contain at least one note');
+  return note;
+};
+
 const commentRangeFromPosition = (position: GqlTextPosition): vscode.Range => {
   const glLine = position.oldLine ?? position.newLine;
   const vsPosition = new vscode.Position(glLine - 1, 0); // VS Code numbers lines starting with 0, GitLab starts with 1
@@ -48,6 +55,8 @@ export class GitLabCommentThread {
     private mr: RestIssuable,
   ) {
     this.vsThread.collapsibleState = vscode.CommentThreadCollapsibleState.Expanded;
+    // TODO: when finishing #339, use the permissions to decide if replies should be allowed
+    // this.vsThread.canReply = firstNoteFrom(gqlDiscussion).userPermissions.createNote;
     this.vsThread.canReply = false;
     this.resolved = gqlDiscussion.resolved;
     this.updateThreadContext();
@@ -125,7 +134,7 @@ export class GitLabCommentThread {
     discussion,
     gitlabService,
   }: CreateThreadOptions): GitLabCommentThread {
-    const { position } = discussion.notes.nodes[0];
+    const { position } = firstNoteFrom(discussion);
     const vsThread = commentController.createCommentThread(
       uriFromPosition(position, workspaceFolder, mr.project_id),
       commentRangeFromPosition(position),

test/integration/fixtures/graphql/discussions.js

@@ -40,6 +40,7 @@ const note1 = {
   userPermissions: {
     resolveNote: true,
     adminNote: true,
+    createNote: true,
   },
   system: false,
   author: {
@@ -64,6 +65,7 @@ const note2 = {
   userPermissions: {
     resolveNote: true,
     adminNote: true,
+    createNote: true,
   },
   system: false,
   author: {
@@ -87,6 +89,7 @@ const noteOnDiff = {
   userPermissions: {
     resolveNote: true,
     adminNote: true,
+    createNote: true,
   },
   system: false,
   author: {

test/integration/webview.test.js

@@ -6,7 +6,7 @@ const { graphql } = require('msw');
 const webviewController = require('../../src/webview_controller');
 const { tokenService } = require('../../src/services/token_service');
 const openIssueResponse = require('./fixtures/rest/open_issue.json');
-const { projectWithIssueDiscussions } = require('./fixtures/graphql/discussions');
+const { projectWithIssueDiscussions, note2 } = require('./fixtures/graphql/discussions');
 
 const { getServer, createJsonEndpoint } = require('./test_infrastructure/mock_server');
 const { GITLAB_URL } = require('./test_infrastructure/constants');
@@ -39,6 +39,7 @@ describe('GitLab webview', () => {
             ctx.data({
               createNote: {
                 errors: [],
+                note: note2,
               },
             }),
           );