Justinxue #6

shoexue · 2025-12-24T16:18:29Z

Forms API – Summary (Create / Clone / Update / Delete)

This PR completes the admin-only Forms management APIs and corresponding service logic, with full unit test coverage for the service layer.

✅ Implemented Endpoints

CREATE Form (Admin only)

POST /api/seasons/{seasonCode}/forms

Creates a new form for a season, optionally with questions.

Verifies admin access via sessionToken
Inserts into form
Optionally inserts related formQuestion rows
Returns 201 Created

CLONE Form (Admin only)

POST /api/seasons/{seasonCode}/forms/{formId}/clone

Creates a new form by cloning an existing one.

Verifies admin access
Validates source form exists (404 if not)
Creates a new form row
Clones all questions with:
- New formId
- New formQuestionId via crypto.randomUUID()
Returns 201 Created

UPDATE Form (Admin only)

POST /api/seasons/{seasonCode}/forms/{formId}

Updates form metadata and optionally replaces questions.

Verifies admin access
Validates form exists (404 if not)
Updates openTime, closeTime, tags
Questions behavior:
- If omitted → leave unchanged
- If provided → delete existing questions, then insert new ones
Returns 200 OK

DELETE Form (Admin only)

DELETE /api/seasons/{seasonCode}/forms/{formId}

Deletes a form and all associated questions.

Verifies admin access
Validates form exists (404 if not)
Deletes in correct order:
1. formQuestion
2. form
Returns 204 No Content (intentional, no response body)

🧠 Service Layer (`forms.service.ts`)

createForm
cloneForm
updateForm
deleteForm

All database interactions:

Use Drizzle ORM
Properly handle foreign key constraints
Normalize optional fields (tags, questions)
Wrap DB errors via handleDbError

The logic is intentionally self-contained and mirrors API behavior closely, so no additional explanation is required beyond the routes.

🧪 Tests

100% coverage for forms.service.ts
Covers:
- Happy paths
- 404 cases
- Question replacement semantics
- Deterministic UUIDs for cloning
Global coverage thresholds satisfied

ℹ️ Notes

Admin validation is centralized via isAdmin(userId)
HTTP semantics respected (e.g. DELETE → 204)
API routes documented via hono-openapi
Ready for frontend and further integration

✅ This PR completes all admin-side Forms mutation endpoints.

BlazingAsher · 2025-12-25T18:14:45Z

src/resources/form/forms.test.ts

Not a fan of mocking your ORM. These should just be integration tests

src/resources/form/forms.routes.ts

BlazingAsher · 2025-12-25T18:17:58Z

src/resources/form/forms.routes.test.ts

+    });
+
+    it("returns 403 when user is not admin", async () => {
+      (isAdmin as Mock).mockResolvedValue(false);


You are mocking isAdmin, which is creating an implementation requirement that the routes use isAdmin to check permissions.

However, this may not necessarily be true. Instead, you should inject the request context with an admin user to make this more resilient and require the test to know less about the implementation

BlazingAsher · 2025-12-25T18:19:57Z

Also less about your code but your PR, I would shy away from using AI to generate the description. It's just a whole bunch of unnecessary detail that makes it annoying to read. Just include a summary of your changes, any big decisions you made (and why, if it's not obvious), and potential things you want a reviewer to focus on

danielp1218

overall just clean up your code, the logic looks OK to me.

danielp1218 · 2025-12-27T05:16:27Z

src/resources/form/forms.routes.ts

+    const params = c.req.valid("param");
+    const body = c.req.valid("json");
+
+    const admin = await isAdmin(body.sessionToken);


just a note that this implementation is subject to change #4

danielp1218 · 2025-12-27T05:24:49Z

src/resources/form/forms.service.ts

+      if (input.questions.length > 0) {
+        await db.insert(formQuestion).values(
+          input.questions.map((q) => ({
+            formQuestionId: q.formQuestionId,


the database will auto-generate IDs, we definitely don't want to supply them with data the client sends. you can just remove this line.

danielp1218 · 2025-12-27T05:28:34Z

src/resources/form/forms.service.ts

+      });
+    }
+
+    // Delete children first (avoid FK issues)


we have cascading deletes set in our schema so this isn't needed

danielp1218 · 2025-12-27T05:31:47Z

src/resources/form/forms.service.ts

+      await db.insert(formQuestion).values(
+        srcQuestions.map((q) => ({
+          // generate a new id so we never collide if formQuestionId is globally unique
+          formQuestionId: crypto.randomUUID(),


DB will auto-generate, can get rid of this line

It auto-generates formQuestionId or do you mean formId.

i mean formQuestionId in the formQuestion table. We have it set up so every table’s surrogate key a.k.a the {table}Id will default to a random uuid.

In the current schema it looks like only formId has a uuid default

danielp1218 · 2025-12-27T07:16:02Z

src/resources/form/forms.service.ts

+  questions?: CreateFormQuestionInput[];
+}
+
+export const createForm = async (input: CreateFormInput) => {


Also, you should wrap all the DB operations in this function within a transaction. (same with updateForm and cloneForm)
Conceptually, we want either all the DB operations to happen or all fail. It will result in wierd data integrity issues if a function fails halfway.

shoexue added 5 commits December 13, 2025 17:08

create form w uncessary tests

cfd5cad

Form creation initial work

a2a136d

update, clone, delete

bda4b8e

fixes from ci

203ce83

quick fix

328374a

shoexue requested review from Lindsay-X, danielp1218, hackthe6ixadmin and ja-der December 24, 2025 16:18

BlazingAsher requested changes Dec 25, 2025

View reviewed changes

danielp1218 requested changes Dec 27, 2025

View reviewed changes

danielp1218 reviewed Dec 27, 2025

View reviewed changes

Justinxue #6

Are you sure you want to change the base?

Justinxue #6

Conversation

shoexue commented Dec 24, 2025

Forms API – Summary (Create / Clone / Update / Delete)

✅ Implemented Endpoints

CREATE Form (Admin only)

CLONE Form (Admin only)

UPDATE Form (Admin only)

DELETE Form (Admin only)

🧠 Service Layer (forms.service.ts)

🧪 Tests

ℹ️ Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BlazingAsher commented Dec 25, 2025

Uh oh!

danielp1218 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielp1218 Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

🧠 Service Layer (`forms.service.ts`)

danielp1218 Dec 27, 2025 •

edited

Loading