Skip to content

Commit

Permalink
Performs specific PR URL match for each connected provider
Browse files Browse the repository at this point in the history
  • Loading branch information
sergeibbb committed Dec 26, 2024
1 parent 7912433 commit 44996dc
Show file tree
Hide file tree
Showing 15 changed files with 384 additions and 207 deletions.
66 changes: 17 additions & 49 deletions src/git/models/__tests__/pullRequest.utils.test.ts
Original file line number Diff line number Diff line change
@@ -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 "/<num>" fragment', () => {
Expand Down
20 changes: 0 additions & 20 deletions src/git/models/pullRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}
30 changes: 9 additions & 21 deletions src/git/models/pullRequest.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 };
}
3 changes: 3 additions & 0 deletions src/plus/integrations/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -1361,4 +1362,6 @@ export abstract class HostingIntegration<
repos?: T[],
cancellation?: CancellationToken,
): Promise<PullRequest[] | undefined>;

getPullRequestIdentityFromMaybeUrl?(search: string): PullRequestUrlIdentity | undefined;
}
6 changes: 6 additions & 0 deletions src/plus/integrations/providers/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';

Expand Down Expand Up @@ -292,6 +294,10 @@ export class GitHubIntegration extends GitHubIntegrationBase<HostingIntegrationI
super.refresh();
}
}

override getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
return getGitHubPullRequestIdentityFromMaybeUrl(search);
}
}

export class GitHubEnterpriseIntegration extends GitHubIntegrationBase<SelfHostedIntegrationId.GitHubEnterprise> {
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
});
});
36 changes: 36 additions & 0 deletions src/plus/integrations/providers/github/github.utils.ts
Original file line number Diff line number Diff line change
@@ -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;
}
19 changes: 0 additions & 19 deletions src/plus/integrations/providers/github/models.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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 {
Expand Down Expand Up @@ -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<PullRequestUrlIdentity, 'provider'> {
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 };
}
6 changes: 6 additions & 0 deletions src/plus/integrations/providers/gitlab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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<HostingIntegrationId.GitLab> {
Expand Down
Loading

0 comments on commit 44996dc

Please sign in to comment.