Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: pull-request fetch for private user profiles #71

Merged
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ junit.xml
cypress/screenshots
script.ts
.wrangler
test-dashboard.md
test-dashboard.md
*.env.json
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"knip-ci": "knip --no-exit-code --reporter json --config .github/knip.ts",
"prepare": "husky install",
"test": "jest --setupFiles dotenv/config --coverage",
"worker": "wrangler dev --env dev --port 4001"
"worker": "wrangler dev --env dev --port 4000"
},
"keywords": [
"typescript",
Expand All @@ -30,6 +30,7 @@
"@octokit/graphql-schema": "15.25.0",
"@octokit/plugin-paginate-graphql": "5.2.2",
"@octokit/rest": "20.1.1",
"@octokit/types": "^13.6.1",
"@octokit/webhooks": "13.2.7",
"@sinclair/typebox": "^0.32.5",
"@supabase/supabase-js": "2.42.0",
Expand Down
4 changes: 2 additions & 2 deletions src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ export async function startStopTask(inputs: PluginInputs, env: Env) {
case "issues.assigned":
return await userSelfAssign(context as Context<"issues.assigned">);
case "pull_request.opened":
return await userPullRequest(context as Context<"pull_request.edited">);
case "pull_request.edited":
return await userPullRequest(context as Context<"pull_request.opened">);
case "pull_request.edited":
return await userPullRequest(context as Context<"pull_request.edited">);
case "issues.unassigned":
return await userUnassigned(context);
default:
Expand Down
91 changes: 91 additions & 0 deletions src/utils/get-pull-requests-fallback.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { RestEndpointMethodTypes } from "@octokit/rest";
import type { Endpoints } from "@octokit/types";
import { Context } from "../types";

function isHttpError(error: unknown): error is { status: number; message: string } {
return typeof error === "object" && error !== null && "status" in error && "message" in error;
}

/**
* Fetches all open pull requests within a specified organization created by a particular user.
* This method is slower than using a search query, but should work even if the user has his activity set to private.
*/
export async function getAllPullRequestsFallback(
context: Context,
state: Endpoints["GET /repos/{owner}/{repo}/pulls"]["parameters"]["state"],
username: string
) {
const { octokit, logger } = context;
const organization = context.payload.repository.owner.login;

try {
const repositories = await octokit.paginate(octokit.rest.repos.listForOrg, {
org: organization,
per_page: 100,
type: "all",
});

const allPrs: RestEndpointMethodTypes["pulls"]["list"]["response"]["data"] = [];

const tasks = repositories.map(async (repo) => {
try {
const prs = await octokit.paginate(octokit.rest.pulls.list, {
owner: organization,
repo: repo.name,
state,
per_page: 100,
});
const userPrs = prs.filter((pr) => pr.user?.login === username);
allPrs.push(...userPrs);
} catch (error) {
if (isHttpError(error) && (error.status === 404 || error.status === 403)) {
logger.error(`Could not find pull requests for repository ${repo.url}, skipping: ${error}`);
return;
}
logger.fatal("Failed to fetch pull requests for repository", { error: error as Error });
throw error;
}
});

await Promise.all(tasks);

return allPrs;
} catch (error) {
logger.fatal("Failed to fetch pull requests for organization", { error: error as Error });
throw error;
}
}

export async function getAssignedIssuesFallback(context: Context, username: string) {
const org = context.payload.repository.owner.login;
const assignedIssues = [];

try {
const repositories = await context.octokit.paginate(context.octokit.rest.repos.listForOrg, {
org,
type: "all",
per_page: 100,
});

for (const repo of repositories) {
const issues = await context.octokit.paginate(context.octokit.rest.issues.listForRepo, {
owner: org,
repo: repo.name,
assignee: username,
state: "open",
per_page: 100,
});

assignedIssues.push(
...issues.filter(
(issue) =>
issue.pull_request === undefined && (issue.assignee?.login === username || issue.assignees?.some((assignee) => assignee.login === username))
)
);
}

return assignedIssues;
} catch (err: unknown) {
throw new Error(context.logger.error("Fetching assigned issues failed!", { error: err as Error }).logMessage.raw);
}
}
42 changes: 29 additions & 13 deletions src/utils/issue.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
import { RestEndpointMethodTypes } from "@octokit/rest";
import { Endpoints } from "@octokit/types";
import ms from "ms";
import { Context } from "../types/context";
import { GitHubIssueSearch, Review } from "../types/payload";
import { getLinkedPullRequests, GetLinkedResults } from "./get-linked-prs";
import { getAllPullRequestsFallback, getAssignedIssuesFallback } from "./get-pull-requests-fallback";

export function isParentIssue(body: string) {
const parentPattern = /-\s+\[( |x)\]\s+#\d+/;
return body.match(parentPattern);
}

export async function getAssignedIssues(context: Context, username: string): Promise<GitHubIssueSearch["items"]> {
export async function getAssignedIssues(context: Context, username: string) {
const payload = context.payload;

try {
return await context.octokit
.paginate(context.octokit.search.issuesAndPullRequests, {
.paginate(context.octokit.rest.search.issuesAndPullRequests, {
q: `org:${payload.repository.owner.login} assignee:${username} is:open is:issue`,
per_page: 100,
order: "desc",
Expand All @@ -25,8 +27,9 @@ export async function getAssignedIssues(context: Context, username: string): Pro
return issue.state === "open" && (issue.assignee?.login === username || issue.assignees?.some((assignee) => assignee.login === username));
})
);
} catch (err: unknown) {
throw new Error(context.logger.error("Fetching assigned issues failed!", { error: err as Error }).logMessage.raw);
} catch (err) {
context.logger.info("Will try re-fetching assigned issues...", { error: err as Error });
return getAssignedIssuesFallback(context, username);
}
}

Expand Down Expand Up @@ -170,7 +173,7 @@ export async function addAssignees(context: Context, issueNo: number, assignees:
await confirmMultiAssignment(context, issueNo, assignees);
}

export async function getAllPullRequests(context: Context, state: "open" | "closed" | "all" = "open", username: string) {
async function getAllPullRequests(context: Context, state: Endpoints["GET /repos/{owner}/{repo}/pulls"]["parameters"]["state"] = "open", username: string) {
const { payload } = context;
const query: RestEndpointMethodTypes["search"]["issuesAndPullRequests"]["parameters"] = {
q: `org:${payload.repository.owner.login} author:${username} state:${state}`,
Expand All @@ -180,19 +183,32 @@ export async function getAllPullRequests(context: Context, state: "open" | "clos
};

try {
return (await context.octokit.paginate(context.octokit.search.issuesAndPullRequests, query)) as GitHubIssueSearch["items"];
return (await context.octokit.paginate(context.octokit.rest.search.issuesAndPullRequests, query)) as GitHubIssueSearch["items"];
} catch (err: unknown) {
throw new Error(context.logger.error("Fetching all pull requests failed!", { error: err as Error, query }).logMessage.raw);
}
}

export async function getAllPullRequestsWithRetry(
context: Context,
state: Endpoints["GET /repos/{owner}/{repo}/pulls"]["parameters"]["state"],
username: string
) {
try {
return await getAllPullRequests(context, state, username);
} catch (error) {
context.logger.info("Will retry re-fetching all pull requests...", { error: error as Error });
return getAllPullRequestsFallback(context, state, username);
}
}

export async function getAllPullRequestReviews(context: Context, pullNumber: number, owner: string, repo: string) {
const {
config: { rolesWithReviewAuthority },
} = context;
try {
return (
await context.octokit.paginate(context.octokit.pulls.listReviews, {
await context.octokit.paginate(context.octokit.rest.pulls.listReviews, {
owner,
repo,
pull_number: pullNumber,
Expand All @@ -219,11 +235,12 @@ export async function getAvailableOpenedPullRequests(context: Context, username:
const { reviewDelayTolerance } = context.config;
if (!reviewDelayTolerance) return [];

const openedPullRequests = await getOpenedPullRequests(context, username);
const result = [] as typeof openedPullRequests;
const openedPullRequests = await getOpenedPullRequestsForUser(context, username);
const result: (typeof openedPullRequests)[number][] = [];

for (let i = 0; i < openedPullRequests.length; i++) {
for (let i = 0; openedPullRequests && i < openedPullRequests.length; i++) {
const openedPullRequest = openedPullRequests[i];
if (!openedPullRequest) continue;
const { owner, repo } = getOwnerRepoFromHtmlUrl(openedPullRequest.html_url);
const reviews = await getAllPullRequestReviews(context, openedPullRequest.number, owner, repo);

Expand Down Expand Up @@ -251,9 +268,8 @@ export function getTimeValue(timeString: string): number {
return timeValue;
}

async function getOpenedPullRequests(context: Context, username: string): Promise<ReturnType<typeof getAllPullRequests>> {
const prs = await getAllPullRequests(context, "open", username);
return prs.filter((pr) => pr.pull_request && pr.state === "open");
async function getOpenedPullRequestsForUser(context: Context, username: string): Promise<ReturnType<typeof getAllPullRequestsWithRetry>> {
return getAllPullRequestsWithRetry(context, "open", username);
}

/**
Expand Down
48 changes: 48 additions & 0 deletions tests/fallbacks.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { RestEndpointMethodTypes } from "@octokit/rest";
import { Logs } from "@ubiquity-os/ubiquity-os-logger";
import { Context } from "../src/types/context";
import { getAllPullRequestsWithRetry } from "../src/utils/issue";

const username = "private-user";

const mockPullRequestData = [
{ id: 1, number: 123, state: "open", user: { login: username } },
{ id: 2, number: 124, state: "open", user: { login: "public-user" } },
] as unknown as RestEndpointMethodTypes["pulls"]["list"]["response"]["data"];

const mockOctokit = {
paginate: jest.fn().mockResolvedValue(mockPullRequestData),
rest: {
pulls: {
list: jest.fn().mockResolvedValue(mockPullRequestData),
},
repos: {
listForOrg: jest.fn().mockResolvedValue(mockPullRequestData),
},
},
};

const context: Context = {
eventName: "pull_request",
payload: {
repository: {
name: "test-repo",
owner: {
login: "test-owner",
},
},
},
octokit: mockOctokit as unknown as Context["octokit"],
logger: new Logs("debug"),
adapters: {},
} as unknown as Context;

describe("getAllPullRequestsWithRetry", () => {
it("should return pull requests even if user information is private", async () => {
const pullRequests = await getAllPullRequestsWithRetry(context, "all", username);
expect(pullRequests).toHaveLength(2);
expect(pullRequests[0].user?.login).toBe(username);
expect(pullRequests[1].user?.login).toBe(username);
console.log(pullRequests);
});
});
Loading