-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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: EmailInbox out of DB Watcher #32501
Conversation
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #32501 +/- ##
===========================================
- Coverage 56.34% 56.34% -0.01%
===========================================
Files 2434 2434
Lines 53708 53712 +4
Branches 11057 11059 +2
===========================================
+ Hits 30263 30264 +1
- Misses 20801 20810 +9
+ Partials 2644 2638 -6
Flags with carried forward coverage won't be shown. Click here to find out more. |
e1bbd83
to
2733672
Compare
…boxes to findActive
2733672
to
935b352
Compare
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.
should findoptions be parameters? generally they should, so people can pass especially whatever subset of projections they need, guess the real question is if that should be done in this PR or not. projections here seem to be specific to the usecases in layers above, and later on somebody else might need to touch these files again to add new ones.
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.
Agree with you, Deb. For this PR, I chose to follow the strict needs of the current use case.
Discussion for the team: should we build methods with "batteries included" from the start, or can we make those changes when they are needed in the future? @sampaiodiego and @MarcosSpessatto, any thoughts?
Co-authored-by: Diego Sampaio <[email protected]>
…retention * 'develop' of github.com:RocketChat/Rocket.Chat: Release 6.9.0 Release 6.9.0-rc.2 fix: User status when setting "Use REST instead of websocket for Meteor calls" is disabled (#32500) fix: User status when setting "Use REST instead of websocket for Meteor calls" is disabled (#32500) regression: Hide prune section based on permission (#32531) fix: Users in role table not rendering the empty state properly (#32412) refactor: EmailInbox out of DB Watcher (#32501) ci: increase issue allowed stale time (#32523) regression: Remove impossible sorting from users table "registration status" column (#32506) feat: Add contentDisposition option to file storages (#31974) i18n: Rocket.Chat language update from LingoHub 🤖 on 2024-05-28Z (#32508) Release 6.9.0-rc.1 fix: Re-login same browser tab issues (#32479) regression: Replace read receipt single icon (#32486) regression: Incorrect retention policy banner's display rule for teams (#32483) chore: Publish npm packages again (#32463) ci: publish missing Omnichannel services to DockerHub (#32462) Release 6.9.0-rc.0
As per the updates mentioned in PROJ-7, SCA-11 and ADR #74, this pull request focuses on relocating
EmailInbox
model out of DB Watcher service.Context
This modification enhances RocketChat's app by allowing it to directly call listeners through the
api.broadcast
global function, bypassing the need for MongoDB Change Stream data propagation.This change offers better control over user notifications via Web Sockets, enabling more precise management of use-cases. Instead of notifying every database change, we can now send an
api.broadcast
call only when necessary, reducing overall network messages. Additionally, this contributes to the future removal of the DB Watcher deployment, optimizing resource utilization.Proposed changes
Key changes include:
dbWatchersDisabled
flag.watch.emailInbox
listener event, subject to thedbWatchersDisabled
flag./api/v1/email-inbox
/api/v1/email-inbox/:_id
Steps to test or reproduce
DISABLE_DB_WATCHERS
environment variable set totrue
.Further comments
To maintain consistency and avoid potential regressions, event names and signatures have been kept unchanged on both the client and app sides. This decision streamlines efforts and mitigates the risk of unintended consequences.
Additionally, since the EmailInbox model was empty, some new model functions have been created to accommodate specific needs when dealing with database operations.