launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/double-bugdelta-js into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #992998 in Launchpad itself: "Changing information type via API results in double-notification"
  https://bugs.launchpad.net/launchpad/+bug/992998

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/double-bugdelta-js/+merge/104316

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.
-- 
https://code.launchpad.net/~stevenk/launchpad/double-bugdelta-js/+merge/104316
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/double-bugdelta-js into lp:launchpad.

=== 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 02:44:18 +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(
@@ -942,12 +941,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 +963,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 02:44:18 +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.")
@@ -1789,8 +1788,6 @@
         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):