-
Notifications
You must be signed in to change notification settings - Fork 4k
refactor(dal): remove soft delete in message collection #8073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(dal): remove soft delete in message collection #8073
Conversation
- Updated message deletion methods to use a single delete operation instead of deleteMany. - Removed the 'deleted' field from various DTOs and entities as part of the cleanup. - Enhanced the update services to handle message removal more efficiently. - Added enforcement of environment ID in message deletion queries. - Cleaned up imports and adjusted return types for consistency.
…usecounts-hook-soft-delete
commit: |
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This reverts commit ec519e6.
…usecounts-hook-soft-delete
messageSchema.pre('find', function filterDeletedFind() { | ||
this.where({ deleted: { $ne: true } }); | ||
}); | ||
messageSchema.pre('findOne', function filterDeletedFindOne() { | ||
this.where({ deleted: { $ne: true } }); | ||
}); | ||
messageSchema.pre('findOneAndUpdate', function filterDeletedFindOneAndUpdate() { | ||
this.where({ deleted: { $ne: true } }); | ||
}); | ||
messageSchema.pre('countDocuments', function filterDeletedCountDocuments() { | ||
this.where({ deleted: { $ne: true } }); | ||
}); | ||
messageSchema.pre('count', function filterDeletedCount() { | ||
this.where({ deleted: { $ne: true } }); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to verify, that's the proper syntax? Just looks a bit weird. Shouldn't we return something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is i tested it locally, but we will still need to double test in staging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this just adds another where clause right? Not overrides the existing one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct
…usecounts-hook-soft-delete
Related tickets:
https://linear.app/novu/issue/NV-5647/investigate-high-response-time-with-usecounts-hook
https://linear.app/novu/issue/NV-5688/dalmessages-remove-backward-compatibility-logic-for-soft-delete
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer