Skip to content
This repository

Notifications for registered and anonymous users #197

Closed
wants to merge 47 commits into from

2 participants

Sebastian Vetter David Winterbottom
Sebastian Vetter
Collaborator

Disclaimer: For now I would like to put this up for review and discussion rather then get it merged into Oscar right away. Currently only product notifications are handled in the frontend although there is backend functionality in place to make it extendibles.

I have almost finished implementing the notification with the following features:

  • product notifications are visible only if a product is not in stock or does not have a stock record (a new product).

  • an anonymous user can sign up for a notification of a product. They receive a confirmation link that they have to open to activate their sign up. An anonymous user does not have any means of managing their notifications in a list. The confirmation email contains a second link, however, with a unsubscribe link that allows for disabling of the notification.

  • a registered user can sign up for notifications without having to confirm it. When a registered user signs up for a notification, the "Notify Me" button disappears from the product information and is replaced by a note stating that the user has already signed up.

  • registered users can manage (activate/deactivate) their notifications in their account settings.

  • an anonymous user that has signed up for notifications and creates an account will pull in their notifications and have them assigned to their account and are then no longer marked as anonymous

  • the notifications app registers a receiver for the post_save signal of StockRecord. Whenever a stock record is updated, the notifications are checked for this particular product and emails are sent out. These notifications are then disabled (not deleted) and marked with the date the email was sent. This hides the message from the registered users account. Notifications are still accessible, however, for staff members in the dashboard

  • In the dashboard, the Customers navigation node is extended with a Notifications child that allows for editing, deleting and viewing all notifications. It also provides filtering capabilities for them based on status, customer name, customer email and/or product keyword.

This should describe the functionality at this stage. Please let me know how to improve the features, templates and code. Any feedback is appreciated.

Sebastian Ve... added some commits May 21, 2012
included stock record partial into product template 66c7108
added notification template for product 19d0b1f
added notification views and templates 672ec75
Merge branch 'master', remote-tracking branch 'upstream/master' into …
…feature/notifications
57a04cc
added test case for general view of notification button 64743c3
placed notification form in paragraph af1f9ab
added missing <div> in product partial 027b3fb
removed separated form for notification of anonymous user b8d86a2
fixed problem with notification test authentication eaeb445
modified notification models and added tests
I updated the notification models to be more generic. The major
properties such as ``User``, ``email`` and ``date_created`` are
stored in an abstract class that allows for implementing other
notifications at a later point in time.
In addition, I made ``email`` and ``user`` optional as only one
of the two should be set in my opinion: ``email`` for anonymous
users and ``user`` for registered users which will provide the
email as part of the profile.

At the current state, the test only implement views related
to registered users.
ffe6b27
updated model for notification and basic view 771ba77
fixed wrong call to super() 531455d
removed middleware for notification ba0337b
implemented confirmation and unsubscription incl. tests
I added dates to the model of to be able to track stale notifications
based on creation and modification dates.

I removed the persistence key from the ``AbstractNotification`` model
and removed the middleware as I do not think that it is required. I
discussed this with Jon Moss and we both think that having a middleware
that processes every HTTP request. As the functionality in the
middleware only applied to anonymous users this seems to be overhead.
But please feel free to convince me of the opposite.
f908d51
clarified definition of key length in URLs 5888f6d
Merge branch 'master' into feature/notifications 9d3d0f8
added deleting notification for signed in users 1e730e6
added administration panel for registered users
I added the admin panel for notifications of registered users. A
registered user is now able to view all their notifications in active
and inactive state. The state can be changed between active and
inactive.
75da2bf
added product link and status to notification panel 0ad1ba1
added sending of notification emails on stock record update
I added the backend funtionality for sending notification emails
when stock records are updated. For now, sending the messages is
triggered by the ``post_save`` signal of the ``StockRecord``
model which means that the user will receive multiple messages
if subscribed to multiple products that are in the same update,
e.g. a batch import. The reason for that is that implementing
to collect batch updates and send one update message per user
can be quite tricky, I think. This needs a proper underlying
concept that can be added at some point.

