From 1ce98cd534f9891885aa4769a2a11b969b3cba84 Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Tue, 5 Oct 2021 22:01:52 +0300 Subject: [PATCH 1/6] feat: Notifications mailing system - Added mailing system every 2 hours for notifications - Added subscription option for every user - Added celery task for sending mails - Added translations for babel --- devops/lms.yml | 20 ++++++ lms/lmsdb/bootstrap.py | 21 +++++++ lms/lmsdb/models.py | 20 ++++++ lms/lmsweb/__init__.py | 5 ++ lms/lmsweb/config.py.example | 3 + .../translations/he/LC_MESSAGES/messages.po | 61 ++++++++++++------- lms/lmsweb/views.py | 8 ++- lms/models/notifications.py | 6 +- lms/models/users.py | 11 ++++ lms/static/my.css | 6 ++ lms/static/my.js | 15 +++++ lms/templates/user.html | 6 ++ lms/utils/__init__.py | 5 ++ lms/utils/config/__init__.py | 5 ++ lms/utils/config/celery.py | 24 ++++++++ lms/utils/mail.py | 38 ++++++++++-- requirements.txt | 1 + 17 files changed, 224 insertions(+), 31 deletions(-) create mode 100644 lms/utils/config/__init__.py create mode 100644 lms/utils/config/celery.py diff --git a/devops/lms.yml b/devops/lms.yml index 78749285..8f455a26 100644 --- a/devops/lms.yml +++ b/devops/lms.yml @@ -29,6 +29,25 @@ services: - ./rabbitmq.cookie:/var/lib/rabbitmq/.erlang.cookie networks: - lms + + utils: + image: lms:latest + command: celery -A lms.utils worker + volumes: + - ../lms/utils/:/app_dir/lms/utils/ + environment: + - CELERY_RABBITMQ_ERLANG_COOKIE=AAVyo5djdSMGIZXiwEQs3JeVaBx5l14z + - CELERY_RABBITMQ_DEFAULT_USER=rabbit-user + - CELERY_RABBITMQ_DEFAULT_PASS=YgKlCvnYVzpTa3T9adG3NrMoUNe4Z5aZ + - CELERY_UTILS_VHOST=utils + - CELERY_RABBITMQ_HOST=rabbitmq + - CELERY_RABBITMQ_PORT=5672 + links: + - rabbitmq + depends_on: + - rabbitmq + networks: + - lms checks-sandbox: image: lms:latest @@ -78,6 +97,7 @@ services: - CELERY_RABBITMQ_ERLANG_COOKIE=AAVyo5djdSMGIZXiwEQs3JeVaBx5l14z - CELERY_RABBITMQ_DEFAULT_USER=rabbit-user - CELERY_RABBITMQ_DEFAULT_PASS=YgKlCvnYVzpTa3T9adG3NrMoUNe4Z5aZ + - CELERY_UTILS_VHOST=utils - CELERY_CHECKS_PUBLIC_VHOST=lmstests-public - CELERY_CHECKS_SANDBOX_VHOST=lmstests-sandbox - CELERY_RABBITMQ_HOST=rabbitmq diff --git a/lms/lmsdb/bootstrap.py b/lms/lmsdb/bootstrap.py index 740c1f20..ac45d9b7 100644 --- a/lms/lmsdb/bootstrap.py +++ b/lms/lmsdb/bootstrap.py @@ -255,6 +255,18 @@ def _add_uuid_to_users_table(table: Model, _column: Field) -> None: user.save() +def _add_mail_subscription_to_users_table( + table: Model, _column: Field, +) -> None: + log.info( + 'Adding mail subscription for users, might take some extra time...', + ) + with db_config.database.transaction(): + for user in table: + user.mail_subscription = True + user.save() + + def _api_keys_migration() -> bool: User = models.User _add_not_null_column(User, User.api_key, _add_api_keys_to_users_table) @@ -299,6 +311,14 @@ def _uuid_migration() -> bool: return True +def _mail_subscription() -> bool: + User = models.User + _add_not_null_column( + User, User.mail_subscription, _add_mail_subscription_to_users_table, + ) + return True + + def main(): with models.database.connection_context(): if models.database.table_exists(models.Exercise.__name__.lower()): @@ -311,6 +331,7 @@ def main(): _api_keys_migration() _last_course_viewed_migration() _uuid_migration() + _mail_subscription() models.database.create_tables(models.ALL_MODELS, safe=True) diff --git a/lms/lmsdb/models.py b/lms/lmsdb/models.py index cf3c8bc1..52deba7e 100644 --- a/lms/lmsdb/models.py +++ b/lms/lmsdb/models.py @@ -166,6 +166,7 @@ class User(UserMixin, BaseModel): api_key = CharField() last_course_viewed = ForeignKeyField(Course, null=True) uuid = UUIDField(default=uuid4, unique=True) + mail_subscription = BooleanField(default=True) def get_id(self): return str(self.uuid) @@ -346,6 +347,25 @@ def on_notification_saved( instance.delete_instance() +class NotificationMail(BaseModel): + user = ForeignKeyField(User, unique=True) + number = IntegerField(default=1) + message = TextField() + + @classmethod + def get_or_create_notification_mail(cls, user: User, message: str): + instance, created = cls.get_or_create(**{ + cls.user.name: user, + }, defaults={ + cls.message.name: message, + }) + if not created: + instance.message += f'\n{message}' + instance.number += 1 + instance.save() + return instance + + class Exercise(BaseModel): subject = CharField() date = DateTimeField() diff --git a/lms/lmsweb/__init__.py b/lms/lmsweb/__init__.py index ec48f2ae..24af2244 100644 --- a/lms/lmsweb/__init__.py +++ b/lms/lmsweb/__init__.py @@ -2,6 +2,7 @@ import shutil from flask import Flask +from flask_apscheduler import APScheduler # type: ignore from flask_babel import Babel # type: ignore from flask_limiter import Limiter # type: ignore from flask_limiter.util import get_remote_address # type: ignore @@ -44,6 +45,10 @@ webmail = Mail(webapp) +webscheduler = APScheduler() +webscheduler.init_app(webapp) +webscheduler.start() + # Must import files after app's creation from lms.lmsdb import models # NOQA: F401, E402, I202 diff --git a/lms/lmsweb/config.py.example b/lms/lmsweb/config.py.example index c3bb4b0c..6b506ee8 100644 --- a/lms/lmsweb/config.py.example +++ b/lms/lmsweb/config.py.example @@ -65,5 +65,8 @@ LOCALE = 'en' LIMITS_PER_MINUTE = 5 LIMITS_PER_HOUR = 50 +# Scheduler +SCHEDULER_API_ENABLED = True + # Change password settings MAX_INVALID_PASSWORD_TRIES = 5 diff --git a/lms/lmsweb/translations/he/LC_MESSAGES/messages.po b/lms/lmsweb/translations/he/LC_MESSAGES/messages.po index bffb9c09..35313cb3 100644 --- a/lms/lmsweb/translations/he/LC_MESSAGES/messages.po +++ b/lms/lmsweb/translations/he/LC_MESSAGES/messages.po @@ -7,7 +7,7 @@ msgid "" msgstr "" "Project-Id-Version: 1.0\n" "Report-Msgid-Bugs-To: EMAIL@ADDRESS\n" -"POT-Creation-Date: 2021-10-01 10:15+0300\n" +"POT-Creation-Date: 2021-10-05 21:56+0300\n" "PO-Revision-Date: 2021-09-29 11:30+0300\n" "Last-Translator: Or Ronai\n" "Language: he\n" @@ -18,7 +18,7 @@ msgstr "" "Content-Transfer-Encoding: 8bit\n" "Generated-By: Babel 2.9.1\n" -#: lmsdb/models.py:815 +#: lmsdb/models.py:851 msgid "Fatal error" msgstr "כישלון חמור" @@ -294,7 +294,7 @@ msgstr "חמ\"ל תרגילים" msgid "Name" msgstr "שם" -#: templates/status.html:13 templates/user.html:43 +#: templates/status.html:13 templates/user.html:49 msgid "Checked" msgstr "נבדק/ו" @@ -342,56 +342,59 @@ msgstr "פרטי משתמש" msgid "Actions" msgstr "פעולות" -#: templates/user.html:24 +#: templates/user.html:23 +msgid "Mail Subscription" +msgstr "מנוי לדואר" + +#: templates/user.html:30 msgid "Exercises Submitted" msgstr "תרגילים שהוגשו" -#: templates/user.html:29 -#, fuzzy +#: templates/user.html:35 msgid "Course name" msgstr "שם קורס" -#: templates/user.html:30 +#: templates/user.html:36 msgid "Exercise name" msgstr "שם תרגיל" -#: templates/user.html:31 +#: templates/user.html:37 msgid "Submission status" msgstr "מצב הגשה" -#: templates/user.html:32 +#: templates/user.html:38 msgid "Submission" msgstr "הגשה" -#: templates/user.html:33 +#: templates/user.html:39 msgid "Checker" msgstr "בודק" -#: templates/user.html:43 +#: templates/user.html:49 msgid "Submitted" msgstr "הוגש" -#: templates/user.html:43 +#: templates/user.html:49 msgid "Not submitted" msgstr "לא הוגש" -#: templates/user.html:54 +#: templates/user.html:60 msgid "Notes" msgstr "פתקיות" -#: templates/user.html:59 templates/user.html:61 +#: templates/user.html:65 templates/user.html:67 msgid "New Note" msgstr "פתקית חדשה" -#: templates/user.html:65 +#: templates/user.html:71 msgid "Related Exercise" msgstr "תרגיל משויך" -#: templates/user.html:74 +#: templates/user.html:80 msgid "Privacy Level" msgstr "רמת פרטיות" -#: templates/user.html:80 +#: templates/user.html:86 msgid "Add Note" msgstr "הוסף פתקית" @@ -468,12 +471,12 @@ msgstr "הערות בודק" msgid "Done Checking" msgstr "סיום בדיקה" -#: utils/mail.py:25 +#: utils/mail.py:28 #, python-format msgid "Confirmation mail - %(site_name)s" msgstr "מייל אימות - %(site_name)s" -#: utils/mail.py:32 +#: utils/mail.py:35 #, python-format msgid "" "Hello %(fullname)s,\n" @@ -482,12 +485,12 @@ msgstr "" "שלום %(fullname)s,\n" "לינק האימות שלך למערכת הוא: %(link)s" -#: utils/mail.py:42 +#: utils/mail.py:45 #, python-format msgid "Reset password mail - %(site_name)s" msgstr "מייל איפוס סיסמה - %(site_name)s" -#: utils/mail.py:49 +#: utils/mail.py:52 #, python-format msgid "" "Hello %(fullname)s,\n" @@ -496,12 +499,12 @@ msgstr "" "שלום %(fullname)s,\n" "לינק לצורך איפוס הסיסמה שלך הוא: %(link)s" -#: utils/mail.py:58 +#: utils/mail.py:61 #, python-format msgid "Changing password - %(site_name)s" msgstr "שינוי סיסמה - %(site_name)s" -#: utils/mail.py:62 +#: utils/mail.py:65 #, python-format msgid "" "Hello %(fullname)s. Your password in %(site_name)s site has been changed." @@ -513,3 +516,15 @@ msgstr "" "אם אתה לא עשית את זה צור קשר עם הנהלת האתר.\n" "כתובת המייל: %(site_mail)s" +#: utils/mail.py:77 +#, fuzzy, python-format +msgid "New notification - %(site_name)s" +msgstr "התראה חדשה - %(site_name)s" + +#: utils/mail.py:81 +#, python-format +msgid "" +"Hello %(fullname)s. You have %(number)d new notification/s:\n" +"%(message)s" +msgstr "היי %(fullname)s. יש לך %(number)d התראה/ות חדשה/ות:\n%(message)s" + diff --git a/lms/lmsweb/views.py b/lms/lmsweb/views.py index cc5c9689..aebe01ea 100644 --- a/lms/lmsweb/views.py +++ b/lms/lmsweb/views.py @@ -34,7 +34,7 @@ PERMISSIVE_CORS, get_next_url, login_manager, ) from lms.models import ( - comments, notes, notifications, share_link, solutions, upload, + comments, notes, notifications, share_link, solutions, upload, users, ) from lms.models.errors import ( FileSizeError, ForbiddenPermission, LmsError, @@ -369,6 +369,12 @@ def read_all_notification(): return jsonify({'success': success_state}) +@webapp.route('/subscribe/', methods=['PATCH']) +def mail_subscription(act: str): + success_state = users.change_mail_subscription(current_user, act) + return jsonify({'success': success_state}) + + @webapp.route('/share', methods=['POST']) @login_required def share(): diff --git a/lms/models/notifications.py b/lms/models/notifications.py index cc3869d4..cfd10229 100644 --- a/lms/models/notifications.py +++ b/lms/models/notifications.py @@ -1,7 +1,7 @@ import enum from typing import Iterable, Optional -from lms.lmsdb.models import Notification, User +from lms.lmsdb.models import Notification, NotificationMail, User class NotificationKind(enum.Enum): @@ -45,6 +45,8 @@ def send( related_id: Optional[int] = None, action_url: Optional[str] = None, ) -> Notification: - return Notification.send( + notification = Notification.send( user, kind.value, message, related_id, action_url, ) + NotificationMail.get_or_create_notification_mail(user, message) + return notification diff --git a/lms/models/users.py b/lms/models/users.py index afdb1de8..20879cd5 100644 --- a/lms/models/users.py +++ b/lms/models/users.py @@ -38,3 +38,14 @@ def auth(username: str, password: str) -> User: def generate_user_token(user: User) -> str: return SERIALIZER.dumps(user.mail_address, salt=retrieve_salt(user)) + + +def change_mail_subscription(user: User, act: str): + if act == 'subscribe': + user.mail_subscription = True + elif act == 'unsubscribe': + user.mail_subscription = False + else: + return False + user.save() + return True diff --git a/lms/static/my.css b/lms/static/my.css index a637d906..e4a99d10 100644 --- a/lms/static/my.css +++ b/lms/static/my.css @@ -833,6 +833,12 @@ code .grader-add .fa { margin-bottom: 5em; } +.mail-subscription { + position: relative; + display: inline-block; + margin-right: -1.4rem; +} + .user-notes { display: flex; flex-flow: row wrap; diff --git a/lms/static/my.js b/lms/static/my.js index 3b04a3ab..f61e6330 100644 --- a/lms/static/my.js +++ b/lms/static/my.js @@ -118,6 +118,20 @@ function trackReadAllNotificationsButton(button) { }); } +function trackMailSubscriptionCheckbox() { + const checkbox = document.getElementById('mail-subscription-checkbox'); + if (checkbox === null) { + return; + } + + checkbox.addEventListener('change', (e) => { + const act = (e.currentTarget.checked) ? 'subscribe' : 'unsubscribe'; + const request = new XMLHttpRequest(); + request.open('PATCH', `/subscribe/${act}`); + return request.send(); + }); +} + function postUploadMessageUpdate(feedbacks, uploadStatus, matchesSpan, missesSpan) { const matches = uploadStatus.exercise_matches; const misses = uploadStatus.exercise_misses; @@ -158,6 +172,7 @@ window.isUserGrader = isUserGrader; window.addEventListener('load', () => { updateNotificationsBadge(); trackReadAllNotificationsButton(document.getElementById('read-notifications')); + trackMailSubscriptionCheckbox(); const codeElement = document.getElementById('code-view'); if (codeElement !== null) { const codeElementData = codeElement.dataset; diff --git a/lms/templates/user.html b/lms/templates/user.html index aa523e15..0533d28a 100644 --- a/lms/templates/user.html +++ b/lms/templates/user.html @@ -17,6 +17,12 @@

{{ _('Actions') }}:

diff --git a/lms/utils/__init__.py b/lms/utils/__init__.py index e69de29b..e74190a9 100644 --- a/lms/utils/__init__.py +++ b/lms/utils/__init__.py @@ -0,0 +1,5 @@ +from lms.utils.config import celery + +celery_app = celery.app + +__all__ = ('celery_app',) diff --git a/lms/utils/config/__init__.py b/lms/utils/config/__init__.py new file mode 100644 index 00000000..e74190a9 --- /dev/null +++ b/lms/utils/config/__init__.py @@ -0,0 +1,5 @@ +from lms.utils.config import celery + +celery_app = celery.app + +__all__ = ('celery_app',) diff --git a/lms/utils/config/celery.py b/lms/utils/config/celery.py new file mode 100644 index 00000000..a8548165 --- /dev/null +++ b/lms/utils/config/celery.py @@ -0,0 +1,24 @@ +import os + +from celery import Celery + +CELERY_RABBITMQ_ERLANG_COOKIE = os.getenv('CELERY_RABBITMQ_ERLANG_COOKIE') +CELERY_RABBITMQ_DEFAULT_USER = os.getenv('CELERY_RABBITMQ_DEFAULT_USER') +CELERY_RABBITMQ_DEFAULT_PASS = os.getenv('CELERY_RABBITMQ_DEFAULT_PASS') +CELERY_UTILS_VHOST = os.getenv('CELERY_UTILS_VHOST') +CELERY_RABBITMQ_HOST = os.getenv('CELERY_RABBITMQ_HOST') +CELERY_RABBITMQ_PORT = os.getenv('CELERY_RABBITMQ_PORT', 222) + +public_broker_url = (f'amqp://{CELERY_RABBITMQ_DEFAULT_USER}:' + f'{CELERY_RABBITMQ_DEFAULT_PASS}@' + f'{CELERY_RABBITMQ_HOST}:{CELERY_RABBITMQ_PORT}/' + f'{CELERY_UTILS_VHOST}') +app = Celery('utils', broker=public_broker_url) + +app.conf.update( + task_serializer='json', + accept_content=['json'], # Ignore other content + result_serializer='json', + enable_utc=True, + task_always_eager=bool(os.getenv('FLASK_DEBUG')), +) diff --git a/lms/utils/mail.py b/lms/utils/mail.py index 5d22311f..89a322c0 100644 --- a/lms/utils/mail.py +++ b/lms/utils/mail.py @@ -4,17 +4,20 @@ from flask_babel import gettext as _ # type: ignore from flask_mail import Message # type: ignore -from lms.lmsdb.models import User -from lms.lmsweb import config, webapp, webmail +from lms.lmsdb.models import NotificationMail, User +from lms.lmsweb import config, webapp, webmail, webscheduler from lms.models.users import generate_user_token +from lms.utils.config.celery import app +@app.task def send_message(func): @wraps(func) def wrapper(*args, **kwargs): - msg = func(*args, **kwargs) - if not webapp.config.get('DISABLE_MAIL'): - webmail.send(msg) + with webapp.app_context(): + msg = func(*args, **kwargs) + if not webapp.config.get('DISABLE_MAIL'): + return webmail.send(msg) return wrapper @@ -67,3 +70,28 @@ def send_change_password_mail(user: User) -> Message: site_mail=config.MAIL_DEFAULT_SENDER, ) return msg + + +@send_message +def send_notification_mail(user: User, message: str, number: int) -> Message: + subject = _( + 'New notification - %(site_name)s', site_name=config.SITE_NAME, + ) + msg = Message(subject, recipients=[user.mail_address]) + msg.body = _( + 'Hello %(fullname)s. You have %(number)d ' + 'new notification/s:\n%(message)s', + fullname=user.fullname, number=number, message=message, + ) + return msg + + +@webscheduler.task('interval', id='mail_notifications', hours=2) +def send_all_notifications_mails(): + for notification in NotificationMail.select(): + if notification.user.mail_subscription: + send_message(send_notification_mail( + notification.user, notification.message.rstrip(), + notification.number, + )) + notification.delete_instance() diff --git a/requirements.txt b/requirements.txt index b8951f64..8228c1df 100644 --- a/requirements.txt +++ b/requirements.txt @@ -30,6 +30,7 @@ flake8-tidy-imports==4.4.1 flake8-todo==0.7 Flask==2.0.1 Flask-Admin==1.5.8 +Flask-APScheduler==1.12.2 Flask-Babel==2.0.0 Flask-Limiter==1.4 Flask-Login==0.5.0 From 1e9e28c99f6524bef5e99298efa309e1e88816bb Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Wed, 6 Oct 2021 09:46:25 +0300 Subject: [PATCH 2/6] - Added tests --- lms/lmsdb/models.py | 4 ++++ lms/lmsweb/__init__.py | 3 +-- lms/utils/mail.py | 5 ++++- tests/conftest.py | 9 ++++++++- tests/test_notifications.py | 38 ++++++++++++++++++++++++++++++++++++- 5 files changed, 54 insertions(+), 5 deletions(-) diff --git a/lms/lmsdb/models.py b/lms/lmsdb/models.py index 52deba7e..62b4022a 100644 --- a/lms/lmsdb/models.py +++ b/lms/lmsdb/models.py @@ -365,6 +365,10 @@ def get_or_create_notification_mail(cls, user: User, message: str): instance.save() return instance + @classmethod + def get_instances_number(cls): + return cls.select(fn.Count(cls.id)) + class Exercise(BaseModel): subject = CharField() diff --git a/lms/lmsweb/__init__.py b/lms/lmsweb/__init__.py index 24af2244..51b91eac 100644 --- a/lms/lmsweb/__init__.py +++ b/lms/lmsweb/__init__.py @@ -45,8 +45,7 @@ webmail = Mail(webapp) -webscheduler = APScheduler() -webscheduler.init_app(webapp) +webscheduler = APScheduler(app=webapp) webscheduler.start() diff --git a/lms/utils/mail.py b/lms/utils/mail.py index 89a322c0..c8644c57 100644 --- a/lms/utils/mail.py +++ b/lms/utils/mail.py @@ -86,7 +86,10 @@ def send_notification_mail(user: User, message: str, number: int) -> Message: return msg -@webscheduler.task('interval', id='mail_notifications', hours=2) +@webscheduler.task( + 'interval', id='mail_notifications', + hours=config.DEFAULT_DO_TASKS_EVERY_HOURS, +) def send_all_notifications_mails(): for notification in NotificationMail.select(): if notification.user.mail_subscription: diff --git a/tests/conftest.py b/tests/conftest.py index 2b73bb3f..bac7d6b8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -16,7 +16,8 @@ from lms.lmsdb.models import ( ALL_MODELS, Comment, CommentText, Course, Exercise, Note, Notification, - Role, RoleOptions, SharedSolution, Solution, User, UserCourse, + NotificationMail, Role, RoleOptions, SharedSolution, Solution, User, + UserCourse, ) from lms.extractors.base import File from lms.lmstests.public import celery_app as public_app @@ -281,6 +282,12 @@ def create_notification( ) +def create_notification_mail(student_user: User) -> NotificationMail: + return NotificationMail.get_or_create_notification_mail( + user=student_user, message='Test message', + ) + + def create_course(index: int = 0) -> Course: return Course.create( number=index, diff --git a/tests/test_notifications.py b/tests/test_notifications.py index 1d1f2704..a2202128 100644 --- a/tests/test_notifications.py +++ b/tests/test_notifications.py @@ -1,10 +1,15 @@ +import datetime import random import string +from unittest.mock import Mock, patch from flask import json import pytest # type: ignore -from lms.lmsdb.models import Exercise, Notification, Solution, User +from lms.lmsdb.models import ( + Exercise, Notification, NotificationMail, Solution, User, +) +from lms.lmsweb import webapp from lms.models import notifications, solutions from tests import conftest @@ -216,3 +221,34 @@ def test_user_commented_after_check( ) assert another_comment_response.status_code == 200 assert len(list(notifications.get(staff_user))) == 2 + + @staticmethod + def test_notifications_subscriptions(student_user: User): + client = conftest.get_logged_user(username=student_user.username) + unsubscribe_response = client.patch('/subscribe/unsubscribe') + assert unsubscribe_response.status_code == 200 + student_user = User.get_by_id(student_user.id) + assert student_user.mail_subscription == False + + subscribe_response = client.patch('/subscribe/subscribe') + assert subscribe_response.status_code == 200 + student_user = User.get_by_id(student_user.id) + assert student_user.mail_subscription == True + + wrong_response = client.patch('/subscribe/wrong') + json_data = json.loads(wrong_response.get_data(as_text=True)) + assert json_data['success'] == False + + @staticmethod + def test_send_mails_notifications(student_user: User): + conftest.create_notification_mail(student_user) + conftest.create_notification_mail(student_user) + assert NotificationMail.get_instances_number() == 2 + + mock_hours=webapp.config.get('DEFAULT_DO_TASKS_EVERY_HOURS') + fake_date = ( + datetime.datetime.now() + + datetime.timedelta(hours=mock_hours, minutes=1) + ) + with patch('datetime.datetime', Mock(return_value=fake_date)): + assert NotificationMail.get_instances_number() == 0 From d3f23df183042f45eb03b98304ef0b494c2ab18f Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Wed, 6 Oct 2021 10:03:06 +0300 Subject: [PATCH 3/6] Added to the config an attribute --- lms/lmsweb/config.py.example | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/lmsweb/config.py.example b/lms/lmsweb/config.py.example index 6b506ee8..2e1fd4b0 100644 --- a/lms/lmsweb/config.py.example +++ b/lms/lmsweb/config.py.example @@ -67,6 +67,7 @@ LIMITS_PER_HOUR = 50 # Scheduler SCHEDULER_API_ENABLED = True +DEFAULT_DO_TASKS_EVERY_HOURS = 2 # Change password settings MAX_INVALID_PASSWORD_TRIES = 5 From 91b4a0ef0ba0fefef38e481d5d9f076bbf901db7 Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Wed, 6 Oct 2021 10:46:08 +0300 Subject: [PATCH 4/6] Fixed a test --- tests/test_notifications.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/tests/test_notifications.py b/tests/test_notifications.py index a2202128..1c7ad153 100644 --- a/tests/test_notifications.py +++ b/tests/test_notifications.py @@ -1,7 +1,5 @@ -import datetime import random import string -from unittest.mock import Mock, patch from flask import json import pytest # type: ignore @@ -9,8 +7,8 @@ from lms.lmsdb.models import ( Exercise, Notification, NotificationMail, Solution, User, ) -from lms.lmsweb import webapp from lms.models import notifications, solutions +from lms.utils.mail import send_all_notifications_mails from tests import conftest @@ -245,10 +243,5 @@ def test_send_mails_notifications(student_user: User): conftest.create_notification_mail(student_user) assert NotificationMail.get_instances_number() == 2 - mock_hours=webapp.config.get('DEFAULT_DO_TASKS_EVERY_HOURS') - fake_date = ( - datetime.datetime.now() + - datetime.timedelta(hours=mock_hours, minutes=1) - ) - with patch('datetime.datetime', Mock(return_value=fake_date)): - assert NotificationMail.get_instances_number() == 0 + send_all_notifications_mails() + assert NotificationMail.get_instances_number() == 0 From c7d1fb674298cfb103cb584e2069ef84da6b21e5 Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Sun, 10 Oct 2021 11:51:58 +0300 Subject: [PATCH 5/6] - Changed The mails table and columns - Added plural for babel message - Changed some logic of the mails --- devops/lms.yml | 5 +- lms/lmsdb/models.py | 32 ++++--- .../translations/he/LC_MESSAGES/messages.po | 94 ++++++++++--------- lms/lmsweb/views.py | 6 +- lms/models/notifications.py | 4 +- lms/models/users.py | 6 +- lms/static/my.js | 4 +- lms/utils/mail.py | 36 ++++--- tests/conftest.py | 11 +-- tests/test_notifications.py | 18 ++-- 10 files changed, 119 insertions(+), 97 deletions(-) diff --git a/devops/lms.yml b/devops/lms.yml index 1a21313c..1c04aab4 100644 --- a/devops/lms.yml +++ b/devops/lms.yml @@ -30,7 +30,7 @@ services: networks: - lms - utils: + mailer: image: lms:latest command: celery -A lms.utils worker volumes: @@ -39,7 +39,7 @@ services: - CELERY_RABBITMQ_ERLANG_COOKIE=AAVyo5djdSMGIZXiwEQs3JeVaBx5l14z - CELERY_RABBITMQ_DEFAULT_USER=rabbit-user - CELERY_RABBITMQ_DEFAULT_PASS=YgKlCvnYVzpTa3T9adG3NrMoUNe4Z5aZ - - CELERY_UTILS_VHOST=utils + - CELERY_MAILER_VHOST=utils - CELERY_RABBITMQ_HOST=rabbitmq - CELERY_RABBITMQ_PORT=5672 links: @@ -97,7 +97,6 @@ services: - CELERY_RABBITMQ_ERLANG_COOKIE=AAVyo5djdSMGIZXiwEQs3JeVaBx5l14z - CELERY_RABBITMQ_DEFAULT_USER=rabbit-user - CELERY_RABBITMQ_DEFAULT_PASS=YgKlCvnYVzpTa3T9adG3NrMoUNe4Z5aZ - - CELERY_UTILS_VHOST=utils - CELERY_CHECKS_PUBLIC_VHOST=lmstests-public - CELERY_CHECKS_SANDBOX_VHOST=lmstests-sandbox - CELERY_RABBITMQ_HOST=rabbitmq diff --git a/lms/lmsdb/models.py b/lms/lmsdb/models.py index 44b073e2..82b1b21c 100644 --- a/lms/lmsdb/models.py +++ b/lms/lmsdb/models.py @@ -309,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() return bool(self.save()) @classmethod @@ -376,23 +379,22 @@ def on_notification_saved( instance.delete_instance() -class NotificationMail(BaseModel): - user = ForeignKeyField(User, unique=True) - number = IntegerField(default=1) - message = TextField() +class MailMessage(BaseModel): + user = ForeignKeyField(User, backref='mails') + notification = ForeignKeyField(Notification, backref='mails') + date = DateTimeField(default=datetime.now) @classmethod - def get_or_create_notification_mail(cls, user: User, message: str): - instance, created = cls.get_or_create(**{ - cls.user.name: user, - }, defaults={ - cls.message.name: message, - }) - if not created: - instance.message += f'\n{message}' - instance.number += 1 - instance.save() - return instance + def distincit_users(cls): + return cls.select(cls.user).distinct() + + @classmethod + def by_user(cls, user: User) -> Iterable['MailMessage']: + return cls.select().where(cls.user == user) + + @classmethod + def user_messages_number(cls, user: User) -> int: + return cls.select(fn.Count(cls.id)).where(cls.user == user).scalar() @classmethod def get_instances_number(cls): diff --git a/lms/lmsweb/translations/he/LC_MESSAGES/messages.po b/lms/lmsweb/translations/he/LC_MESSAGES/messages.po index 3aea680f..c1f43c0c 100644 --- a/lms/lmsweb/translations/he/LC_MESSAGES/messages.po +++ b/lms/lmsweb/translations/he/LC_MESSAGES/messages.po @@ -7,7 +7,7 @@ msgid "" msgstr "" "Project-Id-Version: 1.0\n" "Report-Msgid-Bugs-To: EMAIL@ADDRESS\n" -"POT-Creation-Date: 2021-10-05 21:56+0300\n" +"POT-Creation-Date: 2021-10-10 11:43+0300\n" "PO-Revision-Date: 2021-09-29 11:30+0300\n" "Last-Translator: Or Ronai\n" "Language: he\n" @@ -18,7 +18,7 @@ msgstr "" "Content-Transfer-Encoding: 8bit\n" "Generated-By: Babel 2.9.1\n" -#: lmsdb/models.py:879 +#: lmsdb/models.py:943 msgid "Fatal error" msgstr "כישלון חמור" @@ -47,31 +47,31 @@ msgstr "הבודק האוטומטי נכשל ב־ %(number)d דוגמאות בת msgid "Bro, did you check your code?" msgstr "אחי, בדקת את הקוד שלך?" -#: lmsweb/views.py:129 +#: lmsweb/views.py:130 msgid "Can not register now" msgstr "לא ניתן להירשם כעת" -#: lmsweb/views.py:146 +#: lmsweb/views.py:147 msgid "Registration successfully" msgstr "ההרשמה בוצעה בהצלחה" -#: lmsweb/views.py:169 +#: lmsweb/views.py:170 msgid "The confirmation link is expired, new link has been sent to your email" msgstr "קישור האימות פג תוקף, קישור חדש נשלח אל תיבת המייל שלך" -#: lmsweb/views.py:185 +#: lmsweb/views.py:186 msgid "Your user has been successfully confirmed, you can now login" msgstr "המשתמש שלך אומת בהצלחה, כעת אתה יכול להתחבר למערכת" -#: lmsweb/views.py:208 lmsweb/views.py:265 +#: lmsweb/views.py:209 lmsweb/views.py:266 msgid "Your password has successfully changed" msgstr "הסיסמה שלך שונתה בהצלחה" -#: lmsweb/views.py:224 +#: lmsweb/views.py:225 msgid "Password reset link has successfully sent" msgstr "קישור לאיפוס הסיסמה נשלח בהצלחה" -#: lmsweb/views.py:245 +#: lmsweb/views.py:246 msgid "Reset password link is expired" msgstr "קישור איפוס הסיסמה פג תוקף" @@ -170,7 +170,7 @@ msgstr "אימות סיסמה" msgid "Exercises" msgstr "תרגילים" -#: templates/exercises.html:21 templates/view.html:113 +#: templates/exercises.html:21 templates/view.html:126 msgid "Comments for the solution" msgstr "הערות על התרגיל" @@ -293,7 +293,7 @@ msgstr "חמ\"ל תרגילים" msgid "Name" msgstr "שם" -#: templates/status.html:13 templates/user.html:49 +#: templates/status.html:13 templates/user.html:50 msgid "Checked" msgstr "נבדק/ו" @@ -319,7 +319,7 @@ msgstr "העלאת מחברות" #: templates/upload.html:11 msgid "Drag here the notebook file or click and choose it from your computer." -msgstr "גררו לכאן את קובץ המחברת, או לחצו ובחרו אותה מהמחשב שלכם." +msgstr "גררו לכאן את קובץ המחברת, או לחצו ובחרו אותה מהמחשב שלכם." #: templates/upload.html:14 msgid "Back to Exercises List" @@ -343,7 +343,7 @@ msgstr "פעולות" #: templates/user.html:23 msgid "Mail Subscription" -msgstr "מנוי לדואר" +msgstr "מינוי להודעות דוא\"ל" #: templates/user.html:30 msgid "Exercises Submitted" @@ -369,35 +369,35 @@ msgstr "הגשה" msgid "Checker" msgstr "בודק" -#: templates/user.html:34 templates/view.html:21 templates/view.html:104 -msgid "Verbal note" +#: templates/user.html:40 templates/view.html:25 templates/view.html:112 +msgid "Assessment" msgstr "הערה מילולית" -#: templates/user.html:44 +#: templates/user.html:50 msgid "Submitted" msgstr "הוגש" -#: templates/user.html:44 +#: templates/user.html:50 msgid "Not submitted" msgstr "לא הוגש" -#: templates/user.html:56 +#: templates/user.html:62 msgid "Notes" msgstr "פתקיות" -#: templates/user.html:61 templates/user.html:63 +#: templates/user.html:67 templates/user.html:69 msgid "New Note" msgstr "פתקית חדשה" -#: templates/user.html:67 +#: templates/user.html:73 msgid "Related Exercise" msgstr "תרגיל משויך" -#: templates/user.html:76 +#: templates/user.html:82 msgid "Privacy Level" msgstr "רמת פרטיות" -#: templates/user.html:82 +#: templates/user.html:88 msgid "Add Note" msgstr "הוסף פתקית" @@ -429,52 +429,52 @@ msgstr "הפתרון שלך עדיין לא נבדק." msgid "It's important for us that all exercises will be checked by human eye." msgstr "חשוב לנו שכל תרגיל יעבור בדיקה של עין אנושית." -#: templates/view.html:18 +#: templates/view.html:19 msgid "Solver" msgstr "מגיש" -#: templates/view.html:24 +#: templates/view.html:32 msgid "Navigate in solution versions" msgstr "ניווט בגרסאות ההגשה" -#: templates/view.html:30 +#: templates/view.html:38 msgid "Current page" msgstr "סיסמה נוכחית" -#: templates/view.html:38 +#: templates/view.html:46 msgid "Finish Checking" msgstr "סיום בדיקה" -#: templates/view.html:78 +#: templates/view.html:86 msgid "Automatic Checking" msgstr "בדיקות אוטומטיות" -#: templates/view.html:85 +#: templates/view.html:93 msgid "Error" msgstr "כישלון חמור" -#: templates/view.html:90 +#: templates/view.html:98 msgid "Staff Error" msgstr "כישלון חמור" -#: templates/view.html:121 +#: templates/view.html:134 msgid "General comments" msgstr "הערות כלליות" -#: templates/view.html:129 +#: templates/view.html:142 msgid "Checker comments" msgstr "הערות בודק" -#: templates/view.html:139 +#: templates/view.html:152 msgid "Done Checking" msgstr "סיום בדיקה" -#: utils/mail.py:28 +#: utils/mail.py:30 #, python-format msgid "Confirmation mail - %(site_name)s" msgstr "מייל אימות - %(site_name)s" -#: utils/mail.py:35 +#: utils/mail.py:37 #, python-format msgid "" "Hello %(fullname)s,\n" @@ -483,12 +483,12 @@ msgstr "" "שלום %(fullname)s,\n" "לינק האימות שלך למערכת הוא: %(link)s" -#: utils/mail.py:45 +#: utils/mail.py:47 #, python-format msgid "Reset password mail - %(site_name)s" msgstr "מייל איפוס סיסמה - %(site_name)s" -#: utils/mail.py:52 +#: utils/mail.py:54 #, python-format msgid "" "Hello %(fullname)s,\n" @@ -497,12 +497,12 @@ msgstr "" "שלום %(fullname)s,\n" "לינק לצורך איפוס הסיסמה שלך הוא: %(link)s" -#: utils/mail.py:61 +#: utils/mail.py:63 #, python-format msgid "Changing password - %(site_name)s" msgstr "שינוי סיסמה - %(site_name)s" -#: utils/mail.py:65 +#: utils/mail.py:67 #, python-format msgid "" "Hello %(fullname)s. Your password in %(site_name)s site has been changed." @@ -514,15 +514,23 @@ msgstr "" "אם אתה לא עשית את זה צור קשר עם הנהלת האתר.\n" "כתובת המייל: %(site_mail)s" -#: utils/mail.py:77 -#, fuzzy, python-format +#: utils/mail.py:80 +#, python-format msgid "New notification - %(site_name)s" msgstr "התראה חדשה - %(site_name)s" -#: utils/mail.py:81 +#: utils/mail.py:84 #, python-format msgid "" -"Hello %(fullname)s. You have %(number)d new notification/s:\n" +"Hello %(fullname)s. You have %(num)d new notification:\n" +"%(message)s" +msgid_plural "" +"Hello %(fullname)s. You have %(num)d new notifications:\n" +"%(message)s" +msgstr[0] "" +"שלום %(fullname)s. יש לך %(num)d התראה חדשה:\n" +"%(message)s" +msgstr[1] "" +"שלום %(fullname)s. יש לך %(num)d התראות חדשות:\n" "%(message)s" -msgstr "היי %(fullname)s. יש לך %(number)d התראה/ות חדשה/ות:\n%(message)s" diff --git a/lms/lmsweb/views.py b/lms/lmsweb/views.py index b8485168..afe4249e 100644 --- a/lms/lmsweb/views.py +++ b/lms/lmsweb/views.py @@ -370,9 +370,9 @@ def read_all_notification(): return jsonify({'success': success_state}) -@webapp.route('/subscribe/', methods=['PATCH']) -def mail_subscription(act: str): - success_state = users.change_mail_subscription(current_user, act) +@webapp.route('/mail/', methods=['PATCH']) +def mail_subscription(subscription: str): + success_state = users.change_mail_subscription(current_user, subscription) return jsonify({'success': success_state}) diff --git a/lms/models/notifications.py b/lms/models/notifications.py index cfd10229..4bb381c4 100644 --- a/lms/models/notifications.py +++ b/lms/models/notifications.py @@ -1,7 +1,7 @@ import enum from typing import Iterable, Optional -from lms.lmsdb.models import Notification, NotificationMail, User +from lms.lmsdb.models import MailMessage, Notification, User class NotificationKind(enum.Enum): @@ -48,5 +48,5 @@ def send( notification = Notification.send( user, kind.value, message, related_id, action_url, ) - NotificationMail.get_or_create_notification_mail(user, message) + MailMessage.create(user=user, notification=notification) return notification diff --git a/lms/models/users.py b/lms/models/users.py index 20879cd5..e0be2076 100644 --- a/lms/models/users.py +++ b/lms/models/users.py @@ -40,10 +40,10 @@ def generate_user_token(user: User) -> str: return SERIALIZER.dumps(user.mail_address, salt=retrieve_salt(user)) -def change_mail_subscription(user: User, act: str): - if act == 'subscribe': +def change_mail_subscription(user: User, subscription: str) -> bool: + if subscription == 'subscribe': user.mail_subscription = True - elif act == 'unsubscribe': + elif subscription == 'unsubscribe': user.mail_subscription = False else: return False diff --git a/lms/static/my.js b/lms/static/my.js index f61e6330..0db4fff8 100644 --- a/lms/static/my.js +++ b/lms/static/my.js @@ -125,9 +125,9 @@ function trackMailSubscriptionCheckbox() { } checkbox.addEventListener('change', (e) => { - const act = (e.currentTarget.checked) ? 'subscribe' : 'unsubscribe'; + const subscription = (e.currentTarget.checked) ? 'subscribe' : 'unsubscribe'; const request = new XMLHttpRequest(); - request.open('PATCH', `/subscribe/${act}`); + request.open('PATCH', `/mail/${subscription}`); return request.send(); }); } diff --git a/lms/utils/mail.py b/lms/utils/mail.py index c8644c57..cca7e085 100644 --- a/lms/utils/mail.py +++ b/lms/utils/mail.py @@ -1,10 +1,12 @@ from functools import wraps +from typing import Iterable from flask import url_for from flask_babel import gettext as _ # type: ignore +from flask_babel import ngettext # type: ignore from flask_mail import Message # type: ignore -from lms.lmsdb.models import NotificationMail, User +from lms.lmsdb.models import MailMessage, User from lms.lmsweb import config, webapp, webmail, webscheduler from lms.models.users import generate_user_token from lms.utils.config.celery import app @@ -73,28 +75,40 @@ def send_change_password_mail(user: User) -> Message: @send_message -def send_notification_mail(user: User, message: str, number: int) -> Message: +def send_notification_mail(user_id: int, message: str, number: int) -> Message: + user = User.get_by_id(user_id) subject = _( 'New notification - %(site_name)s', site_name=config.SITE_NAME, ) msg = Message(subject, recipients=[user.mail_address]) - msg.body = _( - 'Hello %(fullname)s. You have %(number)d ' - 'new notification/s:\n%(message)s', - fullname=user.fullname, number=number, message=message, + msg.body = ngettext( + 'Hello %(fullname)s. You have %(num)d ' + 'new notification:\n%(message)s', + 'Hello %(fullname)s. You have %(num)d ' + 'new notifications:\n%(message)s', + fullname=user.fullname, num=number, message=message, ) return msg +def build_notification_message(mails: Iterable[MailMessage]) -> str: + return '\n'.join(mail.notification.message.strip() for mail in mails) + + @webscheduler.task( 'interval', id='mail_notifications', hours=config.DEFAULT_DO_TASKS_EVERY_HOURS, ) def send_all_notifications_mails(): - for notification in NotificationMail.select(): - if notification.user.mail_subscription: + for mail_message_user in MailMessage.distincit_users(): + mails = MailMessage.by_user(mail_message_user.user) + message = build_notification_message(mails) + + if mail_message_user.user.mail_subscription: send_message(send_notification_mail( - notification.user, notification.message.rstrip(), - notification.number, + mail_message_user.user.id, message, + MailMessage.user_messages_number(mail_message_user.user.id), )) - notification.delete_instance() + MailMessage.delete().where( + MailMessage.user == mail_message_user.user, + ).execute() diff --git a/tests/conftest.py b/tests/conftest.py index b6f18f2a..0cfeaf77 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,8 +15,8 @@ import pytest from lms.lmsdb.models import ( - ALL_MODELS, Comment, CommentText, Course, Exercise, Note, Notification, - NotificationMail, Role, RoleOptions, SharedSolution, Solution, + ALL_MODELS, Comment, CommentText, Course, Exercise, MailMessage, Note, + Notification, Role, RoleOptions, SharedSolution, Solution, SolutionAssessment, User, UserCourse, ) from lms.extractors.base import File @@ -283,10 +283,9 @@ def create_notification( ) -def create_notification_mail(student_user: User) -> NotificationMail: - return NotificationMail.get_or_create_notification_mail( - user=student_user, message='Test message', - ) +def create_notification_mail(student_user: User, solution) -> MailMessage: + notification=create_notification(student_user, solution) + return MailMessage.create(user=student_user, notification=notification) def create_course(index: int = 0) -> Course: diff --git a/tests/test_notifications.py b/tests/test_notifications.py index 8587d216..990697bd 100644 --- a/tests/test_notifications.py +++ b/tests/test_notifications.py @@ -5,7 +5,7 @@ import pytest # type: ignore from lms.lmsdb.models import ( - Exercise, Notification, NotificationMail, Solution, User, + Exercise, MailMessage, Notification, Solution, User, ) from lms.models import notifications, solutions from lms.utils.mail import send_all_notifications_mails @@ -223,25 +223,25 @@ def test_user_commented_after_check( @staticmethod def test_notifications_subscriptions(student_user: User): client = conftest.get_logged_user(username=student_user.username) - unsubscribe_response = client.patch('/subscribe/unsubscribe') + unsubscribe_response = client.patch('/mail/unsubscribe') assert unsubscribe_response.status_code == 200 student_user = User.get_by_id(student_user.id) assert student_user.mail_subscription == False - subscribe_response = client.patch('/subscribe/subscribe') + subscribe_response = client.patch('/mail/subscribe') assert subscribe_response.status_code == 200 student_user = User.get_by_id(student_user.id) assert student_user.mail_subscription == True - wrong_response = client.patch('/subscribe/wrong') + wrong_response = client.patch('/mail/wrong') json_data = json.loads(wrong_response.get_data(as_text=True)) assert json_data['success'] == False @staticmethod - def test_send_mails_notifications(student_user: User): - conftest.create_notification_mail(student_user) - conftest.create_notification_mail(student_user) - assert NotificationMail.get_instances_number() == 2 + def test_send_mails_notifications(student_user: User, solution: Solution): + conftest.create_notification_mail(student_user, solution) + conftest.create_notification_mail(student_user, solution) + assert MailMessage.get_instances_number() == 2 send_all_notifications_mails() - assert NotificationMail.get_instances_number() == 0 + assert MailMessage.get_instances_number() == 0 From 9e7dead71bbea5960297cef8221df262484aa2a6 Mon Sep 17 00:00:00 2001 From: Or Ronai Date: Fri, 15 Oct 2021 08:49:26 +0300 Subject: [PATCH 6/6] Changed the notifications viewed logic of reading --- lms/lmsdb/models.py | 20 +++++++++++++++----- lms/utils/mail.py | 2 +- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/lms/lmsdb/models.py b/lms/lmsdb/models.py index c9419453..5f21878b 100644 --- a/lms/lmsdb/models.py +++ b/lms/lmsdb/models.py @@ -317,9 +317,6 @@ class Notification(BaseModel): def read(self) -> bool: self.viewed = True - mail = MailMessage.get_or_none(MailMessage.notification == self) - if mail: - mail.delete_instance() return bool(self.save()) @classmethod @@ -398,11 +395,24 @@ def distincit_users(cls): @classmethod def by_user(cls, user: User) -> Iterable['MailMessage']: - return cls.select().where(cls.user == user) + return ( + cls + .select() + .where(cls.user == user) + .join(Notification) + .where(Notification.viewed == False) # NOQA: E712 + ) @classmethod def user_messages_number(cls, user: User) -> int: - return cls.select(fn.Count(cls.id)).where(cls.user == user).scalar() + return ( + cls + .select(fn.Count(cls.id)) + .where(cls.user == user) + .join(Notification) + .where(Notification.viewed == False) # NOQA: E712 + .scalar() + ) @classmethod def get_instances_number(cls): diff --git a/lms/utils/mail.py b/lms/utils/mail.py index cca7e085..7659e936 100644 --- a/lms/utils/mail.py +++ b/lms/utils/mail.py @@ -97,7 +97,7 @@ def build_notification_message(mails: Iterable[MailMessage]) -> str: @webscheduler.task( 'interval', id='mail_notifications', - hours=config.DEFAULT_DO_TASKS_EVERY_HOURS, + seconds=30, ) def send_all_notifications_mails(): for mail_message_user in MailMessage.distincit_users():