[bug 623982, bug 629520] Anonymous watches. Delete watch when deliver…

…y fails.

* Add an is_active column.
* Add stub implementation for confirmation email.
* Update .notify() to send a confirmation email and raise an ActivationRequestFailed exception if the email message fails to send.
* Add function Event._activation_email, which receives a watch and an email and returns an EmailMessage.
* Make questions anonymous watches work, add extra views for confirming/unsubscribing from watches.
* Add Event.get_activation_url() for use in email templates.
* Add Event.get_watch_description() for events to describe their watches in string form. This comes in handy when sending out emails, to explain why the receiver is getting this.
* Catch SMTPRecipientsRefused exception and show message about it.
* Update wiki tests.

apps/notifications/events.py

@@ -1,6 +1,8 @@
 import random
+from smtplib import SMTPException
 from string import letters
 
+from django.conf import settings
 from django.contrib.auth.models import User
 from django.contrib.contenttypes.models import ContentType
 from django.core import mail
@@ -12,6 +14,12 @@
 from notifications.utils import merge, hash_to_unsigned
 
 
+class ActivationRequestFailed(Exception):
+    """Raised when activation request fails, e.g. if email could not be sent"""
+    def __init__(self, msgs):
+        self.msgs = msgs
+
+
 def _unique_by_email(users_and_watches):
     """Given a sequence of (User/EmailUser, Watch) pairs clustered by email
     address (which is never ''), yield from each cluster...
@@ -195,7 +203,7 @@ def filter_conditions():
             'LEFT JOIN auth_user u ON u.id=w.user_id {joins} '
             'WHERE {wheres} '
             'AND (length(w.email)>0 OR length(u.email)>0) '
-            'AND w.secret IS NULL '
+            'AND w.is_active '
             'ORDER BY u.email DESC, w.email DESC').format(
             joins=' '.join(joins),
             wheres=' AND '.join(wheres))
@@ -263,7 +271,7 @@ def _watches_belonging_to_user(cls, user_or_email, object_id=None,
     # Funny arg name to reserve use of nice ones for filters
     def is_notifying(cls, user_or_email_, object_id=None, **filters):
         """Return whether the user/email is watching this event (either
-        confirmed or unconfirmed), conditional on meeting the criteria in
+        active or inactive watches), conditional on meeting the criteria in
         `filters`.
 
         Count only watches that match the given filters exactly--not ones which
@@ -290,11 +298,18 @@ def notify(cls, user_or_email_, object_id=None, **filters):
         occurs and meets the criteria given in `filters`.
 
         Return the created (or the existing matching) Watch so you can call
-        confirm() on it if you're so inclined.
+        activate() on it if you're so inclined.
 
         Implementations in subclasses may take different arguments; see the
         docstring of is_notifying().
 
+        Send an activation email if an anonymous watch is created and
+        settings.CONFIRM_ANONYMOUS_WATCHES = True. If the activation request
+        fails, raise a ActivationRequestFailed exception.
+
+        Calling notify() twice for an anonymous user will send the email
+        each time.
+
         """
         # A test-for-existence-then-create race condition exists here, but it
         # doesn't matter: de-duplication on fire() and deletion of all matches
@@ -312,27 +327,36 @@ def notify(cls, user_or_email_, object_id=None, **filters):
                     ContentType.objects.get_for_model(cls.content_type)
             create_kwargs['email' if isinstance(user_or_email_, basestring)
                           else 'user'] = user_or_email_
-            # Registered users don't need to confirm => no secret.
-            # ... but anonymous users do.
-            secret = (''.join(random.choice(letters) for x in xrange(10)) if
-                      'email' in create_kwargs else None)
+            secret = ''.join(random.choice(letters) for x in xrange(10))
+            # Registered users don't need to confirm, but anonymous users do.
+            is_active = ('user' in create_kwargs or
+                          not settings.CONFIRM_ANONYMOUS_WATCHES)
             if object_id:
                 create_kwargs['object_id'] = object_id
             watch = Watch.objects.create(
                 secret=secret,
+                is_active=is_active,
                 event_type=cls.event_type,
                 **create_kwargs)
             for k, v in filters.iteritems():
-                # TODO: Auto-hash v into an int if it isn't one?
                 WatchFilter.objects.create(watch=watch, name=k,
                                            value=hash_to_unsigned(v))
+        # Send email for inactive watches.
+        if not watch.is_active:
+            email = watch.user.email if watch.user else watch.email
+            message = cls._activation_email(watch, email)
+            try:
+                message.send()
+            except SMTPException, e:
+                watch.delete()
+                raise ActivationRequestFailed(e.recipients)
         return watch
 
     @classmethod
     def stop_notifying(cls, user_or_email_, **filters):
         """Delete all watches matching the exact user/email and filters.
 
