Merge pull request #1503 from argos-ci/remove-validation-status

refactor: remove `validationStatus` from screenshot_diffs table
argos-ci · Jan 3, 2025 · 606c276 · 606c276
2 parents 1ecf42d + c6237db
commit 606c276
Show file tree

Hide file tree

Showing 11 changed files with 24 additions and 42 deletions.
diff --git a/apps/backend/db/migrations/20250103090503_remove-validation-status-column.js b/apps/backend/db/migrations/20250103090503_remove-validation-status-column.js
@@ -0,0 +1,17 @@
+/**
+ * @param {import('knex').Knex} knex
+ */
+export const up = async (knex) => {
+  await knex.schema.alterTable("screenshot_diffs", async (table) => {
+    table.dropColumn("validationStatus");
+  });
+};
+
+/**
+ * @param {import('knex').Knex} knex
+ */
+export const down = async (knex) => {
+  await knex.schema.alterTable("screenshot_diffs", async (table) => {
+    table.string("validationStatus").notNullable();
+  });
+};
diff --git a/apps/backend/db/structure.sql b/apps/backend/db/structure.sql
@@ -1065,7 +1065,6 @@ CREATE TABLE public.screenshot_diffs (
     "compareScreenshotId" bigint,
     score numeric(10,5),
     "jobStatus" public.job_status,
-    "validationStatus" character varying(255) NOT NULL,
     "createdAt" timestamp with time zone NOT NULL,
     "updatedAt" timestamp with time zone NOT NULL,
     "s3Id" character varying(255),
@@ -3007,4 +3006,5 @@ INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('2024120
 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 ('20250102150800_build-review.js', 1, NOW());
+INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20250102150800_build-review.js', 1, NOW());
+INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20250103090503_remove-validation-status-column.js', 1, NOW());
diff --git a/apps/backend/src/build/createBuildDiffs.e2e.test.ts b/apps/backend/src/build/createBuildDiffs.e2e.test.ts
@@ -158,39 +158,34 @@ describe("#createBuildDiffs", () => {
         baseScreenshotId: null,
         compareScreenshotId: newScreenshot!.id,
         jobStatus: "complete",
-        validationStatus: "unknown",
         stabilityScore: null,
       });
       expect(addDiffWithoutFile).toMatchObject({
         buildId: build.id,
         baseScreenshotId: null,
         compareScreenshotId: newScreenshotWithoutFile!.id,
         jobStatus: "pending",
-        validationStatus: "unknown",
         stabilityScore: null,
       });
       expect(updatedDiff).toMatchObject({
         buildId: build.id,
         baseScreenshotId: classicDiffBaseScreenshot!.id,
         compareScreenshotId: classicDiffCompareScreenshot!.id,
         jobStatus: "pending",
-        validationStatus: "unknown",
         stabilityScore: null,
       });
       expect(removedDiff).toMatchObject({
         buildId: build.id,
         baseScreenshotId: removedScreenshot!.id,
         compareScreenshotId: null,
         jobStatus: "complete",
-        validationStatus: "unknown",
         score: null,
       });
       expect(noFileBaseScreenshotDiff).toMatchObject({
         buildId: build.id,
         baseScreenshotId: noFileBaseScreenshotBase!.id,
         compareScreenshotId: noFileBaseScreenshotCompare!.id,
         jobStatus: "pending",
-        validationStatus: "unknown",
         score: null,
         stabilityScore: null,
       });
@@ -199,7 +194,6 @@ describe("#createBuildDiffs", () => {
         baseScreenshotId: noFileCompareScreenshotBase!.id,
         compareScreenshotId: noFileCompareScreenshotCompare!.id,
         jobStatus: "pending",
-        validationStatus: "unknown",
         score: null,
         stabilityScore: null,
       });
@@ -208,7 +202,6 @@ describe("#createBuildDiffs", () => {
         baseScreenshotId: sameFileScreenshotBase!.id,
         compareScreenshotId: sameFileScreenshotCompare!.id,
         jobStatus: "complete",
-        validationStatus: "unknown",
         stabilityScore: null,
       });
     });
@@ -277,15 +270,13 @@ describe("#createBuildDiffs", () => {
         baseScreenshotId: null,
         compareScreenshotId: newScreenshot!.id,
         jobStatus: "complete",
-        validationStatus: "unknown",
         stabilityScore: null,
       });
       expect(diffs[1]).toMatchObject({
         buildId: build.id,
         baseScreenshotId: null,
         compareScreenshotId: newScreenshotWithoutFile!.id,
         jobStatus: "pending",
-        validationStatus: "unknown",
         stabilityScore: null,
       });
     });

diff --git a/apps/backend/src/build/createBuildDiffs.ts b/apps/backend/src/build/createBuildDiffs.ts
@@ -152,7 +152,6 @@ export async function createBuildDiffs(build: Build) {
         compareScreenshot,
       }),
       score: sameFileId ? 0 : null,
-      validationStatus: "unknown" as const,
       testId: compareScreenshot.testId,
     };
   });
