Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
24 changes: 24 additions & 0 deletions tests/test/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ class TriggerOncePerContentMessage(DefaultEmailMessage):
class TriggerDeleteAfterMessage(DefaultEmailMessage):
template_name = 'test'

TRIGGER_BROKEN = 'trigger_broken'
@message(TRIGGER_BROKEN)
class BrokenMessage(DefaultEmailMessage):
template_name = 'test'

def __init__(self, notification):
super(BrokenMessage, self).__init__(notification)
raise ValueError('This message is broken at init')

class ModelTests(TestCase):

def setUp(self):
Expand Down Expand Up @@ -236,3 +245,18 @@ def test_invalid_data(self):
self.assertEqual(notification.status, Notification.Status.BROKEN)
self.assertEqual(notification.data, '{}')

def test_broken(self):

welcome_path = "{}.{}".format(SimpleMessage.__module__, SimpleMessage.__name__)
trigger_settings = {TRIGGER_BROKEN: welcome_path}
with self.settings(TRANSMISSIONS_TRIGGERS=trigger_settings):

user = factories.User()

notification = BrokenMessage.trigger(user)

with self.assertRaises(ValueError):
notification.send()

self.assertEqual(notification.status, Notification.Status.BROKEN)
self.assertEqual(notification.exception, "ValueError: This message is broken at init")
6 changes: 4 additions & 2 deletions tests/test/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

TRIGGER_NAME = 'task_test'
TRIGGER_SUBJECT = 'This is a task test'
EXCEPTION_MESSAGE = 'This message is broken at init'

@message(TRIGGER_NAME, behavior=None, subject=TRIGGER_SUBJECT)
class TaskTestMessage(DefaultEmailMessage):
Expand All @@ -29,7 +30,7 @@ class BrokenMessage(DefaultEmailMessage):

def __init__(self, notification):
super(BrokenMessage, self).__init__(notification)
raise Exception('This message is broken at init')
raise Exception(EXCEPTION_MESSAGE)

class TasksTests(TestCase):

Expand Down Expand Up @@ -238,7 +239,7 @@ def test_process_broken(self):
self.assertEqual(notification.status, Notification.Status.CREATED)

# Trigger single task
with self.assertRaisesMessage(Exception, 'This message is broken at init'):
with self.assertRaisesMessage(Exception, EXCEPTION_MESSAGE):
tasks.process_all_notifications()
notification = Notification.objects.get(pk=notification.id)

Expand All @@ -249,6 +250,7 @@ def test_process_broken(self):
# Check status
self.assertEqual(notification.status, Notification.Status.BROKEN)
self.assertGreaterEqual(notification.datetime_processed, notification.datetime_scheduled)
self.assertEqual(notification.exception, "Exception: {}".format(EXCEPTION_MESSAGE))

# Check email was sent
self.assertEqual(len(mail.outbox), 0)
9 changes: 5 additions & 4 deletions transmissions/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,20 +38,21 @@ class NotificationAdmin(admin.ModelAdmin):
'status',
'datetime_created',
'datetime_scheduled',
'datetime_processed'
'datetime_processed',
'exception',
]

search_fields = ['=id', 'target_user__email']

readonly_fields = ['id', 'trigger_name', _link_to_target_user, _link_to_trigger_user, 'datetime_created',
'datetime_processed', 'datetime_seen', 'datetime_consumed', 'content']
'datetime_processed', 'datetime_seen', 'datetime_consumed', 'exception', 'content']

fieldsets = (('Notifications', {'fields': ('id', _link_to_target_user, 'trigger_name')}),
('Status', {'fields': (
'status', _link_to_trigger_user, 'datetime_created', 'datetime_scheduled', 'datetime_processed',
'datetime_seen', 'datetime_consumed')}),
'datetime_seen', 'datetime_consumed', 'exception')}),
('Content', {'fields': ('content',)})
)


admin.site.register(Notification, NotificationAdmin)
admin.site.register(Notification, NotificationAdmin)
20 changes: 20 additions & 0 deletions transmissions/migrations/0005_notification_exception.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.9.10 on 2017-09-26 22:29
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('transmissions', '0004_auto_20161027_1149'),
]

operations = [
migrations.AddField(
model_name='notification',
name='exception',
field=models.TextField(blank=True, default=b''),
),
]
5 changes: 4 additions & 1 deletion transmissions/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class Status(EnumDict):

status = models.IntegerField(default=Status.CREATED)

exception = models.TextField(blank=True, default='')
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @pbaranay! Unfortunately, the Notification model is meant to be extremely lean since it's storing 100s of millions of entries in production.

I'd suggest moving this feature to separate table since most notifications shouldn't have any exception anyway!


@property
def data(self):
if not hasattr(self, '_data'):
Expand Down Expand Up @@ -120,8 +122,9 @@ def send(self):
self.delete()
except ChannelSendException:
self.status = self.Status.FAILED
except:
except Exception as e:
self.status = self.Status.BROKEN
self.exception = "{klass}: {message}".format(klass=e.__class__.__name__, message=str(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

Arguably, this could be done through a logging system rather than a permanent data store. I think logging would be a more suitable solution to introduce here since it's only assuming you've configured Django for it right now.

Either way you will want to make it configurable with settings. Logging is controlled by logging routes in settings, but if you introduce a new model/field, please add a setting.

raise
finally:
if self.pk:
Expand Down