Conversation
emmanishikawa
changed the title
models/Notification.ts
Outdated
| const { Schema } = mongoose; | ||
|
|
||
| const notificationSchema = new Schema({ | ||
| _id: { type: String, required: true }, |
There was a problem hiding this comment.
nit: formatting properly would be a plus
| fullDocument: "updateLookup", | ||
| }); | ||
|
|
||
| console.log("[notifications] watcher started"); |
There was a problem hiding this comment.
might want to take this out when sending a PR to merge onto main
| export async function startNotificationWatcher() { | ||
| await connectToDatabase(); | ||
|
|
||
| const collection = mongoose.connection.collection("products"); |
There was a problem hiding this comment.
I believe this should be items instead of products. Products was the default file from the boilerplate.
|
|
||
| const updatedDoc = change.fullDocument; | ||
| if (!updatedDoc) continue; | ||
|
|
There was a problem hiding this comment.
We can add a comment here where we can check the quantity and compare it to the specified threshold. If it is less we continue else we will send a noti
arnavjk007
left a comment
arnavjk007
left a comment
There was a problem hiding this comment.
Just some minor changes
models/Notification.ts
Outdated
| const { Schema } = mongoose; | ||
|
|
||
| const notificationSchema = new Schema({ | ||
| _id: { type: String, required: true }, |
There was a problem hiding this comment.
I think we can delete _id since it is an internal thing that MongoDB handles.
models/Notification.ts
Outdated
| type: { type: String, required: true }, | ||
| labId: { type: String, required: true }, | ||
| resourceId: { type: String, required: true }, | ||
| recipients: { | ||
| type: [String], |
There was a problem hiding this comment.
Shift labId to be the top field just so its consistent with the rest of the schemas
models/Notification.ts
Outdated
| type: { type: String, required: true }, | ||
| labId: { type: String, required: true }, | ||
| resourceId: { type: String, required: true }, | ||
| recipients: { |
There was a problem hiding this comment.
We can also add a field in recipients called roles so we know who should be notified.
arnavjk007
left a comment
arnavjk007
left a comment
There was a problem hiding this comment.
Small comment change but everything else looks good
| /** | ||
| * Get the updated document from the change stream | ||
| * If quantity is below the threshold, continue | ||
| * else send a notification | ||
| */ |
There was a problem hiding this comment.
nit: can we separate these two comments into 1: what the code does now and 2: a reminder to implement the threshold checking here once other parts are implemented
arnavjk007
left a comment
arnavjk007
left a comment
There was a problem hiding this comment.
Small schema type change
models/Notification.ts
Outdated
| resourceId: { type: String, required: true }, | ||
| recipients: { | ||
| type: [String], | ||
| roles: { type: String }, |
There was a problem hiding this comment.
Just noticed this but this should be a list of strings, not {}.
3 participants
Describe your changes
Issue ticket number and link
https://docs.google.com/document/d/1cAICVmUirgPTL-fq3dkHVtx4QkyrG0XCJO7VUxnibdA/edit?usp=sharing
Checklist before requesting a review