Merge pull request #125 from koya0/development

fix: validate presence of price label in issue before checking requir…
ubiquity-os-marketplace · Jan 27, 2025 · 651d84c · 651d84c
2 parents c0459d2 + 7ef84db
commit 651d84c
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 30 deletions.
diff --git a/src/handlers/shared/start.ts b/src/handlers/shared/start.ts
@@ -9,7 +9,7 @@ import { getTransformedRole, getUserRoleAndTaskLimit } from "./get-user-task-lim
 import structuredMetadata from "./structured-metadata";
 import { assignTableComment } from "./table";
 
-async function checkRequirements(context: Context, issue: Context<"issue_comment.created">["payload"]["issue"], login: string) {
+async function checkRequirements(context: Context, issue: Context<"issue_comment.created">["payload"]["issue"], login: string): Promise<Error | null> {
   const {
     config: { requiredLabelsToStart },
     logger,
@@ -25,33 +25,35 @@ async function checkRequirements(context: Context, issue: Context<"issue_comment
 
     // Admins can start any task
     if (userRole === "admin") {
-      return;
+      return null;
     }
 
     if (!currentLabelConfiguration) {
       // If we didn't find the label in the allowed list, then the user cannot start this task.
-      throw logger.error(
-        `This task does not reflect a business priority at the moment. You may start tasks with one of the following labels: ${requiredLabelsToStart.map((label) => label.name).join(", ")}`,
-        {
-          requiredLabelsToStart,
-          issueLabels,
-          issue: issue.html_url,
-        }
-      );
+      const errorText = `This task does not reflect a business priority at the moment. You may start tasks with one of the following labels: ${requiredLabelsToStart.map((label) => label.name).join(", ")}`;
+      logger.error(errorText, {
+        requiredLabelsToStart,
+        issueLabels,
+        issue: issue.html_url,
+      });
+      return new Error(errorText);
     } else if (!currentLabelConfiguration.allowedRoles.includes(userRole)) {
       // If we found the label in the allowed list, but the user role does not match the allowed roles, then the user cannot start this task.
       const humanReadableRoles = [
         ...currentLabelConfiguration.allowedRoles.map((o) => (o === "collaborator" ? "a core team member" : `a ${o}`)),
         "an administrator",
       ].join(", or ");
-      throw logger.error(`You must be ${humanReadableRoles} to start this task`, {
+      const errorText = `You must be ${humanReadableRoles} to start this task`;
+      logger.error(errorText, {
         currentLabelConfiguration,
         issueLabels,
         issue: issue.html_url,
         userAssociation,
       });
+      return new Error(errorText);
     }
   }
+  return null;
 }
 
 export async function start(
@@ -67,7 +69,25 @@ export async function start(
     throw logger.error(`Skipping '/start' since there is no sender in the context.`);
   }
 
-  await checkRequirements(context, issue, sender.login);
+  const labels = issue.labels ?? [];
+  const priceLabel = labels.find((label: Label) => label.name.startsWith("Price: "));
+
+  const startErrors: Error[] = [];
+
+  if (!priceLabel) {
+    const errorText = "No price label is set to calculate the duration";
+    logger.error(errorText, { issueNumber: issue.number });
+    startErrors.push(new Error(errorText));
+  }
+
+  const checkRequirementsError = await checkRequirements(context, issue, sender.login);
+  if (checkRequirementsError) {
+    startErrors.push(checkRequirementsError);
+  }
+
+  if (startErrors.length) {
+    throw new AggregateError(startErrors);
+  }
 
   // is it a child issue?
   if (issue.body && isParentIssue(issue.body)) {
@@ -158,13 +178,6 @@ ${issues}
     return { content: error, status: HttpStatusCode.NOT_MODIFIED };
   }
 
-  const labels = issue.labels ?? [];
-  const priceLabel = labels.find((label: Label) => label.name.startsWith("Price: "));
-
-  if (!priceLabel) {
-    throw logger.error("No price label is set to calculate the duration", { issueNumber: issue.number });
-  }
-
   // Checks if non-collaborators can be assigned to the issue
   for (const label of labels) {
     if (label.description?.toLowerCase().includes("collaborator only")) {

diff --git a/tests/main.test.ts b/tests/main.test.ts
@@ -201,7 +201,14 @@ describe("User start/stop", () => {
 
     context.adapters = createAdapters(getSupabase(), context);
 
-    await expect(userStartStop(context)).rejects.toMatchObject({ logMessage: { raw: "No price label is set to calculate the duration" } });
+    try {
+      await userStartStop(context);
+    } catch (error) {
+      expect(error).toBeInstanceOf(AggregateError);
+      const aggregateError = error as AggregateError;
+      const errorMessages = aggregateError.errors.map((error) => error.message);
+      expect(errorMessages).toEqual(expect.arrayContaining(["No price label is set to calculate the duration"]));
+    }
   });
 
   test("User can't start an issue without a wallet address", async () => {
@@ -316,11 +323,18 @@ describe("User start/stop", () => {
 
     context.adapters = createAdapters(getSupabase(), context);
 
-    await expect(userStartStop(context)).rejects.toMatchObject({
-      logMessage: {
-        raw: "This task does not reflect a business priority at the moment. You may start tasks with one of the following labels: Priority: 3 (High), Priority: 4 (Urgent), Priority: 5 (Emergency)",
-      },
-    });
+    try {
+      await userStartStop(context);
+    } catch (error) {
+      expect(error).toBeInstanceOf(AggregateError);
+      const aggregateError = error as AggregateError;
+      const errorMessages = aggregateError.errors.map((error) => error.message);
+      expect(errorMessages).toEqual(
+        expect.arrayContaining([
+          "This task does not reflect a business priority at the moment. You may start tasks with one of the following labels: Priority: 3 (High), Priority: 4 (Urgent), Priority: 5 (Emergency)",
+        ])
+      );
+    }
   });
 
   test("Should not allow a user to start if the user role is not listed", async () => {
@@ -337,11 +351,14 @@ describe("User start/stop", () => {
 
     context.adapters = createAdapters(getSupabase(), context);
 
-    await expect(userStartStop(context)).rejects.toMatchObject({
-      logMessage: {
-        raw: "You must be a core team member, or an administrator to start this task",
-      },
-    });
+    try {
+      await userStartStop(context);
+    } catch (error) {
+      expect(error).toBeInstanceOf(AggregateError);
+      const aggregateError = error as AggregateError;
+      const errorMessages = aggregateError.errors.map((error) => error.message);
+      expect(errorMessages).toEqual(expect.arrayContaining(["You must be a core team member, or an administrator to start this task"]));
+    }
   });
 });