Add exceptions for deadlines by thesleepyniko · Pull Request #82 · hackclub/resolution

thesleepyniko · 2026-04-21T03:09:49Z

As stated.

vercel · 2026-04-21T03:09:53Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
resolution	Ready	Preview, Comment	Apr 21, 2026 4:12am

Copilot

Pull request overview

Adds a “submission deadline exception” mechanism so specific users can still submit after a week’s submissions are closed, plus an ambassador UI to manage those exceptions.

Changes:

Introduces submission_closure_exception table + Drizzle migration and an ExceptionService for active/non-expired lookups.
Allows shipping/submitting when isSubmissionsOpen is false but a valid exception exists (server routes + API).
Adds an ambassador “Exceptions” page to create/toggle/delete exceptions and surfaces an extension notice on the week page.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`resolution-frontend/src/routes/app/pathway/[pathway]/week/[week]/ship/+page.server.ts`	Allows access to the ship page when an active exception exists.
`resolution-frontend/src/routes/app/pathway/[pathway]/week/[week]/+page.server.ts`	Loads an active exception and treats submissions as open when present.
`resolution-frontend/src/routes/app/pathway/[pathway]/week/[week]/+page.svelte`	Displays a deadline-extension notice when an exception is active.
`resolution-frontend/src/routes/api/ships/submit-project/+server.ts`	Permits submissions when closed if the user has an active exception.
`resolution-frontend/src/routes/app/ambassador/+page.svelte`	Adds navigation to the new Exceptions page.
`resolution-frontend/src/routes/app/ambassador/exceptions/+page.svelte`	New UI for creating and managing exceptions.
`resolution-frontend/src/routes/app/ambassador/exceptions/+page.server.ts`	New load/actions backing the Exceptions UI.
`resolution-frontend/src/lib/server/services/index.ts`	Exports `ExceptionService`.
`resolution-frontend/src/lib/server/services/exceptionService.ts`	Implements active-exception lookup logic.
`resolution-frontend/src/lib/server/services/exceptionService.test.ts`	Adds unit tests for `ExceptionService.getActiveException`.
`resolution-frontend/src/lib/server/db/schema.ts`	Adds the new exceptions table + relations.
`resolution-frontend/drizzle/meta/_journal.json`	Records the new migration.
`resolution-frontend/drizzle/meta/0005_snapshot.json`	New Drizzle snapshot including the table.
`resolution-frontend/drizzle/0005_productive_madripoor.sql`	Creates the `submission_closure_exception` table + indexes/constraints.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

… related logic

…tions

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-21T03:43:43Z

+		} catch {
+			return fail(400, { error: 'An exception already exists for this user, pathway, and week' });
+		}


This catch treats any insert failure as “exception already exists…”, which will mask unexpected DB issues (FK failures, outages, etc.) behind a misleading message. Consider only translating unique-constraint violations to that message, and otherwise rethrow/log + return a 500.

Copilot · 2026-04-21T03:43:43Z

