Merge lp:~stevenk/launchpad/subscription-no-longer-updates into lp:launchpad

[Steve Kowalik]

Do not update IBug.date_last_updated in the subscriber method if no fields on the bug changed.

When you call IBug.subscribe(), the date_last_updated property was not updated, by design. The API broke this rule, calling bug.subscribe(person=launchpad.me) over the API would create the subscription and then update date_last_updated. The API machinery is the cause -- it will call notify(ObjectModifiedEvent(bug)) since it assumes that the object has changed since you just called a method on it.

I am now checking the details of the event in the Zope subscriber for ObjectModifiedEvent, and have added two API tests. I also took the opportunity to shorten update_bug_date_last_updated().

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/bugs/subscribers/buglastupdated.py'
--- lib/lp/bugs/subscribers/buglastupdated.py	2012-03-08 11:51:36 +0000
+++ lib/lp/bugs/subscribers/buglastupdated.py	2012-09-10 08:09:21 +0000
@@ -5,8 +5,9 @@
 
 __metaclass__ = type
 
-import datetime
+from datetime import datetime
 
+from lazr.lifecycle.interfaces import IObjectModifiedEvent
 import pytz
 from zope.security.proxy import removeSecurityProxy
 
@@ -16,17 +17,16 @@
 
 def update_bug_date_last_updated(object, event):
     """Update IBug.date_last_updated to the current date."""
+    # If no fields on the bug have changed, do nothing.
+    if IObjectModifiedEvent.providedBy(event) and not event.edited_fields:
+        return
     if IBug.providedBy(object):
-        current_bug = object
+        bug = object
     elif IHasBug.providedBy(object):
-        current_bug = object.bug
+        bug = object.bug
     else:
         raise AssertionError(
             "Unable to retrieve current bug to update 'date last updated'. "
             "Event handler expects object implementing IBug or IHasBug. "
             "Got: %s" % repr(object))
-
-    UTC = pytz.timezone('UTC')
-    now = datetime.datetime.now(UTC)
-
-    removeSecurityProxy(current_bug).date_last_updated = now
+    removeSecurityProxy(bug).date_last_updated = datetime.now(pytz.UTC)

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2012-09-04 04:27:53 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2012-09-10 08:09:21 +0000
@@ -5,6 +5,10 @@
 
 __metaclass__ = type
 
+from datetime import (
+    datetime,
+    timedelta,
+    )
 import re
 
 from BeautifulSoup import BeautifulSoup
@@ -13,6 +17,7 @@
     BadRequest,
     HTTPError,
     )
+import pytz
 from simplejson import dumps
 from storm.store import Store
 from testtools.matchers import (
@@ -20,6 +25,7 @@
     LessThan,
     )
 from zope.component import getMultiAdapter
+from zope.security.proxy import removeSecurityProxy
 
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
 from lp.bugs.interfaces.bug import IBug
@@ -29,6 +35,7 @@
     )
 from lp.registry.interfaces.product import License
 from lp.services.webapp import snapshot
+from lp.services.webapp.interfaces import OAuthPermission
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
     api_url,
@@ -45,7 +52,10 @@
     LaunchpadFunctionalLayer,
     )
 from lp.testing.matchers import HasQueryCount
-from lp.testing.pages import LaunchpadWebServiceCaller
+from lp.testing.pages import (
+    LaunchpadWebServiceCaller,
+    webservice_for_person,
+    )
 from lp.testing.sampledata import (
     ADMIN_EMAIL,
     USER_EMAIL,
@@ -433,3 +443,43 @@
         bug = bugs_collection.createBug(
             target=api_url(project), title='title', description='desc')
         self.assertEqual('Proprietary', bug.information_type)
+
+
+class TestBugDateLastUpdated(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def make_old_bug(self):
+        bug = self.factory.makeBug()
+        one_year_ago = datetime.now(pytz.UTC) - timedelta(days=365)
+        removeSecurityProxy(bug).date_last_updated = one_year_ago
+        owner = bug.owner
+        with person_logged_in(owner):
+            webservice = webservice_for_person(
+                owner, permission=OAuthPermission.WRITE_PUBLIC)
+        return (bug, owner, webservice)
+
+    def test_subscribe_does_not_update(self):
+        # Calling subscribe over the API does not update date_last_updated.
+        (bug, owner, webservice)  = self.make_old_bug()
+        subscriber = self.factory.makePerson()
+        date_last_updated = bug.date_last_updated
+        api_sub = api_url(subscriber)
+        bug_url = api_url(bug)
+        logout()
+        response = webservice.named_post(bug_url, 'subscribe', person=api_sub)
+        self.assertEqual(200, response.status)
+        with person_logged_in(owner):
+            self.assertEqual(date_last_updated, bug.date_last_updated)
+
+    def test_change_status_does_update(self):
+        # Changing the status of a bugtask does change date_last_updated.
+        (bug, owner, webservice) = self.make_old_bug()
+        task_url = api_url(bug.default_bugtask)
+        date_last_updated = bug.date_last_updated
+        logout()
+        response = webservice.patch(
+            task_url, 'application/json', dumps(dict(status='Invalid')))
+        self.assertEqual(209, response.status)
+        with person_logged_in(owner):
+            self.assertNotEqual(date_last_updated, bug.date_last_updated)