Notification system #914
Notification system #914
Conversation
| @IdProperty({ | ||
| description: | ||
| 'The ID of the user that achieved the wr or sent the map testing request' | ||
| }) | ||
| @IsOptional() | ||
| readonly userID: number; |
There was a problem hiding this comment.
required: false in the IdProperty will apply an IsOptional decorator. Applies to below stuff as well.
libs/db/src/prisma/schema.prisma
Outdated
| targetUser User @relation("notifs_to", fields: [targetUserID], references: [id], onDelete: Cascade) | ||
| targetUserID Int |
There was a problem hiding this comment.
This name still feels ambiguous. Given how annoying these field names can be, best to use something more verbose. I'd suggest notifiedUser.
| @Inject(EXTENDED_PRISMA_SERVICE) private readonly db: ExtendedPrismaService | ||
| ) {} | ||
| async getNotifications(userID: number): Promise<NotificationDto[]> { | ||
| const dbResponse = await this.db.notification.findMany({ |
There was a problem hiding this comment.
Worth leaving a comment here explaining what we're doing; these fields will very often be empty, and we want to fetch only if they exist, and faster than explicit check.
apps/backend/src/app/modules/notifications/notifications.controller.ts
Outdated
Show resolved
Hide resolved
apps/backend/src/app/modules/maps/map-testing-request.service.ts
Outdated
Show resolved
Hide resolved
benjl
force-pushed
the
notifications
branch
2 times, most recently
from
2df19d6 to
992ecaa
Compare
Also removes them when the map goes out of private testing
| export type AnnouncementData = { | ||
| type: NotificationType.ANNOUNCEMENT; | ||
| message: string; | ||
| }; | ||
| export type WRAchievedData = { | ||
| type: NotificationType.WR_ACHIEVED; | ||
| runID: bigint; | ||
| }; | ||
| export type MapStatusChangeData = { | ||
| type: NotificationType.MAP_STATUS_CHANGE; | ||
| mapID: number; | ||
| }; | ||
| export type MapTestReqData = { | ||
| type: NotificationType.MAP_TEST_INVITE; | ||
| requesterID: number; | ||
| mapID: number; | ||
| }; | ||
| export type ReviewPostedData = { | ||
| type: NotificationType.REVIEW_POSTED; | ||
| reviewerID: number; | ||
| mapID: number; | ||
| reviewID: number; | ||
| }; | ||
| export type NotificationData = | ||
| | AnnouncementData | ||
| | WRAchievedData | ||
| | MapStatusChangeData | ||
| | MapTestReqData | ||
| | ReviewPostedData; |
There was a problem hiding this comment.
We're gonna use this in the frontend so should include these in @momentum/constants. Also, we usually use interfaces here. Types and Interfaces are unfortunately very similar in TypeScript, as a good rule of thumb used interface when you can, especially when you're describing object-like structures like this.
Also, I'd be tempted to do a base type to capture the shared structure of these types like
type NotifData<T = keyof NotificationType> = {
type: T;
};
export interface AnnouncementData extends NotifData<NotificationType.ANNOUNCEMENT> {
message: string;
}| throw new Error( | ||
| 'Malformed data argument! Look at the types in notifications.service.ts' | ||
| ); |
There was a problem hiding this comment.
Throwing a generic error here is a bad idea. We've got strong types in place, something has gone really wrong in our code if this happens. The way I'd look at it is, if a method can plausibly throw for some given input, leave a @throws JSDoc tag to make it clear to the caller that they are responsible for catching the specific exception this message may throw, and then do a custom exception e.g. FooException that the caller can use in an error instanceof FooException check.
But this method isn't of that sort, if the caller is being sensible this should never happen, and since we're in NestJS, we can just throw an InternalServerErrorException so we stop execution completely and return a 500 response (this will also get logged at sent to Sentry in prod).
If we are concerned with malformed data getting passed in here (unlikely, but doesn't hurt to check), let's catch cases where the object structure doesn't match the NotificationType, since we could be trying to access a nonexistent key on the data object. I would do
try {
switch (data.type) {
case NotificationType.ANNOUNCEMENT:
newNotif.message = data.message;
break;
case NotificationType.WR_ACHIEVED:
newNotif.runID = data.runID;
break;
case NotificationType.MAP_STATUS_CHANGE:
newNotif.mapID = data.mapID;
break;
case NotificationType.MAP_TEST_INVITE:
newNotif.userID = data.requesterID;
newNotif.mapID = data.mapID;
break;
case NotificationType.REVIEW_POSTED:
newNotif.userID = data.reviewerID;
newNotif.mapID = data.mapID;
newNotif.reviewID = data.reviewID;
break;
default:
throw undefined;
}
} catch {
throw new InteralErrorException(`Bad data for NotificationType ${NotificationType[data.type] ?? data.type}` ${JSON.stringify(data)}`);
}(I had to check if ESLint gets upset by us throwing whilst in a try/catch block like that, it doesn't, though WebStorm shows a warning. I don't think there's anything wrong with doing this!)
| for (const notifID of query.notifIDs) | ||
| if (Number.isNaN(notifID)) | ||
| throw new BadRequestException('notifIDs must contain numbers only'); |
There was a problem hiding this comment.
IntCsvQueryProperty is already doing this check
There was a problem hiding this comment.
IMO cleaner to throw if (!query.all && !query.notifIDs)) then in deleteMany do id: query.notifIDs ? { in: query.notifIDs } : undefined
There was a problem hiding this comment.
You just mentioned to me that IntCsvQueryProperty isn't handling NaN right, reminder for you to take a look at that!
| }); | ||
| it('should not delete a map testing request notification', async () => { |
There was a problem hiding this comment.
Still here. Sorry to nag about this but saves us random format cleanup commits in the future.
Don't skip ci this time pls
Closes #267