@@ -173,7 +172,6 @@ export async function createBuildDiffs(build: Build) {
         compareScreenshotId: null,
         jobStatus: "complete" as const,
         score: null,
-        validationStatus: "unknown" as const,
       })) ?? [];
 
   const allInserts = [...inserts, ...removedScreenshots];

diff --git a/apps/backend/src/build/job.e2e.test.ts b/apps/backend/src/build/job.e2e.test.ts
@@ -60,7 +60,6 @@ describe("build", () => {
       const diffs = await ScreenshotDiff.query().where("buildId", build.id);
       expect(diffs).toHaveLength(1);
       expect(diffs[0]).toHaveProperty("jobStatus", "complete");
-      expect(diffs[0]).toHaveProperty("validationStatus", "unknown");
       expect(diffs[0]).toHaveProperty("score", null);
       expect(diffs[0]).toHaveProperty("s3Id", null);
     });

diff --git a/apps/backend/src/database/models/ScreenshotDiff.test.ts b/apps/backend/src/database/models/ScreenshotDiff.test.ts
@@ -7,7 +7,6 @@ const baseData = {
   baseScreenshotId: "1",
   compareScreenshotId: "2",
   jobStatus: "pending",
-  validationStatus: "unknown",
 };
 
 describe("models/ScreenshotDiff", () => {

diff --git a/apps/backend/src/database/models/ScreenshotDiff.ts b/apps/backend/src/database/models/ScreenshotDiff.ts
@@ -18,12 +18,7 @@ export class ScreenshotDiff extends Model {
   static override tableName = "screenshot_diffs";
 
   static override jsonSchema = mergeSchemas(timestampsSchema, jobModelSchema, {
-    required: [
-      "buildId",
-      "baseScreenshotId",
-      "compareScreenshotId",
-      "validationStatus",
-    ],
+    required: ["buildId", "baseScreenshotId", "compareScreenshotId"],
     properties: {
       buildId: { type: "string" },
       baseScreenshotId: { type: ["string", "null"] },
@@ -32,10 +27,6 @@ export class ScreenshotDiff extends Model {
       fileId: { type: ["string", "null"] },
       score: { type: ["number", "null"], minimum: 0, maximum: 1 },
       stabilityScore: { type: ["number", "null"], minimum: 0, maximum: 100 },
-      validationStatus: {
-        type: "string",
-        enum: ["unknown", "accepted", "rejected"],
-      },
       testId: { type: ["string", "null"] },
       group: { type: ["string", "null"] },
     },
@@ -49,7 +40,6 @@ export class ScreenshotDiff extends Model {
   score!: number | null;
   stabilityScore!: number | null;
   jobStatus!: JobStatus;
-  validationStatus!: "unknown" | "accepted" | "rejected";
   testId!: string | null;
   group!: string | null;
 

diff --git a/apps/backend/src/database/seeds.ts b/apps/backend/src/database/seeds.ts
@@ -290,7 +290,6 @@ export async function seed() {
     compareScreenshotId: screenshots[1]!.id,
     score: null,
     jobStatus: "complete" as const,
-    validationStatus: "unknown" as const,
     s3Id: "penelope-diff-transparent.png",
     createdAt: now,
     updatedAt: now,

diff --git a/apps/backend/src/database/testing/factory.ts b/apps/backend/src/database/testing/factory.ts
@@ -155,7 +155,6 @@ export const ScreenshotDiff = defineFactory(models.ScreenshotDiff, () => ({
   baseScreenshotId: Screenshot.associate("id"),
   compareScreenshotId: Screenshot.associate("id"),
   jobStatus: "complete",
-  validationStatus: "unknown",
   score: 0,
 }));
 

diff --git a/apps/backend/src/graphql/definitions/Build.ts b/apps/backend/src/graphql/definitions/Build.ts
@@ -4,7 +4,6 @@ import gqlTag from "graphql-tag";
 
 import { pushBuildNotification } from "@/build-notification/index.js";
 import { Build, BuildReview, ScreenshotDiff } from "@/database/models/index.js";
-import { transaction } from "@/database/transaction.js";
 
 import {
   IBaseBranchResolution,
@@ -311,17 +310,10 @@ export const resolvers: IResolvers = {
         throw forbidden("You cannot approve or reject this build");
       }
 
-      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",
-          }),
-        ]);
+      await BuildReview.query().insert({
+        buildId,
+        userId: auth.user.id,
+        state: validationStatus === "accepted" ? "approved" : "rejected",
       });
 
       // That might be better suited into a $afterUpdate hook.

diff --git a/apps/backend/src/screenshot-diff/computeScreenshotDiff.e2e.test.ts b/apps/backend/src/screenshot-diff/computeScreenshotDiff.e2e.test.ts
@@ -99,7 +99,6 @@ describe("#computeScreenshotDiff", () => {
         baseScreenshotId: baseScreenshot.id,
         compareScreenshotId: compareScreenshot.id,
         jobStatus: "pending",
-        validationStatus: "unknown",
         testId: test.id,
       });
     });
@@ -153,7 +152,6 @@ describe("#computeScreenshotDiff", () => {
         baseScreenshotId: baseScreenshot.id,
         compareScreenshotId: compareScreenshot.id,
         jobStatus: "pending",
-        validationStatus: "unknown",
       });
     });