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
arnavjk007
left a comment
arnavjk007
left a comment
There was a problem hiding this comment.
Minor structure change
arnavjk007
left a comment
arnavjk007
left a comment
There was a problem hiding this comment.
Looks good! I think the last step is to write a few simple unit tests to make sure that this works. I have already done the setup for the jest testing env in the main branch so just switch to that and do git pull then go back to the feat and do git merge main to get the updated changes.
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