Merge pull request #1502 from argos-ci/clean-build-reviews

clean build reviews

Procfile

@@ -2,4 +2,3 @@ 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/src/build/strategy/strategies/ci/query.e2e.test.ts

@@ -81,12 +81,17 @@ describe("#getBaseBucketForBuildAndCommit", () => {
       });
     });
 
-    describe("if the associated build is a type check with unapproved diff", () => {
+    describe("if the associated build is a type check with a rejected review", () => {
       beforeEach(async () => {
         await baseBucketBuild.$query().patch({ type: "check" });
-        await factory.ScreenshotDiff.create({
+        await factory.BuildReview.create({
           buildId: baseBucketBuild.id,
-          validationStatus: "rejected",
+          state: "approved",
+        });
+        // The last one wins, so it's a rejected build in this case
+        await factory.BuildReview.create({
+          buildId: baseBucketBuild.id,
+          state: "rejected",
         });
       });
 
@@ -100,12 +105,12 @@ describe("#getBaseBucketForBuildAndCommit", () => {
       });
     });
 
-    describe("if the associated build is a type check with approved diff", () => {
+    describe("if the associated build is a type check with approved review", () => {
       beforeEach(async () => {
         await baseBucketBuild.$query().patch({ type: "check" });
-        await factory.ScreenshotDiff.create({
+        await factory.BuildReview.create({
           buildId: baseBucketBuild.id,
-          validationStatus: "accepted",
+          state: "approved",
         });
       });
 

apps/backend/src/build/strategy/strategies/ci/query.ts

@@ -1,8 +1,4 @@
-import {
-  Build,
-  ScreenshotBucket,
-  ScreenshotDiff,
-} from "@/database/models/index.js";
+import { Build, ScreenshotBucket } from "@/database/models/index.js";
 
 /**
  * Get the base bucket for a build and a commit.
@@ -65,13 +61,10 @@ export function queryBaseBucket(
         .where("jobStatus", "complete")
         .whereNot("id", build.id)
         .where((qb) => {
-          // Reference build or check build with accepted diffs
+          // Reference build or check build with approved review
           qb.where("type", "reference").orWhere((qb) => {
             qb.where("type", "check").whereExists(
-              ScreenshotDiff.query()
-                .select(1)
-                .whereRaw('"buildId" = builds.id')
-                .where("validationStatus", "accepted"),
+              Build.submittedReviewQuery().where("state", "approved"),
             );
           });
         }),

apps/backend/src/build/strategy/strategies/monitoring/index.e2e.test.ts

@@ -65,15 +65,15 @@ describe("MonitoringStrategy.getBaseScreenshotBucket", () => {
         approvedWithOtherName,
         approvedWithOtherMode,
       ].map((build) =>
-        factory.ScreenshotDiff.create({
+        factory.BuildReview.create({
           buildId: build!.id,
-          validationStatus: "accepted",
+          state: "approved",
         }),
       ),
     );
-    await factory.ScreenshotDiff.create({
+    await factory.BuildReview.create({
       buildId: nonApproved!.id,
-      validationStatus: "rejected",
+      state: "rejected",
     });
     sourceBuild = source!;
     matchedBuild = lastApproved!;

apps/backend/src/build/strategy/strategies/monitoring/index.ts

@@ -1,6 +1,6 @@
 import { invariant } from "@argos/util/invariant";
 
-import { Build, ScreenshotDiff } from "@/database/models";
+import { Build } from "@/database/models";
 
 import { BuildStrategy, GetBaseResult } from "../../types";
 
@@ -12,12 +12,7 @@ async function getBase(build: Build): GetBaseResult {
     .where("mode", "monitoring")
     .where("jobStatus", "complete")
     .whereNot("id", build.id)
-    .whereExists(
-      ScreenshotDiff.query()
-        .select(1)
-        .whereRaw('"buildId" = builds.id')
-        .where("validationStatus", "accepted"),
-    )
+    .whereExists(Build.submittedReviewQuery().where("state", "approved"))
     .orderBy("id", "desc")
     .first();
 

apps/backend/src/database/models/Build.e2e.test.ts

@@ -305,26 +305,54 @@ describe("models/Build", () => {
       expect(reviewStatuses).toEqual([null, null]);
     });
 
-    it("should return 'accepted' when all diff are accepted", async () => {
+    it("should return 'accepted' when the last review is of type 'approved'", async () => {
       const build = await factory.Build.create({
         conclusion: "changes-detected",
       });
-      await factory.ScreenshotDiff.createMany(2, [
-        { buildId: build.id, score: 0.8, validationStatus: "accepted" },
-        { buildId: build.id, score: 0.4, validationStatus: "accepted" },
-      ]);
+      await factory.BuildReview.create({
+        buildId: build.id,
+        state: "rejected",
+      });
+      await factory.BuildReview.create({
+        buildId: build.id,
+        state: "approved",
+      });
       const reviewStatuses = await Build.getReviewStatuses([build]);
       expect(reviewStatuses).toEqual(["accepted"]);
     });
 
-    it("should return 'rejected' when one diff is rejected", async () => {
+    it("should return 'rejected' when the last review is of type 'rejected'", async () => {
       const build = await factory.Build.create({
         conclusion: "changes-detected",
       });
-      await factory.ScreenshotDiff.createMany(2, [
-        { buildId: build.id, score: 0.8, validationStatus: "accepted" },
-        { buildId: build.id, score: 0.4, validationStatus: "rejected" },
-      ]);
+      await factory.BuildReview.create({
+        buildId: build.id,
+        state: "approved",
+      });
+      await factory.BuildReview.create({
+        buildId: build.id,
+        state: "rejected",
+      });
+      const reviewStatuses = await Build.getReviewStatuses([build]);
+      expect(reviewStatuses).toEqual(["rejected"]);
+    });
+
+    it("should should ignore reviews of type pending", async () => {
+      const build = await factory.Build.create({
+        conclusion: "changes-detected",
+      });
+      await factory.BuildReview.create({
+        buildId: build.id,
+        state: "approved",
+      });
+      await factory.BuildReview.create({
+        buildId: build.id,
+        state: "rejected",
+      });
+      await factory.BuildReview.create({
+        buildId: build.id,
+        state: "pending",
+      });
       const reviewStatuses = await Build.getReviewStatuses([build]);
       expect(reviewStatuses).toEqual(["rejected"]);
     });
@@ -333,10 +361,6 @@ describe("models/Build", () => {
       const build = await factory.Build.create({
         conclusion: "changes-detected",
       });
-      await factory.ScreenshotDiff.createMany(2, [
-        { buildId: build.id, score: 0.8, validationStatus: "accepted" },
-        { buildId: build.id, score: 0.4, validationStatus: "unknown" },
-      ]);
       const reviewStatuses = await Build.getReviewStatuses([build]);
       expect(reviewStatuses).toEqual([null]);
     });

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

@@ -23,6 +23,7 @@ import {
   mergeSchemas,
   timestampsSchema,
 } from "../util/schemas.js";
+import { BuildReview } from "./BuildReview.js";
 import { BuildShard } from "./BuildShard.js";
 import { GithubPullRequest } from "./GithubPullRequest.js";
 import { Project } from "./Project.js";
@@ -442,31 +443,43 @@ export class Build extends Model {
       .filter((build) => build.conclusion === "changes-detected")
       .map((build) => build.id);
 
-    const screenshotDiffs = diffDetectedBuildIds.length
-      ? await ScreenshotDiff.query()
-          .select("buildId", "validationStatus")
+    const reviews = diffDetectedBuildIds.length
+      ? await BuildReview.query()
+          .select("buildId", "state")
           .whereIn("buildId", diffDetectedBuildIds)
-          .groupBy("buildId", "validationStatus")
+          .whereIn("state", ["approved", "rejected"]).andWhereRaw(`"id" IN (
+            SELECT DISTINCT ON ("buildId")
+              "id"
+            FROM "build_reviews"
+            WHERE "buildId" = "build_reviews"."buildId"
+              AND "state" IN ('approved', 'rejected')
+            ORDER BY "buildId", "createdAt" DESC
+          )`)
       : [];
 
     return builds.map((build) => {
       if (build.conclusion !== "changes-detected") {
         return null;
       }
 
-      const status = screenshotDiffs
-        .filter((diff) => diff.buildId === build.id)
-        .map(({ validationStatus }) => validationStatus);
+      const review = reviews.find((review) => review.buildId === build.id);
 
-      if (status.length === 1 && status[0] === "accepted") {
-        return "accepted";
+      if (!review) {
+        return null;
       }
 
-      if (status.includes("rejected")) {
-        return "rejected";
+      switch (review.state) {
+        case "approved":
+          return "accepted";
+        case "rejected":
+          return "rejected";
+        case "pending":
+          throw new Error(
+            "Unexpected review state, should be filtered in query",
+          );
+        default:
+          assertNever(review.state);
       }
-
-      return null;
     });
   }
 
@@ -509,4 +522,21 @@ export class Build extends Model {
     });
     return aggregateStatuses;
   }
+
+  /**
+   * To be used in a `Build.query().whereExists` clause.
+   */
+  static submittedReviewQuery() {
+    return BuildReview.query()
+      .select(1)
+      .whereRaw('build_reviews."buildId" = builds.id')
+      .whereIn("id", (qb) => {
+        qb.select("id")
+          .from("build_reviews")
+          .whereRaw('build_reviews."buildId" = builds.id')
+          .whereIn("state", ["approved", "rejected"])
+          .orderBy("id", "desc")
+          .limit(1);
+      });
+  }
 }

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

@@ -18,7 +18,7 @@ export class BuildReview extends Model {
   });
 
   buildId!: string;
-  userId!: string;
+  userId!: string | null;
   state!: "pending" | "approved" | "rejected";
 
   static override get relationMappings(): RelationMappings {