The current implementation sends out a notification to each
registered and unregistered user that has a notification in
state ``ACTIVE``. Each notification is marked ``INACTIVE``
and stamped with the current date as ``date_notified`` property
for future reference after sending out the emails.
412e65b
filtered processed notifications out in user view 7da09f6
added notification list and delete view to dashboard aee212e
added update view for notifications c907288
fixed minor issue in table of review list template 0ffff28
added bulk editing to notification dashboard 7d475c5
added search form to notification dashboard b2ad788
added notification panel to user details in dashboard b532e59
updated product browsing page to work with notification
I fixed some minor issues with notifications on the product list
page (main product view). The notification form was not passed
to the template and therefore would not be able to add a notification
as the email was missing. This is fixed now.

I also removed the "Add To Basket" button for products that are not
in stock. This is replaced by the "notify me" button.

Some FE optimisation has to take place to make it a bit more shiny
but the functionality is there.
b1009d5
added transfering of anonymous notifications at user registration
I updated the registration process for new users so that it checks
the user's email address after successful registration for existing
anonymous notifications. These are then transfered into the users
account.
bd45103
added templatetag for notification signup info
I added a template tag for notifications to determine if a user
has already signed up to receive notifications for a specific
product. The tag provides a boolean flag in the template context
that can be used to show/hide the notification button & form if
a user has already signed up.
3d54f75
moved templatetags for notification app to oscar/templatetags 903a101
added clean up command for unconfirmed notifications
I have added a management command to clean up unconfirmed
notifications that are older then a certain date. The expiry date
is calculated relative to the current date and time and can be
specified in ``days`` or ``hours`` (or both). A valid clean up
command could look like this:

    ./manage.py oscar_cleanup_notifications --days 1 --hours 12

Currently this will remove all notifications directly derived from
``AbstractNotification``. This must be adjusted after a decision
has been made on how to handle the derived classed from abstract
bases (InheritanceManager, django-model-utils, __subclasses__).

I also added a ``tests.py`` to the commands folder to test the
functionality of the command.
12a70bf
merged master into feature/notifications 32dd2ef
merged master into feature/notifications 5aa80ae
minor renaming of views and url names in notifications aec117c
PEP8 and documentation updateds in notification c06d156
PEP8 cleanup 0e131dd
Merge branch 'feature/notifications' of github.com:elbaschid/django-o…
…scar into feature/notifications

Conflicts:
	oscar/apps/customer/tests.py
	oscar/apps/customer/views.py
	sandbox/deploy/nginx/sandbox.conf
374ec07
merge 'master' into 'feature/notifications' 079576e
David Winterbottom
Owner

Sorry to be a pain, but can you merge master back into your pull request as the diff is really hard to read (181 files)?

David Winterbottom codeinthehole commented on the diff June 13, 2012
oscar/templates/notification/partials/notification_pane.html
... ...
@@ -0,0 +1,55 @@
  1
+<div class="sub-header">
  2
+    <h3>Notifications</h3>
  3
+</div>
  4
+{% if not notification_list %}
2
David Winterbottom Owner

This bit wasn't working for me. I was seeing an empty table when I had no notifications.

Sebastian Vetter Collaborator
elbaschid added a note June 13, 2012

That's strange. I just created a new user and it doesn't show the table if no notifications are present. Signing up for a notification, the table is displayed as expected. Not sure what could have caused the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
oscar/templates/dashboard/notification/list.html
... ...
@@ -0,0 +1,94 @@
  1
+{% extends 'dashboard/layout.html' %}
  2
+{% load currency_filters %}
  3
+{% block body_class %}users{% endblock %}
  4
+
  5
+{% block title %}
  6
+Notification management | {{ block.super }}
  7
+{% endblock %}
  8
+
  9
+{% block breadcrumbs %}
  10
+<ul class="breadcrumb">
  11
+    <li>
  12
+        <a href="{% url dashboard:index %}">Dashboard</a>
  13
+        <span class="divider">/</span>
  14
+    </li>
  15
+    <li class="active"><a href=".">Users</a></li>
2
David Winterbottom Owner

Some of the breadcrumb blocks need correcting.

Sebastian Vetter Collaborator
elbaschid added a note June 13, 2012

Sorry, about that. I just fixed it for the 3 templates in the dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
oscar/templatetags/notification_tags.py
((3 lines not shown))
  3
+register = template.Library()
  4
+
  5
+@register.assignment_tag
  6
+def has_signed_up(user, product):
  7
+    """
  8
+    Check if the user has already signed up to receive a notification
  9
+    for this product. Anonymous users are ignored. If a registered
  10
+    user has signed up for a notification the tag returns ``True``. 
  11
+    It returns ``False`` in all other cases.
  12
+    """
  13
