Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Notifications for registered and anonymous users #197

Closed
wants to merge 47 commits into from

2 participants

@elbaschid

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
Sebastian Vetter included stock record partial into product template 66c7108
Sebastian Vetter added notification template for product 19d0b1f
Sebastian Vetter added notification views and templates 672ec75
Sebastian Vetter Merge branch 'master', remote-tracking branch 'upstream/master' into …
…feature/notifications
57a04cc
Sebastian Vetter added test case for general view of notification button 64743c3
Sebastian Vetter placed notification form in paragraph af1f9ab
Sebastian Vetter added missing <div> in product partial 027b3fb
Sebastian Vetter removed separated form for notification of anonymous user b8d86a2
Sebastian Vetter fixed problem with notification test authentication eaeb445
Sebastian Vetter 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
Sebastian Vetter updated model for notification and basic view 771ba77
Sebastian Vetter fixed wrong call to super() 531455d
Sebastian Vetter removed middleware for notification ba0337b
Sebastian Vetter 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
Sebastian Vetter clarified definition of key length in URLs 5888f6d
Sebastian Vetter Merge branch 'master' into feature/notifications 9d3d0f8
Sebastian Vetter added deleting notification for signed in users 1e730e6
Sebastian Vetter 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
Sebastian Vetter added product link and status to notification panel 0ad1ba1
Sebastian Vetter 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
Sebastian Vetter filtered processed notifications out in user view 7da09f6
Sebastian Vetter added notification list and delete view to dashboard aee212e
Sebastian Vetter added update view for notifications c907288
Sebastian Vetter fixed minor issue in table of review list template 0ffff28
Sebastian Vetter added bulk editing to notification dashboard 7d475c5
Sebastian Vetter added search form to notification dashboard b2ad788
Sebastian Vetter added notification panel to user details in dashboard b532e59
Sebastian Vetter 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
Sebastian Vetter 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
Sebastian Vetter 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
Sebastian Vetter moved templatetags for notification app to oscar/templatetags 903a101
Sebastian Vetter 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
Sebastian Vetter merged master into feature/notifications 32dd2ef
Sebastian Vetter merged master into feature/notifications 5aa80ae
Sebastian Vetter minor renaming of views and url names in notifications aec117c
Sebastian Vetter PEP8 and documentation updateds in notification c06d156
Sebastian Vetter PEP8 cleanup 0e131dd
Sebastian Vetter 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
Sebastian Vetter merge 'master' into 'feature/notifications' 079576e
@codeinthehole

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

@codeinthehole codeinthehole commented on the diff
...emplates/notification/partials/notification_pane.html
@@ -0,0 +1,55 @@
+<div class="sub-header">
+ <h3>Notifications</h3>
+</div>
+{% if not notification_list %}
@codeinthehole Owner

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

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 @@
+{% extends 'dashboard/layout.html' %}
+{% load currency_filters %}
+{% block body_class %}users{% endblock %}
+
+{% block title %}
+Notification management | {{ block.super }}
+{% endblock %}
+
+{% block breadcrumbs %}
+<ul class="breadcrumb">
+ <li>
+ <a href="{% url dashboard:index %}">Dashboard</a>
+ <span class="divider">/</span>
+ </li>
+ <li class="active"><a href=".">Users</a></li>
@codeinthehole Owner

Some of the breadcrumb blocks need correcting.

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))
+register = template.Library()
+
+@register.assignment_tag
+def has_signed_up(user, product):
+ """
+ Check if the user has already signed up to receive a notification
+ for this product. Anonymous users are ignored. If a registered
+ user has signed up for a notification the tag returns ``True``.
+ It returns ``False`` in all other cases.
+ """
+ if not user.is_authenticated():
+ return False
+
+ if user.notifications.filter(product=product).count() > 0:
+ return True
+ return False
@codeinthehole Owner

Could be replaced by::

