From 6c1b8e298f36fc8f7fe90c8255cc5ad13751bc28 Mon Sep 17 00:00:00 2001 From: Sergei Shmakov Date: Wed, 18 Dec 2024 19:46:41 +0100 Subject: [PATCH] Performs specific PR URL match for each connected provider (#3788, #3795) --- .../__tests__/pullRequest.utils.test.ts | 66 +++------ src/git/models/pullRequest.ts | 20 --- src/git/models/pullRequest.utils.ts | 30 ++-- src/plus/integrations/integration.ts | 3 + src/plus/integrations/providers/github.ts | 6 + .../github/__tests__/github.utils.test.ts | 87 ++++++++++++ .../providers/github/github.utils.ts | 36 +++++ .../integrations/providers/github/models.ts | 19 --- src/plus/integrations/providers/gitlab.ts | 6 + .../gitlab/__tests__/gitlab.utils.test.ts | 113 +++++++++++++++ .../providers/gitlab/gitlab.utils.ts | 34 +++++ .../integrations/providers/gitlab/models.ts | 17 --- src/plus/launchpad/launchpad.ts | 3 +- src/plus/launchpad/launchpadProvider.ts | 134 +++++++++++------- src/plus/launchpad/utils.ts | 24 ---- 15 files changed, 391 insertions(+), 207 deletions(-) create mode 100644 src/plus/integrations/providers/github/__tests__/github.utils.test.ts create mode 100644 src/plus/integrations/providers/github/github.utils.ts create mode 100644 src/plus/integrations/providers/gitlab/__tests__/gitlab.utils.test.ts create mode 100644 src/plus/integrations/providers/gitlab/gitlab.utils.ts 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 365b6255e44ac..a56afbad1b9b1 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'; @@ -417,21 +415,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 51744f239d39e..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|-\/merge_requests)\/(\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|-\/merge_requests)\/(\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..e5e4811d28587 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'; @@ -1361,4 +1362,6 @@ export abstract class HostingIntegration< repos?: T[], cancellation?: CancellationToken, ): Promise; + + getPullRequestIdentityFromMaybeUrl?(search: string): PullRequestUrlIdentity | undefined; } diff --git a/src/plus/integrations/providers/github.ts b/src/plus/integrations/providers/github.ts index a5d73cd3402b8..cb90d9b8c969f 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 3eb7a8404e026..8d1f19fcefc99 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'; @@ -393,6 +395,10 @@ abstract class GitLabIntegrationBase< username: currentUser.username || undefined, }; } + + override getPullRequestIdentityFromMaybeUrl(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.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 be86578ff073c..a86d70356099e 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, PullRequestMergeableState } from '../../../../git/models/pullRequest'; -import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils'; import type { Provider } from '../../../../git/models/remoteProvider'; import type { Integration } from '../../integration'; import type { ProviderPullRequest } from '../models'; @@ -162,18 +160,3 @@ export function fromGitLabMergeRequestProvidersApi(pr: ProviderPullRequest, prov }; return fromProviderPullRequest(wrappedPr, provider); } - -const prUrlRegex = /^(?:https?:\/\/)?(?:gitlab\.com\/)?(.+?)\/-\/merge_requests\/(\d+)/i; - -export function isMaybeGitLabPullRequestUrl(url: string): boolean { - return prUrlRegex.test(url); -} - -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 }; - - return { prNumber: match[2], ownerAndRepo: match[1], provider: HostingIntegrationId.GitLab }; -} 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 23956243be8d5..ac63715a21684 100644 --- a/src/plus/launchpad/launchpadProvider.ts +++ b/src/plus/launchpad/launchpadProvider.ts @@ -22,6 +22,10 @@ import { 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'; @@ -40,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 { HostingIntegration, 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, @@ -60,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)!]; @@ -211,11 +214,7 @@ 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 prUrlIdentity = getPullRequestIdentityFromMaybeUrl(search); + const prUrlIdentity: PullRequestUrlIdentity | undefined = await this.getPullRequestIdentityFromMaybeUrl(search); const result: { readonly value: SearchedPullRequest[]; duration: number } = { value: [], duration: 0, @@ -223,20 +222,55 @@ export class LaunchpadProvider implements Disposable { const connectedIntegrations = await this.getConnectedIntegrations(); + 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 }; + } + } + return undefined; + }; + + const findByQuery = async ( + integration: HostingIntegration, + ): Promise> => { + const prs = await withDurationAndSlowEventOnTimeout( + integration?.searchPullRequests(search, undefined, cancellation), + 'searchPullRequests', + this.container, + ); + if (prs != null) { + return { value: prs.value?.map(pr => ({ pullRequest: pr, reasons: [] })), duration: prs.duration }; + } + 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: HostingIntegrationId) => { + .map(async (id: SupportedLaunchpadIntegrationIds) => { const integration = await this.container.integrations.get(id); - const searchResult = await this.searchIntegrationPRs( - search, - prUrlIdentity, - integration, - cancellation, - ); + const searchResult = await searchIntegrationPRs(integration); const prs = searchResult?.value; if (prs) { result.value?.push(...prs); @@ -250,45 +284,6 @@ export class LaunchpadProvider implements Disposable { }; } - private async searchIntegrationPRs( - search: string, - { ownerAndRepo, prNumber, provider }: PullRequestUrlIdentity, - integration: HostingIntegration, - cancellation: CancellationToken | undefined, - ): Promise> { - 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 result; - } - } else { - 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 result; - } - } - return undefined; - } - private _enrichedItems: CachedLaunchpadPromise> | undefined; @debug({ args: { 0: o => `force=${o?.force}` } }) private async getEnrichedItems(options?: { cancellation?: CancellationToken; force?: boolean }) { @@ -586,6 +581,35 @@ export class LaunchpadProvider implements Disposable { return repoRemotes; } + isMaybeSupportedLaunchpadPullRequestSearchUrl(search: string): boolean { + return ( + isMaybeGitHubPullRequestUrl(search) || + isMaybeGitLabPullRequestUrl(search) || + isMaybeNonSpecificPullRequestSearchUrl(search) + ); + } + //// ??? or maybe better just to do this: + // async isMaybeSupportedLaunchpadPullRequestSearchUrl(search: string): Promise { + // return (await this.getPullRequestIdentityFromMaybeUrl(search)) != null; + // } + // - pros: it corresponds to active integrations + // - cons: it's async + // What do you think? + + async getPullRequestIdentityFromMaybeUrl(search: string): Promise { + const connectedIntegrations = await this.getConnectedIntegrations(); + 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