+			<section class="card">
+				<h2>Active Exceptions ({data.exceptions.length})</h2>
+				<div class="exceptions-list">
+					{#each data.exceptions as exception}
+						{@const info = pathwayInfo[exception.pathway]}
+						<div class="exception-item" class:inactive={!exception.isActive}>


The heading says “Active Exceptions” but data.exceptions includes inactive entries (and the count uses the full length). Either filter data.exceptions to only active (and ideally non-expired) exceptions, or update the heading/count to reflect that inactive exceptions are included.

Copilot · 2026-04-21T03:43:44Z

+										<span class="status-badge" class:active={exception.isActive}>
+											{exception.isActive ? 'Active' : 'Inactive'}
+										</span>
+										<span class="exception-date">
+											Expires {formatDate(exception.expiresAt)}


ExceptionService treats exceptions as active only if isActive and expiresAt >= CURRENT_DATE, but the UI status badge is driven solely by exception.isActive. This can display “Active” for expired exceptions that won’t actually allow submissions; consider computing/displaying an “Expired” state (or deriving an isCurrentlyActive field server-side).

Copilot · 2026-04-21T03:43:44Z

+	return {
+		assignments: assignedPathways,
+		season: {


If currentUser.isAdmin is true and they have no ambassadorPathway rows, assignedPathways will be empty, so the pathway dropdown will have no options and admins can’t create exceptions. Consider returning PATHWAY_IDS (or all pathways) for admins instead of only assignments.map(...).

Copilot · 2026-04-21T03:43:44Z

+			.where(
+				and(
+					eq(programEnrollment.seasonId, season.id),
+					eq(programEnrollment.status, 'ACTIVE')
+				)


enrolledUsers currently pulls all active enrollments for the season (including emails) regardless of the ambassador’s assigned pathways. That leaks user PII to ambassadors who shouldn’t have access. Consider scoping this query for non-admins by joining userPathway and filtering to assignedPathways (and/or only returning the minimal fields needed for search).

Copilot · 2026-04-21T03:43:44Z

+		const weekNumber = parseInt(formData.get('weekNumber') as string, 10);
+		const reason = formData.get('reason') as string;
+		const expiresAt = formData.get('expiresAt') as string;
+
+		if (!userId || !pathway || !weekNumber || !reason || !expiresAt) {


expiresAt is accepted directly from form data without validating it’s a valid date string and not already in the past. That can create exceptions that will never be considered active (since ExceptionService checks expiresAt >= CURRENT_DATE). Consider validating expiresAt format and enforcing expiresAt >= today before inserting.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

jeninh

Nice feature, gating is consistent across the three chokepoints (week load, ship load, submit API). A few things I'd like addressed before merge.

Correctness

exceptions/+page.server.ts createException validation

There's no check that expiresAt is in the future, and no check that the selected userId is actually enrolled in the target pathway. An ambassador can currently create an exception with a past date (which is immediately inert) or for a user who isn't even in that pathway. Please validate both.

exceptions/+page.server.ts:130-141 overly broad catch

} catch {
    return fail(400, { error: 'An exception already exists for this user, pathway, and week' });
}

Any insert failure gets reported as "already exists", which is misleading if it was actually a FK violation or a DB hiccup. Either narrow to the Postgres unique violation code (23505), or do a pre-check with a select before inserting.

exceptions/+page.server.ts:98 falsy check on weekNumber

if (!userId || !pathway || !weekNumber || !reason || !expiresAt) {

!weekNumber treats 0 as missing. Not a live bug since weeks are 1 to N, but Number.isNaN(weekNumber) is clearer and wouldn't trip on edge cases.

exceptionService.ts:35 inclusive expiry

gte(submissionClosureException.expiresAt, sql`CURRENT_DATE`)

gte makes the expiration day itself still valid. Probably what you want (user can submit on the day it expires), but worth confirming since the field is called expiresAt.

Security

exceptions/+page.server.ts:121, 170, 210 pathway as any casts

eq(ambassadorPathway.pathway, pathway as any)

You already validate with PATHWAY_IDS.includes(pathway) above, so after that check you can type it as PathwayId and drop the cast. Keeping as any around defeats the point of the enum.

No audit trail surfaced

The table has createdBy which is great, but nothing exposes it in the UI or logs. For a tool that grants submission bypass, I'd want to be able to see who created what. At minimum log it server side.

Tests

exceptionService.test.ts doesn't test what matters

The tests mock the entire Drizzle query builder chain, so they verify that db.select() was called and that the result is returned, but they never verify the where clause filters by isActive, seasonId, or expiresAt >= CURRENT_DATE. If someone deleted the isActive filter tomorrow, these tests would still pass. The whole point of this service is those filters. Please either use pg-mem / a test container, or capture the where expression and assert against it.

No tests for the action handlers or the submit API exception branch. At least one happy-path test per action (createException, toggleException, deleteException) would catch regressions in the auth checks.

Minor

+page.server.ts:55 (week page) overloads isSubmissionsOpen

isSubmissionsOpen: content.isSubmissionsOpen || !!exception,

Downstream already reads data.exception directly, so a separate hasException field would be cleaner than conflating the two meanings into isSubmissionsOpen.

exceptions/+page.svelte:227 stray whitespace

<strong>
    {[exception.userName, exception.userLastName].filter(Boolean).join(' ')}  

</strong>

Trailing tab plus two spaces and a blank line inside the strong tag. Looks like a merge artifact.

exceptions/+page.svelte user picker UX

selectedUserId is only set by clicking a search result. If someone types a full name and hits submit, nothing happens and the disabled button gives no hint why. A visible "Selected: X" indicator would help.

Two migrations for one feature

0005 creates expires_at as timestamp, 0006 changes it to date. If 0005 hasn't shipped to prod, squash them into one. If it has, leave it.

jeninh · 2026-04-22T12:48:33Z

1 more thing — the load function in the exceptions page fetches all enrolled users into memory and sends them to the client for search (L30-45 in +page.server.ts). This works fine now but will scale poorly as enrollment grows. Consider a server-side search endpoint (e.g. /api/users/search?q=...) that queries on demand instead of shipping the full user list on every page load.

thesleepyniko added 7 commits April 20, 2026 21:12

feat: add exceptions to schema

736d738

feat: add isActive for manual expiry

8eacfff

feat: add template for exceptions page

2a7960d

feat: db changes & migrations

741be26

feat: add exception service

43011a6

feat: implement user exceptions

e34b04f

test: tests for exception service

41b935d

thesleepyniko self-assigned this Apr 21, 2026

thesleepyniko requested review from Copilot and jeninh April 21, 2026 03:10

Copilot started reviewing on behalf of thesleepyniko April 21, 2026 03:10 View session

github-code-quality Bot found potential problems Apr 21, 2026

View reviewed changes

Comment thread resolution-frontend/src/routes/app/ambassador/exceptions/+page.server.ts Fixed

Comment thread resolution-frontend/src/lib/server/db/schema.ts Fixed

style: add semicolon

d9814b3

Copilot AI reviewed Apr 21, 2026

View reviewed changes

vercel Bot deployed to Preview April 21, 2026 03:16 View deployment

fix: prevent ambassadors from viewing exceptions for unassigned pathways

0f8be92

vercel Bot deployed to Preview April 21, 2026 03:18 View deployment

fix: ensure space between first and last name

84465e4

vercel Bot deployed to Preview April 21, 2026 03:20 View deployment

fix: render name with space

d98719a

vercel Bot deployed to Preview April 21, 2026 03:21 View deployment

fix: update exception expiration handling to use date type and adjust…

e8b7f67

… related logic

vercel Bot deployed to Preview April 21, 2026 03:35 View deployment

thesleepyniko added 2 commits April 20, 2026 23:37

fix: db FK stuff

544598d

fix: add pathway assignment check for non-admin users in exception ac…

c68a44a

…tions

thesleepyniko requested a review from Copilot April 21, 2026 03:37

vercel Bot deployed to Preview April 21, 2026 03:37 View deployment

Copilot started reviewing on behalf of thesleepyniko April 21, 2026 03:37 View session

Copilot AI reviewed Apr 21, 2026

View reviewed changes

fix: increase accessibility

f75f5eb

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

vercel Bot deployed to Preview April 21, 2026 04:12 View deployment

jeninh requested changes Apr 22, 2026

View reviewed changes

Conversation

thesleepyniko commented Apr 21, 2026

Uh oh!

vercel Bot commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

jeninh left a comment

Choose a reason for hiding this comment

Correctness

Security

Tests

Minor

Uh oh!

jeninh commented Apr 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel Bot commented Apr 21, 2026 •

edited

Loading