feat: Notifications mailing system #327

orronai · 2021-10-05T19:03:24Z

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 · 2021-10-05T19:07:13Z

Codecov Report

Merging #327 (859742e) into master (6f13160) will increase coverage by 0.20%.
The diff coverage is 90.62%.

❗ Current head 859742e differs from pull request most recent head 48d4811. Consider uploading reports for the commit 48d4811 to get more accurate results

@@            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

Impacted Files	Coverage Δ
lms/lmsdb/bootstrap.py	`15.96% <20.00%> (+0.17%)`	⬆️
lms/utils/mail.py	`98.27% <96.29%> (+1.05%)`	⬆️
lms/lmsdb/models.py	`91.33% <100.00%> (+0.25%)`	⬆️
lms/lmsweb/__init__.py	`97.82% <100.00%> (+0.15%)`	⬆️
lms/lmsweb/views.py	`93.27% <100.00%> (+0.15%)`	⬆️
lms/models/notifications.py	`90.00% <100.00%> (+0.71%)`	⬆️
lms/models/users.py	`94.28% <100.00%> (+1.69%)`	⬆️
lms/utils/__init__.py	`100.00% <100.00%> (ø)`
lms/utils/config/__init__.py	`100.00% <100.00%> (ø)`
lms/utils/config/celery.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f13160...48d4811. Read the comment docs.

devops/lms.yml

lms/lmsdb/models.py

lms/lmsweb/translations/he/LC_MESSAGES/messages.po

lms/static/my.js

yammesicka · 2021-10-09T15:50:46Z

lms/utils/__init__.py

@@ -0,0 +1,5 @@
+from lms.utils.config import celery


You put this both in utils/ and in utils/config/. Are they both necessary?

I'd like to have some help there

@gal432 , can you give a hand?

I am not sure I understand why did you do that...

lms/utils/mail.py

yammesicka · 2021-10-09T15:55:35Z

lms/utils/mail.py

+                notification.user, notification.message.rstrip(),
+                notification.number,
+            ))
+        notification.delete_instance()


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

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

lms/lmsweb/translations/he/LC_MESSAGES/messages.po

lms/static/my.js

yammesicka · 2021-10-10T21:22:22Z

lms/utils/__init__.py

@@ -0,0 +1,5 @@
+from lms.utils.config import celery


@gal432 , can you give a hand?

yammesicka · 2021-10-10T21:26:35Z

lms/lmsdb/models.py

@@ -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()


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(?)

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?

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

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-ai · 2021-10-15T11:57:29Z

Sourcery Code Quality Report

✅ Merging this PR will increase code quality in the affected files by 0.32%.

Quality metrics	Before	After	Change
Complexity	1.81 ⭐	1.77 ⭐	-0.04 👍
Method Length	37.28 ⭐	37.21 ⭐	-0.07 👍
Working memory	6.70 🙂	6.64 🙂	-0.06 👍
Quality	80.66% ⭐	80.98% ⭐	0.32% 👍

Other metrics	Before	After	Change
Lines	2550	2698	148

Changed files	Quality Before	Quality After	Quality Change
lms/lmsdb/bootstrap.py	79.77% ⭐	80.37% ⭐	0.60% 👍
lms/lmsdb/models.py	84.48% ⭐	84.68% ⭐	0.20% 👍
lms/lmsweb/init.py	64.84% 🙂	62.45% 🙂	-2.39% 👎
lms/lmsweb/views.py	75.31% ⭐	75.47% ⭐	0.16% 👍
lms/models/notifications.py	86.95% ⭐	85.84% ⭐	-1.11% 👎
lms/models/users.py	89.57% ⭐	88.61% ⭐	-0.96% 👎
lms/utils/init.py	%	99.83% ⭐	%
lms/utils/mail.py	85.34% ⭐	85.30% ⭐	-0.04% 👎
tests/conftest.py	90.98% ⭐	91.07% ⭐	0.09% 👍
tests/test_notifications.py	70.46% 🙂	73.00% 🙂	2.54% 👍

Here are some functions in these files that still need a tune-up:

File	Function	Complexity	Length	Working Memory	Quality	Recommendation
lms/lmsweb/views.py	comment	13 🙂	202 😞	8 🙂	50.15% 🙂	Try splitting into smaller methods
tests/test_notifications.py	TestNotification.test_user_commented_after_check	0 ⭐	215 ⛔	12 😞	53.71% 🙂	Try splitting into smaller methods. Extract out complex expressions
lms/lmsweb/views.py	view	8 ⭐	140 😞	10 😞	56.16% 🙂	Try splitting into smaller methods. Extract out complex expressions
lms/lmsweb/views.py	login	5 ⭐	129 😞	12 😞	56.69% 🙂	Try splitting into smaller methods. Extract out complex expressions
lms/lmsdb/models.py	Solution._base_next_unchecked	0 ⭐	95 🙂	20 ⛔	57.42% 🙂	Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

⭐ excellent
🙂 good
😞 poor
⛔ very poor

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!

gal432 · 2021-10-17T20:10:38Z

lms/lmsdb/bootstrap.py

