Simplify community lists further by letting email subscriptions reuse

the existing infrastructure for accounts and emails, instead of a
having a separate confirmation step
 - Legacy-Id: 10951

Author: OleLaursen

ietf/community/forms.py

@@ -88,25 +88,24 @@ def restrict_state(state_type, slug=None):
             f.required = True
 
 
-class SubscriptionForm(forms.Form):
-    notify_on = forms.ChoiceField(choices=[("all", "All changes"), ("significant", "Only significant state changes")], widget=forms.RadioSelect, initial="all")
-    email = forms.EmailField(label="Your email")
-
-    def __init__(self, operation, clist, *args, **kwargs):
-        self.operation = operation
+class SubscriptionForm(forms.ModelForm):
+    def __init__(self, user, clist, *args, **kwargs):
         self.clist = clist
+        self.user = user
 
         super(SubscriptionForm, self).__init__(*args, **kwargs)
 
-        if operation == "subscribe":
-            self.fields["notify_on"].label = "Get notified on"
-        else:
-            self.fields["notify_on"].label = "For notifications on"
+        self.fields["notify_on"].widget = forms.RadioSelect(choices=self.fields["notify_on"].choices)
+        self.fields["email"].queryset = self.fields["email"].queryset.filter(person__user=user, active=True).order_by("-primary")
+        self.fields["email"].widget = forms.RadioSelect(choices=[t for t in self.fields["email"].choices if t[0]])
+
+        if self.fields["email"].queryset:
+            self.fields["email"].initial = self.fields["email"].queryset[0]
 
     def clean(self):
-        if self.operation == "subscribe":
-            if EmailSubscription.objects.filter(community_list=self.clist, email=self.cleaned_data["email"], significant=self.cleaned_data["notify_on"] == "significant").exists():
-                raise forms.ValidationError("This email address is already subscribed.")
-        else:
-            if not EmailSubscription.objects.filter(community_list=self.clist, email=self.cleaned_data["email"], significant=self.cleaned_data["notify_on"] == "significant").exists():
-                raise forms.ValidationError("Couldn't find a matching subscription?")
+        if EmailSubscription.objects.filter(community_list=self.clist, email=self.cleaned_data["email"], notify_on=self.cleaned_data["notify_on"]).exists():
+            raise forms.ValidationError("You already have a subscription like this.")
+
+    class Meta:
+        model = EmailSubscription
+        fields = ("notify_on", "email")

ietf/community/migrations/0003_cleanup.py

@@ -105,4 +105,10 @@ class Migration(migrations.Migration):
             name='searchrule',
             unique_together=set([]),
         ),
+        migrations.AddField(
+            model_name='emailsubscription',
+            name='notify_on',
+            field=models.CharField(default=b'all', max_length=30, choices=[(b'all', b'All changes'), (b'significant', b'Only significant state changes')]),
+            preserve_default=True,
+        ),
     ]

ietf/community/migrations/0004_cleanup_data.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 from __future__ import unicode_literals
 
-from django.db import migrations
+from django.db import migrations, models
 
 def port_rules_to_typed_system(apps, schema_editor):
     SearchRule = apps.get_model("community", "SearchRule")
@@ -163,6 +163,37 @@ def get_rid_of_empty_lists(apps, schema_editor):
         if not cl.added_docs.exists() and not cl.searchrule_set.exists() and not cl.emailsubscription_set.exists():
             cl.delete()
 
+def move_email_subscriptions_to_preregistered_email(apps, schema_editor):
+    EmailSubscription = apps.get_model("community", "EmailSubscription")
+    Email = apps.get_model("person", "Email")
+    Person = apps.get_model("person", "Person")
+
+    for e in EmailSubscription.objects.all():
+        email_obj = None
+        try:
+            email_obj = Email.objects.get(address=e.email)
+        except Email.DoesNotExist:
+            if e.community_list.user:
+                person = Person.objects.filter(user=e.community_list.user).first()
+
+                #print "creating", e.email, person.ascii
+                # we'll register it on the user, on the assumption
+                # that the user and the subscriber is the same person
+                email_obj = Email.objects.create(
+                    address=e.email,
+                    person=person,
+                )
+
+        if not email_obj:
+            print "deleting", e.email
+            e.delete()
+
+def fill_in_notify_on(apps, schema_editor):
+    EmailSubscription = apps.get_model("community", "EmailSubscription")
+
+    EmailSubscription.objects.filter(significant=False, notify_on="all")
+    EmailSubscription.objects.filter(significant=True, notify_on="significant")
+
 def noop(apps, schema_editor):
     pass
 
