diff --git a/CHANGELOG.md b/CHANGELOG.md index 03e5cb3b67f27..cd4dfb4b70a5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p ### Added +- Adds new ability to search for a GitLab MR in the _Launchpad_ — closes [#3788](https://github.com/gitkraken/vscode-gitlens/issues/3788) - Adds go to home view button to the commit graph title section — closes [#3873](https://github.com/gitkraken/vscode-gitlens/issues/3873) - Adds a _Contributors_ section to comparison results in the views diff --git a/src/git/models/__tests__/pullRequest.utils.test.ts b/src/git/models/__tests__/pullRequest.utils.test.ts index adeaec4f240b8..8fee9ade39287 100644 --- a/src/git/models/__tests__/pullRequest.utils.test.ts +++ b/src/git/models/__tests__/pullRequest.utils.test.ts @@ -1,62 +1,30 @@ import * as assert from 'assert'; import { suite, test } from 'mocha'; -import { getPullRequestIdentityValuesFromSearch } from '../pullRequest.utils'; +import { getPullRequestIdentityFromMaybeUrl } from '../pullRequest.utils'; -suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => { +suite('Test PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => { function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) { assert.deepStrictEqual( - getPullRequestIdentityValuesFromSearch(query), - { - ownerAndRepo: ownerAndRepo, - prNumber: prNumber, - }, + getPullRequestIdentityFromMaybeUrl(query), + prNumber == null + ? undefined + : { + ownerAndRepo: ownerAndRepo, + prNumber: prNumber, + provider: undefined, + }, `${message} (${JSON.stringify(query)})`, ); } - test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => { - t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens'); - t( - 'with suffix', - 'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello', - '1', - 'eamodio/vscode-gitlens', - ); - t( - 'with query', - 'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello', - '1', - 'eamodio/vscode-gitlens', - ); - - t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens'); - t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens'); - t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1'); - - t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1'); - t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1'); - }); - - test('no domain, should parse to ownerAndRepo and prNumber', () => { - t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1'); - t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens'); - t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1'); - }); - - test('domain vs. no domain', () => { - t( - 'with anchor', - 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16', - '1', - 'eamodio/vscode-gitlens', - ); - }); + test('cannot recognize GitHub or GitLab URLs, sees only numbers', () => { + t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16'); + t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '1'); - test('has "pull/" fragment', () => { - t('with leading slash', '/pull/16/files#hello', '16'); - t('without leading slash', 'pull/16?diff=unified#hello', '16'); - t('with numeric repo name', '1/pull/16?diff=unified#hello', '16'); - t('with double slash', '1//pull/16?diff=unified#hello', '16'); + t('no protocol', '/github.com/sergeibbb/1/pull/16?diff=unified', '1'); + t('no domain', '/sergeibbb/1/pull/16#hello', '1'); + t('domain vs. no domain', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/2/pull/16', '1'); + t('has "pull/" fragment', '/pull/16/files#hello', '16'); }); test('has "/" fragment', () => { diff --git a/src/git/models/pullRequest.ts b/src/git/models/pullRequest.ts index b9ea08a7d0d00..c7c83ed41df56 100644 --- a/src/git/models/pullRequest.ts +++ b/src/git/models/pullRequest.ts @@ -2,12 +2,10 @@ import { Uri, window } from 'vscode'; import { Schemes } from '../../constants'; import { Container } from '../../container'; import type { RepositoryIdentityDescriptor } from '../../gk/models/repositoryIdentities'; -import type { EnrichablePullRequest } from '../../plus/integrations/providers/models'; import { formatDate, fromNow } from '../../system/date'; import { memoize } from '../../system/decorators/memoize'; import type { LeftRightCommitCountResult } from '../gitProvider'; import type { IssueOrPullRequest, IssueRepository, IssueOrPullRequestState as PullRequestState } from './issue'; -import type { PullRequestUrlIdentity } from './pullRequest.utils'; import type { ProviderReference } from './remoteProvider'; import type { Repository } from './repository'; import { createRevisionRange, shortenRevision } from './revision.utils'; @@ -419,21 +417,3 @@ export async function getOpenedPullRequestRepo( const repo = await getOrOpenPullRequestRepository(container, pr, { promptIfNeeded: true }); return repo; } - -export function doesPullRequestSatisfyRepositoryURLIdentity( - pr: EnrichablePullRequest | undefined, - { ownerAndRepo, prNumber }: PullRequestUrlIdentity, -): boolean { - if (pr == null) { - return false; - } - const satisfiesPrNumber = prNumber != null && pr.number === parseInt(prNumber, 10); - if (!satisfiesPrNumber) { - return false; - } - const satisfiesOwnerAndRepo = ownerAndRepo != null && pr.repoIdentity.name === ownerAndRepo; - if (!satisfiesOwnerAndRepo) { - return false; - } - return true; -} diff --git a/src/git/models/pullRequest.utils.ts b/src/git/models/pullRequest.utils.ts index 28e9ee2353d3f..2df6779a61573 100644 --- a/src/git/models/pullRequest.utils.ts +++ b/src/git/models/pullRequest.utils.ts @@ -8,31 +8,19 @@ export interface PullRequestUrlIdentity { provider?: HostingIntegrationId; ownerAndRepo?: string; - prNumber?: string; + prNumber: string; } -export function getPullRequestIdentityValuesFromSearch(search: string): PullRequestUrlIdentity { - let ownerAndRepo: string | undefined = undefined; +export function isMaybeNonSpecificPullRequestSearchUrl(search: string): boolean { + return getPullRequestIdentityFromMaybeUrl(search) != null; +} + +export function getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined { let prNumber: string | undefined = undefined; - let match = search.match(/([^/]+\/[^/]+)\/pull\/(\d+)/); // with org and rep name + let match = search.match(/(?:\/)(\d+)/); // any number starting with "/" if (match != null) { - ownerAndRepo = match[1]; - prNumber = match[2]; - } - - if (prNumber == null) { - match = search.match(/(?:\/|^)pull\/(\d+)/); // without repo name - if (match != null) { - prNumber = match[1]; - } - } - - if (prNumber == null) { - match = search.match(/(?:\/)(\d+)/); // any number starting with "/" - if (match != null) { - prNumber = match[1]; - } + prNumber = match[1]; } if (prNumber == null) { @@ -42,5 +30,5 @@ export function getPullRequestIdentityValuesFromSearch(search: string): PullRequ } } - return { ownerAndRepo: ownerAndRepo, prNumber: prNumber }; + return prNumber == null ? undefined : { ownerAndRepo: undefined, prNumber: prNumber, provider: undefined }; } diff --git a/src/plus/integrations/integration.ts b/src/plus/integrations/integration.ts index c7aabf3a2c54a..bbef057b30676 100644 --- a/src/plus/integrations/integration.ts +++ b/src/plus/integrations/integration.ts @@ -16,6 +16,7 @@ import type { PullRequestState, SearchedPullRequest, } from '../../git/models/pullRequest'; +import type { PullRequestUrlIdentity } from '../../git/models/pullRequest.utils'; import type { RepositoryMetadata } from '../../git/models/repositoryMetadata'; import { showIntegrationDisconnectedTooManyFailedRequestsWarningMessage } from '../../messages'; import { gate } from '../../system/decorators/gate'; @@ -556,7 +557,7 @@ export abstract class IntegrationBase< const connected = this.maybeConnected ?? (await this.isConnected()); if (!connected) return undefined; - const pr = this.container.cache.getPullRequest(id, resource, this, () => ({ + const pr = await this.container.cache.getPullRequest(id, resource, this, () => ({ value: (async () => { try { const result = await this.getProviderPullRequest?.(this._session!, resource, id); @@ -1361,4 +1362,10 @@ export abstract class HostingIntegration< repos?: T[], cancellation?: CancellationToken, ): Promise; + + getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined { + return this.getProviderPullRequestIdentityFromMaybeUrl?.(search); + } + + protected getProviderPullRequestIdentityFromMaybeUrl?(search: string): PullRequestUrlIdentity | undefined; } diff --git a/src/plus/integrations/providers/github.ts b/src/plus/integrations/providers/github.ts index a5d73cd3402b8..4c8570a80dbb9 100644 --- a/src/plus/integrations/providers/github.ts +++ b/src/plus/integrations/providers/github.ts @@ -11,6 +11,7 @@ import type { PullRequestState, SearchedPullRequest, } from '../../../git/models/pullRequest'; +import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils'; import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata'; import { log } from '../../../system/decorators/log'; import { ensurePaidPlan } from '../../utils'; @@ -20,6 +21,7 @@ import type { } from '../authentication/integrationAuthentication'; import type { RepositoryDescriptor, SupportedIntegrationIds } from '../integration'; import { HostingIntegration } from '../integration'; +import { getGitHubPullRequestIdentityFromMaybeUrl } from './github/github.utils'; import { providersMetadata } from './models'; import type { ProvidersApi } from './providersApi'; @@ -292,6 +294,10 @@ export class GitHubIntegration extends GitHubIntegrationBase { diff --git a/src/plus/integrations/providers/github/__tests__/github.utils.test.ts b/src/plus/integrations/providers/github/__tests__/github.utils.test.ts new file mode 100644 index 0000000000000..7b4108ca7e75a --- /dev/null +++ b/src/plus/integrations/providers/github/__tests__/github.utils.test.ts @@ -0,0 +1,87 @@ +import * as assert from 'assert'; +import { suite, test } from 'mocha'; +//import { getGitHubPullRequestIdentityFromMaybeUrl } from '../github/models'; +import { getGitHubPullRequestIdentityFromMaybeUrl, isMaybeGitHubPullRequestUrl } from '../github.utils'; + +suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => { + function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) { + assert.deepStrictEqual( + getGitHubPullRequestIdentityFromMaybeUrl(query), + prNumber == null + ? undefined + : { + ownerAndRepo: ownerAndRepo, + prNumber: prNumber, + provider: 'github', + }, + `Parse: ${message} (${JSON.stringify(query)})`, + ); + assert.equal( + isMaybeGitHubPullRequestUrl(query), + prNumber != null, + `Check: ${message} (${JSON.stringify(query)})`, + ); + } + + test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => { + t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens'); + t( + 'with suffix', + 'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + t( + 'with query', + 'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + + t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens'); + t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens'); + t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1'); + + t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1'); + t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1'); + }); + + test('no domain, should parse to ownerAndRepo and prNumber', () => { + t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1'); + t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens'); + t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1'); + }); + + test('domain vs. no domain', () => { + t( + 'with anchor', + 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16', + '1', + 'eamodio/vscode-gitlens', + ); + }); + + test('numbers', () => { + t('has "pull/" fragment', '/pull/16/files#hello', '16'); + t('has "pull/" fragment with double slash', '1//pull/16?diff=unified#hello', '16'); + t('with leading slash', '/16/files#hello', undefined); + t('just a number', '16', undefined); + t('with a hash', '#16', undefined); + }); + + test('does not match', () => { + t('without leading slash', '16?diff=unified#hello', undefined); + t('with leading hash', '/#16/files#hello', undefined); + t('number is a part of a word', 'hello16', undefined); + t('number is a part of a word', '16hello', undefined); + + t('GitLab', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/16', undefined); + + t('with a number', '1/16?diff=unified#hello', undefined); + t('with a number and slash', '/1/16?diff=unified#hello', undefined); + t('with a word', 'anything/16?diff=unified#hello', undefined); + + t('with a wrong character leading to pull', 'sergeibbb/1/-pull/16?diff=unified#hello', undefined); + t('with a wrong character leading to pull', 'sergeibbb/1-pull/16?diff=unified#hello', undefined); + }); +}); diff --git a/src/plus/integrations/providers/github/github.utils.ts b/src/plus/integrations/providers/github/github.utils.ts new file mode 100644 index 0000000000000..1d375b650ee75 --- /dev/null +++ b/src/plus/integrations/providers/github/github.utils.ts @@ -0,0 +1,36 @@ +// GitHub provider: github.ts pulls many dependencies through Container and some of them break the unit tests. +// That's why this file has been created that can collect more simple functions which +// don't require Container and can be tested. + +import { HostingIntegrationId } from '../../../../constants.integrations'; +import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils'; + +export function isMaybeGitHubPullRequestUrl(url: string): boolean { + if (url == null) return false; + + return getGitHubPullRequestIdentityFromMaybeUrl(url) != null; +} + +export function getGitHubPullRequestIdentityFromMaybeUrl( + search: string, +): (PullRequestUrlIdentity & { provider: HostingIntegrationId.GitHub }) | undefined { + let ownerAndRepo: string | undefined = undefined; + let prNumber: string | undefined = undefined; + + let match = search.match(/([^/]+\/[^/]+)\/(?:pull)\/(\d+)/); // with org and rep name + if (match != null) { + ownerAndRepo = match[1]; + prNumber = match[2]; + } + + if (prNumber == null) { + match = search.match(/(?:\/|^)(?:pull)\/(\d+)/); // without repo name + if (match != null) { + prNumber = match[1]; + } + } + + return prNumber != null + ? { ownerAndRepo: ownerAndRepo, prNumber: prNumber, provider: HostingIntegrationId.GitHub } + : undefined; +} diff --git a/src/plus/integrations/providers/github/models.ts b/src/plus/integrations/providers/github/models.ts index 73dd37800a0bc..f3af4be632d58 100644 --- a/src/plus/integrations/providers/github/models.ts +++ b/src/plus/integrations/providers/github/models.ts @@ -1,5 +1,4 @@ import type { Endpoints } from '@octokit/types'; -import { HostingIntegrationId } from '../../../../constants.integrations'; import { GitFileIndexStatus } from '../../../../git/models/file'; import type { IssueLabel } from '../../../../git/models/issue'; import { Issue, RepositoryAccessLevel } from '../../../../git/models/issue'; @@ -11,7 +10,6 @@ import { PullRequestReviewState, PullRequestStatusCheckRollupState, } from '../../../../git/models/pullRequest'; -import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils'; import type { Provider } from '../../../../git/models/remoteProvider'; export interface GitHubBlame { @@ -511,20 +509,3 @@ export function fromCommitFileStatus( } return undefined; } - -const prUrlRegex = /^(?:https?:\/\/)?(?:github\.com\/)?([^/]+\/[^/]+)\/pull\/(\d+)/i; - -export function isMaybeGitHubPullRequestUrl(url: string): boolean { - if (url == null) return false; - - return prUrlRegex.test(url); -} - -export function getGitHubPullRequestIdentityFromMaybeUrl(url: string): RequireSome { - if (url == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitHub }; - - const match = prUrlRegex.exec(url); - if (match == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitHub }; - - return { prNumber: match[2], ownerAndRepo: match[1], provider: HostingIntegrationId.GitHub }; -} diff --git a/src/plus/integrations/providers/gitlab.ts b/src/plus/integrations/providers/gitlab.ts index 3c44a6eec4ce3..ab553b269d9a1 100644 --- a/src/plus/integrations/providers/gitlab.ts +++ b/src/plus/integrations/providers/gitlab.ts @@ -12,6 +12,7 @@ import type { PullRequestState, SearchedPullRequest, } from '../../../git/models/pullRequest'; +import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils'; import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata'; import { log } from '../../../system/decorators/log'; import { uniqueBy } from '../../../system/iterable'; @@ -22,6 +23,7 @@ import type { } from '../authentication/integrationAuthentication'; import type { RepositoryDescriptor } from '../integration'; import { HostingIntegration } from '../integration'; +import { getGitLabPullRequestIdentityFromMaybeUrl } from './gitlab/gitlab.utils'; import { fromGitLabMergeRequestProvidersApi } from './gitlab/models'; import type { ProviderPullRequest } from './models'; import { ProviderPullRequestReviewState, providersMetadata, toSearchedIssue } from './models'; @@ -163,6 +165,23 @@ abstract class GitLabIntegrationBase< }); } + protected override async getProviderPullRequest( + { accessToken }: AuthenticationSession, + resource: GitLabRepositoryDescriptor, + id: string, + ): Promise { + return (await this.container.gitlab)?.getPullRequest( + this, + accessToken, + resource.owner, + resource.name, + parseInt(id, 10), + { + baseUrl: this.apiBaseUrl, + }, + ); + } + protected override async getProviderRepositoryMetadata( { accessToken }: AuthenticationSession, repo: GitLabRepositoryDescriptor, @@ -295,6 +314,29 @@ abstract class GitLabIntegrationBase< .filter((result): result is SearchedIssue => result != null); } + protected override async searchProviderPullRequests( + { accessToken }: AuthenticationSession, + searchQuery: string, + repos?: GitLabRepositoryDescriptor[], + cancellation?: CancellationToken, + ): Promise { + const api = await this.container.gitlab; + if (!api) { + return undefined; + } + + return api.searchPullRequests( + this, + accessToken, + { + search: searchQuery, + repos: repos?.map(r => `${r.owner}/${r.name}`), + baseUrl: this.apiBaseUrl, + }, + cancellation, + ); + } + protected override async mergeProviderPullRequest( _session: AuthenticationSession, pr: PullRequest, @@ -353,6 +395,10 @@ abstract class GitLabIntegrationBase< username: currentUser.username || undefined, }; } + + protected override getProviderPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined { + return getGitLabPullRequestIdentityFromMaybeUrl(search); + } } export class GitLabIntegration extends GitLabIntegrationBase { diff --git a/src/plus/integrations/providers/gitlab/__tests__/gitlab.utils.test.ts b/src/plus/integrations/providers/gitlab/__tests__/gitlab.utils.test.ts new file mode 100644 index 0000000000000..7b879bdf36537 --- /dev/null +++ b/src/plus/integrations/providers/gitlab/__tests__/gitlab.utils.test.ts @@ -0,0 +1,113 @@ +import * as assert from 'assert'; +import { suite, test } from 'mocha'; +import { getGitLabPullRequestIdentityFromMaybeUrl, isMaybeGitLabPullRequestUrl } from '../gitlab.utils'; + +suite('Test GitLab PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => { + function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) { + assert.deepStrictEqual( + getGitLabPullRequestIdentityFromMaybeUrl(query), + prNumber == null + ? undefined + : { + ownerAndRepo: ownerAndRepo, + prNumber: prNumber, + provider: 'gitlab', + }, + `Parse: ${message} (${JSON.stringify(query)})`, + ); + assert.equal( + isMaybeGitLabPullRequestUrl(query), + prNumber != null, + `Check: ${message} (${JSON.stringify(query)})`, + ); + } + + test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => { + t('full URL', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1', '1', 'eamodio/vscode-gitlens'); + t( + 'with suffix', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1/files?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + t( + 'with query', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + + t( + 'with anchor', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1#hello', + '1', + 'eamodio/vscode-gitlens', + ); + t( + 'a weird suffix', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1-files', + '1', + 'eamodio/vscode-gitlens', + ); + t('numeric repo name', 'https://gitlab.com/sergeibbb/1/-/merge_requests/16', '16', 'sergeibbb/1'); + + t( + 'no protocol with leading slash', + '/gitlab.com/sergeibbb/1/-/merge_requests/16?diff=unified', + '16', + 'sergeibbb/1', + ); + t('no protocol without leading slash', 'gitlab.com/sergeibbb/1/-/merge_requests/16/files', '16', 'sergeibbb/1'); + }); + test('no domain, should parse to ownerAndRepo and prNumber', () => { + t('with leading slash', '/sergeibbb/1/-/merge_requests/16#hello', '16', 'sergeibbb/1'); + t( + 'words in repo name', + 'eamodio/vscode-gitlens/-/merge_requests/1?diff=unified#hello', + '1', + 'eamodio/vscode-gitlens', + ); + t('numeric repo name', 'sergeibbb/1/-/merge_requests/16/files', '16', 'sergeibbb/1'); + }); + + test('domain vs. no domain', () => { + t( + 'with anchor', + 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1#hello/sergeibbb/1/-/merge_requests/16', + '1', + 'eamodio/vscode-gitlens', + ); + }); + + test('numbers', () => { + t('has "-/merge_requests/" fragment', '/-/merge_requests/16/files#hello', '16'); + t('has "-/merge_requests/" fragment with double slash', '1//-/merge_requests/16?diff=unified#hello', '16'); + t('with leading slash', '/16/files#hello', undefined); + t('just a number', '16', undefined); + t('with a hash', '#16', undefined); + }); + + test('does not match', () => { + t('without leading slash', '16?diff=unified#hello', undefined); + t('with leading hash', '/#16/files#hello', undefined); + t('number is a part of a word', 'hello16', undefined); + t('number is a part of a word', '16hello', undefined); + + t('GitHub', 'https://github.com/eamodio/vscode-gitlens/pull/16', undefined); + + t('with a number', '1/16?diff=unified#hello', undefined); + t('with a number and slash', '/1/16?diff=unified#hello', undefined); + t('with a word', 'anything/16?diff=unified#hello', undefined); + + t( + 'with a wrong character leading to "-/merge_requests/"', + 'sergeibbb/1/--/merge_requests/16?diff=unified#hello', + undefined, + ); + t( + 'with a wrong character leading to "-/merge_requests/"', + 'sergeibbb/1--/merge_requests/16?diff=unified#hello', + undefined, + ); + }); +}); diff --git a/src/plus/integrations/providers/gitlab/gitlab.ts b/src/plus/integrations/providers/gitlab/gitlab.ts index 20ff1650acf8c..cba9ab433a586 100644 --- a/src/plus/integrations/providers/gitlab/gitlab.ts +++ b/src/plus/integrations/providers/gitlab/gitlab.ts @@ -35,17 +35,22 @@ import type { GitLabCommit, GitLabIssue, GitLabMergeRequest, + GitLabMergeRequestFull, GitLabMergeRequestREST, GitLabMergeRequestState, GitLabProjectREST, GitLabUser, } from './models'; -import { fromGitLabMergeRequestREST, fromGitLabMergeRequestState } from './models'; +import { fromGitLabMergeRequest, fromGitLabMergeRequestREST, fromGitLabMergeRequestState } from './models'; // drop it as soon as we switch to @gitkraken/providers-api const gitlabUserIdPrefix = 'gid://gitlab/User/'; -function buildGitLabUserId(id: string | undefined): string | undefined { - return id?.startsWith(gitlabUserIdPrefix) ? id.substring(gitlabUserIdPrefix.length) : id; +const gitlabMergeRequestIdPrefix = 'gid://gitlab/MergeRequest/'; + +function buildGitLabUserId(id: string | number | undefined): string | undefined { + return typeof id === 'string' && id?.startsWith(gitlabUserIdPrefix) + ? id.substring(gitlabUserIdPrefix.length) + : String(id); } export class GitLabApi implements Disposable { @@ -571,6 +576,94 @@ export class GitLabApi implements Disposable { } } + @debug({ args: { 0: p => p.name, 1: '' } }) + async getPullRequest( + provider: Provider, + token: string, + owner: string, + repo: string, + id: number, + options?: { + baseUrl?: string; + }, + cancellation?: CancellationToken, + ): Promise { + const scope = getLogScope(); + + interface QueryResult { + data: { + project: { + mergeRequest: GitLabMergeRequestFull | null; + } | null; + }; + } + + try { + const query = `query getMergeRequest( + $fullPath: ID! + $iid: String! +) { + project(fullPath: $fullPath) { + mergeRequest(iid: $iid) { + id, + iid + state, + author { + id + name + avatarUrl + webUrl + } + diffRefs { + baseSha + headSha + } + title + description + webUrl + createdAt + updatedAt + mergedAt + targetBranch + sourceBranch + project { + id + fullPath + webUrl + } + sourceProject { + id + fullPath + webUrl + } + } + } +}`; + + const rsp = await this.graphql( + provider, + token, + options?.baseUrl, + query, + { + fullPath: `${owner}/${repo}`, + iid: String(id), + }, + cancellation, + scope, + ); + + if (rsp?.data?.project?.mergeRequest == null) return undefined; + + const pr = rsp.data.project.mergeRequest; + return fromGitLabMergeRequest(pr, provider); + } catch (ex) { + if (ex instanceof RequestNotFoundError) return undefined; + + throw this.handleException(ex, provider, scope); + } + } + @debug({ args: { 0: p => p.name, 1: '' } }) async getRepositoryMetadata( provider: Provider, @@ -622,6 +715,116 @@ export class GitLabApi implements Disposable { } } + @debug({ args: { 0: p => p.name, 1: '' } }) + async searchPullRequests( + provider: Provider, + token: string, + options?: { search?: string; user?: string; repos?: string[]; baseUrl?: string; avatarSize?: number }, + cancellation?: CancellationToken, + ): Promise { + const scope = getLogScope(); + const search = options?.search; + if (!search) { + return []; + } + try { + const perPageLimit = 20; // with bigger amount we exceed the max GraphQL complexity in the next query + const restPRs = await this.request( + provider, + token, + options?.baseUrl, + `v4/search/?scope=merge_requests&search=${search}&per_page=${perPageLimit}`, + { + method: 'GET', + }, + cancellation, + scope, + ); + if (restPRs.length === 0) { + return []; + } + + interface QueryResult { + data: Record<`mergeRequest_${number}`, GitLabMergeRequestFull>; + } + + const queryArgs = restPRs.map((_, i) => `$id_${i}: MergeRequestID!`).join('\n'); + const queryFields = restPRs + .map((_, i) => `mergeRequest_${i}: mergeRequest(id: $id_${i}) { ...mergeRequestFields }`) + .join('\n'); + // Set of fields includes only additional fields that are not included in GitLabMergeRequestREST. + // It's limited as much as possible to reduce complexity of the query. + const queryMrFields = `fragment mergeRequestFields on MergeRequest { + diffRefs { + baseSha + headSha + } + project { + id + fullPath + webUrl + } + sourceProject { + id + fullPath + webUrl + } + }`; + const query = `query getMergeRequests (${queryArgs}) {${queryFields}} ${queryMrFields}`; + + const params = restPRs.reduce>((ids, gitlabRestPr, i) => { + ids[`id_${i}`] = `${gitlabMergeRequestIdPrefix}${gitlabRestPr.id}`; + return ids; + }, {}); + const rsp = await this.graphql( + provider, + token, + options?.baseUrl, + query, + params, + cancellation, + scope, + ); + if (rsp?.data != null) { + const resultPRs = restPRs.reduce((accum, restPR, i) => { + const graphQlPR = rsp.data[`mergeRequest_${i}`]; + if (graphQlPR == null) { + return accum; + } + + const fullPr: GitLabMergeRequestFull = { + ...graphQlPR, + iid: String(restPR.iid), + id: String(restPR.id), + state: restPR.state, + author: { + id: buildGitLabUserId(restPR.author?.id) ?? '', + name: restPR.author?.name ?? 'Unknown', + avatarUrl: restPR.author?.avatar_url ?? '', + webUrl: restPR.author?.web_url ?? '', + }, + title: restPR.title, + description: restPR.description, + webUrl: restPR.web_url, + createdAt: restPR.created_at, + updatedAt: restPR.updated_at, + mergedAt: restPR.merged_at, + sourceBranch: restPR.source_branch, + targetBranch: restPR.target_branch, + }; + accum.push(fromGitLabMergeRequest(fullPr, provider)); + return accum; + }, []); + return resultPRs; + } + return []; + } catch (ex) { + if (ex instanceof RequestNotFoundError) return []; + + throw this.handleException(ex, provider, scope); + } + } + private async findUser( provider: Provider, token: string, diff --git a/src/plus/integrations/providers/gitlab/gitlab.utils.ts b/src/plus/integrations/providers/gitlab/gitlab.utils.ts new file mode 100644 index 0000000000000..a4f3c308bacfa --- /dev/null +++ b/src/plus/integrations/providers/gitlab/gitlab.utils.ts @@ -0,0 +1,34 @@ +// GitLab provider: gitlab.ts pulls many dependencies through Container and some of them break the unit tests. +// That's why this file has been created that can collect more simple functions which +// don't require Container and can be tested. + +import { HostingIntegrationId } from '../../../../constants.integrations'; +import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils'; + +export function isMaybeGitLabPullRequestUrl(url: string): boolean { + return getGitLabPullRequestIdentityFromMaybeUrl(url) != null; +} + +export function getGitLabPullRequestIdentityFromMaybeUrl( + search: string, +): (PullRequestUrlIdentity & { provider: HostingIntegrationId.GitLab }) | undefined { + let ownerAndRepo: string | undefined = undefined; + let prNumber: string | undefined = undefined; + + let match = search.match(/([^/]+\/[^/]+)\/(?:-\/merge_requests)\/(\d+)/); // with org and rep name + if (match != null) { + ownerAndRepo = match[1]; + prNumber = match[2]; + } + + if (prNumber == null) { + match = search.match(/(?:\/|^)(?:-\/merge_requests)\/(\d+)/); // without repo name + if (match != null) { + prNumber = match[1]; + } + } + + return prNumber != null + ? { ownerAndRepo: ownerAndRepo, prNumber: prNumber, provider: HostingIntegrationId.GitLab } + : undefined; +} diff --git a/src/plus/integrations/providers/gitlab/models.ts b/src/plus/integrations/providers/gitlab/models.ts index 86a6f16a39b1c..82556326878fe 100644 --- a/src/plus/integrations/providers/gitlab/models.ts +++ b/src/plus/integrations/providers/gitlab/models.ts @@ -1,7 +1,5 @@ -import { HostingIntegrationId } from '../../../../constants.integrations'; -import type { PullRequestState } from '../../../../git/models/pullRequest'; -import { PullRequest } from '../../../../git/models/pullRequest'; -import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils'; +import type { PullRequestRefs, PullRequestState } from '../../../../git/models/pullRequest'; +import { PullRequest, PullRequestMergeableState } from '../../../../git/models/pullRequest'; import type { Provider } from '../../../../git/models/remoteProvider'; import type { Integration } from '../../integration'; import type { ProviderPullRequest } from '../models'; @@ -67,6 +65,24 @@ export interface GitLabMergeRequest { webUrl: string; } +export interface GitLabRepositoryStub { + id: string; + fullPath: string; + webUrl: string; +} + +export interface GitLabMergeRequestFull extends GitLabMergeRequest { + id: string; + targetBranch: string; + sourceBranch: string; + diffRefs: { + baseSha: string | null; + headSha: string; + } | null; + project: GitLabRepositoryStub; + sourceProject: GitLabRepositoryStub | null; +} + export type GitLabMergeRequestState = 'opened' | 'closed' | 'locked' | 'merged'; export function fromGitLabMergeRequestState(state: GitLabMergeRequestState): PullRequestState { @@ -93,6 +109,15 @@ export interface GitLabMergeRequestREST { updated_at: string; closed_at: string | null; merged_at: string | null; + diff_refs: { + base_sha: string; + head_sha: string; + start_sha: string; + }; + source_branch: string; + source_project_id: number; + target_branch: string; + target_project_id: number; web_url: string; } @@ -153,17 +178,79 @@ export function fromGitLabMergeRequestProvidersApi(pr: ProviderPullRequest, prov return fromProviderPullRequest(wrappedPr, provider); } -const prUrlRegex = /^(?:https?:\/\/)?(?:gitlab\.com\/)?(.+?)\/-\/merge_requests\/(\d+)/i; +export function fromGitLabMergeRequest(pr: GitLabMergeRequestFull, provider: Provider): PullRequest { + let avatarUrl: string | undefined; + try { + avatarUrl = new URL(pr.author?.avatarUrl ?? '').toString(); + } catch { + try { + const authorUrl = new URL(pr.author?.webUrl ?? ''); + authorUrl.pathname = ''; + authorUrl.search = ''; + authorUrl.hash = ''; + avatarUrl = pr.author?.avatarUrl ? authorUrl.toString() + pr.author?.avatarUrl : undefined; + } catch { + avatarUrl = undefined; + } + } + const [owner, repo] = pr.project.fullPath.split('/'); -export function isMaybeGitLabPullRequestUrl(url: string): boolean { - return prUrlRegex.test(url); + return new PullRequest( + provider, + { + // author + id: pr.author?.id ?? '', + name: pr.author?.name ?? 'Unknown', + avatarUrl: avatarUrl, + url: pr.author?.webUrl ?? '', + }, + pr.iid, // id + pr.id, // nodeId + pr.title, + pr.webUrl || '', + { + // IssueRepository + owner: owner, + repo: repo, + url: pr.project.webUrl, + }, + fromGitLabMergeRequestState(pr.state), // PullRequestState + new Date(pr.createdAt), + new Date(pr.updatedAt), + // TODO@eamodio this isn't right, but GitLab doesn't seem to provide a closedAt on merge requests in GraphQL + pr.state !== 'closed' ? undefined : new Date(pr.updatedAt), + pr.mergedAt == null ? undefined : new Date(pr.mergedAt), + PullRequestMergeableState.Unknown, + undefined, // viewerCanUpdate + fromGitLabMergeRequestRefs(pr), // PullRequestRefs + ); } -export function getGitLabPullRequestIdentityFromMaybeUrl(url: string): RequireSome { - if (url == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitLab }; - - const match = prUrlRegex.exec(url); - if (match == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitLab }; +function fromGitLabMergeRequestRefs(pr: GitLabMergeRequestFull): PullRequestRefs | undefined { + if (pr.sourceProject == null) { + return undefined; + } + return { + base: { + owner: getRepoNamespace(pr.sourceProject.fullPath), + branch: pr.sourceBranch, + exists: true, + url: pr.sourceProject.webUrl, + repo: pr.sourceProject.fullPath, + sha: pr.diffRefs?.baseSha || '', + }, + head: { + owner: getRepoNamespace(pr.project.fullPath), + branch: pr.targetBranch, + exists: true, + url: pr.project.webUrl, + repo: pr.project.fullPath, + sha: pr.diffRefs?.headSha || '', + }, + isCrossRepository: pr.sourceProject.id !== pr.project.id, + }; +} - return { prNumber: match[2], ownerAndRepo: match[1], provider: HostingIntegrationId.GitLab }; +function getRepoNamespace(projectFullPath: string) { + return projectFullPath.split('/').slice(0, -1).join('/'); } diff --git a/src/plus/launchpad/launchpad.ts b/src/plus/launchpad/launchpad.ts index 35d5c5605d23b..78a697662a060 100644 --- a/src/plus/launchpad/launchpad.ts +++ b/src/plus/launchpad/launchpad.ts @@ -63,7 +63,6 @@ import { } from './launchpadProvider'; import type { LaunchpadAction, LaunchpadGroup, LaunchpadTargetAction } from './models'; import { actionGroupMap, launchpadGroupIconMap, launchpadGroupLabelMap, launchpadGroups } from './models'; -import { isMaybeSupportedLaunchpadPullRequestSearchUrl } from './utils'; export interface LaunchpadItemQuickPickItem extends QuickPickItem { readonly type: 'item'; @@ -757,7 +756,7 @@ export class LaunchpadCommand extends QuickCommand { } // In API search mode, search the API and update the quickpick - if (context.inSearch || isMaybeSupportedLaunchpadPullRequestSearchUrl(value)) { + if (context.inSearch || this.container.launchpad.isMaybeSupportedLaunchpadPullRequestSearchUrl(value)) { let immediate = false; if (!context.inSearch) { immediate = value.length >= 3; diff --git a/src/plus/launchpad/launchpadProvider.ts b/src/plus/launchpad/launchpadProvider.ts index 12a7746083a71..a96eb16426125 100644 --- a/src/plus/launchpad/launchpadProvider.ts +++ b/src/plus/launchpad/launchpadProvider.ts @@ -21,6 +21,11 @@ import { getOrOpenPullRequestRepository, getRepositoryIdentityForPullRequest, } from '../../git/models/pullRequest'; +import type { PullRequestUrlIdentity } from '../../git/models/pullRequest.utils'; +import { + getPullRequestIdentityFromMaybeUrl, + isMaybeNonSpecificPullRequestSearchUrl, +} from '../../git/models/pullRequest.utils'; import type { GitRemote } from '../../git/models/remote'; import type { Repository } from '../../git/models/repository'; import type { CodeSuggestionCounts, Draft } from '../../gk/models/drafts'; @@ -39,10 +44,10 @@ import type { UriTypes } from '../../uris/deepLinks/deepLink'; import { DeepLinkActionType, DeepLinkType } from '../../uris/deepLinks/deepLink'; import { showInspectView } from '../../webviews/commitDetails/actions'; import type { ShowWipArgs } from '../../webviews/commitDetails/protocol'; -import type { IntegrationResult } from '../integrations/integration'; +import type { HostingIntegration, IntegrationResult, RepositoryDescriptor } from '../integrations/integration'; import type { ConnectionStateChangeEvent } from '../integrations/integrationService'; -import type { GitHubRepositoryDescriptor } from '../integrations/providers/github'; -import type { GitLabRepositoryDescriptor } from '../integrations/providers/gitlab'; +import { isMaybeGitHubPullRequestUrl } from '../integrations/providers/github/github.utils'; +import { isMaybeGitLabPullRequestUrl } from '../integrations/providers/gitlab/gitlab.utils'; import type { EnrichablePullRequest, ProviderActionablePullRequest } from '../integrations/providers/models'; import { fromProviderPullRequest, @@ -59,7 +64,6 @@ import { prActionsMap, sharedCategoryToLaunchpadActionCategoryMap, } from './models'; -import { getPullRequestIdentityFromMaybeUrl } from './utils'; export function getSuggestedActions(category: LaunchpadActionCategory, isCurrentBranch: boolean): LaunchpadAction[] { const actions = [...prActionsMap.get(category)!]; @@ -210,44 +214,76 @@ export class LaunchpadProvider implements Disposable { } private async getSearchedPullRequests(search: string, cancellation?: CancellationToken) { - // TODO: This needs to be generalized to work outside of GitHub, - // The current idea is that we should iterate the connected integrations and apply their parsing. - // Probably we even want to build a map like this: { integrationId: identity } - // Then we iterate connected integrations and search in each of them with the corresponding identity. - const { ownerAndRepo, prNumber, provider } = getPullRequestIdentityFromMaybeUrl(search); - let result: TimedResult | undefined; - - if (provider != null && prNumber != null && ownerAndRepo != null) { - // TODO: This needs to be generalized to work outside of GitHub/GitLab - const integration = await this.container.integrations.get(provider); - const [owner, repo] = ownerAndRepo.split('/', 2); - const descriptor: GitHubRepositoryDescriptor | GitLabRepositoryDescriptor = { - key: ownerAndRepo, - owner: owner, - name: repo, - }; - const pr = await withDurationAndSlowEventOnTimeout( - integration?.getPullRequest(descriptor, prNumber), - 'getPullRequest', - this.container, - ); - if (pr?.value != null) { - result = { value: [{ pullRequest: pr.value, reasons: [] }], duration: pr.duration }; - return { prs: result, suggestionCounts: undefined }; + const connectedIntegrations = await this.getConnectedIntegrations(); + const prUrlIdentity: PullRequestUrlIdentity | undefined = await this.getPullRequestIdentityFromSearch( + search, + connectedIntegrations, + ); + const result: { readonly value: SearchedPullRequest[]; duration: number } = { + value: [], + duration: 0, + }; + + const findByPrIdentity = async ( + integration: HostingIntegration, + ): Promise> => { + const { provider, ownerAndRepo, prNumber } = prUrlIdentity ?? {}; + const providerMatch = provider == null || provider === integration.id; + if (providerMatch && prNumber != null && ownerAndRepo != null) { + const [owner, repo] = ownerAndRepo.split('/', 2); + const descriptor: RepositoryDescriptor = { + key: ownerAndRepo, + owner: owner, + name: repo, + }; + const pr = await withDurationAndSlowEventOnTimeout( + integration?.getPullRequest(descriptor, prNumber), + 'getPullRequest', + this.container, + ); + if (pr?.value != null) { + return { value: [{ pullRequest: pr.value, reasons: [] }], duration: pr.duration }; + } } - } else { - const integration = await this.container.integrations.get(HostingIntegrationId.GitHub); + return undefined; + }; + + const findByQuery = async ( + integration: HostingIntegration, + ): Promise> => { const prs = await withDurationAndSlowEventOnTimeout( integration?.searchPullRequests(search, undefined, cancellation), 'searchPullRequests', this.container, ); if (prs != null) { - result = { value: prs.value?.map(pr => ({ pullRequest: pr, reasons: [] })), duration: prs.duration }; - return { prs: result, suggestionCounts: undefined }; + return { value: prs.value?.map(pr => ({ pullRequest: pr, reasons: [] })), duration: prs.duration }; } - } - return { prs: undefined, suggestionCounts: undefined }; + return undefined; + }; + + const searchIntegrationPRs = prUrlIdentity ? findByPrIdentity : findByQuery; + + await Promise.allSettled( + [...connectedIntegrations.keys()] + .filter( + (id: IntegrationId): id is SupportedLaunchpadIntegrationIds => + (connectedIntegrations.get(id) && isSupportedLaunchpadIntegrationId(id)) ?? false, + ) + .map(async (id: SupportedLaunchpadIntegrationIds) => { + const integration = await this.container.integrations.get(id); + const searchResult = await searchIntegrationPRs(integration); + const prs = searchResult?.value; + if (prs) { + result.value?.push(...prs); + result.duration = Math.max(result.duration, searchResult.duration); + } + }), + ); + return { + prs: result, + suggestionCounts: undefined, + }; } private _enrichedItems: CachedLaunchpadPromise> | undefined; @@ -547,6 +583,30 @@ export class LaunchpadProvider implements Disposable { return repoRemotes; } + isMaybeSupportedLaunchpadPullRequestSearchUrl(search: string): boolean { + return ( + isMaybeGitHubPullRequestUrl(search) || + isMaybeGitLabPullRequestUrl(search) || + isMaybeNonSpecificPullRequestSearchUrl(search) + ); + } + + async getPullRequestIdentityFromSearch( + search: string, + connectedIntegrations: Map, + ): Promise { + for (const integrationId of supportedLaunchpadIntegrations) { + if (connectedIntegrations.get(integrationId)) { + const integration = await this.container.integrations.get(integrationId); + const prIdentity = integration.getPullRequestIdentityFromMaybeUrl(search); + if (prIdentity) { + return prIdentity; + } + } + } + return getPullRequestIdentityFromMaybeUrl(search); + } + @gate( o => `${o?.force ?? false}|${ diff --git a/src/plus/launchpad/utils.ts b/src/plus/launchpad/utils.ts index 8a02ddd9b413b..9f12d5960f24e 100644 --- a/src/plus/launchpad/utils.ts +++ b/src/plus/launchpad/utils.ts @@ -1,14 +1,5 @@ import type { Container } from '../../container'; -import type { PullRequestUrlIdentity } from '../../git/models/pullRequest.utils'; import { configuration } from '../../system/vscode/configuration'; -import { - getGitHubPullRequestIdentityFromMaybeUrl, - isMaybeGitHubPullRequestUrl, -} from '../integrations/providers/github/models'; -import { - getGitLabPullRequestIdentityFromMaybeUrl, - isMaybeGitLabPullRequestUrl, -} from '../integrations/providers/gitlab/models'; import type { LaunchpadSummaryResult } from './launchpadIndicator'; import { generateLaunchpadSummary } from './launchpadIndicator'; import type { LaunchpadGroup } from './models'; @@ -25,18 +16,3 @@ export async function getLaunchpadSummary(container: Container): Promise