+    if not user.is_authenticated():
  14
+        return False
  15
+
  16
+    if user.notifications.filter(product=product).count() > 0:
  17
+        return True
  18
+    return False
2
David Winterbottom Owner

Could be replaced by::

return user.notifications.filter(product=product).count() > 0
Sebastian Vetter Collaborator
elbaschid added a note June 13, 2012

Yeah, that's obviously much cleaner. Changed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
David Winterbottom codeinthehole commented on the diff June 13, 2012
oscar/templatetags/sorting_tags.py
... ...
@@ -0,0 +1 @@
  1
+from django_sorting.templatetags.sorting_tags import *
2
David Winterbottom Owner

This seems a bit odd. I suppose it means that django_sorting does not need to be added to INSTALLED_APPS. Is that the main reason?

Sebastian Vetter Collaborator
elbaschid added a note June 13, 2012

I am not sure what the reason is for that. It came in with a merge. It came with commit 57f46a8 as it seems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
oscar/apps/catalogue/notification/models.py
((81 lines not shown))
  81
+        up. In this case, the notification will be transfered to the
  82
+        specific user account.
  83
+        """
  84
+        if not self.user:
  85
+            self.user = user
  86
+            self.email = None
  87
+            self.confirm_key, self.unsubscribe_key = None, None
  88
+            self.save()
  89
+
  90
+    @models.permalink
  91
+    def get_confirm_url(self):
  92
+        """
  93
+        Get confirmation URL for this notification. Needs to be
  94
+        implemented in subclasses.
  95
+        """
  96
+        raise NotImplementedError
2
David Winterbottom Owner

You need to instantiate::

raise NotImplementedError()
Sebastian Vetter Collaborator
elbaschid added a note June 13, 2012

Fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
oscar/apps/catalogue/notification/models.py
((106 lines not shown))
  106
+    def save(self, *args, **kwargs):
  107
+        """
  108
+        Save the current notification instance. If the user is not
  109
+        a registered/authenticated user, a confirmation and
  110
+        unsubscription key will be generated for the notification.
  111
+        """
  112
+        if self.email and not self.user:
  113
+            if not self.confirm_key:
  114
+                self.confirm_key = self.get_random_key()
  115
+            if not self.unsubscribe_key:
  116
+                self.unsubscribe_key = self.get_random_key()
  117
+        return super(AbstractNotification, self).save(*args, **kwargs)
  118
+
  119
+    def __unicode__(self):
  120
+        """ Unicode representation of this notification """
  121
+        return _(u'Notification for %s - %s') % (self.user or "anonymous",
1
David Winterbottom Owner

Don't think Django's i18n works like this. You need to use named placeholders::

return _(u'Notification for %(user)s - %(email)s) % {'user': self.user or _('anonymous'), 'email': self.email}

See https://docs.djangoproject.com/en/dev/topics/i18n/translation/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
oscar/apps/catalogue/notification/receivers.py
... ...
@@ -0,0 +1,97 @@
  1
+from django.conf import settings
  2
+import datetime
  3
+
  4
+from django.core import mail
  5
+from django.dispatch import receiver
  6
+from django.template import loader, Context
  7
+from django.contrib.sites.models import Site
  8
+from django.db.models.signals import post_save
  9
+from django.utils.translation import ugettext as _
  10
+
  11
+from oscar.core.loading import get_class
  12
+
  13
+StockRecord = get_class('partner.models', 'StockRecord')
  14
+ProductNotification = get_class('catalogue.notification.models',
  15
+                                'ProductNotification')
2
David Winterbottom Owner

This is fine although most of oscar uses django.db.models.get_model when loading models and the get_class* functions for general classes.

Sebastian Vetter Collaborator
elbaschid added a note June 13, 2012

The reason for using get_class instead of get_model is that the receiver.py module is imported in models.py which means that the models are not yet available to be loaded using get_model. At least that's the experience I had when implementing it. Is there a way to make get_model work in this situation? Or should I stick with get_class then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
oscar/apps/catalogue/notification/receivers.py
((13 lines not shown))
  13
+StockRecord = get_class('partner.models', 'StockRecord')
  14
+ProductNotification = get_class('catalogue.notification.models',
  15
+                                'ProductNotification')
  16
+
  17
+
  18
+def _create_email_from_context(email, template, context):
  19
+    """
  20
