Merge lp:~nigelbabu/launchpad/90628-spec-sub into lp:launchpad

[Nigel Babu]

Updated subscriptions to specifications to be sorted by name and cached.

[Curtis Hovey (sinzui)]

Thank you very much for this branch.

In our initial review of the code, we discovered that one change did not make sense. We discovered the the test was faulty so an additional change was need to test just the new subscription email. Testing also revealed a guard was need around my own suggestion to update the property_cache in subscribe(person).

This branch looks good to land, and I will do so now.

review: Approve (code)

=== modified file 'lib/lp/blueprints/doc/specification-notifications.txt'
--- lib/lp/blueprints/doc/specification-notifications.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/blueprints/doc/specification-notifications.txt	2011-05-23 20:02:28 +0000
@@ -63,21 +63,27 @@
 
     >>> for subscription in svg_support.subscriptions:
     ...     print subscription.person.preferredemail.email
+    [e-mail]
+    [e-mail]
     [e-mail]
     [e-mail]
-    [e-mail]
     [e-mail]
-    [e-mail]
 
 Let's set a different approver and add a subscriber.
 
     >>> from lp.registry.interfaces.person import IPersonSet
+
+    >>> stub.test_emails = []
     >>> mark = getUtility(IPersonSet).getByEmail('[e-mail]')
     >>> sample_person = getUtility(IPersonSet).getByEmail(
     ...     '[e-mail]')
     >>> svg_support.approver = mark
     >>> svg_support.subscribe(sample_person, sample_person, False)
     <...>
+    >>> transaction.commit()
+    >>> for fromaddr, toaddrs, message in stub.test_emails:
+    ...     print toaddrs
+    ['[e-mail]']
 
 Now if we edit the status, a notification will be sent to all the
 previous people, and to the approver and the added subscriber:
@@ -107,7 +113,6 @@
     ['[e-mail]']
     ['[e-mail]']
     ['[e-mail]']
-    ['[e-mail]']
 
 Now let's take a look at what the notification looks like:
 

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2011-03-16 08:41:17 +0000
+++ lib/lp/blueprints/model/specification.py	2011-05-23 20:02:28 +0000
@@ -52,6 +52,10 @@
 from canonical.launchpad.helpers import (
     get_contact_email_addresses,
     )
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 from lp.blueprints.adapters import SpecificationDelta
 from lp.blueprints.enums import (
     NewSpecificationDefinitionStatus,
@@ -182,7 +186,7 @@
     date_started = UtcDateTimeCol(notNull=False, default=None)
 
     # useful joins
-    subscriptions = SQLMultipleJoin('SpecificationSubscription',
+    _subscriptions = SQLMultipleJoin('SpecificationSubscription',
         joinColumn='specification', orderBy='id')
     subscribers = SQLRelatedJoin('Person',
         joinColumn='specification', otherColumn='person',
@@ -213,6 +217,12 @@
         otherColumn='specification', orderBy='title',
         intermediateTable='SpecificationDependency')
 
+    @cachedproperty
+    def subscriptions(self):
+        """Sort the subscriptions"""
+        return sorted(
+            self._subscriptions, key=lambda sub: sub.person.displayname)
+
     @property
     def target(self):
         """See ISpecification."""
@@ -545,6 +555,11 @@
         # since no previous subscription existed, create and return a new one
         sub = SpecificationSubscription(specification=self,
             person=person, essential=essential)
+        property_cache = get_property_cache(self)
+        if 'subscription' in property_cache:
+            property_cache.subscriptions.append(sub)
+            property_cache.subscriptions.sort(
+                key=lambda sub: sub.person.displayname)
         notify(ObjectCreatedEvent(sub, user=user))
         return sub
 
@@ -553,6 +568,7 @@
         # see if a relevant subscription exists, and if so, delete it
         for sub in self.subscriptions:
             if sub.person.id == person.id:
+                get_property_cache(self).subscriptions.remove(sub)
                 SpecificationSubscription.delete(sub.id)
                 return
 
@@ -694,7 +710,6 @@
             self.id, self.name, self.target.name)
 
 
-
 class HasSpecificationsMixin:
     """A mixin class that implements many of the common shortcut properties
     for other classes that have specifications.

=== modified file 'lib/lp/blueprints/stories/standalone/subscribing.txt'
--- lib/lp/blueprints/stories/standalone/subscribing.txt	2010-12-07 01:03:17 +0000
+++ lib/lp/blueprints/stories/standalone/subscribing.txt	2011-05-23 20:02:28 +0000
@@ -273,10 +273,9 @@
     ...     img = a_tags[0].first('img')
     ...     print img['src'],
     ...     print a_tags[1].string
+    /@@/subscriber-essential Andrew Bennetts
+    /@@/subscriber-inessential Dafydd Harries
     /@@/subscriber-inessential Foo Bar
     /@@/subscriber-essential Robert Collins
-    /@@/subscriber-essential Andrew Bennetts
     /@@/subscriber-inessential Stuart Bishop
-    /@@/subscriber-inessential Dafydd Harries
-