Fix issues

src/helpers/get-assignee-activity.ts

@@ -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,
@@ -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,
@@ -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 commitCommiter = "committer" in event ? event.committer : null;
+
+      if (commitAuthor || commitCommiter) {
+        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();
+  });
 }

src/helpers/task-deadline.ts

@@ -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) || [];
 
@@ -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 };

src/helpers/task-update.ts

@@ -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),
-    });
   }
 }

src/types/github-types.ts

@@ -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];

src/types/plugin-inputs.ts

@@ -31,6 +31,13 @@ function thresholdType(options?: StringOptions) {
     });
 }
 
+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
@@ -49,6 +56,10 @@ export const userActivityWatcherSettingsSchema = T.Object({
   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) })),
 });
 
 export type UserActivityWatcherSettings = StaticDecode<typeof userActivityWatcherSettingsSchema>;

tests/__mocks__/results/valid-configuration.json

@@ -3,5 +3,6 @@
   "disqualification": "7 days",
   "watch": {
     "optOut": ["private-repo"]
-  }
+  },
+  "eventWhitelist": ["review_requested", "ready_for_review", "commented", "committed"]
 }

tests/main.test.ts

@@ -33,14 +33,20 @@ describe("User start/stop", () => {
 
   it("Should parse thresholds", async () => {
     const settings = Value.Decode(userActivityWatcherSettingsSchema, Value.Default(userActivityWatcherSettingsSchema, cfg));
-    expect(settings).toEqual({ warning: 302400000, disqualification: 604800000, watch: { optOut: [STRINGS.PRIVATE_REPO_NAME] } });
+    expect(settings).toEqual({
+      warning: 302400000,
+      disqualification: 604800000,
+      watch: { optOut: [STRINGS.PRIVATE_REPO_NAME] },
+      eventWhitelist: ["review_requested", "ready_for_review", "commented", "committed"],
+    });
     expect(() =>
       Value.Decode(
         userActivityWatcherSettingsSchema,
         Value.Default(userActivityWatcherSettingsSchema, {
           warning: "12 foobars",
           disqualification: "2 days",
           watch: { optOut: [STRINGS.PRIVATE_REPO_NAME] },
+          eventWhitelist: ["review_requested", "ready_for_review", "commented", "committed"],
         })
       )
     ).toThrow(TransformDecodeError);
@@ -59,13 +65,13 @@ describe("User start/stop", () => {
     // The logs skipped just contain the timestamp infos: "last check was on...."
 
     expect(infoSpy).toHaveBeenNthCalledWith(1, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(2, `Nothing to do for ${getIssueHtmlUrl(1)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(3, `Nothing to do for ${getIssueHtmlUrl(1)}, still within due-time.`);
     expect(infoSpy).toHaveBeenNthCalledWith(4, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(5, `Nothing to do for ${getIssueHtmlUrl(2)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(6, `Nothing to do for ${getIssueHtmlUrl(2)}, still within due-time.`);
     expect(infoSpy).toHaveBeenNthCalledWith(7, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(8, `Nothing to do for ${getIssueHtmlUrl(3)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(9, `Nothing to do for ${getIssueHtmlUrl(3)}, still within due-time.`);
     expect(infoSpy).toHaveBeenNthCalledWith(10, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(11, `Nothing to do for ${getIssueHtmlUrl(4)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(12, `Nothing to do for ${getIssueHtmlUrl(4)}, still within due-time.`);
     expect(infoSpy).not.toHaveBeenNthCalledWith(14, `Nothing to do for https://github.com/ubiquity/private-repo/issues/5, still within due-time.`);
   });
 
@@ -75,15 +81,15 @@ describe("User start/stop", () => {
     await runPlugin(context);
 
     expect(infoSpy).toHaveBeenNthCalledWith(1, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(2, `Nothing to do for ${getIssueHtmlUrl(1)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(3, `Nothing to do for ${getIssueHtmlUrl(1)}, still within due-time.`);
     expect(infoSpy).toHaveBeenNthCalledWith(4, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(5, `Nothing to do for ${getIssueHtmlUrl(2)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(6, `Nothing to do for ${getIssueHtmlUrl(2)}, still within due-time.`);
     expect(infoSpy).toHaveBeenNthCalledWith(7, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(8, `Nothing to do for ${getIssueHtmlUrl(3)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(9, `Nothing to do for ${getIssueHtmlUrl(3)}, still within due-time.`);
     expect(infoSpy).toHaveBeenNthCalledWith(10, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(11, `Nothing to do for ${getIssueHtmlUrl(4)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(12, `Nothing to do for ${getIssueHtmlUrl(4)}, still within due-time.`);
     expect(infoSpy).toHaveBeenNthCalledWith(13, STRINGS.USING_ASSIGNMENT_EVENT);
-    expect(infoSpy).toHaveBeenNthCalledWith(14, `Nothing to do for https://github.com/ubiquity/private-repo/issues/5, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(15, `Nothing to do for https://github.com/ubiquity/private-repo/issues/5, still within due-time.`);
   });
 
   it("Should eject the user after the disqualification period", async () => {
@@ -98,9 +104,9 @@ describe("User start/stop", () => {
 
     await runPlugin(context);
 
-    expect(infoSpy).toHaveBeenNthCalledWith(1, `Nothing to do for ${getIssueHtmlUrl(1)}, still within due-time.`);
-    expect(infoSpy).toHaveBeenNthCalledWith(3, `Passed the deadline on ${getIssueHtmlUrl(2)} and no activity is detected, removing assignees.`);
-    expect(infoSpy).toHaveBeenNthCalledWith(4, `Passed the deadline on ${getIssueHtmlUrl(3)} and no activity is detected, removing assignees.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(2, `Passed the deadline on ${getIssueHtmlUrl(1)} and no activity is detected, removing assignees.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(4, `Passed the deadline on ${getIssueHtmlUrl(2)} and no activity is detected, removing assignees.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(6, `Passed the deadline on ${getIssueHtmlUrl(3)} and no activity is detected, removing assignees.`);
 
     const updatedIssue = db.issue.findFirst({ where: { id: { equals: 4 } } });
     expect(updatedIssue?.assignees).toEqual([]);
@@ -138,9 +144,9 @@ describe("User start/stop", () => {
 
     await runPlugin(context);
 
-    expect(infoSpy).toHaveBeenNthCalledWith(1, `Nothing to do for ${getIssueHtmlUrl(1)}, still within due-time.`);
-    expect(infoSpy).toHaveBeenNthCalledWith(3, `Nothing to do for ${getIssueHtmlUrl(2)}, still within due-time.`);
-    expect(infoSpy).toHaveBeenNthCalledWith(5, `Nothing to do for ${getIssueHtmlUrl(3)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(2, `Nothing to do for ${getIssueHtmlUrl(1)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(4, `Nothing to do for ${getIssueHtmlUrl(2)}, still within due-time.`);
+    expect(infoSpy).toHaveBeenNthCalledWith(6, `Nothing to do for ${getIssueHtmlUrl(3)}, still within due-time.`);
 
     const updatedIssue = db.issue.findFirst({ where: { id: { equals: 4 } } });
     expect(updatedIssue?.assignees).toEqual([{ login: STRINGS.USER, id: 2 }]);
@@ -222,6 +228,7 @@ function createContext(issueId: number, senderId: number, optOut = [STRINGS.PRIV
       disqualification: ONE_DAY * 7,
       warning: ONE_DAY * 3.5,
       watch: { optOut },
+      eventWhitelist: ["review_requested", "ready_for_review", "commented", "committed"],
     },
     octokit: new octokit.Octokit(),
     eventName: "issue_comment.created",