Merge pull request #1501 from argos-ci/build-reviews

feat: store build reviews in a dedicated table

Procfile

@@ -2,3 +2,4 @@ release: ./scripts/release.sh
 web: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/web.js
 buildAndSynchronize: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/build-and-synchronize.js
 screenshotDiff: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/screenshot-diff.js
+setBuildReviews: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/create-build-reviews.js

apps/backend/db/migrations/20250102150800_build-review.js

@@ -0,0 +1,22 @@
+/**
+ * @param {import('knex').Knex} knex
+ */
+export const up = async (knex) => {
+  await knex.schema.createTable("build_reviews", (table) => {
+    table.bigIncrements("id").primary();
+    table.dateTime("createdAt").notNullable();
+    table.dateTime("updatedAt").notNullable();
+    table.bigInteger("userId").index();
+    table.foreign("userId").references("users.id");
+    table.bigInteger("buildId").index().notNullable();
+    table.foreign("buildId").references("builds.id");
+    table.enum("state", ["pending", "approved", "rejected"]).notNullable();
+  });
+};
+
+/**
+ * @param {import('knex').Knex} knex
+ */
+export const down = async (knex) => {
+  await knex.schema.dropTable("build_reviews");
+};

apps/backend/db/structure.sql

@@ -157,6 +157,44 @@ ALTER TABLE public.build_notifications_id_seq OWNER TO postgres;
 ALTER SEQUENCE public.build_notifications_id_seq OWNED BY public.build_notifications.id;
 
 
+--
+-- Name: build_reviews; Type: TABLE; Schema: public; Owner: postgres
+--
+
+CREATE TABLE public.build_reviews (
+    id bigint NOT NULL,
+    "createdAt" timestamp with time zone NOT NULL,
+    "updatedAt" timestamp with time zone NOT NULL,
+    "userId" bigint,
+    "buildId" bigint NOT NULL,
+    state text NOT NULL,
+    CONSTRAINT build_reviews_state_check CHECK ((state = ANY (ARRAY['pending'::text, 'approved'::text, 'rejected'::text])))
+);
+
+
+ALTER TABLE public.build_reviews OWNER TO postgres;
+
+--
+-- Name: build_reviews_id_seq; Type: SEQUENCE; Schema: public; Owner: postgres
+--
+
+CREATE SEQUENCE public.build_reviews_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+
+ALTER TABLE public.build_reviews_id_seq OWNER TO postgres;
+
+--
+-- Name: build_reviews_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: postgres
+--
+
+ALTER SEQUENCE public.build_reviews_id_seq OWNED BY public.build_reviews.id;
+
+
 --
 -- Name: build_shards; Type: TABLE; Schema: public; Owner: postgres
 --
