From 976a880312b1d0037d3f4afd1e1ceffb3cf912e4 Mon Sep 17 00:00:00 2001 From: counterweight Date: Sat, 20 Dec 2025 11:58:35 +0100 Subject: [PATCH] second review --- backend/main.py | 75 +++++++------------ backend/models.py | 4 +- frontend/app/admin/invites/page.tsx | 13 +--- frontend/app/signup/[code]/page.tsx | 4 +- frontend/test-results/.last-run.json | 4 - .../error-context.md | 35 +++++++++ .../error-context.md | 35 +++++++++ 7 files changed, 105 insertions(+), 65 deletions(-) delete mode 100644 frontend/test-results/.last-run.json create mode 100644 frontend/test-results/admin-invites-Admin-Invite-6f540-min-can-access-invites-page/error-context.md create mode 100644 frontend/test-results/admin-invites-Admin-Invite-8ae1f-th-users-not-a-number-input/error-context.md diff --git a/backend/main.py b/backend/main.py index 73598e1..bce9b3e 100644 --- a/backend/main.py +++ b/backend/main.py @@ -534,6 +534,22 @@ class InviteResponse(BaseModel): revoked_at: datetime | None +def build_invite_response(invite: Invite) -> InviteResponse: + """Build an InviteResponse from an Invite with loaded relationships.""" + return InviteResponse( + id=invite.id, + identifier=invite.identifier, + godfather_id=invite.godfather_id, + godfather_email=invite.godfather.email, + status=invite.status.value, + used_by_id=invite.used_by_id, + used_by_email=invite.used_by.email if invite.used_by else None, + created_at=invite.created_at, + spent_at=invite.spent_at, + revoked_at=invite.revoked_at, + ) + + MAX_INVITE_COLLISION_RETRIES = 3 @@ -544,17 +560,16 @@ async def create_invite( _current_user: User = Depends(require_permission(Permission.MANAGE_INVITES)), ): """Create a new invite for a specified godfather user.""" - # Validate godfather exists and get their info + # Validate godfather exists result = await db.execute( - select(User.id, User.email).where(User.id == data.godfather_id) + select(User.id).where(User.id == data.godfather_id) ) - godfather_row = result.one_or_none() - if not godfather_row: + godfather_id = result.scalar_one_or_none() + if not godfather_id: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail="Godfather user not found", ) - godfather_id, godfather_email = godfather_row # Try to create invite with retry on collision invite: Invite | None = None @@ -568,7 +583,7 @@ async def create_invite( db.add(invite) try: await db.commit() - await db.refresh(invite) + await db.refresh(invite, ["godfather"]) break except IntegrityError: await db.rollback() @@ -578,19 +593,12 @@ async def create_invite( detail="Failed to generate unique invite code. Please try again.", ) - assert invite is not None # We either succeeded or raised an exception above - return InviteResponse( - id=invite.id, - identifier=invite.identifier, - godfather_id=invite.godfather_id, - godfather_email=godfather_email, - status=invite.status.value, - used_by_id=invite.used_by_id, - used_by_email=None, - created_at=invite.created_at, - spent_at=invite.spent_at, - revoked_at=invite.revoked_at, - ) + if invite is None: + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to create invite", + ) + return build_invite_response(invite) class UserInviteResponse(BaseModel): @@ -693,20 +701,7 @@ async def list_all_invites( invites = result.scalars().all() # Build responses using preloaded relationships - records = [] - for invite in invites: - records.append(InviteResponse( - id=invite.id, - identifier=invite.identifier, - godfather_id=invite.godfather_id, - godfather_email=invite.godfather.email, - status=invite.status.value, - used_by_id=invite.used_by_id, - used_by_email=invite.used_by.email if invite.used_by else None, - created_at=invite.created_at, - spent_at=invite.spent_at, - revoked_at=invite.revoked_at, - )) + records = [build_invite_response(invite) for invite in invites] return PaginatedInviteRecords( records=records, @@ -744,16 +739,4 @@ async def revoke_invite( await db.commit() await db.refresh(invite) - # Use preloaded relationships (selectin loading) - return InviteResponse( - id=invite.id, - identifier=invite.identifier, - godfather_id=invite.godfather_id, - godfather_email=invite.godfather.email, - status=invite.status.value, - used_by_id=invite.used_by_id, - used_by_email=invite.used_by.email if invite.used_by else None, - created_at=invite.created_at, - spent_at=invite.spent_at, - revoked_at=invite.revoked_at, - ) + return build_invite_response(invite) diff --git a/backend/models.py b/backend/models.py index 25a4154..e0ad83c 100644 --- a/backend/models.py +++ b/backend/models.py @@ -206,7 +206,7 @@ class Invite(Base): godfather: Mapped[User] = relationship( "User", foreign_keys=[godfather_id], - lazy="selectin", + lazy="joined", ) # User who used this invite (null until spent) @@ -216,7 +216,7 @@ class Invite(Base): used_by: Mapped[User | None] = relationship( "User", foreign_keys=[used_by_id], - lazy="selectin", + lazy="joined", ) # Timestamps diff --git a/frontend/app/admin/invites/page.tsx b/frontend/app/admin/invites/page.tsx index d60b219..2937072 100644 --- a/frontend/app/admin/invites/page.tsx +++ b/frontend/app/admin/invites/page.tsx @@ -80,7 +80,7 @@ export default function AdminInvitesPage() { const handleCreateInvite = async () => { if (!newGodfatherId) { - setCreateError("Please enter a godfather user ID"); + setCreateError("Please select a godfather"); return; } @@ -104,6 +104,7 @@ export default function AdminInvitesPage() { const handleRevoke = async (inviteId: number) => { try { await api.post(`/api/admin/invites/${inviteId}/revoke`); + setError(null); fetchInvites(page, statusFilter); } catch (err) { setError(err instanceof Error ? err.message : "Failed to revoke invite"); @@ -328,16 +329,6 @@ const pageStyles: Record = { fontSize: "0.8rem", color: "rgba(255, 255, 255, 0.5)", }, - input: { - fontFamily: "'DM Sans', system-ui, sans-serif", - fontSize: "0.9rem", - padding: "0.75rem", - background: "rgba(255, 255, 255, 0.05)", - border: "1px solid rgba(255, 255, 255, 0.1)", - borderRadius: "8px", - color: "#fff", - maxWidth: "300px", - }, select: { fontFamily: "'DM Sans', system-ui, sans-serif", fontSize: "0.9rem", diff --git a/frontend/app/signup/[code]/page.tsx b/frontend/app/signup/[code]/page.tsx index 6cc8ae3..2395dea 100644 --- a/frontend/app/signup/[code]/page.tsx +++ b/frontend/app/signup/[code]/page.tsx @@ -19,7 +19,8 @@ export default function SignupWithCodePage() { router.replace("/"); } else { // Redirect to signup with code as query param - router.replace(`/signup?code=${encodeURIComponent(code)}`); + // Invite codes only contain [a-z0-9-] so no encoding needed + router.replace(`/signup?code=${code}`); } }, [user, isLoading, code, router]); @@ -37,4 +38,3 @@ export default function SignupWithCodePage() { ); } - diff --git a/frontend/test-results/.last-run.json b/frontend/test-results/.last-run.json deleted file mode 100644 index cbcc1fb..0000000 --- a/frontend/test-results/.last-run.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "status": "passed", - "failedTests": [] -} \ No newline at end of file diff --git a/frontend/test-results/admin-invites-Admin-Invite-6f540-min-can-access-invites-page/error-context.md b/frontend/test-results/admin-invites-Admin-Invite-6f540-min-can-access-invites-page/error-context.md new file mode 100644 index 0000000..943bea3 --- /dev/null +++ b/frontend/test-results/admin-invites-Admin-Invite-6f540-min-can-access-invites-page/error-context.md @@ -0,0 +1,35 @@ +# Page snapshot + +```yaml +- generic [active] [ref=e1]: + - main [ref=e2]: + - generic [ref=e4]: + - generic [ref=e5]: + - heading "Welcome back" [level=1] [ref=e6] + - paragraph [ref=e7]: Sign in to your account + - generic [ref=e8]: + - generic [ref=e9]: Failed to fetch + - generic [ref=e10]: + - generic [ref=e11]: Email + - textbox "Email" [ref=e12]: + - /placeholder: you@example.com + - text: admin@example.com + - generic [ref=e13]: + - generic [ref=e14]: Password + - textbox "Password" [ref=e15]: + - /placeholder: •••••••• + - text: admin123 + - button "Sign in" [ref=e16] [cursor=pointer] + - paragraph [ref=e17]: + - text: Don't have an account? + - link "Sign up" [ref=e18] [cursor=pointer]: + - /url: /signup + - status [ref=e19]: + - generic [ref=e20]: + - img [ref=e22] + - generic [ref=e24]: + - text: Static route + - button "Hide static indicator" [ref=e25] [cursor=pointer]: + - img [ref=e26] + - alert [ref=e29] +``` \ No newline at end of file diff --git a/frontend/test-results/admin-invites-Admin-Invite-8ae1f-th-users-not-a-number-input/error-context.md b/frontend/test-results/admin-invites-Admin-Invite-8ae1f-th-users-not-a-number-input/error-context.md new file mode 100644 index 0000000..943bea3 --- /dev/null +++ b/frontend/test-results/admin-invites-Admin-Invite-8ae1f-th-users-not-a-number-input/error-context.md @@ -0,0 +1,35 @@ +# Page snapshot + +```yaml +- generic [active] [ref=e1]: + - main [ref=e2]: + - generic [ref=e4]: + - generic [ref=e5]: + - heading "Welcome back" [level=1] [ref=e6] + - paragraph [ref=e7]: Sign in to your account + - generic [ref=e8]: + - generic [ref=e9]: Failed to fetch + - generic [ref=e10]: + - generic [ref=e11]: Email + - textbox "Email" [ref=e12]: + - /placeholder: you@example.com + - text: admin@example.com + - generic [ref=e13]: + - generic [ref=e14]: Password + - textbox "Password" [ref=e15]: + - /placeholder: •••••••• + - text: admin123 + - button "Sign in" [ref=e16] [cursor=pointer] + - paragraph [ref=e17]: + - text: Don't have an account? + - link "Sign up" [ref=e18] [cursor=pointer]: + - /url: /signup + - status [ref=e19]: + - generic [ref=e20]: + - img [ref=e22] + - generic [ref=e24]: + - text: Static route + - button "Hide static indicator" [ref=e25] [cursor=pointer]: + - img [ref=e26] + - alert [ref=e29] +``` \ No newline at end of file