diff --git a/src/handlers/shared/start.ts b/src/handlers/shared/start.ts index fbe77ba9..e8262c78 100644 --- a/src/handlers/shared/start.ts +++ b/src/handlers/shared/start.ts @@ -1,4 +1,4 @@ -import { Assignee, Context, ISSUE_TYPE, Label } from "../../types"; +import { Context, ISSUE_TYPE, Label } from "../../types"; import { isParentIssue, getAvailableOpenedPullRequests, getAssignedIssues, addAssignees, addCommentToIssue } from "../../utils/issue"; import { calculateDurations } from "../../utils/shared"; import { checkTaskStale } from "./check-task-stale"; @@ -6,7 +6,7 @@ import { generateAssignmentComment } from "./generate-assignment-comment"; import structuredMetadata from "./structured-metadata"; import { assignTableComment } from "./table"; -export async function start(context: Context, issue: Context["payload"]["issue"], sender: Context["payload"]["sender"]) { +export async function start(context: Context, issue: Context["payload"]["issue"], sender: Context["payload"]["sender"], teammates: string[]) { const { logger, config } = context; const { maxConcurrentTasks } = config.miscellaneous; const { taskStaleTimeoutDuration } = config.timers; @@ -18,13 +18,13 @@ export async function start(context: Context, issue: Context["payload"]["issue"] "```diff\n# Please select a child issue from the specification checklist to work on. The '/start' command is disabled on parent issues.\n```" ); logger.error(`Skipping '/start' since the issue is a parent issue`); - throw new Error("Issue is a parent issue"); + return { output: "Parent issue detected" }; } let commitHash: string | null = null; try { - const hashResponse = await context.octokit.repos.getCommit({ + const hashResponse = await context.octokit.rest.repos.getCommit({ owner: context.payload.repository.owner.login, repo: context.payload.repository.name, ref: context.payload.repository.default_branch, @@ -34,64 +34,54 @@ export async function start(context: Context, issue: Context["payload"]["issue"] logger.error("Error while getting commit hash", { error: e as Error }); } - // check max assigned issues - - const openedPullRequests = await getAvailableOpenedPullRequests(context, sender.login); - logger.info(`Opened Pull Requests with approved reviews or with no reviews but over 24 hours have passed: ${JSON.stringify(openedPullRequests)}`); - - const assignedIssues = await getAssignedIssues(context, sender.login); - logger.info("Max issue allowed is", { maxConcurrentTasks }); - - // check for max and enforce max - - if (assignedIssues.length - openedPullRequests.length >= maxConcurrentTasks) { - const log = logger.error("Too many assigned issues, you have reached your max limit", { - assignedIssues: assignedIssues.length, - openedPullRequests: openedPullRequests.length, - maxConcurrentTasks, - }); - await addCommentToIssue(context, log?.logMessage.diff as string); - throw new Error(`Too many assigned issues, you have reached your max limit of ${maxConcurrentTasks} issues.`); - } - // is it assignable? if (issue.state === ISSUE_TYPE.CLOSED) { - const log = logger.error("This issue is closed, please choose another.", { issueNumber: issue.number }); - await addCommentToIssue(context, log?.logMessage.diff as string); - throw new Error("Issue is closed"); + throw logger.error("This issue is closed, please choose another.", { issueNumber: issue.number }); } - const assignees = (issue?.assignees ?? []).filter(Boolean); + const assignees = issue?.assignees ?? []; + + // find out if the issue is already assigned if (assignees.length !== 0) { - const log = logger.error("The issue is already assigned. Please choose another unassigned task.", { issueNumber: issue.number }); - await addCommentToIssue(context, log?.logMessage.diff as string); - throw new Error("Issue is already assigned"); + const isCurrentUserAssigned = !!assignees.find((assignee) => assignee?.login === sender.login); + throw logger.error( + isCurrentUserAssigned ? "You are already assigned to this task." : "This issue is already assigned. Please choose another unassigned task.", + { issueNumber: issue.number } + ); } - // get labels + teammates.push(sender.login); + // check max assigned issues + for (const user of teammates) { + await handleTaskLimitChecks(user, context, maxConcurrentTasks, logger, sender.login); + } + + // get labels const labels = issue.labels; const priceLabel = labels.find((label: Label) => label.name.startsWith("Price: ")); if (!priceLabel) { - const log = logger.error("No price label is set to calculate the duration", { issueNumber: issue.number }); - await addCommentToIssue(context, log?.logMessage.diff as string); - throw new Error("No price label is set to calculate the duration"); + throw logger.error("No price label is set to calculate the duration", { issueNumber: issue.number }); } const duration: number = calculateDurations(labels).shift() ?? 0; - const { id, login } = sender; - const logMessage = logger.info("Task assigned successfully", { duration, priceLabel, revision: commitHash?.substring(0, 7) }); + const { id } = sender; + const logMessage = logger.info("Task assigned successfully", { + duration, + priceLabel, + revision: commitHash?.substring(0, 7), + assignees: teammates, + issue: issue.number, + }); const assignmentComment = await generateAssignmentComment(context, issue.created_at, issue.number, id, duration); const metadata = structuredMetadata.create("Assignment", logMessage); - // add assignee - if (!assignees.map((i: Partial) => i?.login).includes(login)) { - await addAssignees(context, issue.number, [login]); - } + // assign the issue + await addAssignees(context, issue.number, teammates); const isTaskStale = checkTaskStale(taskStaleTimeoutDuration, issue.created_at); @@ -111,3 +101,17 @@ export async function start(context: Context, issue: Context["payload"]["issue"] return { output: "Task assigned successfully" }; } + +async function handleTaskLimitChecks(username: string, context: Context, maxConcurrentTasks: number, logger: Context["logger"], sender: string) { + const openedPullRequests = await getAvailableOpenedPullRequests(context, username); + const assignedIssues = await getAssignedIssues(context, username); + + // check for max and enforce max + if (assignedIssues.length - openedPullRequests.length >= maxConcurrentTasks) { + throw logger.error(username === sender ? "You have reached your max task limit" : `${username} has reached their max task limit`, { + assignedIssues: assignedIssues.length, + openedPullRequests: openedPullRequests.length, + maxConcurrentTasks, + }); + } +} diff --git a/src/handlers/shared/stop.ts b/src/handlers/shared/stop.ts index 4f101867..3a2541e4 100644 --- a/src/handlers/shared/stop.ts +++ b/src/handlers/shared/stop.ts @@ -11,9 +11,7 @@ export async function stop(context: Context, issue: Context["payload"]["issue"], const userToUnassign = assignees.find((assignee: Partial) => assignee?.login?.toLowerCase() === sender.login.toLowerCase()); if (!userToUnassign) { - const log = logger.error("You are not assigned to this task", { issueNumber, user: sender.login }); - await addCommentToIssue(context, log?.logMessage.diff as string); - return { output: "You are not assigned to this task" }; + throw new Error(logger.error("You are not assigned to this task", { issueNumber, user: sender.login })?.logMessage.diff as string); } // close PR @@ -26,16 +24,24 @@ export async function stop(context: Context, issue: Context["payload"]["issue"], // remove assignee - await context.octokit.rest.issues.removeAssignees({ - owner: login, - repo: name, - issue_number: issueNumber, - assignees: [sender.login], - }); + try { + await context.octokit.rest.issues.removeAssignees({ + owner: login, + repo: name, + issue_number: issueNumber, + assignees: [userToUnassign.login], + }); + } catch (err) { + throw logger.error(`Error while removing ${userToUnassign.login} from the issue: `, { + err, + issueNumber, + user: userToUnassign.login, + }); + } const unassignedLog = logger.info("You have been unassigned from the task", { issueNumber, - user: sender.login, + user: userToUnassign.login, }); await addCommentToIssue(context, unassignedLog?.logMessage.diff as string); diff --git a/src/handlers/user-start-stop.ts b/src/handlers/user-start-stop.ts index 46148f3e..a7c8815b 100644 --- a/src/handlers/user-start-stop.ts +++ b/src/handlers/user-start-stop.ts @@ -6,11 +6,15 @@ export async function userStartStop(context: Context): Promise<{ output: string const { payload } = context; const { issue, comment, sender, repository } = payload; const slashCommand = comment.body.split(" ")[0].replace("/", ""); + const teamMates = comment.body + .split("@") + .slice(1) + .map((teamMate) => teamMate.split(" ")[0]); if (slashCommand === "stop") { return await stop(context, issue, sender, repository); } else if (slashCommand === "start") { - return await start(context, issue, sender); + return await start(context, issue, sender, teamMates); } return { output: null }; diff --git a/src/plugin.ts b/src/plugin.ts index ec2bf094..617a319f 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -1,9 +1,10 @@ import { Octokit } from "@octokit/rest"; import { createClient } from "@supabase/supabase-js"; -import { Logs } from "@ubiquity-dao/ubiquibot-logger"; +import { LogReturn, Logs } from "@ubiquity-dao/ubiquibot-logger"; import { createAdapters } from "./adapters"; import { userStartStop } from "./handlers/user-start-stop"; import { Context, Env, PluginInputs } from "./types"; +import { addCommentToIssue } from "./utils/issue"; export async function startStopTask(inputs: PluginInputs, env: Env) { const octokit = new Octokit({ auth: inputs.authToken }); @@ -22,8 +23,26 @@ export async function startStopTask(inputs: PluginInputs, env: Env) { context.adapters = createAdapters(supabase, context); if (context.eventName === "issue_comment.created") { - await userStartStop(context); + try { + return await userStartStop(context); + } catch (err) { + let errorMessage; + if (err instanceof LogReturn) { + errorMessage = context.logger.error(`Failed to run comment evaluation. ${err.logMessage?.raw || err}`, { err }); + } else { + errorMessage = context.logger.error(`Failed to run comment evaluation. ${err}`, { err }); + } + + await addCommentToIssue(context, `${errorMessage?.logMessage.diff}\n`); + } } else { context.logger.error(`Unsupported event: ${context.eventName}`); } } + +function sanitizeMetadata(obj: LogReturn["metadata"]): string { + return JSON.stringify(obj, null, 2) + .replace(//g, ">") + .replace(/--/g, "--") +} diff --git a/src/utils/get-linked-prs.ts b/src/utils/get-linked-prs.ts index 187f8087..5614ce54 100644 --- a/src/utils/get-linked-prs.ts +++ b/src/utils/get-linked-prs.ts @@ -22,7 +22,7 @@ export async function getLinkedPullRequests(context: Context, { owner, repositor throw new Error("Issue is not defined"); } - const { data: timeline } = (await context.octokit.issues.listEventsForTimeline({ + const { data: timeline } = (await context.octokit.rest.issues.listEventsForTimeline({ owner, repo: repository, issue_number: issue, @@ -42,7 +42,5 @@ export async function getLinkedPullRequests(context: Context, { owner, repositor state: pr.state, body: pr.body, }; - }) - .filter((pr) => pr !== null) - .filter((pr) => pr.state === "open") as GetLinkedResults[]; + }).filter((pr) => pr !== null && pr.state === "open") as GetLinkedResults[]; } diff --git a/src/utils/issue.ts b/src/utils/issue.ts index e6746e80..d019d377 100644 --- a/src/utils/issue.ts +++ b/src/utils/issue.ts @@ -1,5 +1,5 @@ import { Context } from "../types/context"; -import { Issue, ISSUE_TYPE, PullRequest, Review } from "../types/payload"; +import { Issue, PullRequest, Review } from "../types/payload"; import { getLinkedPullRequests, GetLinkedResults } from "./get-linked-prs"; export function isParentIssue(body: string) { @@ -8,22 +8,14 @@ export function isParentIssue(body: string) { } export async function getAssignedIssues(context: Context, username: string): Promise { - const payload = context.payload; + const { payload } = context; try { - return await context.octokit.paginate( - context.octokit.issues.listForRepo, - { - owner: payload.repository.owner.login, - repo: payload.repository.name, - state: ISSUE_TYPE.OPEN, - per_page: 100, - }, - ({ data: issues }) => issues.filter((issue: Issue) => !issue.pull_request && issue.assignee && issue.assignee.login === username) - ); + return (await context.octokit.paginate(context.octokit.rest.search.issuesAndPullRequests, { + q: `is:issue is:open assignee:${username} org:${payload.repository.owner.login}`, + })) as Issue[]; } catch (err: unknown) { - context.logger.error("Fetching assigned issues failed!", { error: err as Error }); - return []; + throw context.logger.error("Fetching assigned issues failed!", { error: err as Error }); } } @@ -34,14 +26,14 @@ export async function addCommentToIssue(context: Context, message: string | null const issueNumber = payload.issue.number; try { - await context.octokit.issues.createComment({ + await context.octokit.rest.issues.createComment({ owner: payload.repository.owner.login, repo: payload.repository.name, issue_number: issueNumber, body: comment, }); } catch (err: unknown) { - context.logger.error("Adding a comment failed!", { error: err as Error }); + throw context.logger.error("Adding a comment failed!", { error: err as Error }); } } @@ -57,15 +49,17 @@ export async function closePullRequest(context: Context, results: GetLinkedResul state: "closed", }); } catch (err: unknown) { - context.logger.error("Closing pull requests failed!", { error: err as Error }); + throw context.logger.error("Closing pull requests failed!", { error: err as Error }); } } export async function closePullRequestForAnIssue(context: Context, issueNumber: number, repository: Context["payload"]["repository"], author: string) { const { logger } = context; if (!issueNumber) { - logger.error("Issue is not defined"); - return; + throw logger.error("Issue is not defined", { + issueNumber, + repository: repository.name, + }); } const linkedPullRequests = await getLinkedPullRequests(context, { @@ -112,6 +106,35 @@ export async function closePullRequestForAnIssue(context: Context, issueNumber: return logger.info(comment); } +async function confirmMultiAssignment(context: Context, issueNumber: number, usernames: string[]) { + const { logger, payload, octokit } = context; + + if (usernames.length < 2) { + return; + } + + const { private: isPrivate } = payload.repository; + + const { + data: { assignees }, + } = await octokit.rest.issues.get({ + owner: payload.repository.owner.login, + repo: payload.repository.name, + issue_number: issueNumber, + }); + + if (!assignees?.length) { + throw logger.error("We detected that this task was not assigned to anyone. Please report this to the maintainers.", { issueNumber, usernames }); + } + + if (isPrivate && assignees?.length <= 1) { + const log = logger.info("This task belongs to a private repo and can only be assigned to one user without an official paid GitHub subscription.", { + issueNumber, + }); + await addCommentToIssue(context, log?.logMessage.diff as string); + } +} + export async function addAssignees(context: Context, issueNo: number, assignees: string[]) { const payload = context.payload; @@ -125,6 +148,8 @@ export async function addAssignees(context: Context, issueNo: number, assignees: } catch (e: unknown) { throw context.logger.error("Adding the assignee failed", { assignee: assignees, issueNo, error: e as Error }); } + + await confirmMultiAssignment(context, issueNo, assignees); } export async function getAllPullRequests(context: Context, state: "open" | "closed" | "all" = "open") { @@ -138,8 +163,7 @@ export async function getAllPullRequests(context: Context, state: "open" | "clos per_page: 100, })) as PullRequest[]; } catch (err: unknown) { - context.logger.error("Fetching all pull requests failed!", { error: err as Error }); - return []; + throw context.logger.error("Fetching all pull requests failed!", { error: err as Error }); } } @@ -160,8 +184,7 @@ export async function getAllPullRequestReviews(context: Context, pullNumber: num }, })) as Review[]; } catch (err: unknown) { - context.logger.error("Fetching all pull request reviews failed!", { error: err as Error }); - return []; + throw context.logger.error("Fetching all pull request reviews failed!", { error: err as Error }); } } diff --git a/tests/__mocks__/handlers.ts b/tests/__mocks__/handlers.ts index d975b95c..f247b080 100644 --- a/tests/__mocks__/handlers.ts +++ b/tests/__mocks__/handlers.ts @@ -81,7 +81,7 @@ export const handlers = [ db.issue.update({ where: { id: { equals: issue.id } }, data: { - assignees, + assignees: [...issue.assignees, ...assignees], }, }); } @@ -107,4 +107,17 @@ export const handlers = [ http.delete("https://api.github.com/repos/:owner/:repo/issues/:issue_number/assignees", ({ params: { owner, repo, issue_number: issueNumber } }) => HttpResponse.json({ owner, repo, issueNumber }) ), + // search issues + http.get("https://api.github.com/search/issues", () => { + const issues = [db.issue.findFirst({ where: { number: { equals: 1 } } })]; + return HttpResponse.json({ items: issues }); + }), + // get issue by number + http.get("https://api.github.com/repos/:owner/:repo/issues/:issue_number", ({ params: { owner, repo, issue_number: issueNumber } }) => + HttpResponse.json( + db.issue.findFirst({ + where: { owner: { equals: owner as string }, repo: { equals: repo as string }, number: { equals: Number(issueNumber) } }, + }) + ) + ), ]; diff --git a/tests/main.test.ts b/tests/main.test.ts index 15181f1c..8c109d0e 100644 --- a/tests/main.test.ts +++ b/tests/main.test.ts @@ -52,6 +52,23 @@ describe("User start/stop", () => { expect(output).toEqual("Task assigned successfully"); }); + test("User can start an issue with teammates", async () => { + const issue = db.issue.findFirst({ where: { id: { equals: 1 } } }) as unknown as Issue; + const sender = db.users.findFirst({ where: { id: { equals: 1 } } }) as unknown as Sender; + + const context = createContext(issue, sender, "/start @user2"); + + context.adapters = createAdapters(getSupabase(), context as unknown as Context); + + const { output } = await userStartStop(context as unknown as Context); + + expect(output).toEqual("Task assigned successfully"); + + const issue2 = db.issue.findFirst({ where: { id: { equals: 1 } } }) as unknown as Issue; + expect(issue2.assignees).toHaveLength(2); + expect(issue2.assignees).toEqual(expect.arrayContaining(["ubiquity", "user2"])); + }); + test("User can stop an issue", async () => { const issue = db.issue.findFirst({ where: { id: { equals: 2 } } }) as unknown as Issue; const sender = db.users.findFirst({ where: { id: { equals: 2 } } }) as unknown as Sender; @@ -93,9 +110,7 @@ describe("User start/stop", () => { context.adapters = createAdapters(getSupabase(), context as unknown as Context); - const output = await userStartStop(context as unknown as Context); - - expect(output).toEqual({ output: "You are not assigned to this task" }); + await expect(userStartStop(context as unknown as Context)).rejects.toThrow("```diff\n! You are not assigned to this task\n```"); }); test("User can't stop an issue without assignees", async () => { @@ -103,12 +118,9 @@ describe("User start/stop", () => { const sender = db.users.findFirst({ where: { id: { equals: 1 } } }) as unknown as Sender; const context = createContext(issue, sender, "/stop"); - context.adapters = createAdapters(getSupabase(), context as unknown as Context); - const output = await userStartStop(context as unknown as Context); - - expect(output).toEqual({ output: "You are not assigned to this task" }); + await expect(userStartStop(context as unknown as Context)).rejects.toThrow("```diff\n! You are not assigned to this task\n```"); }); test("User can't start an issue that's already assigned", async () => { @@ -119,7 +131,7 @@ describe("User start/stop", () => { context.adapters = createAdapters(getSupabase(), context as unknown as Context); - const err = "Issue is already assigned"; + const err = "```diff\n! This issue is already assigned. Please choose another unassigned task.\n```"; try { await userStartStop(context as unknown as Context); @@ -183,40 +195,6 @@ describe("User start/stop", () => { } }); - test("User can't start if command is disabled", async () => { - const issue = db.issue.findFirst({ where: { id: { equals: 1 } } }) as unknown as Issue; - const sender = db.users.findFirst({ where: { id: { equals: 1 } } }) as unknown as Sender; - - const context = createContext(issue, sender, "/start"); - - context.adapters = createAdapters(getSupabase(), context as unknown as Context); - - try { - await userStartStop(context as unknown as Context); - } catch (error) { - if (error instanceof Error) { - expect(error.message).toEqual("The '/start' command is disabled for this repository."); - } - } - }); - - test("User can't stop if command is disabled", async () => { - const issue = db.issue.findFirst({ where: { id: { equals: 1 } } }) as unknown as Issue; - const sender = db.users.findFirst({ where: { id: { equals: 1 } } }) as unknown as Sender; - - const context = createContext(issue, sender, "/stop"); - - context.adapters = createAdapters(getSupabase(), context as unknown as Context); - - try { - await userStartStop(context as unknown as Context); - } catch (error) { - if (error instanceof Error) { - expect(error.message).toEqual("The '/stop' command is disabled for this repository."); - } - } - }); - test("User can't start an issue that's a parent issue", async () => { const issue = db.issue.findFirst({ where: { id: { equals: 1 } } }) as unknown as Issue; const sender = db.users.findFirst({ where: { id: { equals: 1 } } }) as unknown as Sender; @@ -416,7 +394,6 @@ async function setupTests() { }, body: "Pull request body", owner: "ubiquity", - repo: "test-repo", state: "open", closed_at: null,