-        Delete both confirmed and unconfirmed watches. If duplicate watches
+        Delete both active and inactive watches. If duplicate watches
         exist due to the get-then-create race condition, delete them all.
 
         Implementations in subclasses may take different arguments; see the
@@ -377,6 +401,35 @@ def _users_watching(self, **kwargs):
         """
         return self._users_watching_by_filter(**kwargs)
 
+    @classmethod
+    def _activation_email(cls, watch, email):
+        """Return an EmailMessage to send to anonymous watchers.
+
+        They are expected to follow the activation URL sent in the email to
+        activate their watch, so you should include at least that.
+
+        """
+        # TODO: basic implementation.
+        return mail.EmailMessage('TODO', 'Activate!',
+                                 settings.NOTIFICATIONS_FROM_ADDRESS,
+                                 [email])
+
+    @classmethod
+    def _activation_url(cls, watch):
+        """Return a URL pointing to the watch activation.
+
+        TODO: provide generic implementation of this before liberating.
+        Generic implementation could involve a setting to the default reverse()
+        path, e.g. 'notifications.activate_watch'.
+
+        """
+        raise NotImplementedError
+
+    @classmethod
+    def watch_description(cls, watch):
+        """Return a description of the watch which can be used in emails."""
+        raise NotImplementedError
+
 
 class EventUnion(Event):
     """Fireable conglomeration of multiple events

apps/notifications/models.py

@@ -89,21 +89,22 @@ class Watch(ModelBase):
     # Email stored only in the case of anonymous users:
     email = models.EmailField(db_index=True, null=True, blank=True)
 
-    # Secret for confirming anonymous watch email addresses. We clear the
-    # secret upon confirmation. Until then, the watch is inactive.
+    # Secret for activating anonymous watch email addresses.
     secret = models.CharField(max_length=10, null=True, blank=True)
+    # Active watches receive notifications, inactive watches don't.
+    is_active = models.BooleanField(default=False, db_index=True)
 
     def __unicode__(self):
         rest = self.content_object or self.content_type or self.object_id
         return u'[%s] %s, %s' % (self.pk, self.event_type, str(rest))
 
-    def confirm(self):
-        """Activate this watch so it actually fires.
+    def activate(self):
+        """Enable this watch so it actually fires.
 
         Return self to support method chaining.
 
         """
-        self.secret = None
+        self.is_active = True
         return self
 
 

apps/notifications/tests/__init__.py

@@ -9,7 +9,8 @@
 
 def watch(save=False, **kwargs):
     # TODO: better defaults, when there are events available.
-    defaults = {'user': kwargs.get('user') or user()}
+    defaults = {'user': kwargs.get('user') or user(),
+                'is_active': True}
     defaults.update(kwargs)
     w = Watch.objects.create(**defaults)
     if save:

apps/notifications/tests/test_events.py

@@ -2,6 +2,7 @@
 import mock
 from nose.tools import eq_
 
+from django.conf import settings
 from django.contrib.auth.models import AnonymousUser
 from django.contrib.contenttypes.models import ContentType
 from django.core import mail
@@ -65,11 +66,11 @@ def test_simple(self):
         watch(event_type='something else', email='never@fires.com').save()
         self._emails_eq(['regist@ered.com', 'anon@ymous.com'], SimpleEvent())
 
-    def test_unconfirmed(self):
-        """Make sure unconfirmed watches don't fire."""
-        watch(event_type=TYPE, email='anon@ymous.com', secret='x' * 10).save()
-        watch(event_type=TYPE, email='confirmed@one.com').save()
-        self._emails_eq(['confirmed@one.com'], SimpleEvent())
+    def test_inactive(self):
+        """Make sure inactive watches don't fire."""
+        watch(event_type=TYPE, email='anon@ymous.com', is_active=False).save()
+        watch(event_type=TYPE, email='active@one.com').save()
+        self._emails_eq(['active@one.com'], SimpleEvent())
 
     def test_content_type(self):
         """Make sure watches filter properly by content type."""
