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 issues #22

Closed
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion .cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,22 @@
"ignorePaths": ["**/*.json", "**/*.css", "node_modules", "**/*.log", "**/*.http", "**/*.toml", "src/types/database.ts", "supabase/migrations/**", "tests/**"],
"useGitignore": true,
"language": "en",
"words": ["dataurl", "devpool", "outdir", "servedir", "typebox", "supabase", "ubiquibot", "mswjs", "luxon", "millis", "handl"],
"words": [
"dataurl",
"devpool",
"outdir",
"servedir",
"typebox",
"supabase",
"ubiquibot",
"mswjs",
"luxon",
"millis",
"handl",
"sonarjs",
"mischeck",
"unassigns"
],
"dictionaries": ["typescript", "node", "software-terms"],
"import": ["@cspell/dict-typescript/cspell-ext.json", "@cspell/dict-node/cspell-ext.json", "@cspell/dict-software-terms"],
"ignoreRegExpList": ["[0-9a-fA-F]{6}"]
Expand Down
5 changes: 5 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,9 @@ yarn test
optOut:
- "repoName"
- "repoName2"
eventWhitelist: # these are the tail of the webhook event i.e pull_request.review_requested
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just use the full identifier. Shortening just will lead to confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the config would take the full pull_request.review_requested and I'll handle string splitting, is that what you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this was ever updated in the readme.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was never fixed. Should use full webhook names.