@@ -175,9 +206,21 @@ class Migration(migrations.Migration):
     operations = [
         migrations.RunPython(port_rules_to_typed_system, delete_extra_person_rules),
         migrations.RunPython(rename_rule_type_forwards, rename_rule_type_backwards),
+        migrations.RunPython(move_email_subscriptions_to_preregistered_email, noop),
         migrations.RunPython(get_rid_of_empty_lists, noop),
+        migrations.RunPython(fill_in_notify_on, noop),
         migrations.RemoveField(
             model_name='searchrule',
             name='value',
         ),
+        migrations.AlterField(
+            model_name='emailsubscription',
+            name='email',
+            field=models.ForeignKey(to='person.Email'),
+            preserve_default=True,
+        ),
+        migrations.RemoveField(
+            model_name='emailsubscription',
+            name='significant',
+        ),
     ]

ietf/community/models.py

@@ -4,7 +4,7 @@
 
 from ietf.doc.models import Document, DocEvent, State
 from ietf.group.models import Group
-from ietf.person.models import Person
+from ietf.person.models import Person, Email
 
 class CommunityList(models.Model):
     user = models.ForeignKey(User, blank=True, null=True)
@@ -62,11 +62,16 @@ class SearchRule(models.Model):
 
 class EmailSubscription(models.Model):
     community_list = models.ForeignKey(CommunityList)
-    email = models.CharField(max_length=200)
-    significant = models.BooleanField(default=False)
+    email = models.ForeignKey(Email)
+
+    NOTIFICATION_CHOICES = [
+        ("all", "All changes"),
+        ("significant", "Only significant state changes")
+    ]
+    notify_on = models.CharField(max_length=30, choices=NOTIFICATION_CHOICES, default="all")
 
     def __unicode__(self):
-        return u"%s to %s (%s changes)" % (self.email, self.community_list, "significant" if self.significant else "all")
+        return u"%s to %s (%s changes)" % (self.email, self.community_list, self.notify_on)
 
 
 def notify_events(sender, instance, **kwargs):

ietf/community/resources.py

@@ -51,8 +51,8 @@ class Meta:
         #resource_name = 'emailsubscription'
         filtering = { 
             "id": ALL,
-            "email": ALL,
-            "significant": ALL,
+            "email": ALL_WITH_RELATIONS,
+            "notify_on": ALL,
             "community_list": ALL_WITH_RELATIONS,
         }
 api.community.register(EmailSubscriptionResource())

ietf/community/tests.py

@@ -9,7 +9,7 @@
 from ietf.community.utils import docs_matching_community_list_rule, community_list_rules_matching_doc
 from ietf.doc.models import State
 from ietf.doc.utils import add_state_change_event
-from ietf.person.models import Person
+from ietf.person.models import Person, Email
 from ietf.utils.test_data import make_test_data
 from ietf.utils.test_utils import login_testing_unauthorized, TestCase
 from ietf.utils.mail import outbox
@@ -217,28 +217,18 @@ def test_feed(self):
         self.assertEqual(r.status_code, 200)
         self.assertTrue('<entry>' not in r.content)
 
-    def extract_confirm_url(self, confirm_email):
-        # dig out confirm_email link
-        msg = confirm_email.get_payload(decode=True)
-        line_start = "http"
-        confirm_url = None
-        for line in msg.split("\n"):
-            if line.strip().startswith(line_start):
-                confirm_url = line.strip()
-        self.assertTrue(confirm_url)
-
-        return confirm_url
-
     def test_subscription(self):
         draft = make_test_data()
 
-        url = urlreverse("community_personal_subscription", kwargs={ "operation": "subscribe", "username": "plain" })
+        url = urlreverse("community_personal_subscription", kwargs={ "username": "plain" })
 
