Skip to content

Commit

Permalink
feat: optimize bulk changes notifications
Browse files Browse the repository at this point in the history
- avoid sending post_save signal for each change
- support processing multiple changes in notifications
- introduce change_bulk_create signal for third-party code
  • Loading branch information
nijel committed Dec 11, 2024
1 parent d849948 commit 05157d6
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 34 deletions.
2 changes: 2 additions & 0 deletions docs/changes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ Not yet released.

* :ref:`rollbar-errors` integration no longer includes client-side error collection.
* Weblate now requires Git 2.28 or newer.
* Any custom code that relied on `Change` models signals should be reviewed.
* :ref:`fedora-messaging` integration needs to be updated to be compatible with this release.

**Upgrading**

Expand Down
8 changes: 8 additions & 0 deletions weblate/accounts/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,14 @@ def is_notificable_action(action: int) -> bool:
return action in NOTIFICATIONS_ACTIONS


def dispatch_changes_notifications(changes: Iterable[Change]) -> None:
from weblate.accounts.tasks import notify_changes

notify_changes.delay_on_commit(
[change.pk for change in changes if is_notificable_action(change.action)]
)


class Notification:
actions: Iterable[int] = ()
verbose: StrOrPromise = ""
Expand Down
61 changes: 40 additions & 21 deletions weblate/accounts/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from email.mime.image import MIMEImage
from smtplib import SMTP, SMTPConnectError
from types import MethodType
from typing import TypedDict
from typing import TYPE_CHECKING, TypedDict

import sentry_sdk
from celery.schedules import crontab
Expand All @@ -24,6 +24,11 @@
from weblate.utils.errors import report_error
from weblate.utils.html import HTML2Text

if TYPE_CHECKING:
from collections.abc import Generator

from weblate.accounts.notifications import Notification

LOGGER = logging.getLogger("weblate.smtp")


Expand Down Expand Up @@ -72,30 +77,44 @@ def cleanup_auditlog() -> None:
audit.save(update_fields=["params"])


class NotificationFactory:
def __init__(self):
self.perm_cache: dict[int, set[int]] = {}
self.outgoing: list[OutgoingEmail] = []
self.instances: dict[str, Notification] = {}

def for_action(self, action: int) -> Generator[Notification]:
from weblate.accounts.notifications import NOTIFICATIONS_ACTIONS

if action not in NOTIFICATIONS_ACTIONS:
return
for notification_cls in NOTIFICATIONS_ACTIONS[action]:
name = notification_cls.get_name()
try:
yield self.instances[name]
except KeyError:
result = self.instances[name] = notification_cls(
self.outgoing, self.perm_cache
)
yield result

def send_queued(self) -> None:
if self.outgoing:
send_mails.delay(self.outgoing)
self.outgoing.clear()


@app.task(trail=False)
def notify_change(change_id) -> None:
from weblate.accounts.notifications import (
NOTIFICATIONS_ACTIONS,
is_notificable_action,
)
def notify_changes(change_ids: list[int]) -> None:
from weblate.trans.models import Change

try:
change = Change.objects.prefetch_for_get().get(pk=change_id)
except Change.DoesNotExist:
# The change was removed meanwhile
return
perm_cache: dict[int, set[int]] = {}

# The same condition is applied at notify_change.delay, but we need to recheck
# here to make sure this doesn't break on upgrades when processing older tasks
if is_notificable_action(change.action):
outgoing: list[OutgoingEmail] = []
for notification_cls in NOTIFICATIONS_ACTIONS[change.action]:
notification = notification_cls(outgoing, perm_cache)
changes = Change.objects.prefetch_for_get().filter(pk__in=change_ids)
factory = NotificationFactory()

for change in changes:
for notification in factory.for_action(change.action):
notification.notify_immediate(change)
if outgoing:
send_mails.delay(outgoing)
factory.send_queued()


def notify_digest(method) -> None:
Expand Down
10 changes: 5 additions & 5 deletions weblate/accounts/tests/test_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
NotificationScope,
)
from weblate.accounts.tasks import (
notify_change,
notify_changes,
notify_daily,
notify_monthly,
notify_weekly,
Expand Down Expand Up @@ -152,7 +152,7 @@ def test_notify_merge_failure(self) -> None:

# Add project owner
self.component.project.add_user(self.anotheruser, "Administration")
notify_change(change.pk)
notify_changes([change.pk])

# Check mail
self.validate_notifications(2, "[Weblate] Repository failure in Test/Test")
Expand All @@ -165,7 +165,7 @@ def test_notify_repository(self) -> None:

# Add project owner
self.component.project.add_user(self.anotheruser, "Administration")
notify_change(change.pk)
notify_changes([change.pk])

# Check mail
self.validate_notifications(2, "[Weblate] Repository operation in Test/Test")
Expand All @@ -181,7 +181,7 @@ def test_notify_parse_error(self) -> None:

# Add project owner
self.component.project.add_user(self.anotheruser, "Administration")
notify_change(change.pk)
notify_changes([change.pk])

# Check mail
self.validate_notifications(3, "[Weblate] Parse error in Test/Test")
Expand Down Expand Up @@ -235,7 +235,7 @@ def test_notify_new_language(self) -> None:

# Add project owner
self.component.project.add_user(anotheruser, "Administration")
notify_change(change.pk)
notify_changes([change.pk])

# Check mail
self.validate_notifications(2, "[Weblate] New language request in Test/Test")
Expand Down
20 changes: 12 additions & 8 deletions weblate/trans/models/change.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from weblate.trans.mixins import UserDisplayMixin
from weblate.trans.models.alert import ALERTS
from weblate.trans.models.project import Project
from weblate.trans.signals import change_bulk_create
from weblate.utils.decorators import disable_for_loaddata
from weblate.utils.pii import mask_email
from weblate.utils.state import StringState
Expand Down Expand Up @@ -225,11 +226,16 @@ def bulk_create(self, *args, **kwargs) -> list[Change]:
Add processing to bulk creation.
"""
from weblate.accounts.notifications import dispatch_changes_notifications

changes = super().bulk_create(*args, **kwargs)
# Executes post save to ensure messages are sent as notifications
# or to fedora messaging
for change in changes:
post_save.send(change.__class__, instance=change, created=True)

# Dispatch notifications
dispatch_changes_notifications(changes)

# Executes post save to ensure messages are sent to fedora messaging
change_bulk_create.send(Change, instances=changes)

# Store last content change in cache for improved performance
translations = set()
for change in reversed(changes):
Expand Down Expand Up @@ -1020,8 +1026,6 @@ def show_unit_state(self):
@receiver(post_save, sender=Change)
@disable_for_loaddata
def change_notify(sender, instance, created=False, **kwargs) -> None:
from weblate.accounts.notifications import is_notificable_action
from weblate.accounts.tasks import notify_change
from weblate.accounts.notifications import dispatch_changes_notifications

if is_notificable_action(instance.action):
notify_change.delay_on_commit(instance.pk)
dispatch_changes_notifications([instance])
1 change: 1 addition & 0 deletions weblate/trans/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@
unit_pre_create = Signal()
user_pre_delete = Signal()
store_post_load = Signal()
change_bulk_create = Signal()

0 comments on commit 05157d6

Please sign in to comment.