Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions devops/lms.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,25 @@ services:
- ./rabbitmq.cookie:/var/lib/rabbitmq/.erlang.cookie
networks:
- lms

mailer:
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_MAILER_VHOST=utils
- CELERY_RABBITMQ_HOST=rabbitmq
- CELERY_RABBITMQ_PORT=5672
links:
- rabbitmq
depends_on:
- rabbitmq
networks:
- lms

checks-sandbox:
image: lms:latest
Expand Down
20 changes: 20 additions & 0 deletions lms/lmsdb/bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Copy link
Member

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

Copy link
Member

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?

Copy link
Collaborator Author

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

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)
Expand Down Expand Up @@ -309,6 +321,13 @@ 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,
)


def _assessment_migration() -> bool:
Solution = models.Solution
_add_not_null_column(Solution, Solution.assessment)
Expand All @@ -328,6 +347,7 @@ def main():
_api_keys_migration()
_last_course_viewed_migration()
_uuid_migration()
_mail_subscription()

if models.database.table_exists(models.UserCourse.__name__.lower()):
_add_user_course_constaint()
Expand Down
26 changes: 26 additions & 0 deletions lms/lmsdb/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,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)
Expand Down Expand Up @@ -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()
Copy link
Member

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

Copy link
Collaborator Author

@orronai orronai Oct 11, 2021

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?

Copy link
Member

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

Copy link
Collaborator Author

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.

return bool(self.save())

@classmethod
Expand Down Expand Up @@ -375,6 +379,28 @@ def on_notification_saved(
instance.delete_instance()


class MailMessage(BaseModel):
user = ForeignKeyField(User, backref='mails')
notification = ForeignKeyField(Notification, backref='mails')
date = DateTimeField(default=datetime.now)

@classmethod
def distincit_users(cls):
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And please add types

Copy link
Collaborator Author

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.

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):
return cls.select(fn.Count(cls.id))


class Exercise(BaseModel):
subject = CharField()
date = DateTimeField()
Expand Down
4 changes: 4 additions & 0 deletions lms/lmsweb/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import typing

from flask import Flask
from flask_apscheduler import APScheduler # type: ignore
from flask_babel import Babel # type: ignore
from flask_httpauth import HTTPBasicAuth
from flask_limiter import Limiter # type: ignore
Expand Down Expand Up @@ -48,6 +49,9 @@

webmail = Mail(webapp)

webscheduler = APScheduler(app=webapp)
Copy link
Member

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

webscheduler.start()


# Must import files after app's creation
from lms.lmsdb import models # NOQA: F401, E402, I202
Expand Down
4 changes: 4 additions & 0 deletions lms/lmsweb/config.py.example
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ LOCALE = 'en'
LIMITS_PER_MINUTE = 5
LIMITS_PER_HOUR = 50

# Scheduler
SCHEDULER_API_ENABLED = True
DEFAULT_DO_TASKS_EVERY_HOURS = 2

# Change password settings
MAX_INVALID_PASSWORD_TRIES = 5

Expand Down
Loading