return user.notifications.filter(product=product).count() > 0

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
@codeinthehole codeinthehole commented on the diff
oscar/templatetags/sorting_tags.py
@@ -0,0 +1 @@
+from django_sorting.templatetags.sorting_tags import *
@codeinthehole 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?

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))
+ up. In this case, the notification will be transfered to the
+ specific user account.
+ """
+ if not self.user:
+ self.user = user
+ self.email = None
+ self.confirm_key, self.unsubscribe_key = None, None
+ self.save()
+
+ @models.permalink
+ def get_confirm_url(self):
+ """
+ Get confirmation URL for this notification. Needs to be
+ implemented in subclasses.
+ """
+ raise NotImplementedError
@codeinthehole Owner

You need to instantiate::

raise NotImplementedError()

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))
+ def save(self, *args, **kwargs):
+ """
+ Save the current notification instance. If the user is not
+ a registered/authenticated user, a confirmation and
+ unsubscription key will be generated for the notification.
+ """
+ if self.email and not self.user:
+ if not self.confirm_key:
+ self.confirm_key = self.get_random_key()
+ if not self.unsubscribe_key:
+ self.unsubscribe_key = self.get_random_key()
+ return super(AbstractNotification, self).save(*args, **kwargs)
+
+ def __unicode__(self):
+ """ Unicode representation of this notification """
+ return _(u'Notification for %s - %s') % (self.user or "anonymous",
@codeinthehole 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 @@
+from django.conf import settings
+import datetime
+
+from django.core import mail
+from django.dispatch import receiver
+from django.template import loader, Context
+from django.contrib.sites.models import Site
+from django.db.models.signals import post_save
+from django.utils.translation import ugettext as _
+
+from oscar.core.loading import get_class
+
+StockRecord = get_class('partner.models', 'StockRecord')
+ProductNotification = get_class('catalogue.notification.models',
+ 'ProductNotification')
@codeinthehole 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.

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))
+StockRecord = get_class('partner.models', 'StockRecord')
+ProductNotification = get_class('catalogue.notification.models',
+ 'ProductNotification')
+
+
+def _create_email_from_context(email, template, context):
+ """
+ Create ``EmailMessage`` with message body composed as HTML from
+ *template* rendered with *context*. The email address to send the
+ message to is provided by *email*. The content subtype of the
+ message is set to ``html``.
+
+ Returns a ``EmailMessage`` instance.
+ """
+ subject = _("[Product Notification] Product '%s' back in stock!")
+ subject = subject % context['product'].title
@codeinthehole Owner

Same i18n issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elbaschid

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.

Sebastian Vetter 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
@elbaschid

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.

@elbaschid

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.

@elbaschid

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

@elbaschid elbaschid closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 21, 2012
  1. included stock record partial into product template

    Sebastian Vetter authored
  2. added notification template for product

    Sebastian Vetter authored
  3. added notification views and templates

    Sebastian Vetter authored
Commits on May 24, 2012
  1. Merge branch 'master', remote-tracking branch 'upstream/master' into …

    Sebastian Vetter authored
    …feature/notifications
  2. added test case for general view of notification button

    Sebastian Vetter authored
  3. placed notification form in paragraph

    Sebastian Vetter authored
  4. added missing <div> in product partial

    Sebastian Vetter authored
  5. removed separated form for notification of anonymous user

    Sebastian Vetter authored
  6. fixed problem with notification test authentication

    Sebastian Vetter authored
  7. modified notification models and added tests

    Sebastian Vetter authored
    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.
Commits on May 25, 2012
  1. updated model for notification and basic view

    Sebastian Vetter authored
  2. fixed wrong call to super()

    Sebastian Vetter authored
  3. removed middleware for notification

    Sebastian Vetter authored
  4. implemented confirmation and unsubscription incl. tests

    Sebastian Vetter authored
    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.
  5. clarified definition of key length in URLs

    Sebastian Vetter authored
Commits on May 27, 2012
  1. Merge branch 'master' into feature/notifications

    Sebastian Vetter authored
Commits on May 28, 2012
  1. added deleting notification for signed in users

    Sebastian Vetter authored
  2. added administration panel for registered users

    Sebastian Vetter authored
    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.
  3. added product link and status to notification panel

    Sebastian Vetter authored
  4. added sending of notification emails on stock record update

    Sebastian Vetter authored
    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.
Commits on May 29, 2012
  1. filtered processed notifications out in user view

    Sebastian Vetter authored
  2. added notification list and delete view to dashboard

    Sebastian Vetter authored
  3. added update view for notifications

    Sebastian Vetter authored
  4. fixed minor issue in table of review list template

    Sebastian Vetter authored
  5. added bulk editing to notification dashboard

    Sebastian Vetter authored
  6. added search form to notification dashboard

    Sebastian Vetter authored
  7. added notification panel to user details in dashboard

    Sebastian Vetter authored
  8. updated product browsing page to work with notification

    Sebastian Vetter authored
    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.
  9. added transfering of anonymous notifications at user registration

    Sebastian Vetter authored
    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.
  10. added templatetag for notification signup info

    Sebastian Vetter authored
    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.
Commits on May 30, 2012
  1. moved templatetags for notification app to oscar/templatetags

    Sebastian Vetter authored
  2. added clean up command for unconfirmed notifications

    Sebastian Vetter authored
    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.
  3. merged master into feature/notifications

    Sebastian Vetter authored
Commits on May 31, 2012
  1. merged master into feature/notifications

    Sebastian Vetter authored
  2. minor renaming of views and url names in notifications

    Sebastian Vetter authored
  3. PEP8 and documentation updateds in notification

    Sebastian Vetter authored
  4. PEP8 cleanup

    Sebastian Vetter authored
  5. Merge branch 'feature/notifications' of github.com:elbaschid/django-o…

    Sebastian Vetter authored
    …scar into feature/notifications
    
    Conflicts:
    	oscar/apps/customer/tests.py
    	oscar/apps/customer/views.py
    	sandbox/deploy/nginx/sandbox.conf
Commits on Jun 4, 2012
  1. merge 'master' into 'feature/notifications'

    Sebastian Vetter authored
Commits on Jun 13, 2012
  1. merged branch 'master' into 'feature/notifications'

    Sebastian Vetter authored
Commits on Jun 14, 2012
  1. fixed typo in template for user notifications

    Sebastian Vetter authored
  2. fixed breadcrumbs for notifications in dashboard

    Sebastian Vetter authored
  3. cleaned the notification return statement

    Sebastian Vetter authored
  4. fixed instantiating NotImplmentedError in notification model

    Sebastian Vetter authored
  5. fixed i18n issues in notification models and receivers

    Sebastian Vetter authored
  6. added model_utils and made notifications more generic

    Sebastian Vetter authored
    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.
  7. updated cleanup command to use Notification model

    Sebastian Vetter authored
Something went wrong with that request. Please try again.