-        # subscribe without list
+        login_testing_unauthorized(self, "plain", url)
+
+        # subscription without list
         r = self.client.get(url)
         self.assertEqual(r.status_code, 404)
 
-        # subscribe with list
+        # subscription with list
         clist = CommunityList.objects.create(user=User.objects.get(username="plain"))
         clist.added_docs.add(draft)
         SearchRule.objects.create(
@@ -250,42 +240,19 @@ def test_subscription(self):
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
 
-        # do subscribe
-        mailbox_before = len(outbox)
-        r = self.client.post(url, { "email": "subscriber@example.com", "notify_on": "significant" })
-        self.assertEqual(r.status_code, 200)
-        self.assertEqual(len(outbox), mailbox_before + 1)
-
-        # go to confirm page
-        confirm_url = self.extract_confirm_url(outbox[-1])
-        r = self.client.get(confirm_url)
-        self.assertEqual(r.status_code, 200)
-
-        # confirm subscribe
-        r = self.client.post(confirm_url, { 'action': 'confirm' })
+        # subscribe
+        email = Email.objects.filter(person__user__username="plain").first()
+        r = self.client.post(url, { "email": email.pk, "notify_on": "significant", "action": "subscribe" })
         self.assertEqual(r.status_code, 302)
-        self.assertEqual(EmailSubscription.objects.filter(community_list=clist, email="subscriber@example.com", significant=True).count(), 1)
-
-        # unsubscribe
-        url = urlreverse("community_personal_subscription", kwargs={ "operation": "unsubscribe", "username": "plain" })
-        r = self.client.get(url)
-        self.assertEqual(r.status_code, 200)
 
-        # do unsubscribe
-        mailbox_before = len(outbox)
-        r = self.client.post(url, { "email": "subscriber@example.com", "notify_on": "significant" })
-        self.assertEqual(r.status_code, 200)
-        self.assertEqual(len(outbox), mailbox_before + 1)
+        subscription = EmailSubscription.objects.filter(community_list=clist, email=email, notify_on="significant").first()
 
-        # go to confirm page
-        confirm_url = self.extract_confirm_url(outbox[-1])
-        r = self.client.get(confirm_url)
-        self.assertEqual(r.status_code, 200)
+        self.assertTrue(subscription)
 
-        # confirm unsubscribe
-        r = self.client.post(confirm_url, { 'action': 'confirm' })
+        # delete subscription
+        r = self.client.post(url, { "subscription_id": subscription.pk, "action": "unsubscribe" })
         self.assertEqual(r.status_code, 302)
-        self.assertEqual(EmailSubscription.objects.filter(community_list=clist, email="subscriber@example.com", significant=True).count(), 0)
+        self.assertEqual(EmailSubscription.objects.filter(community_list=clist, email=email, notify_on="significant").count(), 0)
 
     def test_notification(self):
         draft = make_test_data()
@@ -299,7 +266,7 @@ def test_notification(self):
             text="test",
         )
 
-        EmailSubscription.objects.create(community_list=clist, email="subscriber@example.com", significant=True)
+        EmailSubscription.objects.create(community_list=clist, email=Email.objects.filter(person__user__username="plain").first(), notify_on="significant")
 
         mailbox_before = len(outbox)
         active_state = State.objects.get(type="draft", slug="active")

ietf/community/urls.py

@@ -8,15 +8,13 @@
     url(r'^personal/(?P<username>[^/]+)/untrackdocument/(?P<name>[^/]+)/$', 'untrack_document', name='community_personal_untrack_document'),
     url(r'^personal/(?P<username>[^/]+)/csv/$', 'export_to_csv', name='community_personal_csv'),
     url(r'^personal/(?P<username>[^/]+)/feed/$', 'feed', name='community_personal_feed'),
