From e77688b4e316e683c1c096316f5241e34e62f944 Mon Sep 17 00:00:00 2001 From: Tomas Vik Date: Tue, 22 Jun 2021 10:04:33 +0200 Subject: [PATCH] refactor: use the new RestMr interface for MRs Credit: [@Musisimaru](https://gitlab.com/Musisimaru) (originally introduced in [!229](https://gitlab.com/gitlab-org/gitlab-vscode-extension/-/merge_requests/229)) --- src/data_providers/current_branch.ts | 4 ++-- src/data_providers/items/changed_file_item.test.ts | 4 ++-- src/data_providers/items/changed_file_item.ts | 9 ++------- .../items/custom_query_item_model.ts | 10 ++++------ src/data_providers/items/mr_item_model.ts | 2 +- src/git/wrapped_repository.ts | 4 ++-- src/gitlab/gitlab_new_service.ts | 14 +++++++------- src/gitlab_service.ts | 6 +++--- src/review/commenting_range_provider.ts | 4 ++-- src/review/gitlab_comment_thread.ts | 4 ++-- src/status_bar.ts | 4 ++-- src/test_utils/entities.ts | 3 ++- 12 files changed, 31 insertions(+), 37 deletions(-) diff --git a/src/data_providers/current_branch.ts b/src/data_providers/current_branch.ts index 50ecc5a..1192959 100644 --- a/src/data_providers/current_branch.ts +++ b/src/data_providers/current_branch.ts @@ -20,7 +20,7 @@ class DataProvider implements vscode.TreeDataProvider { @@ -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); }, ); diff --git a/src/data_providers/items/changed_file_item.ts b/src/data_providers/items/changed_file_item.ts index 4499b2f..287f2e2 100644 --- a/src/data_providers/items/changed_file_item.ts +++ b/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 })); diff --git a/src/data_providers/items/custom_query_item_model.ts b/src/data_providers/items/custom_query_item_model.ts index c898343..9ddf222 100644 --- a/src/data_providers/items/custom_query_item_model.ts +++ b/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)); diff --git a/src/data_providers/items/mr_item_model.ts b/src/data_providers/items/mr_item_model.ts index dcb6168..7f1834c 100644 --- a/src/data_providers/items/mr_item_model.ts +++ b/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(); } diff --git a/src/git/wrapped_repository.ts b/src/git/wrapped_repository.ts index 3686cd0..40c0b38 100644 --- a/src/git/wrapped_repository.ts +++ b/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 { + async reloadMr(mr: RestMr): Promise { const mrVersion = await this.getGitLabService().getMrDiff(mr); const cachedMr = { mr, diff --git a/src/gitlab/gitlab_new_service.ts b/src/gitlab/gitlab_new_service.ts index 596e139..c45e2df 100644 --- a/src/gitlab/gitlab_new_service.ts +++ b/src/gitlab/gitlab_new_service.ts @@ -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 { + async getMrDiff(mr: RestMr): Promise { 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 { - const projectPath = getProjectPath(issuable); + async canUserCommentOnMr(mr: RestMr): Promise { + 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 { + private async getMrNote(mr: RestMr, noteId: number): Promise { 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 { const latestNote = await this.getMrNote(mr, getRestIdFromGraphQLId(noteGqlId)); // This check is the best workaround we can do in the lack of optimistic locking diff --git a/src/gitlab_service.ts b/src/gitlab_service.ts index 71b2984..c8aa013 100644 --- a/src/gitlab_service.ts +++ b/src/gitlab_service.ts @@ -375,7 +375,7 @@ export async function fetchLastJobsForCurrentBranch( export async function fetchOpenMergeRequestForCurrentBranch( repositoryRoot: string, -): Promise { +): Promise { 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 { 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 diff --git a/src/review/commenting_range_provider.ts b/src/review/commenting_range_provider.ts index 5ecc9a1..bbdbe8b 100644 --- a/src/review/commenting_range_provider.ts +++ b/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; } diff --git a/src/review/gitlab_comment_thread.ts b/src/review/gitlab_comment_thread.ts index 5538932..3655283 100644 --- a/src/review/gitlab_comment_thread.ts +++ b/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 => diff --git a/src/status_bar.ts b/src/status_bar.ts index b3db1a6..e71fe3a 100644 --- a/src/status_bar.ts +++ b/src/status_bar.ts @@ -142,7 +142,7 @@ export class StatusBar { this.firstRun = false; } - async fetchMrClosingIssue(mr: RestIssuable | null, repositoryRoot: string): Promise { + async fetchMrClosingIssue(mr: RestMr | null, repositoryRoot: string): Promise { 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 diff --git a/src/test_utils/entities.ts b/src/test_utils/entities.ts index 5e32ff2..c2ff24c 100644 --- a/src/test_utils/entities.ts +++ b/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 = { -- GitLab