+    Create ``EmailMessage`` with message body composed as HTML from
  21
+    *template* rendered with *context*. The email address to send the
  22
+    message to is provided by *email*. The content subtype of the
  23
+    message is set to ``html``.
  24
+
  25
+    Returns a ``EmailMessage`` instance.
  26
+    """
  27
+    subject = _("[Product Notification] Product '%s' back in stock!")
  28
+    subject = subject % context['product'].title
1
David Winterbottom Owner

Same i18n issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sebastian Vetter
Collaborator

Thanks for the feedback, David. I merged 'master' into the pull request and updated the things pointed out above. All changes are pushed and the range of this PR is updated accordingly. I hope I covered everything. Let me know if there's anything else you need me to do.

added model_utils and made notifications more generic
I added the model_utils django package to Oscar and updated the
implementation of the notifications app to use the
``InheritanceManager``. To make the notifications work with the
more generic approach, I moved the abstract base notification to
``abstract_models.py`` and introduced a non-abstract base class
``Notification`` to be able to us the inheritance manager.
I also added a method to the model that provides the notification
item of a notification, e.g. the product for a
``ProductNotification``. This allows for using a more generic
approach in the templates using the
``generate_notification_description`` tag which renders a template
for each notification type to make it possible to customise the
description of a notification in the template, e.g. add a link to
the product page for a product notification.
9b5f847
Sebastian Vetter
Collaborator

I updated the notification feature so that it is more generic and can now handle any type of notification as long as it is derived from Notification. This base class uses the InheritanceManager provided in the model_utils package to lookup model instances casted down to their actual model class.
I also added a new template tag that makes it easier to handle descriptions for Notification instance and provide customised output for each subclass of Notification. It renders a customisable template based on the model name that allows for providing links to the item the notification is used for, e.g. a link to the product page for the ProductNotification. It is used in the notification overview of the dashboard showing the description of the notification.

Sebastian Vetter
Collaborator

I updated the notification feature so that it is more generic and can now handle any type of notification as long as it is derived from Notification. This base class uses the InheritanceManager provided in the model_utils package to lookup model instances casted down to their actual model class.
I also added a new template tag that makes it easier to handle descriptions for Notification instance and provide customised output for each subclass of Notification. It renders a customisable template based on the model name that allows for providing links to the item the notification is used for, e.g. a link to the product page for the ProductNotification. It is used in the notification overview of the dashboard showing the description of the notification.

Sebastian Vetter
Collaborator

I am closing this pull request as #269 supersedes it.

Sebastian Vetter elbaschid closed this August 03, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 47 unique commits by 1 author.

May 21, 2012
included stock record partial into product template 66c7108
added notification template for product 19d0b1f
added notification views and templates 672ec75
May 24, 2012
Merge branch 'master', remote-tracking branch 'upstream/master' into …
…feature/notifications
57a04cc
added test case for general view of notification button 64743c3
placed notification form in paragraph af1f9ab
added missing <div> in product partial 027b3fb
removed separated form for notification of anonymous user b8d86a2
fixed problem with notification test authentication eaeb445
modified notification models and added tests
I updated the notification models to be more generic. The major
properties such as ``User``, ``email`` and ``date_created`` are
stored in an abstract class that allows for implementing other
notifications at a later point in time.
In addition, I made ``email`` and ``user`` optional as only one
of the two should be set in my opinion: ``email`` for anonymous
users and ``user`` for registered users which will provide the
email as part of the profile.

At the current state, the test only implement views related
to registered users.
ffe6b27
May 25, 2012
updated model for notification and basic view 771ba77
fixed wrong call to super() 531455d
removed middleware for notification ba0337b
implemented confirmation and unsubscription incl. tests
I added dates to the model of to be able to track stale notifications
based on creation and modification dates.

I removed the persistence key from the ``AbstractNotification`` model
and removed the middleware as I do not think that it is required. I
discussed this with Jon Moss and we both think that having a middleware
that processes every HTTP request. As the functionality in the
middleware only applied to anonymous users this seems to be overhead.
But please feel free to convince me of the opposite.
f908d51
clarified definition of key length in URLs 5888f6d
May 28, 2012
Merge branch 'master' into feature/notifications 9d3d0f8
added deleting notification for signed in users 1e730e6
added administration panel for registered users
I added the admin panel for notifications of registered users. A
registered user is now able to view all their notifications in active
and inactive state. The state can be changed between active and
inactive.
75da2bf
added product link and status to notification panel 0ad1ba1
added sending of notification emails on stock record update
I added the backend funtionality for sending notification emails
when stock records are updated. For now, sending the messages is
triggered by the ``post_save`` signal of the ``StockRecord``
model which means that the user will receive multiple messages
if subscribed to multiple products that are in the same update,
e.g. a batch import. The reason for that is that implementing
to collect batch updates and send one update message per user
can be quite tricky, I think. This needs a proper underlying
concept that can be added at some point.

The current implementation sends out a notification to each
registered and unregistered user that has a notification in
state ``ACTIVE``. Each notification is marked ``INACTIVE``
and stamped with the current date as ``date_notified`` property
for future reference after sending out the emails.
412e65b
May 29, 2012
filtered processed notifications out in user view 7da09f6
added notification list and delete view to dashboard aee212e
added update view for notifications c907288
fixed minor issue in table of review list template 0ffff28
added bulk editing to notification dashboard 7d475c5
added search form to notification dashboard b2ad788
added notification panel to user details in dashboard b532e59
updated product browsing page to work with notification
I fixed some minor issues with notifications on the product list
page (main product view). The notification form was not passed
to the template and therefore would not be able to add a notification
as the email was missing. This is fixed now.

I also removed the "Add To Basket" button for products that are not
in stock. This is replaced by the "notify me" button.

Some FE optimisation has to take place to make it a bit more shiny
but the functionality is there.
b1009d5
added transfering of anonymous notifications at user registration
I updated the registration process for new users so that it checks
the user's email address after successful registration for existing
anonymous notifications. These are then transfered into the users
account.
bd45103
added templatetag for notification signup info
I added a template tag for notifications to determine if a user
has already signed up to receive notifications for a specific
product. The tag provides a boolean flag in the template context
that can be used to show/hide the notification button & form if
a user has already signed up.
3d54f75
May 30, 2012
moved templatetags for notification app to oscar/templatetags 903a101
added clean up command for unconfirmed notifications
I have added a management command to clean up unconfirmed
notifications that are older then a certain date. The expiry date
is calculated relative to the current date and time and can be
specified in ``days`` or ``hours`` (or both). A valid clean up
command could look like this:

    ./manage.py oscar_cleanup_notifications --days 1 --hours 12

Currently this will remove all notifications directly derived from
``AbstractNotification``. This must be adjusted after a decision
has been made on how to handle the derived classed from abstract
bases (InheritanceManager, django-model-utils, __subclasses__).

I also added a ``tests.py`` to the commands folder to test the
functionality of the command.
12a70bf
merged master into feature/notifications 32dd2ef
May 31, 2012
merged master into feature/notifications 5aa80ae
minor renaming of views and url names in notifications aec117c
PEP8 and documentation updateds in notification c06d156
PEP8 cleanup 0e131dd
Merge branch 'feature/notifications' of github.com:elbaschid/django-o…
…scar into feature/notifications

Conflicts:
	oscar/apps/customer/tests.py
	oscar/apps/customer/views.py
	sandbox/deploy/nginx/sandbox.conf
374ec07
Jun 04, 2012
merge 'master' into 'feature/notifications' 079576e
Jun 14, 2012
merged branch 'master' into 'feature/notifications' 8c3ca33
fixed typo in template for user notifications b6b58cc
fixed breadcrumbs for notifications in dashboard 2083eed
cleaned the notification return statement d5d9e57
fixed instantiating NotImplmentedError in notification model 1355ff3
fixed i18n issues in notification models and receivers 8a29b04
added model_utils and made notifications more generic
I added the model_utils django package to Oscar and updated the
implementation of the notifications app to use the
``InheritanceManager``. To make the notifications work with the
more generic approach, I moved the abstract base notification to
``abstract_models.py`` and introduced a non-abstract base class
``Notification`` to be able to us the inheritance manager.
I also added a method to the model that provides the notification
item of a notification, e.g. the product for a
``ProductNotification``. This allows for using a more generic
approach in the templates using the
``generate_notification_description`` tag which renders a template
for each notification type to make it possible to customise the
description of a notification in the template, e.g. add a link to
the product page for a product notification.
9b5f847
updated cleanup command to use Notification model 4b4ffad
Something went wrong with that request. Please try again.