[bug 599028] Support unsubscribe links in notifications app. Implement them for all wiki notifications.

* Add unsubscribe view to notifications app. Currently depends on Jingo and a template called "base.html".
* Rename get_watch_description() to description_of_watch(). Trying to get away from get_* functions and name them (shorter, better-flowing) nouns (unambiguously) instead.
* Stop using easy-to-misidentify letters like I and l in secrets.
* Add a secret to the default value of watch() so unsubscribing in a test requires actual secret data.

apps/notifications/events.py

@@ -1,6 +1,5 @@
 import random
 from smtplib import SMTPException
-from string import letters
 
 from django.conf import settings
 from django.contrib.auth.models import User
@@ -331,7 +330,12 @@ class Event(object):
                     ContentType.objects.get_for_model(cls.content_type)
             create_kwargs['email' if isinstance(user_or_email_, basestring)
                           else 'user'] = user_or_email_
-            secret = ''.join(random.choice(letters) for x in xrange(10))
+            # Letters that can't be mistaken for other letters or numbers in
+            # most fonts, in case people try to type these:
+            distinguishable_letters = \
+                'abcdefghjkmnpqrstuvwxyzABCDEFGHJKLMNPQRTUVWXYZ'
+            secret = ''.join(random.choice(distinguishable_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)
@@ -430,8 +434,12 @@ class Event(object):
         raise NotImplementedError
 
     @classmethod
-    def watch_description(cls, watch):
-        """Return a description of the watch which can be used in emails."""
+    def description_of_watch(cls, watch):
+        """Return a description of the Watch which can be used in emails.
+
+        For example, "changes to English articles"
+
+        """
         raise NotImplementedError
 
 

apps/notifications/models.py

@@ -2,8 +2,13 @@ from django.db import models, connections, router
 from django.contrib.auth.models import User, AnonymousUser
 from django.contrib.contenttypes import generic
 from django.contrib.contenttypes.models import ContentType
+from django.contrib.sites.models import Site
 
+from sumo.helpers import urlparams
 from sumo.models import ModelBase
+# TODO: Find the implementation of reverse() according to a setting, or turn
+# sumo's reverse() into a monkeypatch to Django's.
+from sumo.urlresolvers import reverse
 
 
 def multi_raw(query, params, models):
@@ -55,6 +60,8 @@ class Watch(ModelBase):
     is_active = models.BooleanField(default=False, db_index=True)
 
     def __unicode__(self):
+        # TODO: Trace event_type back to find the Event subclass, and ask it
+        # how to describe me in English.
         rest = self.content_object or self.content_type or self.object_id
         return u'Watch %s: %s, %s' % (self.pk, self.event_type,
                                          unicode(rest))
@@ -68,6 +75,14 @@ class Watch(ModelBase):
         self.is_active = True
         return self
 
+    def unsubscribe_url(self):
+        """Return the absolute URL to visit to delete me."""
+        server_relative = urlparams(
+            reverse('notifications.unsubscribe', args=[self.pk]),
+            s=self.secret)
+        return 'https://%s%s' % (Site.objects.get_current().domain,
+                                 server_relative)
+
 
 class WatchFilter(ModelBase):
     """Additional key/value pairs that pare down the scope of a watch"""

apps/notifications/templates/notifications/email/unsubscribe.ltxt

@@ -0,0 +1,4 @@
+{% load i18n %}
+--
+{% blocktrans with watch.unsubscribe_url as unsubscribe_url %}Unsubscribe from these emails:
+{{ unsubscribe_url }}{% endblocktrans %}

apps/notifications/templates/notifications/gone.html

@@ -1,12 +0,0 @@
-{# vim: set ts=2 et sts=2 sw=2: #}
-{% extends "base.html" %}
-{% set title = _('Unsubscribed') %}
-{% set crumbs = [(None, title)] %}
-
-{% block content %}
-<article id="notification-unsubscribe" class="main">
-  <h1>{{ title }}</h1>
-
-  <p>{{ _('You have been unsubscribed from this notification.') }}</p>
-</article>
-{% endblock %}

apps/notifications/templates/notifications/remove.html

@@ -1,21 +0,0 @@
-{# vim: set ts=2 et sts=2 sw=2: #}
-{% extends "base.html" %}
-{% set title = _('Unsubscribe') %}
-{% set crumbs = [(None, title)] %}
-
-{% block content %}
-<article id="notification-unsubscribe" class="main">
-  <h1>{{ title }}</h1>
-
-  {# L10n: {0} is an object title, {1} is an object type. #}
-  <p>{{ _('Do you want to stop receiving email notifications from the {1} <strong>{0}</strong>?')|fe(watched, watch.content_type) }}</p>
-
-  <form action="{{ url('notifications.remove', watch.hash)|urlparams(email=watch.email) }}" method="post">
-    {{ csrf() }}
-    <div class="submit-or-cancel">
-      <a href="{{ watched.get_absolute_url() }}">{{ _('Cancel') }}</a>
-      <input type="submit" value="Unsubscribe"/>
-    </div>
-  </form>
-</article>
-{% endblock %}

apps/notifications/templates/notifications/unsubscribe.html

@@ -0,0 +1,18 @@
+{# vim: set ts=2 et sts=2 sw=2: #}
+{% extends "base.html" %}
+{% set title = _('Unsubscribe') %}
+{% set crumbs = [(None, title)] %}
+
+{% block content %}
+  <article class="main">
+    <h1 class="title">{{ _('Are you sure you want to unsubscribe?') }}</h1>
+    {# TODO: Once Watches know how to describe themselves, print that description. #}
+    <form action="" method="post">
+      {{ csrf() }}
+      <div class="submit-or-cancel">
+        <input type="submit" value="{{ _('Unsubscribe') }}" />
+        <a href="/"{# Discards l10n. Better ideas? #}>{{ _('Cancel') }}</a>
+      </div>
+    </form>
+  </article>
+{% endblock %}

apps/notifications/templates/notifications/unsubscribe_error.html

@@ -0,0 +1,20 @@
+{# vim: set ts=2 et sts=2 sw=2: #}
+{% extends "base.html" %}
+{% set title = _('Unsubscribe') %}
+{% set crumbs = [(None, title)] %}
+
+{% block content %}
+<article class="main">
+  <h1>{{ _('Unsubscribe Error') }}</h1>
+
+  <p>
+    {% trans %}
+      We could not find your subscription. Either it has already been
+      cancelled, or there was a mistake in the unsubscribe link. Please make
+      sure the entire link from the email made it into the browser. If the last
+      part of the link wraps onto a second line in the email, try copying and
+      pasting the link into the browser.
+    {% endtrans %}
+  </p>
+</article>
+{% endblock %}

apps/notifications/templates/notifications/unsubscribe_success.html

@@ -0,0 +1,12 @@
+{# vim: set ts=2 et sts=2 sw=2: #}
+{% extends "base.html" %}
+{% set title = _('Unsubscribe') %}{# Having this change to "Unsubscribed" draws my eye and bothers me. #}
+{% set crumbs = [(None, title)] %}
+
+{% block content %}
+<article class="main">
+  <h1>{{ _('Unsubscribed') }}</h1>
+
+  <p>{{ _('You have been unsubscribed.') }}</p>
+</article>
+{% endblock %}

apps/notifications/templatetags/unsubscribe_instructions.py

@@ -0,0 +1,10 @@
+from django import template
+
+
+register = template.Library()
+
+
+@register.inclusion_tag('notifications/email/unsubscribe.ltxt')
+def unsubscribe_instructions(watch):
+    """Return instructions and link for unsubscribing from the given watch."""
+    return {'watch': watch}

apps/notifications/tests/__init__.py

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

apps/notifications/tests/test_models.py

@@ -1,7 +1,7 @@
 from nose.tools import eq_
 
 from notifications.models import WatchFilter, EmailUser
-from notifications.tests import watch_filter
+from notifications.tests import watch, watch_filter
 from sumo.tests import TestCase
 
 
@@ -10,6 +10,14 @@ from sumo.tests import TestCase
 # E.g. No duplicates in this list: [et.lower() for et in EVENT_TYPES]
 
 
+class WatchTests(TestCase):
+    """Tests for Watch model"""
+
+    def test_unsubscribe_url(self):
+        """Make sure unsubscribe_url() returns something URL-ish."""
+        assert watch().unsubscribe_url().startswith('http')
+
+
 class WatchFilterTests(TestCase):
     """Tests for WatchFilter"""
 

apps/notifications/tests/test_templates.py

@@ -0,0 +1,63 @@
+from nose.tools import eq_
+
+from notifications.models import Watch
+from notifications.tests import watch
+from sumo.helpers import urlparams
+from sumo.tests import TestCase
+from sumo.urlresolvers import reverse
+
+
+FAILURE_STRING = 'We could not find your subscription'
+
+
+class UnsubscribeTests(TestCase):
+    """Integration tests for unsubscribe view"""
+
+    def test_confirmation(self):
+        """Ensure the confirmation page renders if you feed it valid data."""
+        w = watch(save=True)
+        response = self.client.get(
+            urlparams(reverse('notifications.unsubscribe',
+                              locale='en-US',
+                              args=[w.pk]),
+                      s=w.secret))
+        self.assertContains(response, 'Are you sure you want to unsubscribe?')
+
+    def test_no_such_watch(self):
+        """Assert it complains when asked for a nonexistent Watch."""
+        for method in [self.client.get, self.client.post]:
+            response = method(reverse('notifications.unsubscribe',
+                                      locale='en-US',
+                                      args=[33]))
+            self.assertContains(response, FAILURE_STRING)
+
+    def test_no_secret(self):
+        """Assert it complains when no secret is given."""
+        w = watch(save=True)
+        for method in [self.client.get, self.client.post]:
+            response = method(reverse('notifications.unsubscribe',
+                                      locale='en-US',
+                                      args=[w.pk]))
+            self.assertContains(response, FAILURE_STRING)
+
+    def test_wrong_secret(self):
+        """Assert it complains when an incorrect secret is given."""
+        w = watch(save=True)
+        for method in [self.client.get, self.client.post]:
+            response = method(
+                urlparams(reverse('notifications.unsubscribe',
+                                  locale='en-US',
+                                  args=[w.pk]),
+                          s='WRONGwrong'))
+            self.assertContains(response, FAILURE_STRING)
+
+    def test_success(self):
+        """Ensure the watch deletes and view says "yay" when all goes well."""
+        w = watch(save=True)
+        response = self.client.post(
+            urlparams(reverse('notifications.unsubscribe',
+                              locale='en-US',
+                              args=[w.pk]),
+                      s=w.secret))
+        self.assertContains(response, '<h1>Unsubscribed</h1>')
+        eq_(0, Watch.objects.count())

apps/notifications/tests/test_templatetags.py

@@ -0,0 +1,16 @@
+from django.template import Context, Template
+
+from notifications.tests import watch
+from sumo.tests import TestCase
+
+
+class Tests(TestCase):
+    """Tests for our lone template tag"""
+
+    def test_unsubscribe_instructions(self):
+        """Make sure unsubscribe_instructions renders and contains the
+        unsubscribe URL."""
+        w = watch(save=True)
+        template = Template('{% load unsubscribe_instructions %}'
+                            '{% unsubscribe_instructions watch %}')
+        assert w.unsubscribe_url() in template.render(Context({'watch': w}))

apps/notifications/urls.py

@@ -0,0 +1,8 @@
+from django.conf.urls.defaults import patterns, url
+
+
+urlpatterns = patterns('notifications.views',
+    url(r'^unsubscribe/(?P<watch_id>\d+)$',
+        'unsubscribe',
+        name='notifications.unsubscribe')
+)

apps/notifications/views.py

@@ -0,0 +1,22 @@
+import jingo
+
+from notifications.models import Watch
+
+
+def unsubscribe(request, watch_id):
+    """Unsubscribe from (i.e. delete) a Watch."""
+    # Grab the watch and secret; complain if either is wrong:
+    try:
+        watch = Watch.objects.get(pk=watch_id)
+        secret = request.GET.get('s')  # 's' is for 'secret' but saves wrapping
+                                       # in mails
+        if secret != watch.secret:
+            raise Watch.DoesNotExist
+    except Watch.DoesNotExist:
+        return jingo.render(request, 'notifications/unsubscribe_error.html')
+
+    if request.method == 'POST':
+        watch.delete()
+        return jingo.render(request, 'notifications/unsubscribe_success.html')
+
+    return jingo.render(request, 'notifications/unsubscribe.html')

apps/questions/events.py

@@ -25,7 +25,7 @@ class QuestionEvent(InstanceEvent):
         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)}
+                        'watch_description': cls.description_of_watch(watch)}
         template_path = 'questions/email/activate_watch.ltxt'
         message = loader.render_to_string(template_path, email_kwargs)
         return EmailMessage(subject, message,
@@ -63,8 +63,9 @@ class QuestionReplyEvent(QuestionEvent):
              'host': Site.objects.get_current().domain,
              'answer_url': self.answer.get_absolute_url()}
 
-        for u, dummy in users_and_watches:
+        for u, w in users_and_watches:
             c['username'] = u.username
+            c['watch'] = w
             is_asker = asker_id == u.id
             content = (asker_template if is_asker else
                        watcher_template).render(Context(c))
@@ -75,7 +76,7 @@ class QuestionReplyEvent(QuestionEvent):
                                [u.email])
 
     @classmethod
-    def get_watch_description(cls, watch):
+    def description_of_watch(cls, watch):
         return _('New answers for question: %s') % watch.content_object.title
 
 
@@ -97,8 +98,9 @@ class QuestionSolvedEvent(QuestionEvent):
              'question_title': question.title,
              'host': Site.objects.get_current().domain,
              'solution_url': question.solution.get_absolute_url()}
-        for u, dummy in users_and_watches:
+        for u, w in users_and_watches:
             c['username'] = u.username  # '' if anonymous
+            c['watch'] = w
             content = t.render(Context(c))
             yield EmailMessage(
                 subject,
@@ -107,6 +109,6 @@ class QuestionSolvedEvent(QuestionEvent):
                 [u.email])
 
     @classmethod
-    def get_watch_description(cls, watch):
+    def description_of_watch(cls, watch):
         question = watch.content_object
         return _('Solution found for question: %s') % question.title

apps/sumo/parser.py

@@ -6,7 +6,6 @@ from wikimarkup.parser import Parser
 
 from gallery.models import Image
 from sumo.urlresolvers import reverse
-from wiki.models import Document
 
 
 ALLOWED_ATTRIBUTES = {
@@ -96,6 +95,11 @@ def _get_wiki_link(title, locale):
     found is False if the document does not exist.
 
     """
+    # Prevent circular import. sumo is conceptually a utils apps and shouldn't
+    # have import-time (or really, any, but that's not going to happen)
+    # dependencies on client apps.
+    from wiki.models import Document
+
     d = get_object_fallback(Document, locale=locale, title=title,
                             is_template=False)
     if d:

apps/wiki/events.py

@@ -25,11 +25,12 @@ def notification_mails(revision, subject, template, url, users_and_watches):
          'creator': revision.creator,
          'url': url,
          'host': Site.objects.get_current().domain}
-    content = t.render(Context(c))
-    mail = EmailMessage(subject, content, settings.NOTIFICATIONS_FROM_ADDRESS)
+    mail = EmailMessage(subject, '', settings.NOTIFICATIONS_FROM_ADDRESS)
 
-    for u, dummy in users_and_watches:
+    for u, w in users_and_watches:
+        c['watch'] = w
         mail.to = [u.email]
+        mail.body = t.render(Context(c))
         yield mail
 
 

apps/wiki/templates/wiki/email/approved.ltxt

@@ -1,4 +1,4 @@
-{% load i18n %}
+{% load i18n %}{% load unsubscribe_instructions %}
 {# L10n: This is an email. Whitespace matters! #}
 {% blocktrans %}A new revision has been approved for the document
 {{ document_title }}.
@@ -7,3 +7,4 @@ To view the updated document, click the following
 link, or paste it into your browser's location bar:
 {% endblocktrans %}
 https://{{ host }}{{ url }}
+{% unsubscribe_instructions watch %}

apps/wiki/templates/wiki/email/edited.ltxt

@@ -1,4 +1,4 @@
-{% load i18n %}
+{% load i18n %}{% load unsubscribe_instructions %}
 {# L10n: This is an email. Whitespace matters! #}
 {% blocktrans %}
 {{ creator }} created a new revision to the document
@@ -8,3 +8,4 @@ To view this document's history, click the following
 link, or paste it into your browser's location bar:
 {% endblocktrans %}
 https://{{ host }}{{ url }}
+{% unsubscribe_instructions watch %}

apps/wiki/templates/wiki/email/ready_for_review.ltxt

@@ -1,4 +1,4 @@
-{% load i18n %}
+{% load i18n %}{% load unsubscribe_instructions %}
 {# L10n: This is an email. Whitespace matters! #}
 {% blocktrans %}
 {{ creator }} submitted a new revision to the document
@@ -8,3 +8,4 @@ To review this revision, click the following
 link, or paste it into your browser's location bar:
 {% endblocktrans %}
 https://{{ host }}{{ url }}
+{% unsubscribe_instructions watch %}

apps/wiki/templates/wiki/email/reviewed.ltxt

@@ -15,4 +15,4 @@
 To view the history of this document, click the following
 link, or paste it into your browser's location bar:
 {% endblocktrans %}
-https://{{ host }}{{ url }}
+https://{{ host }}{{ url }}

apps/wiki/tests/test_tasks.py

@@ -33,8 +33,7 @@ Message from the reviewer:
 To view the history of this document, click the following
 link, or paste it into your browser's location bar:
 
-https://testserver/en-US/kb/%s/history
-"""
+https://testserver/en-US/kb/%s/history"""
 
 
 class RebuildTestCase(TestCase):

apps/wiki/tests/test_templates.py

@@ -32,7 +32,10 @@ To review this revision, click the following
 link, or paste it into your browser's location bar:
 
 https://testserver/en-US/kb/%s/review/%s
-"""
+
+--
+Unsubscribe from these emails:
+https://testserver/en-US/unsubscribe/%s?s=%s"""
 
 DOCUMENT_EDITED_EMAIL_CONTENT = """
 
@@ -44,7 +47,10 @@ To view this document's history, click the following
 link, or paste it into your browser's location bar:
 
 https://testserver/en-US/kb/%s/history
-"""
+
+--
+Unsubscribe from these emails:
+https://testserver/en-US/unsubscribe/%s?s=%s"""
 
 APPROVED_EMAIL_CONTENT = """
 
@@ -55,7 +61,10 @@ To view the updated document, click the following
 link, or paste it into your browser's location bar:
 
 https://testserver/en-US/kb/%s
-"""
+
+--
+Unsubscribe from these emails:
+https://testserver/en-US/unsubscribe/%s?s=%s"""
 
 
 class DocumentTests(TestCaseBase):
@@ -474,9 +483,11 @@ class NewRevisionTests(TestCaseBase):
         get_current.return_value.domain = 'testserver'
 
         # Sign up for notifications:
-        EditDocumentEvent.notify('sam@example.com', self.d).activate().save()
-        ReviewableRevisionInLocaleEvent.notify('joe@example.com',
-            locale='en-US').activate().save()
+        edit_watch = EditDocumentEvent.notify('sam@example.com', self.d)
+        edit_watch.activate().save()
+        reviewable_watch = ReviewableRevisionInLocaleEvent.notify(
+            'joe@example.com', locale='en-US')
+        reviewable_watch.activate().save()
 
         # Edit a document:
         response = self.client.post(
@@ -494,13 +505,15 @@ class NewRevisionTests(TestCaseBase):
                  subject=u'%s is ready for review (%s)' % (self.d.title,
                                                            new_rev.creator),
                  body=READY_FOR_REVIEW_EMAIL_CONTENT %
-                    (self.d.title, self.d.slug, new_rev.id),
+                    (self.d.title, self.d.slug, new_rev.id,
+                     reviewable_watch.pk, reviewable_watch.secret),
                  to=['joe@example.com'])
         attrs_eq(mail.outbox[1],
                  subject=u'%s was edited by %s' % (self.d.title,
                                                    new_rev.creator),
                  body=DOCUMENT_EDITED_EMAIL_CONTENT %
-                    (self.d.title, self.d.slug),
+                    (self.d.title, self.d.slug, edit_watch.pk,
+                     edit_watch.secret),
                  to=['sam@example.com'])
 
     @mock.patch.object(ReviewableRevisionInLocaleEvent, 'fire')
@@ -730,8 +743,9 @@ class ReviewRevisionTests(TestCaseBase):
         get_current.return_value.domain = 'testserver'
 
         # Subscribe to approvals:
-        ApproveRevisionInLocaleEvent.notify('joe@example.com',
-                                            locale='en-US').activate().save()
+        watch = ApproveRevisionInLocaleEvent.notify('joe@example.com',
+                                                    locale='en-US')
+        watch.activate().save()
 
         # Approve something:
         significance = SIGNIFICANCES[0][0]
@@ -753,7 +767,8 @@ class ReviewRevisionTests(TestCaseBase):
                  subject='%s (%s) has a new approved revision' %
                      (self.document.title, self.document.locale),
                  body=APPROVED_EMAIL_CONTENT %
-                    (self.document.title, self.document.slug),
+                    (self.document.title, self.document.slug, watch.pk,
+                     watch.secret),
                  to=['joe@example.com'])
 
     @mock.patch.object(send_reviewed_notification, 'delay')

media/css/main.css

@@ -874,7 +874,7 @@ div.submit-or-cancel {
 }
 
 div.submit-or-cancel input[type="submit"] {
-    margin: 0 0 0 1em;
+    margin: 0 1em 0 0;
 }
 
 /* Image markup styling */

urls.py

@@ -32,8 +32,9 @@ urlpatterns = patterns('',
     url(r'^jsi18n/.*$', cache_page(60 * 60 * 24 * 365)(javascript_catalog),
         {'domain': 'javascript', 'packages': ['kitsune']}, name='jsi18n'),
 
-    url(r'^', include('dashboards.urls')),
-    url(r'^', include('landings.urls')),
+    (r'^', include('dashboards.urls')),
+    (r'^', include('landings.urls')),
+    (r'^', include('notifications.urls')),  # Keep short for email wrapping.
 
     # Users
     ('', include('users.urls')),