+    log.info(
+        'Adding mail subscription for users, might take some extra time...',
+    )
+    with db_config.database.transaction():


why not update query instead of iterating all users in the system? will be much faster than that

And if its default true, are you sure you need this?

I tried to migrate it on my environment and it created the column with null for all the objects

gal432 · 2021-10-17T20:13:08Z

lms/lmsdb/models.py

+    date = DateTimeField(default=datetime.now)
+
+    @classmethod
+    def distincit_users(cls):


why do you need distinct here?

And please add types

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.

gal432 · 2021-10-17T20:16:17Z

lms/lmsweb/__init__.py

@@ -48,6 +49,9 @@

 webmail = Mail(webapp)

+webscheduler = APScheduler(app=webapp)


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

gal432 · 2021-10-17T20:17:30Z

lms/lmsweb/views.py

+def mail_subscription(subscription: str):
+    success_state = users.change_mail_subscription(current_user, subscription)
+    title = _('Mail Subscription')
+    if subscription == 'subscribe':


move 'subscribe' and 'unsubscribe' to enum and replace the usage

gal432 · 2021-10-17T20:19:22Z

lms/static/my.js

+    xhr.setRequestHeader('Content-Type', 'application/json');
+    xhr.responseType = 'json';
+    xhr.onreadystatechange = () => {
+      if (xhr.readyState === 4) {


why not === 5? please make move 4 to indicative variable name

It's the same as all the other xhr's in the JS files

gal432 · 2021-10-17T20:20:44Z

lms/utils/__init__.py

@@ -0,0 +1,5 @@
+from lms.utils.config import celery


I am not sure I understand why did you do that...

gal432 · 2021-10-17T20:21:30Z

lms/utils/config/celery.py

@@ -0,0 +1,24 @@
+import os


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?

gal432 · 2021-10-17T20:23:00Z

lms/utils/mail.py

+    seconds=30,
+)
+def send_all_notifications_mails():
+    for mail_message_user in MailMessage.distincit_users():


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

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.

feat: Notifications mailing system

1ce98cd

- Added mailing system every 2 hours for notifications - Added subscription option for every user - Added celery task for sending mails - Added translations for babel

orronai linked an issue Oct 5, 2021 that may be closed by this pull request

Create mailing system for the notifications #224

Open

orronai added 3 commits October 6, 2021 09:46

- Added tests

1e9e28c

Added to the config an attribute

d3f23df

Fixed a test

91b4a0e

orronai requested review from yammesicka and gal432 October 6, 2021 07:55

Merged master and fixed conflicts

4c475e1

yammesicka reviewed Oct 9, 2021

View reviewed changes

devops/lms.yml Outdated Show resolved Hide resolved

yammesicka reviewed Oct 9, 2021

View reviewed changes

devops/lms.yml Outdated Show resolved Hide resolved

yammesicka requested changes Oct 9, 2021

View reviewed changes

- Changed The mails table and columns

c7d1fb6

- Added plural for babel message - Changed some logic of the mails

yammesicka requested changes Oct 10, 2021

View reviewed changes

orronai added 4 commits October 11, 2021 10:49

- Merged with master branch

b741e53

- Fixed translations - Added a toast on the bast.html file - Added toast while changing mail subscription

Merge branch 'master' of https://github.com/PythonFreeCourse/lms into…

3274ef7

… notifications-mailing-system

Changed the notifications viewed logic of reading

9e7dead

fix merge conflicts

48d4811

gal432 requested changes Oct 17, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Notifications mailing system #327

feat: Notifications mailing system #327

orronai commented Oct 5, 2021

codecov bot commented Oct 5, 2021 •

edited

yammesicka Oct 9, 2021

orronai Oct 10, 2021

yammesicka Oct 10, 2021

gal432 Oct 17, 2021

yammesicka Oct 9, 2021

orronai Oct 10, 2021

yammesicka Oct 10, 2021

yammesicka Oct 10, 2021

orronai Oct 11, 2021 •

edited

yammesicka Oct 15, 2021

orronai Oct 15, 2021

sourcery-ai bot commented Oct 15, 2021

gal432 Oct 17, 2021

gal432 Oct 17, 2021

orronai Oct 18, 2021

gal432 Oct 17, 2021

gal432 Oct 17, 2021

orronai Oct 18, 2021

gal432 Oct 17, 2021

gal432 Oct 17, 2021

gal432 Oct 17, 2021

orronai Oct 18, 2021

gal432 Oct 17, 2021

gal432 Oct 17, 2021

gal432 Oct 17, 2021

orronai Oct 18, 2021

		@@ -48,6 +49,9 @@

		webmail = Mail(webapp)

		webscheduler = APScheduler(app=webapp)

feat: Notifications mailing system #327

Are you sure you want to change the base?

feat: Notifications mailing system #327

Conversation

orronai commented Oct 5, 2021

codecov bot commented Oct 5, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orronai Oct 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcery-ai bot commented Oct 15, 2021

Sourcery Code Quality Report

Legend and Explanation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 5, 2021 •

edited

orronai Oct 11, 2021 •

edited