-    url(r'^personal/(?P<username>[^/]+)/(?P<operation>subscribe|unsubscribe)/$', 'subscription', name='community_personal_subscription'),
-    url(r'^personal/(?P<username>[^/]+)/(?P<operation>subscribe|unsubscribe)/confirm/(?P<auth>[^/]+)/$', 'confirm_subscription', name='community_personal_confirm_subscription'),
+    url(r'^personal/(?P<username>[^/]+)/subscription/$', 'subscription', name='community_personal_subscription'),
 
     url(r'^group/(?P<acronym>[\w.@+-]+)/$', 'view_list', name='community_group_view_list'),
     url(r'^group/(?P<acronym>[\w.@+-]+)/manage/$', 'manage_list', name='community_group_manage_list'),
     url(r'^group/(?P<acronym>[\w.@+-]+)/trackdocument/(?P<name>[^/]+)/$', 'track_document', name='community_group_track_document'),
     url(r'^group/(?P<acronym>[\w.@+-]+)/untrackdocument/(?P<name>[^/]+)/$', 'untrack_document', name='community_group_untrack_document'),
     url(r'^group/(?P<acronym>[\w.@+-]+)/csv/$', 'export_to_csv', name='community_group_csv'),
     url(r'^group/(?P<acronym>[\w.@+-]+)/feed/$', 'feed', name='community_group_feed'),
-    url(r'^group/(?P<acronym>[^/]+)/(?P<operation>subscribe|unsubscribe)/$', 'subscription', name='community_group_subscription'),
-    url(r'^group/(?P<acronym>[^/]+)/(?P<operation>subscribe|unsubscribe)/confirm/(?P<auth>[^/]+)/$', 'confirm_subscription', name='community_group_confirm_subscription'),
+    url(r'^group/(?P<acronym>[\w.@+-]+)/subscription/$', 'subscription', name='community_group_subscription'),
 )

ietf/community/utils.py

@@ -1,8 +1,5 @@
 from django.db.models import Q
 from django.conf import settings
-from django.contrib.sites.models import Site
-from django.http import Http404
-import django.core.signing
 
 from ietf.community.models import CommunityList, EmailSubscription, SearchRule
 from ietf.doc.models import Document, State
@@ -143,49 +140,14 @@ def notify_event_to_subscribers(event):
     subscriptions = EmailSubscription.objects.filter(community_list__in=community_lists_tracking_doc(event.doc)).distinct()
 
     if not significant:
-        subscriptions = subscriptions.filter(significant=False)
+        subscriptions = subscriptions.filter(notify_on="all")
 
-    for sub in subscriptions.select_related("community_list"):
+    for sub in subscriptions.select_related("community_list", "email"):
         clist = sub.community_list
         subject = '%s notification: Changes to %s' % (clist.long_name(), event.doc.name)
 
-        send_mail(None, sub.email, settings.DEFAULT_FROM_EMAIL, subject, 'community/notification_email.txt',
+        send_mail(None, sub.email.address, settings.DEFAULT_FROM_EMAIL, subject, 'community/notification_email.txt',
                   context = {
                       'event': event,
                       'clist': clist,
                   })
-
-def confirmation_salt(operation, clist):
-    return ":".join(["community",
-                     operation,
-                     "personal" if clist.user else "group",
-                     clist.user.username if clist.user else clist.group.acronym])
-
-def send_subscription_confirmation_email(request, clist, operation, to_email, significant):
-    domain = Site.objects.get_current().domain
-    subject = 'Confirm list subscription: %s' % clist
-    from_email = settings.DEFAULT_FROM_EMAIL
-
-    auth = django.core.signing.dumps([to_email, 1 if significant else 0], salt=confirmation_salt("subscribe", clist))
-
-    send_mail(request, to_email, from_email, subject, 'community/confirm_email.txt', {
-        'domain': domain,
-        'clist': clist,
-        'auth': auth,
-        'operation': operation,
-    })
-
-def verify_confirmation_data(auth, clist, operation):
-    try:
-        data = django.core.signing.loads(auth, salt=confirmation_salt(operation, clist), max_age=24 * 60 * 60)
-    except django.core.signing.BadSignature:
-        raise Http404("Invalid or expired auth")
-
-    try:
-        to_email, significant = data[:2]
-    except ValueError:
-        raise Http404("Invalid data")
-
-    return to_email, bool(significant)
-
-