Skip to content

Commit

Permalink
Fix retrying email notification task (#4772)
Browse files Browse the repository at this point in the history
# What this PR does
Email tasks are failing and retrying when they use the fallback default
notification policy as it does not get saved in the DB. This PR uses the
same way as UserNotificationPolicyLogRecord to set that field to null to
avoid `ValueError("save() prohibited to prevent data loss due to unsaved
related object 'notification_policy'.").` when that is the case.

## Which issue(s) this PR closes

Related to [issue link here]

<!--
*Note*: If you want the issue to be auto-closed once the PR is merged,
change "Related to" to "Closes" in the line above.
If you have more than one GitHub issue that this PR closes, be sure to
preface
each issue link with a [closing
keyword](https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue).
This ensures that the issue(s) are auto-closed once the PR has been
merged.
-->

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] Added the relevant release notes label (see labels prefixed w/
`release:`). These labels dictate how your PR will
    show up in the autogenerated release notes.
  • Loading branch information
mderynck authored Aug 1, 2024
1 parent eb6bd41 commit 4c1639c
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 4 deletions.
13 changes: 11 additions & 2 deletions engine/apps/email/models.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
import logging
import uuid

from django.db import models

logger = logging.getLogger(__name__)

class EmailMessageQuerySet(models.QuerySet):
def create(self, **kwargs):
from apps.base.models.user_notification_policy_log_record import (
_check_if_notification_policy_is_transient_fallback,
)

_check_if_notification_policy_is_transient_fallback(kwargs)
return super().create(**kwargs)


class EmailMessage(models.Model):
objects = EmailMessageQuerySet.as_manager()

message_uuid = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)

exceeded_limit = models.BooleanField(null=True, default=None)
Expand Down
9 changes: 7 additions & 2 deletions engine/apps/email/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ def _create_user_notification_policy_log_record(**kwargs):
**kwargs, using_fallback_default_notification_policy_step=using_fallback_default_notification_policy_step
)

def _create_email_message(**kwargs):
return EmailMessage.objects.create(
**kwargs, using_fallback_default_notification_policy_step=using_fallback_default_notification_policy_step
)

# create an error log in case EMAIL_HOST is not specified
if not live_settings.EMAIL_HOST:
_create_user_notification_policy_log_record(
Expand All @@ -88,7 +93,7 @@ def _create_user_notification_policy_log_record(**kwargs):
notification_channel=notification_policy.notify_by,
notification_error_code=UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_MAIL_LIMIT_EXCEEDED,
)
EmailMessage.objects.create(
_create_email_message(
represents_alert_group=alert_group,
notification_policy=notification_policy,
receiver=user,
Expand All @@ -115,7 +120,7 @@ def _create_user_notification_policy_log_record(**kwargs):

try:
send_mail(subject, message, from_email, recipient_list, html_message=html_message, connection=connection)
EmailMessage.objects.create(
_create_email_message(
represents_alert_group=alert_group,
notification_policy=notification_policy,
receiver=user,
Expand Down
32 changes: 32 additions & 0 deletions engine/apps/email/tests/test_notify_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord
from apps.email.alert_rendering import build_subject_and_message
from apps.email.models import EmailMessage
from apps.email.tasks import get_from_email, notify_user_async
from apps.user_management.subscription_strategy.free_public_beta_subscription_strategy import (
FreePublicBetaSubscriptionStrategy,
Expand Down Expand Up @@ -212,3 +213,34 @@ def test_subject_newlines_removed(

subject, _ = build_subject_and_message(alert_group, 1)
assert subject == "testnewlines"


@pytest.mark.django_db
def test_notify_user_fallback_default_policy(
settings,
make_organization,
make_user_for_organization,
make_token_for_organization,
make_alert_receive_channel,
make_alert_group,
make_alert,
make_user_notification_policy,
):
settings.EMAIL_BACKEND = "django.core.mail.backends.locmem.EmailBackend"
settings.EMAIL_HOST = "test"

organization = make_organization()
user = make_user_for_organization(organization)

alert_receive_channel = make_alert_receive_channel(organization)
alert_group = make_alert_group(alert_receive_channel)

make_alert(alert_group=alert_group, raw_request_data=alert_receive_channel.config.example_payload)

notify_user_async(user.pk, alert_group.pk, None)
assert len(mail.outbox) == 1

log_record = UserNotificationPolicyLogRecord.objects.filter(author=user, alert_group=alert_group).first()
assert log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS

EmailMessage.objects.get(receiver=user, represents_alert_group=alert_group, notification_policy=None)

0 comments on commit 4c1639c

Please sign in to comment.