Merge branch '63-refactor-rest-issuable' into 'main'

refactor: create a separate interface for MR REST API response (RestMr)

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

src/data_providers/current_branch.ts

@@ -20,7 +20,7 @@ class DataProvider implements vscode.TreeDataProvider<ItemModel | vscode.TreeIte
 
   onDidChangeTreeData = this.eventEmitter.event;
 
-  private mr: RestIssuable | null = null;
+  private mr: RestMr | null = null;
 
   private disposableChildren: vscode.Disposable[] = [];
 
@@ -51,7 +51,7 @@ class DataProvider implements vscode.TreeDataProvider<ItemModel | vscode.TreeIte
     return new ExternalUrlItem(message, url);
   }
 
-  async createMrItem(mr: RestIssuable | null, repository: WrappedRepository) {
+  async createMrItem(mr: RestMr | null, repository: WrappedRepository) {
     if (!mr) {
       return new vscode.TreeItem('No merge request found');
     }

src/data_providers/items/changed_file_item.test.ts

 import { PROGRAMMATIC_COMMANDS } from '../../command_names';
-import { diffFile, issue, mrVersion } from '../../test_utils/entities';
+import { diffFile, mr, mrVersion } from '../../test_utils/entities';
 import { ChangedFileItem } from './changed_file_item';
 
 describe('ChangedFileItem', () => {
@@ -8,7 +8,7 @@ describe('ChangedFileItem', () => {
       'should not show diff for %s',
       extension => {
         const changedImageFile = { ...diffFile, new_path: `file${extension}` };
-        const item = new ChangedFileItem(issue, mrVersion, changedImageFile, '/repository/fsPath');
+        const item = new ChangedFileItem(mr, mrVersion, changedImageFile, '/repository/fsPath');
         expect(item.command?.command).toBe(PROGRAMMATIC_COMMANDS.NO_IMAGE_REVIEW);
       },
     );

src/data_providers/items/changed_file_item.ts

@@ -29,7 +29,7 @@ const looksLikeImage = (filePath: string) =>
   imageExtensions.includes(path.extname(filePath).toLowerCase());
 
 export class ChangedFileItem extends TreeItem {
-  mr: RestIssuable;
+  mr: RestMr;
 
   mrVersion: RestMrVersion;
 
@@ -37,12 +37,7 @@ export class ChangedFileItem extends TreeItem {
 
   file: RestDiffFile;
 
-  constructor(
-    mr: RestIssuable,
-    mrVersion: RestMrVersion,
-    file: RestDiffFile,
-    repositoryPath: string,
-  ) {
+  constructor(mr: RestMr, mrVersion: RestMrVersion, file: RestDiffFile, repositoryPath: string) {
     const changeType = getChangeType(file);
     const query = new URLSearchParams([['changeType', changeType]]).toString();
     super(Uri.file(file.new_path).with({ query }));

src/data_providers/items/custom_query_item_model.ts

@@ -51,22 +51,20 @@ export class CustomQueryItemModel extends ItemModel {
     const { MR, ISSUE, SNIPPET, EPIC, VULNERABILITY } = CustomQueryType;
     switch (this.customQuery.type) {
       case MR: {
-        const mrModels = issues.map((mr: RestIssuable) => new MrItemModel(mr, this.repository));
+        const mrModels = issues.map((mr: RestMr) => new MrItemModel(mr, this.repository));
         this.setDisposableChildren(mrModels);
         return mrModels;
       }
       case ISSUE:
-        return issues.map(
-          (issue: RestIssuable) => new IssueItem(issue, this.repository.rootFsPath),
-        );
+        return issues.map((issue: RestMr) => new IssueItem(issue, this.repository.rootFsPath));
       case SNIPPET:
         return issues.map(
-          (snippet: RestIssuable) =>
+          (snippet: RestMr) =>
             new ExternalUrlItem(`$${snippet.id} · ${snippet.title}`, snippet.web_url),
         );
       case EPIC:
         return issues.map(
-          (epic: RestIssuable) => new ExternalUrlItem(`&${epic.iid} · ${epic.title}`, epic.web_url),
+          (epic: RestMr) => new ExternalUrlItem(`&${epic.iid} · ${epic.title}`, epic.web_url),
         );
       case VULNERABILITY:
         return issues.map((v: RestVulnerability) => new VulnerabilityItem(v));

src/data_providers/items/mr_item_model.ts

@@ -16,7 +16,7 @@ const isTextDiffDiscussion = (discussion: GqlDiscussion): discussion is GqlTextD
 };
 
 export class MrItemModel extends ItemModel {
-  constructor(readonly mr: RestIssuable, readonly repository: WrappedRepository) {
+  constructor(readonly mr: RestMr, readonly repository: WrappedRepository) {
     super();
   }
 

src/git/wrapped_repository.ts

@@ -57,7 +57,7 @@ function getInstanceUrlFromRemotes(gitRemoteUrls: string[]): string {
 }
 
 export interface CachedMr {
-  mr: RestIssuable;
+  mr: RestMr;
   mrVersion: RestMrVersion;
 }
 
@@ -99,7 +99,7 @@ export class WrappedRepository {
     return Boolean(this.cachedProject);
   }
 
-  async reloadMr(mr: RestIssuable): Promise<CachedMr> {
+  async reloadMr(mr: RestMr): Promise<CachedMr> {
     const mrVersion = await this.getGitLabService().getMrDiff(mr);
     const cachedMr = {
       mr,

src/gitlab/gitlab_new_service.ts

@@ -38,6 +38,7 @@ import {
 import { createDiffNoteMutation, GqlDiffPositionInput } from './graphql/create_diff_comment';
 import { removeLeadingSlash } from '../utils/remove_leading_slash';
 import { log, logError } from '../log';
+import { isMr } from '../utils/is_mr';
 
 interface CreateNoteResult {
   createNote: {
@@ -109,7 +110,6 @@ const updateNoteBodyMutation = gql`
 `;
 
 const getProjectPath = (issuable: RestIssuable) => issuable.references.full.split(/[#!]/)[0];
-const isMr = (issuable: RestIssuable) => Boolean(issuable.sha);
 const getIssuableGqlId = (issuable: RestIssuable) =>
   `gid://gitlab/${isMr(issuable) ? 'MergeRequest' : 'Issue'}/${issuable.id}`;
 const getMrGqlId = (id: number) => `gid://gitlab/MergeRequest/${id}`;
@@ -203,7 +203,7 @@ export class GitLabNewService {
   }
 
   // This method has to use REST API till https://gitlab.com/gitlab-org/gitlab/-/issues/280803 gets done
-  async getMrDiff(mr: RestIssuable): Promise<RestMrVersion> {
+  async getMrDiff(mr: RestMr): Promise<RestMrVersion> {
     const versionsUrl = `${this.instanceUrl}/api/v4/projects/${mr.project_id}/merge_requests/${mr.iid}/versions`;
     const versionsResult = await crossFetch(versionsUrl, this.fetchOptions);
     if (!versionsResult.ok) {
@@ -278,14 +278,14 @@ export class GitLabNewService {
     return discussions.nodes.map(n => this.addHostToUrl(n));
   }
 
-  async canUserCommentOnMr(issuable: RestIssuable): Promise<boolean> {
-    const projectPath = getProjectPath(issuable);
+  async canUserCommentOnMr(mr: RestMr): Promise<boolean> {
+    const projectPath = getProjectPath(mr);
     const queryOptions: MrPermissionsQueryOptions = {
       projectPath,
-      iid: String(issuable.iid),
+      iid: String(mr.iid),
     };
     const result = await this.client.request(getMrPermissionsQuery, queryOptions);
-    assert(result?.project?.mergeRequest, `MR ${issuable.references.full} was not found.`);
+    assert(result?.project?.mergeRequest, `MR ${mr.references.full} was not found.`);
     return Boolean(result.project.mergeRequest.userPermissions?.createNote);
   }
 
@@ -369,7 +369,7 @@ 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> {
+  private async getMrNote(mr: RestMr, 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) {
@@ -382,7 +382,7 @@ export class GitLabNewService {
     noteGqlId: string,
     body: string,
     originalBody: string,
-    mr: RestIssuable,
+    mr: RestMr,
   ): 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

src/gitlab_service.ts

@@ -375,7 +375,7 @@ export async function fetchLastJobsForCurrentBranch(
 
 export async function fetchOpenMergeRequestForCurrentBranch(
   repositoryRoot: string,
-): Promise<RestIssuable | null> {
+): Promise<RestMr | null> {
   const project = await fetchCurrentProject(repositoryRoot);
   const branchName = await gitExtensionWrapper
     .getRepository(repositoryRoot)
@@ -394,7 +394,7 @@ export async function fetchOpenMergeRequestForCurrentBranch(
 
 export async function fetchLastPipelineForMr(
   repositoryRoot: string,
-  mr: RestIssuable,
+  mr: RestMr,
 ): Promise<RestPipeline | null> {
   const path = `/projects/${mr.project_id}/merge_requests/${mr.iid}/pipelines`;
   const { response: pipelines } = await fetch(repositoryRoot, path);
@@ -405,7 +405,7 @@ export async function fetchPipelineAndMrForCurrentBranch(
   repositoryRoot: string,
 ): Promise<{
   pipeline: RestPipeline | null;
-  mr: RestIssuable | null;
+  mr: RestMr | null;
 }> {
   // TODO: implement more granular approach to errors (deciding between expected and critical)
   // This can be done when we migrate the code to gitlab_new_service.ts

src/review/commenting_range_provider.ts

@@ -8,11 +8,11 @@ const lastLineEmpty = (document: vscode.TextDocument): boolean => {
   return lastLIne.isEmptyOrWhitespace;
 };
 export class CommentingRangeProvider implements vscode.CommentingRangeProvider {
-  private mr: RestIssuable;
+  private mr: RestMr;
 
   private mrVersion: RestMrVersion;
 
-  constructor(mr: RestIssuable, mrVersion: RestMrVersion) {
+  constructor(mr: RestMr, mrVersion: RestMrVersion) {
     this.mr = mr;
     this.mrVersion = mrVersion;
   }

src/review/gitlab_comment_thread.ts

@@ -32,7 +32,7 @@ const pathAndCommitFromPosition = (position: GqlTextPosition) => {
 interface CreateThreadOptions {
   commentController: vscode.CommentController;
   repositoryRoot: string;
-  mr: RestIssuable;
+  mr: RestMr;
   discussion: GqlTextDiffDiscussion;
   gitlabService: GitLabNewService;
 }
@@ -45,7 +45,7 @@ export class GitLabCommentThread {
     private vsThread: vscode.CommentThread,
     private gqlDiscussion: GqlTextDiffDiscussion,
     private gitlabService: GitLabNewService,
-    private mr: RestIssuable,
+    private mr: RestMr,
   ) {
     // SIDE-EFFECT
     this.vsThread.comments = gqlDiscussion.notes.nodes.map(note =>

src/status_bar.ts

@@ -142,7 +142,7 @@ export class StatusBar {
     this.firstRun = false;
   }
 
-  async fetchMrClosingIssue(mr: RestIssuable | null, repositoryRoot: string): Promise<void> {
+  async fetchMrClosingIssue(mr: RestMr | null, repositoryRoot: string): Promise<void> {
     if (!this.mrIssueStatusBarItem) return;
     if (mr) {
       const issues = await gitLabService.fetchMRIssues(mr.iid, repositoryRoot);
@@ -162,7 +162,7 @@ export class StatusBar {
     }
   }
 
-  updateMrItem(mr: RestIssuable | null): void {
+  updateMrItem(mr: RestMr | null): void {
     if (!this.mrStatusBarItem) return;
     this.mrStatusBarItem.show();
     this.mrStatusBarItem.command = mr

src/test_utils/entities.ts

@@ -21,7 +21,7 @@ export const issue: RestIssuable = {
   name: 'Issuable Name',
 };
 
-export const mr: RestIssuable = {
+export const mr: RestMr = {
   ...issue,
   id: 2,
   iid: 2000,
@@ -29,6 +29,7 @@ export const mr: RestIssuable = {
   references: {
     full: 'gitlab-org/gitlab!2000',
   },
+  sha: '69ad609e8891b8aa3db85a35cd2c5747705bd76a',
 };
 
 export const diffFile: RestDiffFile = {

src/types.d.ts

@@ -14,7 +14,6 @@ interface RestIssuable {
   project_id: number;
   web_url: string;
   author: { name: string; avatar_url: string | null };
-  sha?: string; // only present in MR, legacy logic uses the presence to decide issuable type
   references: {
     full: string; // e.g. "gitlab-org/gitlab#219925"
   };
@@ -22,6 +21,10 @@ interface RestIssuable {
   name: string;
 }
 
+interface RestMr extends RestIssuable {
+  sha: string;
+}
+
 interface RestMrVersion {
   head_commit_sha: string;
   base_commit_sha: string;

src/utils/is_mr.ts

+export const isMr = (issuable: RestIssuable): issuable is RestMr =>
+  Boolean((issuable as RestMr).sha);

src/webview_controller.ts

@@ -7,6 +7,7 @@ import * as gitLabService from './gitlab_service';
 import { createGitLabNewService } from './service_factory';
 import { logError } from './log';
 import { getInstanceUrl } from './utils/get_instance_url';
+import { isMr } from './utils/is_mr';
 
 const webviewResourcePaths = {
   appScriptUri: 'src/webview/dist/js/app.js',
@@ -140,8 +141,7 @@ class WebviewController {
     const lightMrIcon = getIconUri('light', 'merge_requests.svg');
     const darkIssueIcon = getIconUri('dark', 'issues.svg');
     const darkMrIcon = getIconUri('dark', 'merge_requests.svg');
-    const isMr = issuable.sha !== undefined;
-    return isMr
+    return isMr(issuable)
       ? { light: lightMrIcon, dark: darkMrIcon }
       : { light: lightIssueIcon, dark: darkIssueIcon };
   }