feat: admin and moderation features #105
Conversation
ascpixi
left a comment
ascpixi
left a comment
There was a problem hiding this comment.
Thank you so much for the PR! Here's the first round of feedback.
apps/web/prisma/schema.prisma
Outdated
| isBanned Boolean @default(false) | ||
| bannedAt DateTime? | ||
| bannedReason String @default("") | ||
| bannedReasonInternal String @default("") |
There was a problem hiding this comment.
We should only use BanRecord as our single source of truth. We should fetch the latest record for the user to get more information about a ban. isBanned can remain denormalized.
There was a problem hiding this comment.
so only 1 ban record? Instead of bannedReason and bannedReasonInternal
There was a problem hiding this comment.
We should have as many ban records as how many times the user's ban state changes. So, to get the latest ban reason for a user, you'd select the most recent BanRecord, and display the reason stored in that row.
I'd remove bannedAt, bannedReason, and bannedReasonInternal from User, and instead get that data via BanRecord, as we already store that there. The extra database hit is fine, as banned users are an exception, and isBanned should remain because if it's false, we can avoid fetching any BanRecords for regular user actions.
| import { Button } from "@/client/components/ui/Button"; | ||
| import { useAuthContext } from "@/client/context/AuthContext"; | ||
|
|
||
| export default function BannedPage() { |
There was a problem hiding this comment.
We should redirect users that are not actually banned to /.
| if (comment.authorId !== req.ctx.user.id) | ||
| return apiErr("NO_PERMISSION", "You can only delete your own comments."); | ||
| const isAuthor = comment.authorId === req.ctx.user.id; | ||
| const isAdmin = req.ctx.user.permissionLevel in oneOf("ADMIN", "ROOT"); |
There was a problem hiding this comment.
This is a great use of the oneOf function, but I wonder if we can create a helper isAdmin function that accepts a user object - e.g.:
export function isAdmin(user: User) {
return user.permissionLevel in oneOf("ADMIN", "ROOT");
}| /** | ||
| * The internal reason for the ban (only visible to admins). | ||
| */ | ||
| bannedReasonInternal: z.string() |
There was a problem hiding this comment.
I feel like being banned is an exceptional state. I recommend creating a ban object that is either null or the ban information.
| /** | ||
| * Sets the ban status of a user. Only administrators can use this endpoint. | ||
| */ | ||
| setBanStatus: protectedProcedure() |
There was a problem hiding this comment.
I think we should have all administrative actions in /api/admin.ts.
apps/web/tsconfig.json
Outdated
| "exclude": [ | ||
| "node_modules" | ||
| "node_modules", | ||
| "vitest.config.ts" |
There was a problem hiding this comment.
Why are we excluding vitest configuration from TypeScript's config?
- Remove bannedAt, bannedReason, bannedReasonInternal from User model - Keep isBanned as denormalized flag to avoid BanRecord lookups - Add BanInfoSchema with ban details fetched from latest BanRecord - Update dtoUser to fetch ban info from BanRecord when user is banned
- Create new admin.ts with setBanStatus, getBanHistory, setPermissionLevel, list, deleteUser - Add getMyBanInfo endpoint for banned page to fetch ban details - Update _app.ts to include admin router - Update admin page to use trpc.admin.* endpoints
vitest.config.ts imports from vitest/config which is a dev dependency not available during production builds. Test files are handled by vitest's own TypeScript compilation.
- Add banRecord to database mock - Update user tests to use createAdminCaller for admin endpoints - Update mock setup for with array pattern
|
Made all of the changes ready for round 2! |
ascpixi
left a comment
ascpixi
left a comment
There was a problem hiding this comment.
Thank you! I feel like we're quite close to getting this merged into prod!
apps/web/src/client/trpc.ts
Outdated
| } | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
A blank line before this return statement, please!
apps/web/src/client/trpc.ts
Outdated
| banRedirectTriggered = true; | ||
| window.location.href = "/banned"; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
A blank line before this return statement, please!
apps/web/src/client/trpc.ts
Outdated
| export function handleBanError(error: unknown): boolean { | ||
| if ( | ||
| error instanceof TRPCClientError && | ||
| error.message === "Your account has been banned." |
There was a problem hiding this comment.
I'm not sure how I feel about relying on the error message being something specific. Instead of throwing tRPC errors, how about creating a BANNED error code and returning that as an apiErr(...) result?
This would require some special handling on the client, but I feel like would result in a cleaner implementation.
apps/web/src/pages/_app.tsx
Outdated
| initLogBucket(); | ||
|
|
||
| if (typeof window !== "undefined") { | ||
| window.addEventListener("unhandledrejection", (event) => { |
There was a problem hiding this comment.
Can we hook into tRPC on the client? If so, before receiving any Result objects, we can check if the error is "BANNED". If this is the case, we'd do what we do here.
apps/web/src/pages/admin/index.tsx
Outdated
| <RootLayout title="Admin Dashboard - Lapse" showHeader> | ||
| <div className="container mx-auto px-4 py-8"> | ||
| <div className="flex items-center gap-3 mb-6"> | ||
| <Icon glyph="admin" width={32} height={32} className="text-red" /> |
There was a problem hiding this comment.
Increase the size of the icons a bit more, please! The icon should be roughly the same size as the "A" in "Admin Dashboard".
apps/web/src/pages/admin/index.tsx
Outdated
|
|
||
| <div className="mb-6"> | ||
| <div className="flex items-center gap-2 mb-4"> | ||
| <Icon glyph="search" width={20} height={20} className="text-muted" /> |
There was a problem hiding this comment.
Same here - please increase the size of the icon to match the text.
apps/web/src/pages/admin/index.tsx
Outdated
| </Button> | ||
| {isRoot && !userIsRoot && ( | ||
| <Button kind="regular" onClick={() => openPromoteModal(user)}> | ||
| <Icon glyph="admin" width={20} height={20} /> |
There was a problem hiding this comment.
Seems like the admin glyph has some padding around it, making it look smaller than e.g. history. We should increase the size of this specific icon.
| )} | ||
| {canModerate && !userIsRoot && ( | ||
| <> | ||
| <Button |
There was a problem hiding this comment.
I'd place Ban before the delete button, so that we have a continous row of 3 icon buttons and a text button at the end. Minor aesthetic change!
apps/web/src/pages/admin/index.tsx
Outdated
| )} | ||
| </div> | ||
| <div className="flex gap-2" onClick={e => e.preventDefault()}> | ||
| <Button kind="regular" onClick={() => openHistoryModal(user)}> |
There was a problem hiding this comment.
We have some horizontal padding on Button that doesn't make sense for icon-only buttons. We can either:
- create a
isSquareprop onButton, so that X padding = Y padding, - make the icons clickable without a
Button, similar to what we have on our headers near the user profile picture.
Leaving this choice up to you - either one should look fine!
- Create userSchemas.ts with pure schema definitions (no router imports) - Move PermissionLevelSchema, KnownDeviceSchema, UserSchema, BanRecordSchema, etc. - Move DTO functions (dtoPublicUser, dtoKnownDevice, dtoBanRecord) - Re-export from user.ts for backwards compatibility - Fixes 'Cannot access PublicUserSchema before initialization' error
- Add BANNED to KnownErrorSchema - Change ban error message to 'BANNED' for consistent detection - Create banCheckLink tRPC link to intercept BANNED errors - Rename handleBanError to isBannedError (check only, no side effects) - Set isBanned state from user data in AuthContext - Add banReason state for better ban info handling
- Add isSquare prop to Button for icon-only buttons - Increase admin icon size (40x40) to match 'Admin Dashboard' text - Increase search icon size (24x24) to match 'Search User' text - Increase admin glyph in user row (24x24) to account for padding - Use isSquare on all icon-only action buttons - Change Ban/Unban to use private icon instead of text
|
@ascpixi ready for review! |
|
Hey @DragonSenseiGuy, sorry this took so long! Can you give me write access to your fork, please? I'm trying to commit some changes so that we don't have to go through even more rounds of review 😅 |
2 participants
Closes #56