@@ -280,7 +281,8 @@ def test_hashing(self):
         FilteredEvent.notify('red@x.com', color='red', flavor=6)
         FilteredEvent.notify('blue@x.com', color=u'blüe', flavor=7)
         assert FilteredEvent.is_notifying('red@x.com', color='red', flavor=6)
-        assert FilteredEvent.is_notifying('blue@x.com', color=u'blüe', flavor=7)
+        assert FilteredEvent.is_notifying('blue@x.com', color=u'blüe',
+                                          flavor=7)
         assert not FilteredEvent.is_notifying('blue@x.com', color='red',
                                               flavor=7)
 
@@ -306,9 +308,10 @@ def test_mock_model(self):
 class MailTests(TestCase):
     """Tests for mail-sending and templating"""
 
+    @mock.patch_object(settings._wrapped, 'CONFIRM_ANONYMOUS_WATCHES', False)
     def test_fire(self):
         """Assert that fire() runs and that generated mails get sent."""
-        SimpleEvent.notify('hi@there.com').confirm().save()
+        SimpleEvent.notify('hi@there.com').activate().save()
         SimpleEvent().fire()
 
         eq_(1, len(mail.outbox))
@@ -317,17 +320,38 @@ def test_fire(self):
         eq_('Subject!', first_mail.subject)
         eq_('Body!', first_mail.body)
 
+    def test_anonymous_notify_and_fire(self):
+        """Calling notify() sends confirmation email, and calling fire() sends
+        notification email."""
+        w = SimpleEvent.notify('hi@there.com')
+
+        eq_(1, len(mail.outbox))
+        first_mail = mail.outbox[0]
+        eq_(['hi@there.com'], first_mail.to)
+        eq_('TODO', first_mail.subject)
+        eq_('Activate!', first_mail.body)
+
+        w.activate().save()
+        SimpleEvent().fire()
+
+        second_mail = mail.outbox[1]
+        eq_(['hi@there.com'], second_mail.to)
+        eq_('Subject!', second_mail.subject)
+        eq_('Body!', second_mail.body)
+
+    @mock.patch_object(settings._wrapped, 'CONFIRM_ANONYMOUS_WATCHES', False)
     def test_exclude(self):
         """Assert the `exclude` arg to fire() excludes the given user."""
-        SimpleEvent.notify('du@de.com').confirm().save()
+        SimpleEvent.notify('du@de.com').activate().save()
         registered_user = user(email='ex@clude.com', save=True)
-        SimpleEvent.notify(registered_user).confirm().save()
+        SimpleEvent.notify(registered_user).activate().save()
 
         SimpleEvent().fire(exclude=registered_user)
 
         eq_(1, len(mail.outbox))
         first_mail = mail.outbox[0]
         eq_(['du@de.com'], first_mail.to)
+        eq_('Subject!', first_mail.subject)
 
 
 def test_anonymous_user_compares():

apps/questions/events.py

@@ -7,6 +7,7 @@
 
 from notifications.events import InstanceEvent
 from questions.models import Question
+from sumo.urlresolvers import reverse
 
 
 class QuestionEvent(InstanceEvent):
@@ -17,6 +18,24 @@ def __init__(self, answer):
         super(QuestionEvent, self).__init__(answer.question)
         self.answer = answer
 