@@ -1376,6 +1414,13 @@ ALTER TABLE ONLY public.accounts ALTER COLUMN id SET DEFAULT nextval('public.acc
 ALTER TABLE ONLY public.build_notifications ALTER COLUMN id SET DEFAULT nextval('public.build_notifications_id_seq'::regclass);
 
 
+--
+-- Name: build_reviews id; Type: DEFAULT; Schema: public; Owner: postgres
+--
+
+ALTER TABLE ONLY public.build_reviews ALTER COLUMN id SET DEFAULT nextval('public.build_reviews_id_seq'::regclass);
+
+
 --
 -- Name: build_shards id; Type: DEFAULT; Schema: public; Owner: postgres
 --
@@ -1619,6 +1664,14 @@ ALTER TABLE ONLY public.build_notifications
     ADD CONSTRAINT build_notifications_pkey PRIMARY KEY (id);
 
 
+--
+-- Name: build_reviews build_reviews_pkey; Type: CONSTRAINT; Schema: public; Owner: postgres
+--
+
+ALTER TABLE ONLY public.build_reviews
+    ADD CONSTRAINT build_reviews_pkey PRIMARY KEY (id);
+
+
 --
 -- Name: build_shards build_shards_pkey; Type: CONSTRAINT; Schema: public; Owner: postgres
 --
@@ -1989,6 +2042,20 @@ CREATE INDEX accounts_userid_index ON public.accounts USING btree ("userId");
 CREATE INDEX build_notifications_buildid_index ON public.build_notifications USING btree ("buildId");
 
 
+--
+-- Name: build_reviews_buildid_index; Type: INDEX; Schema: public; Owner: postgres
+--
+
+CREATE INDEX build_reviews_buildid_index ON public.build_reviews USING btree ("buildId");
+
+
+--
+-- Name: build_reviews_userid_index; Type: INDEX; Schema: public; Owner: postgres
+--
+
+CREATE INDEX build_reviews_userid_index ON public.build_reviews USING btree ("userId");
+
+
 --
 -- Name: build_shards_buildid_index; Type: INDEX; Schema: public; Owner: postgres
 --
@@ -2451,6 +2518,22 @@ ALTER TABLE ONLY public.build_notifications
     ADD CONSTRAINT build_notifications_buildid_foreign FOREIGN KEY ("buildId") REFERENCES public.builds(id);
 
 
+--
+-- Name: build_reviews build_reviews_buildid_foreign; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+--
+
+ALTER TABLE ONLY public.build_reviews
+    ADD CONSTRAINT build_reviews_buildid_foreign FOREIGN KEY ("buildId") REFERENCES public.builds(id);
+
+
+--
+-- Name: build_reviews build_reviews_userid_foreign; Type: FK CONSTRAINT; Schema: public; Owner: postgres
+--
+
+ALTER TABLE ONLY public.build_reviews
+    ADD CONSTRAINT build_reviews_userid_foreign FOREIGN KEY ("userId") REFERENCES public.users(id);
+
+
 --
 -- Name: build_shards build_shards_buildid_foreign; Type: FK CONSTRAINT; Schema: public; Owner: postgres
 --
@@ -2923,4 +3006,5 @@ INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('2024120
 INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241201210158_google-account.js', 1, NOW());
 INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241201222408_gitlab-last-loggedat.js', 1, NOW());
 INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241215091232_project-default-role.js', 1, NOW());
-INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241231154644_build-status.js', 1, NOW());
+INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241231154644_build-status.js', 1, NOW());
+INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20250102150800_build-review.js', 1, NOW());

apps/backend/src/build/bin/queue-build-reviews.ts

@@ -0,0 +1,40 @@
+#!/usr/bin/env node
+import { callbackify } from "node:util";
+
+import { Build } from "@/database/models/index.js";
+import logger from "@/logger/index.js";
+
+import { buildReviewJob } from "../build-review-job.js";
+
+const main = callbackify(async () => {
+  const batch = 2000;
+  const totalCount = await Build.query()
+    .where("conclusion", "changes-detected")
+    .resultSize();
+
+  let total = 0;
+  for (let offset = 0; offset < totalCount; offset += batch) {
+    const nodes = await Build.query()
+      .select("id")
+      .where("conclusion", "changes-detected")
+      .limit(batch)
+      .offset(offset)
+      .orderBy("id", "desc");
+
+    const ids = nodes.map((node) => node.id);
+    const percentage = Math.round((total / totalCount) * 100);
+    logger.info(
+      `Processing ${total}/${totalCount} (${percentage}%) - Pushing ${ids.length} builds in queue`,
+    );
+    await buildReviewJob.push(...ids);
+    total += nodes.length;
+  }
+
+  logger.info(`${total} builds pushed in queue (100% complete)`);
+});
+
+main((err) => {
+  if (err) {
+    throw err;
+  }
+});

apps/backend/src/build/build-review-job.ts

@@ -0,0 +1,55 @@
+import { assertNever } from "@argos/util/assertNever";
+import { invariant } from "@argos/util/invariant";
+
+import { Build } from "@/database/models/Build.js";
+import { BuildReview } from "@/database/models/BuildReview.js";
+import { createJob } from "@/job-core/index.js";
+import logger from "@/logger/index.js";
+
+export const buildReviewJob = createJob("build-reviews", {
+  complete: () => {},
+  error: (value, error) => {
+    console.error("Error while processing build", value, error);
+  },
+  perform: async (buildId: string) => {
+    logger.info(`[${buildId}] Processing for review`);
+    const build = await Build.query().findById(buildId).throwIfNotFound();
+    const [[status], review] = await Promise.all([
+      Build.getReviewStatuses([build]),
+      BuildReview.query().select("id").where("buildId", build.id).first(),
+    ]);
+    if (review) {
+      logger.info(`[${buildId}] Review already exists`);
+      return;
+    }
+    invariant(status !== undefined, "Status should be defined");
+    if (status) {
+      logger.info(`[${buildId}] Review created`);
+      const state = (() => {
+        switch (status) {
+          case "accepted":
+            return "approved" as const;
+          case "rejected":
+            return "rejected" as const;
+          default:
+            assertNever(status);
+        }
+      })();
+
+      // Simulate 10 minutes after build creation
+      const date = new Date(
+        new Date(build.createdAt).getTime() + 10 * 60 * 1000,
+      ).toISOString();
+
+      await BuildReview.query().insert({
+        buildId: build.id,
+        createdAt: date,
+        updatedAt: date,
+        state,
+      });
+      logger.info(`[${buildId}] Review created ${state}`);
+    } else {
+      logger.info(`[${buildId}] No review needed`);
+    }
+  },
+});

apps/backend/src/database/models/BuildReview.ts

@@ -0,0 +1,47 @@
+import type { RelationMappings } from "objection";
+
+import { Model } from "../util/model.js";
+import { mergeSchemas, timestampsSchema } from "../util/schemas.js";
+import { Build } from "./Build.js";
+import { User } from "./User.js";
+
+export class BuildReview extends Model {
+  static override tableName = "build_reviews";
+
+  static override jsonSchema = mergeSchemas(timestampsSchema, {
+    required: ["buildId", "state"],
+    properties: {
+      buildId: { type: "string" },
+      userId: { type: ["string", "null"] },
+      state: { type: "string", enum: ["pending", "approved", "rejected"] },
+    },
+  });
+
+  buildId!: string;
+  userId!: string;
+  state!: "pending" | "approved" | "rejected";
+
+  static override get relationMappings(): RelationMappings {
+    return {
+      build: {
+        relation: Model.BelongsToOneRelation,
+        modelClass: Build,
+        join: {
+          from: "build_reviews.buildId",
+          to: "builds.id",
+        },
+      },
+      user: {
+        relation: Model.BelongsToOneRelation,
+        modelClass: User,
+        join: {
+          from: "build_reviews.userId",
+          to: "users.id",
+        },
+      },
+    };
+  }
+
+  build?: Build;
+  user?: User;
+}

apps/backend/src/database/models/index.ts

@@ -2,6 +2,7 @@ export * from "./Account.js";
 export * from "./Build.js";
 export * from "./BuildShard.js";
 export * from "./BuildNotification.js";
+export * from "./BuildReview.js";
 export * from "./Capture.js";
 export * from "./Crawl.js";
 export * from "./File.js";

apps/backend/src/database/util/model.ts

@@ -15,8 +15,9 @@ export class Model extends ObjectionModel {
     if (!this.createdAt) {
       this.createdAt = new Date().toISOString();
     }
-
-    this.updatedAt = new Date().toISOString();
+    if (!this.updatedAt) {
+      this.updatedAt = new Date().toISOString();
+    }
   }
 
   override $beforeUpdate(_opt: ModelOptions, _queryContext: QueryContext) {

apps/backend/src/graphql/definitions/Build.ts

@@ -3,15 +3,16 @@ import { invariant } from "@argos/util/invariant";
 import gqlTag from "graphql-tag";
 
 import { pushBuildNotification } from "@/build-notification/index.js";
-import { Build, ScreenshotDiff } from "@/database/models/index.js";
+import { Build, BuildReview, ScreenshotDiff } from "@/database/models/index.js";
+import { transaction } from "@/database/transaction.js";
 
 import {
   IBaseBranchResolution,
   IBuildStatus,
   type IResolvers,
 } from "../__generated__/resolver-types.js";
 import type { Context } from "../context.js";
-import { forbidden, notFound, unauthenticated } from "../util.js";
+import { badUserInput, forbidden, notFound, unauthenticated } from "../util.js";
 import { paginateResult } from "./PageInfo.js";
 
 const { gql } = gqlTag;
@@ -283,11 +284,17 @@ export const resolvers: IResolvers = {
   },
   Mutation: {
     setValidationStatus: async (_root, args, ctx) => {
-      if (!ctx.auth) {
+      const { auth } = ctx;
+      if (!auth) {
         throw unauthenticated();
       }
 
       const { buildId, validationStatus } = args;
+
+      if (validationStatus !== "accepted" && validationStatus !== "rejected") {
+        throw badUserInput("validationStatus must be 'accepted' or 'rejected'");
+      }
+
       const build = await Build.query()
         .findById(buildId)
         .withGraphFetched("project.account");
@@ -298,15 +305,24 @@ export const resolvers: IResolvers = {
 
       invariant(build.project?.account);
 
-      const permissions = await build.project.$getPermissions(ctx.auth.user);
+      const permissions = await build.project.$getPermissions(auth.user);
 
       if (!permissions.includes("review")) {
         throw forbidden("You cannot approve or reject this build");
       }
 
-      await ScreenshotDiff.query()
-        .where({ buildId })
-        .patch({ validationStatus });
+      await transaction(async (trx) => {
+        await Promise.all([
+          ScreenshotDiff.query(trx)
+            .where({ buildId })
+            .patch({ validationStatus }),
+          BuildReview.query(trx).insert({
+            buildId,
+            userId: auth.user.id,
+            state: validationStatus === "accepted" ? "approved" : "rejected",
+          }),
+        ]);
+      });
 
       // That might be better suited into a $afterUpdate hook.
       switch (validationStatus) {

apps/backend/src/processes/proc/create-build-reviews.ts

@@ -0,0 +1,6 @@
+import "../setup.js";
+
+import { buildReviewJob } from "@/build/build-review-job.js";
+import { createJobWorker } from "@/job-core/index.js";
+
+createJobWorker(buildReviewJob);