-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: Notifications mailing system #327
base: master
Are you sure you want to change the base?
Conversation
orronai
commented
Oct 5, 2021
- Added mailing system every 2 hours for notifications
- Added subscription option for every user
- Added celery task for sending mails
- Added translations for babel
- Added mailing system every 2 hours for notifications - Added subscription option for every user - Added celery task for sending mails - Added translations for babel
Codecov Report
@@ Coverage Diff @@
## master #327 +/- ##
==========================================
+ Coverage 84.08% 84.28% +0.20%
==========================================
Files 63 66 +3
Lines 2946 3035 +89
==========================================
+ Hits 2477 2558 +81
- Misses 469 477 +8
Continue to review full report at Codecov.
|
@@ -0,0 +1,5 @@ | |||
from lms.utils.config import celery |
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.
You put this both in utils/
and in utils/config/
. Are they both necessary?
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.
I'd like to have some help there
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.
@gal432 , can you give a hand?
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.
I am not sure I understand why did you do that...
lms/utils/mail.py
Outdated
notification.user, notification.message.rstrip(), | ||
notification.number, | ||
)) | ||
notification.delete_instance() |
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.
Maybe we should just delete all the instances at once?
The current behavior may take a very long time for large courses.
Note: We should beware of a race condition - deleting messages that we've INSERTed while we've sent the emails
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.
I changed the logic there.
But not exactly the way you wanted.
What do you think that we'd delete all the instances at once after the loop of sending all the messages?
- Added plural for babel message - Changed some logic of the mails
@@ -0,0 +1,5 @@ | |||
from lms.utils.config import celery |
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.
@gal432 , can you give a hand?
lms/lmsdb/models.py
Outdated
@@ -308,6 +309,9 @@ class Notification(BaseModel): | |||
|
|||
def read(self) -> bool: | |||
self.viewed = True | |||
mail = MailMessage.get_or_none(MailMessage.notification == self) | |||
if mail: | |||
mail.delete_instance() |
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.
I like the logic, but I'm afraid it's too heavy - be aware that every delete
is a query, and the most costy operation in our system is the connection with the database.
Maybe if you can wrap things with transaction somehow and make sure that everything will be processed together(?)
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.
This function is only to reading a single notification. Maybe did you mean the read
function in models/notifications.py
and the read_related
function where the user might read few notifications at once?
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.
Yeah, try to think what happens if the function that calls read
need to process about 1,000 notifications per second. We should have bulk_read
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.
I changed the logic of the notifications viewed, in this way it'd be deleted with all the other mails, but it wouldn't be sent to the user.
- Fixed translations - Added a toast on the bast.html file - Added toast while changing mail subscription
… notifications-mailing-system
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 0.32%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
log.info( | ||
'Adding mail subscription for users, might take some extra time...', | ||
) | ||
with db_config.database.transaction(): |
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.
why not update
query instead of iterating all users in the system? will be much faster than that
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.
And if its default true, are you sure you need this?
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.
I tried to migrate it on my environment and it created the column with null for all the objects
date = DateTimeField(default=datetime.now) | ||
|
||
@classmethod | ||
def distincit_users(cls): |
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.
why do you need distinct 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.
And please add types
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.
For each message there is an instance. I need distinct in order to get the distinct users, for sending only 1 message for each one of them.
@@ -48,6 +49,9 @@ | |||
|
|||
webmail = Mail(webapp) | |||
|
|||
webscheduler = APScheduler(app=webapp) |
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.
why this scheduler is part of lmsweb? is it running under the WSGI app?
if so, it's bad. you should use the celery scheduler and run specific daemon for that
def mail_subscription(subscription: str): | ||
success_state = users.change_mail_subscription(current_user, subscription) | ||
title = _('Mail Subscription') | ||
if subscription == 'subscribe': |
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.
move 'subscribe'
and 'unsubscribe'
to enum and replace the usage
xhr.setRequestHeader('Content-Type', 'application/json'); | ||
xhr.responseType = 'json'; | ||
xhr.onreadystatechange = () => { | ||
if (xhr.readyState === 4) { |
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.
why not === 5
? please make move 4
to indicative variable name
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's the same as all the other xhr's in the JS files
@@ -0,0 +1,5 @@ | |||
from lms.utils.config import celery |
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.
I am not sure I understand why did you do that...
@@ -0,0 +1,24 @@ | |||
import os |
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.
I don't think utils
is the right place to define celery elements. why not making a new python module for the async mail queue system?
seconds=30, | ||
) | ||
def send_all_notifications_mails(): | ||
for mail_message_user in MailMessage.distincit_users(): |
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.
This method is not atomic. you are deleting emails that you didn't send for sure. you should make your delete method works only on the sent objects
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.
I'm sending the mails for users who are subscribed and the messages that the users haven't seen yet. I also delete the instances of them right here, also the messages that we don't send in purpose.