+    @classmethod
+    def _activation_email(cls, watch, email):
+        """Return an EmailMessage containing the activation URL to be sent to
+        a new watcher."""
+        subject = _('Please confirm your email address')
+        email_kwargs = {'activation_url': cls._activation_url(watch),
+                        'domain': Site.objects.get_current().domain,
+                        'watch_description': cls.get_watch_description(watch)}
+        template_path = 'questions/email/activate_watch.ltxt'
+        message = loader.render_to_string(template_path, email_kwargs)
+        return EmailMessage(subject, message,
+                            settings.NOTIFICATIONS_FROM_ADDRESS, [email])
+
+    @classmethod
+    def _activation_url(cls, watch):
+        return reverse('questions.activate_watch',
+                       args=[watch.id, watch.secret])
+
 
 class QuestionReplyEvent(QuestionEvent):
     """An event which fires when a new answer is posted for a question"""
@@ -39,6 +58,10 @@ def _mails(self, users_and_watches):
                              [u.email]) for
                 u, dummy in users_and_watches)
 
+    @classmethod
+    def get_watch_description(cls, watch):
+        return _('New answers for question: %s') % watch.content_object.title
+
 
 class QuestionSolvedEvent(QuestionEvent):
     """An event which fires when a Question gets solved"""
@@ -63,3 +86,8 @@ def _mails(self, users_and_watches):
                              settings.NOTIFICATIONS_FROM_ADDRESS,
                              [u.email]) for
                 u, dummy in users_and_watches)
+
+    @classmethod
+    def get_watch_description(cls, watch):
+        question = watch.content_object
+        return _('Solution found for question: %s') % question.title

apps/questions/templates/questions/activate_watch.html

@@ -0,0 +1,36 @@
+{# vim: set ts=2 et sts=2 sw=2: #}
+{% extends "questions/base.html" %}
+{% if is_active %}
+  {% set title = _('Subscription confirmed.') %}
+{% else %}
+  {% set title = _('There was a problem confirming your subscription.') %}
+{% endif %}
+{% set classes = 'questions' %}
+
+{% block content %}
+<div class="activate-watch">
+  <h2>{{ question.title }}</h2>
+  {% if is_active %}
+    <h3>
+        {{ _('You will now receive updates via email.') }}
+    </h3>
+    <ul>
+      <li>
+        <a href="{{ question.get_absolute_url() }}" id="activate-watch">
+          {{ _('Go back to the question.') }}
+        </a>
+      </li>
+      <li>
+        <a id="remove-watch" href="{{ unsubscribe_url }}">
+          {{ _('Stop receiving updates via email for this question.') }}
+        </a>
+      </li>
+    </ul>
+  {% else %}
+    <h3>{{ title }}</h3>
+    <a href="{{ question.get_absolute_url() }}" id="activate-watch">
+      {{ _('Go back to the question and try again.') }}
+    </a>
+  {% endif %}
+</div>
+{% endblock %}

apps/questions/templates/questions/email/activate_watch.ltxt

@@ -0,0 +1,13 @@
+{% load i18n %}{# L10n: This is an email.  Whitespace matters! #}{% blocktrans %}
+You are about to subscribe to:
+{{ watch_description }}
+
+To confirm your subscription, please click the link below
+or copy and paste the whole thing into your browser's location bar:
+
+https://{{ domain }}{{ activation_url }}
+
+Thanks!
+
+The {{ domain }} team
+{% endblocktrans %}

apps/questions/templates/questions/unsubscribe_watch.html

@@ -0,0 +1,29 @@
+{# vim: set ts=2 et sts=2 sw=2: #}
+{% extends "questions/base.html" %}
+{% if success %}
+  {% set title = _('You have been unsubscribed.') %}
+{% else %}
+  {% set title = _('There was a problem unsubscribing.') %}
+{% endif %}
+{% set classes = 'questions' %}
+
+{% block content %}
+<div class="unsubscribe-watch">
+  <h2>{{ title }}</h2>
+  <p>
+    {% if success %}
+      {% trans question_title=question.title %}
+        You will no longer receive email updates for this question:
+        <a href="{{ question_url }}">{{ question_title }}</a>
+      {% endtrans %}
+    {% else %}
+      {% trans question_title=question.title,
+               question_url=question.get_absolute_url() %}
+        We could not unsubscribe you from this question:
+        <a href="{{ question_url }}">{{ question_title }}</a>.
+        Please make sure you copied and pasted the unsubscribe link correctly.
+      {% endtrans %}
+    {% endif %}
+  </p>
+</div>
+{% endblock %}