Merge lp:~stevenk/launchpad/double-bugdelta-js into lp:launchpad

[Steve Kowalik]

Insist that IBug.transitionToInformationType() does not do notification.

Currently, if IBug.transitionToInformationType() is called over the API, two notification events are fired -- one from lazr.restful via lazr.lifecycle, and one from transitionToInformationType() itself. This impacts the IChoiceSource that is used if the show_information_type_in_ui feature flag is enabled, since then the bug activity log shows two entires for the same change.

As a result, I have moved the notify() call into the submit method on Bug:+secrecy, like we do for everything else.

This did not impact the legacy code, since that overlay makes a submit call to Bug:+secrecy and does not make use of the API to change the privacy or security related-ness of a bug.

[Ian Booth (wallyworld)]

Jolly good that you found this. Let's land the fucker and do more cleanup once legacy stuff is removed.

review: Approve

[Ian Booth (wallyworld)]

As discussed, please a a test before landing since the issue did not trigger any test failures.

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2012-04-23 09:28:45 +0000
+++ lib/lp/bugs/browser/bug.py	2012-05-02 23:19:19 +0000
@@ -854,8 +854,7 @@
         # We handle duplicate changes by hand instead of leaving it to
         # the usual machinery because we must use bug.markAsDuplicate().
         bug = self.context.bug
-        bug_before_modification = Snapshot(
-            bug, providing=providedBy(bug))
+        bug_before_modification = Snapshot(bug, providing=providedBy(bug))
         duplicateof = data.pop('duplicateof')
         bug.markAsDuplicate(duplicateof)
         notify(
@@ -864,6 +863,8 @@
         self.updateBugFromData(data)
 
 
+# XXX: This can move to using LaunchpadEditFormView when 
+# show_information_type_in_ui is removed.
 class BugSecrecyEditView(LaunchpadFormView, BugSubscriptionPortletDetails):
     """Form for marking a bug as a private/public."""
 
@@ -942,12 +943,19 @@
         """Update the bug."""
         data = dict(data)
         bug = self.context.bug
+        bug_before_modification = Snapshot(bug, providing=providedBy(bug))
         if bool(getFeatureFlag(
             'disclosure.show_information_type_in_ui.enabled')):
             information_type = data.pop('information_type')
+            changed_fields = ['information_type']
         else:
+            changed_fields = []
             private = data.pop('private', bug.private)
+            if bug.private != private:
+                changed_fields.append('private')
             security_related = data.pop('security_related')
+            if bug.security_related != security_related:
+                changed_fields.append('security_related')
             information_type = convert_to_information_type(
                 private, security_related)
         user_will_be_subscribed = (
@@ -957,6 +965,10 @@
             information_type, self.user)
         if changed:
             self._handlePrivacyChanged(user_will_be_subscribed)
+            notify(
+                ObjectModifiedEvent(
+                    bug, bug_before_modification, changed_fields,
+                    user=self.user))
         if self.request.is_ajax:
             if changed:
                 return self._getSubscriptionDetails()

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-04-25 21:55:08 +0000
+++ lib/lp/bugs/model/bug.py	2012-05-02 23:19:19 +0000
@@ -1726,7 +1726,6 @@
     def transitionToInformationType(self, information_type, who,
                                     from_api=False):
         """See `IBug`."""
-        bug_before_modification = Snapshot(self, providing=providedBy(self))
         if from_api and information_type == InformationType.PROPRIETARY:
             raise BugCannotBePrivate(
                 "Cannot transition the information type to proprietary.")
@@ -1749,7 +1748,6 @@
         for attachment in self.attachments_unpopulated:
             attachment.libraryfile.restricted = (
                 information_type in PRIVATE_INFORMATION_TYPES)
-        self.updateHeat()
 
         # There are several people we need to ensure are subscribed.
         # If the information type is userdata, we need to check for bug
@@ -1785,12 +1783,11 @@
                 self.subscribe(s, who)
 
         self.information_type = information_type
+        self.updateHeat()
         # Set the legacy attributes for now.
         self._private = information_type in PRIVATE_INFORMATION_TYPES
         self._security_related = (
             information_type in SECURITY_INFORMATION_TYPES)
-        notify(ObjectModifiedEvent(
-                self, bug_before_modification, ['information_type'], user=who))
         return True
 
     def getRequiredSubscribers(self, information_type, who):

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2012-04-29 22:54:49 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2012-05-02 23:19:19 +0000
@@ -813,25 +813,6 @@
             )
         [self.assertEqual(m[1], m[0].information_type) for m in mapping]
 