it("Should correctly transform the eventWhitelist", () => {

- "review_requested"
- "ready_for_review"
- "commented"
- "committed"
```
58 changes: 52 additions & 6 deletions src/helpers/get-assignee-activity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import { DateTime } from "luxon";
import { collectLinkedPullRequests } from "../handlers/collect-linked-pulls";
import { Context } from "../types/context";
import { parseIssueUrl } from "./github-url";
import { GitHubListEvents, ListIssueForRepo } from "../types/github-types";
import { GitHubTimelineEvents, ListIssueForRepo } from "../types/github-types";

/**
* Retrieves all the activity for users that are assigned to the issue. Also takes into account linked pull requests.
*/
export async function getAssigneesActivityForIssue(context: Context, issue: ListIssueForRepo, assigneeIds: number[]) {
const gitHubUrl = parseIssueUrl(issue.html_url);
const issueEvents: GitHubListEvents[] = await context.octokit.paginate(context.octokit.rest.issues.listEvents, {
const issueEvents: GitHubTimelineEvents[] = await context.octokit.paginate(context.octokit.rest.issues.listEventsForTimeline, {
owner: gitHubUrl.owner,
repo: gitHubUrl.repo,
issue_number: gitHubUrl.issue_number,
Expand All @@ -18,7 +18,7 @@ export async function getAssigneesActivityForIssue(context: Context, issue: List
const linkedPullRequests = await collectLinkedPullRequests(context, gitHubUrl);
for (const linkedPullRequest of linkedPullRequests) {
const { owner, repo, issue_number } = parseIssueUrl(linkedPullRequest.url || "");
const events = await context.octokit.paginate(context.octokit.rest.issues.listEvents, {
const events: GitHubTimelineEvents[] = await context.octokit.paginate(context.octokit.rest.issues.listEventsForTimeline, {
owner,
repo,
issue_number,
Expand All @@ -27,7 +27,53 @@ export async function getAssigneesActivityForIssue(context: Context, issue: List
issueEvents.push(...events);
}

return issueEvents
.filter((o) => o.actor && o.actor.id && assigneeIds.includes(o.actor.id))
.sort((a, b) => DateTime.fromISO(b.created_at).toMillis() - DateTime.fromISO(a.created_at).toMillis());
return filterEvents(issueEvents, assigneeIds);
}

function filterEvents(issueEvents: GitHubTimelineEvents[], assigneeIds: number[]) {
const userIdMap = new Map<string, number>();

let assigneeEvents = [];

for (const event of issueEvents) {
let actorId = null;
let actorLogin = null;
let createdAt = null;
let eventName = event.event;

if ("actor" in event && event.actor) {
actorLogin = event.actor.login.toLowerCase();
if (!userIdMap.has(actorLogin)) {
userIdMap.set(actorLogin, event.actor.id);
}
actorId = userIdMap.get(actorLogin);
createdAt = event.created_at;
} else if (event.event === "committed") {
const commitAuthor = "author" in event ? event.author : null;
const commitCommitter = "committer" in event ? event.committer : null;

if (commitAuthor || commitCommitter) {
assigneeEvents.push({
event: eventName,
created_at: createdAt,
});

continue;
}
}

if (actorId && assigneeIds.includes(actorId)) {
assigneeEvents.push({
event: eventName,
created_at: createdAt,
});
}
}

return assigneeEvents.sort((a, b) => {
if (!a.created_at || !b.created_at) {
return 0;
}
return DateTime.fromISO(b.created_at).toMillis() - DateTime.fromISO(a.created_at).toMillis();
});
}
27 changes: 20 additions & 7 deletions src/helpers/task-deadline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ export async function getDeadlineWithThreshold(
issue: ListIssueForRepo,
lastCheck: DateTime
) {
const { logger, config } = context;
const {
logger,
config: { disqualification, warning, eventWhitelist },
} = context;

const assigneeIds = issue.assignees?.map((o) => o.id) || [];

Expand All @@ -32,16 +35,26 @@ export async function getDeadlineWithThreshold(
}

const activity = (await getAssigneesActivityForIssue(context, issue, assigneeIds)).filter((o) => {
if (!o.created_at) {
return false;
}
return DateTime.fromISO(o.created_at) > lastCheck;
});

let deadlineWithThreshold = deadline.plus({ milliseconds: config.disqualification });
let reminderWithThreshold = deadline.plus({ milliseconds: config.warning });
const filteredActivity = activity.filter((o) => {
if (!o.event) {
return false;
}
return eventWhitelist.includes(o.event);
});

let deadlineWithThreshold = deadline.plus({ milliseconds: disqualification });
let reminderWithThreshold = deadline.plus({ milliseconds: warning });

if (activity?.length) {
const lastActivity = DateTime.fromISO(activity[0].created_at);
deadlineWithThreshold = lastActivity.plus({ milliseconds: config.disqualification });
reminderWithThreshold = lastActivity.plus({ milliseconds: config.warning });
if (filteredActivity?.length) {
const lastActivity = filteredActivity[0].created_at ? DateTime.fromISO(filteredActivity[0].created_at) : deadline;
deadlineWithThreshold = lastActivity.plus({ milliseconds: disqualification });
reminderWithThreshold = lastActivity.plus({ milliseconds: warning });
}

return { deadlineWithThreshold, reminderWithThreshold, now };
Expand Down
47 changes: 28 additions & 19 deletions src/helpers/task-metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,38 +22,47 @@ export async function getTaskMetadata(
const assignmentRegex = /Ubiquity - Assignment - start -/gi;
const botAssignmentComments = botComments
.filter((o) => assignmentRegex.test(o?.body || ""))
.sort((a, b) => DateTime.fromISO(a.created_at).toMillis() - DateTime.fromISO(b.created_at).toMillis());
.sort((a, b) => DateTime.fromISO(b.created_at).toMillis() - DateTime.fromISO(a.created_at).toMillis());

// Has the bot previously reminded them?
const botFollowup = /<!-- Ubiquity - Followup - remindAssignees/gi;
const botFollowupComments = botComments
.filter((o) => botFollowup.test(o?.body || ""))
.sort((a, b) => DateTime.fromISO(a.created_at).toMillis() - DateTime.fromISO(b.created_at).toMillis());
.sort((a, b) => DateTime.fromISO(b.created_at).toMillis() - DateTime.fromISO(a.created_at).toMillis());

// `lastCheck` represents the last time the bot intervened in the issue, separate from the activity tracking of a user.
const lastCheckComment = botFollowupComments[0]?.created_at ? botFollowupComments[0] : botAssignmentComments[0];
let lastCheck = lastCheckComment?.created_at ? DateTime.fromISO(lastCheckComment.created_at) : null;

// if we don't have a lastCheck yet, use the assignment event
if (!lastCheck) {
logger.info("No last check found, using assignment event");
const assignmentEvents = await octokit.paginate(octokit.rest.issues.listEvents, {
owner: repo.owner.login,
repo: repo.name,
issue_number: issue.number,
});

const assignmentEvent = assignmentEvents.find((o) => o.event === "assigned");
if (assignmentEvent) {
lastCheck = DateTime.fromISO(assignmentEvent.created_at);
} else {
logger.error(`Failed to find last check for ${issue.html_url}`);
return false;
}
// incase their was self-assigning after the lastCheck
const assignmentEvents = await octokit.paginate(octokit.rest.issues.listEvents, {
owner: repo.owner.login,
repo: repo.name,
issue_number: issue.number,
});

const assignedEvents = assignmentEvents
.filter((o) => o.event === "assigned")
.sort((a, b) => DateTime.fromISO(b.created_at).toMillis() - DateTime.fromISO(a.created_at).toMillis());

const latestUserAssignment = assignedEvents.find((o) => o.actor?.type === "User");
const latestBotAssignment = assignedEvents.find((o) => o.actor?.type === "Bot");

let mostRecentAssignmentEvent = latestUserAssignment || latestBotAssignment;

if (latestUserAssignment && latestBotAssignment && DateTime.fromISO(latestUserAssignment.created_at) > DateTime.fromISO(latestBotAssignment.created_at)) {
mostRecentAssignmentEvent = latestUserAssignment;
} else {
mostRecentAssignmentEvent = latestBotAssignment;
}

if (mostRecentAssignmentEvent && (!lastCheck || DateTime.fromISO(mostRecentAssignmentEvent.created_at) > lastCheck)) {
lastCheck = DateTime.fromISO(mostRecentAssignmentEvent.created_at);
logger.debug(`Using assignment event`, { mostRecentAssignmentEvent });
}

if (!lastCheck) {
logger.error(`Failed to find last check for ${issue.html_url}`);
logger.error(`No last check found for ${issue.html_url}`);
return false;
}

Expand Down
11 changes: 6 additions & 5 deletions src/helpers/task-update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,17 @@ export async function updateTaskReminder(context: Context, repo: ListForOrg["dat
return false;
}

logger.info(`Last check was on ${lastCheck.toLocaleString(DateTime.DATETIME_MED)}`, {
now: now.toLocaleString(DateTime.DATETIME_MED),
reminder: reminderWithThreshold.toLocaleString(DateTime.DATETIME_MED),
deadline: deadlineWithThreshold.toLocaleString(DateTime.DATETIME_MED),
});

if (now >= deadlineWithThreshold) {
await unassignUserFromIssue(context, issue);
} else if (now >= reminderWithThreshold) {
await remindAssigneesForIssue(context, issue);
} else {
logger.info(`Nothing to do for ${issue.html_url}, still within due-time.`);
logger.info(`Last check was on ${lastCheck.toISO()}`, {
now: now.toLocaleString(DateTime.DATETIME_MED),
reminder: reminderWithThreshold.toLocaleString(DateTime.DATETIME_MED),
deadline: deadlineWithThreshold.toLocaleString(DateTime.DATETIME_MED),
});
}
}
2 changes: 1 addition & 1 deletion src/types/github-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ import { RestEndpointMethodTypes } from "@octokit/rest";
export type ListForOrg = RestEndpointMethodTypes["repos"]["listForOrg"]["response"];
export type ListIssueForRepo = RestEndpointMethodTypes["issues"]["listForRepo"]["response"]["data"][0];
export type ListCommentsForIssue = RestEndpointMethodTypes["issues"]["listComments"]["response"]["data"][0];
export type GitHubListEvents = RestEndpointMethodTypes["issues"]["listEvents"]["response"]["data"][0];
export type GitHubTimelineEvents = RestEndpointMethodTypes["issues"]["listEventsForTimeline"]["response"]["data"][0];
52 changes: 33 additions & 19 deletions src/types/plugin-inputs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,38 @@ function thresholdType(options?: StringOptions) {
});
}

export const userActivityWatcherSettingsSchema = T.Object({
/**
* Delay to send reminders. 0 means disabled. Any other value is counted in days, e.g. 1,5 days
*/
warning: thresholdType({ default: "3.5 days" }),
/**
* By default all repositories are watched. Use this option to opt-out from watching specific repositories
* within your organization. The value is an array of repository names.
*/
watch: T.Object({
optOut: T.Array(T.String()),
}),
/**
* Delay to unassign users. 0 means disabled. Any other value is counted in days, e.g. 7 days
*/
disqualification: thresholdType({
default: "7 days",
}),
});
const eventWhitelist = {
review_requested: "review_requested",
ready_for_review: "ready_for_review",
commented: "commented",
committed: "committed",
};

export const userActivityWatcherSettingsSchema = T.Object(
{
/**
* Delay to send reminders. 0 means disabled. Any other value is counted in days, e.g. 1,5 days
*/
warning: thresholdType({ default: "3.5 days" }),
/**
* By default all repositories are watched. Use this option to opt-out from watching specific repositories
* within your organization. The value is an array of repository names.
*/
watch: T.Object({
optOut: T.Array(T.String()),
}),
/**
* Delay to unassign users. 0 means disabled. Any other value is counted in days, e.g. 7 days
*/
disqualification: thresholdType({
default: "7 days",
}),
/**
* List of events to consider as valid activity on a task
*/
eventWhitelist: T.Array(T.String(), { default: Object.values(eventWhitelist) }),
},
{ default: {} }
);

export type UserActivityWatcherSettings = StaticDecode<typeof userActivityWatcherSettingsSchema>;
7 changes: 3 additions & 4 deletions tests/__mocks__/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,21 @@ export function createIssue(id: number, assignees: { login: string; id: number }
});
}

export function createEvent() {
export function createEvent(id: number, created_at = new Date(Date.now() - ONE_DAY).toISOString()) {
db.event.create({
id: 1,
id,
actor: {
id: 1,
type: "User",
login: "ubiquity",
name: null,
},
owner: "ubiquity",
repo: "test-repo",
issue_number: 1,
event: "assigned",
commit_id: null,
commit_url: "https://github.com/ubiquity/test-repo/commit/1",
created_at: new Date(Date.now() - ONE_DAY).toISOString(),
created_at,
assignee: {
login: "ubiquity",
},
Expand Down
7 changes: 7 additions & 0 deletions tests/__mocks__/mock-users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,16 @@ export default [
{
id: 1,
login: "ubiquity",
type: "User",
},
{
id: 2,
login: "bot",
type: "Bot",
},
{
id: 100000000,
login: "user2",
type: "User",
},
];
3 changes: 2 additions & 1 deletion tests/__mocks__/results/valid-configuration.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
"disqualification": "7 days",
"watch": {
"optOut": ["private-repo"]
}
},
"eventWhitelist": ["review_requested", "ready_for_review", "commented", "committed"]
}
Loading