-    def test_information_type_modified_event(self):
-        # When a bug's information_type is changed, the expected object
-        # modified event is published.
-        self.event_edited_fields = []
-        self.event_object = None
-
-        def event_callback(object, event):
-            self.event_edited_fields = event.edited_fields
-            self.event_object = event.object
-
-        TestEventListener(IBug, IObjectModifiedEvent, event_callback)
-        owner = self.factory.makePerson()
-        bug = self.factory.makeBug(
-            private=True, security_related=True, owner=owner)
-        with person_logged_in(owner):
-            bug.transitionToInformationType(InformationType.PUBLIC, owner)
-        self.assertEqual(['information_type'], self.event_edited_fields)
-        self.assertEqual(bug, self.event_object)
-
     def test_private_to_public_information_type(self):
         # A private bug transitioning to public has the correct information
         # type.

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2012-05-02 12:45:53 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2012-05-02 23:19:19 +0000
@@ -30,7 +30,9 @@
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
+    api_url,
     celebrity_logged_in,
+    launchpadlib_for,
     login_person,
     person_logged_in,
     TestCaseWithFactory,
@@ -561,7 +563,11 @@
     def test_make_private(self):
         # Marking a bug as private adds items to the bug's activity log
         # and notifications.
+        bug_before_modification = Snapshot(
+            self.bug, providing=providedBy(self.bug))
         self.bug.setPrivate(True, self.user)
+        notify(ObjectModifiedEvent(
+            self.bug, bug_before_modification, ['private'], user=self.user))
 
         visibility_change_activity = {
             'person': self.user,
@@ -586,7 +592,12 @@
         self.saveOldChanges(private_bug)
         self.assertTrue(private_bug.private)
 
+        bug_before_modification = Snapshot(
+            private_bug, providing=providedBy(private_bug))
         private_bug.setPrivate(False, self.user)
+        notify(ObjectModifiedEvent(
+            private_bug, bug_before_modification, ['private'],
+            user=self.user))
 
         visibility_change_activity = {
             'person': self.user,
@@ -612,9 +623,13 @@
         self.saveOldChanges(bug=bug)
         feature_flag = {
             'disclosure.show_information_type_in_ui.enabled': 'on'}
+        bug_before_modification = Snapshot(bug, providing=providedBy(bug))
         with FeatureFixture(feature_flag):
             bug.transitionToInformationType(
                 InformationType.EMBARGOEDSECURITY, self.user)
+            notify(ObjectModifiedEvent(
+                bug, bug_before_modification, ['information_type'],
+                user=self.user))
 
         information_type_change_activity = {
             'person': self.user,
@@ -641,9 +656,13 @@
         feature_flags = {
             'disclosure.show_information_type_in_ui.enabled': 'on',
             'disclosure.display_userdata_as_private.enabled': 'on'}
+        bug_before_modification = Snapshot(bug, providing=providedBy(bug))
         with FeatureFixture(feature_flags):
             bug.transitionToInformationType(
                 InformationType.USERDATA, self.user)
+            notify(ObjectModifiedEvent(
+                bug, bug_before_modification, ['information_type'],
+                user=self.user))
 
         information_type_change_activity = {
             'person': self.user,
@@ -660,6 +679,37 @@
             expected_notification=information_type_change_notification,
             bug=bug)
 
+    def test_change_information_type_using_api(self):
+        # Changing the information type of a bug adds items to the activity
+        # log and notifications.
+        person = self.factory.makePerson()
+        bug = self.factory.makeBug(owner=person)
+        self.saveOldChanges(bug=bug)
+        feature_flag = {
+            'disclosure.show_information_type_in_ui.enabled': 'on'}
+        webservice = launchpadlib_for('test', person)
+        lp_bug = webservice.load(api_url(bug))
+        with FeatureFixture(feature_flag):
+            lp_bug.transitionToInformationType(
+                information_type='Embargoed Security')
+
+        information_type_change_activity = {
+            'person': person,
+            'whatchanged': 'information type',
+            'oldvalue': 'Public',
+            'newvalue': 'Embargoed Security',
+            }
+        information_type_change_notification = {
+            'text': '** Information type changed from Public to Embargoed '
+                'Security',
+            'person': person,
+            }
+        with person_logged_in(person):
+            self.assertRecordedChange(
+                expected_activity=information_type_change_activity,
+                expected_notification=information_type_change_notification,
+                bug=bug)
+
     def test_tags_added(self):
         # Adding tags to a bug will add BugActivity and BugNotification
         # entries.

=== modified file 'lib/lp/bugs/tests/test_bugtaskflat_triggers.py'
--- lib/lp/bugs/tests/test_bugtaskflat_triggers.py	2012-04-03 06:14:09 +0000
+++ lib/lp/bugs/tests/test_bugtaskflat_triggers.py	2012-05-02 23:19:19 +0000
@@ -316,8 +316,8 @@
         task = self.makeLoggedInTask(private=True)
         with self.bugtaskflat_is_updated(
             task, [
-                'information_type', 'date_last_updated', 'heat',
-                'access_policies', 'access_grants']):
+                'information_type', 'heat', 'access_policies',
+                'access_grants']):
             task.bug.setPrivate(False, task.owner)
 
     def test_bug_change_unflattened(self):