Merge lp:~danilo/launchpad/remove-primary-duplicate-reason into lp:launchpad

[Данило Шеган]

[r=adeuring][bug=761596] Stop treating bug duplication as symmetric for the purposes of bug subscription descriptions. It's not.

= Remove symmetric handling of duplicates =

Atm, the code assumes that bug duplication is a symmetric process as far as email notifications go. Since that's not the case (I double-checked that), we need to stop treating it as such.

== Implementation details ==

Remove the bug join condition that fetches the primary bug for a duplicate. The condition list was Or()d together.

The test has been updated to confirm this is the case.

Lint fixes in a separate commit, getting pushed after review :)

== Tests ==

bin/test -cvvt PersonSubscriptionInfo.*duplicate

== Demo and Q/A ==

  1. Turn on 'malone.advanced-structural-subscriptions.enabled' feature flag
  2. Mark a bug https://bugs.launchpad.dev/firefox/+bug/6 as duplicate of https://bugs.launchpad.dev/firefox/+bug/5
  3. Subscribe to bug 5
  4. Make sure the https://bugs.launchpad.dev/firefox/+bug/6/+subscriptions doesn't say how bug 5 is a duplicate of bug 6

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/personsubscriptioninfo.py
  lib/lp/bugs/model/tests/test_personsubscriptioninfo.py

./lib/lp/bugs/model/personsubscriptioninfo.py
     119: E501 line too long (80 characters)
     257: E301 expected 1 blank line, found 0
     267: E301 expected 1 blank line, found 0
     274: E301 expected 1 blank line, found 0
     305: E202 whitespace before ')'
     313: E202 whitespace before ')'
     119: Line exceeds 78 characters.
./lib/lp/bugs/model/tests/test_personsubscriptioninfo.py
      12: 'searchbuilder' imported but unused
      29: 'anonymous_logged_in' imported but unused
       8: 'Store' imported but unused
      16: 'BugTaskStatus' imported but unused
      10: 'ProxyFactory' imported but unused
       9: 'Unauthorized' imported but unused
      16: 'BugTaskImportance' imported but unused
      29: 'login_person' imported but unused
      26: 'BugSubscriptionFilter' imported but unused
      13: 'IStore' imported but unused
      37: E302 expected 2 blank lines, found 1

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/lp/bugs/model/personsubscriptioninfo.py'
--- lib/lp/bugs/model/personsubscriptioninfo.py	2011-04-13 10:01:27 +0000
+++ lib/lp/bugs/model/personsubscriptioninfo.py	2011-04-15 10:49:53 +0000
@@ -116,7 +116,8 @@
             person, administrated_teams)
         self._principal_bug_to_infos = {}
 
-    def _add_item_to_collection(self, collection, principal, bug, subscription):
+    def _add_item_to_collection(self, collection, principal,
+                                bug, subscription):
         info = RealSubscriptionInfo(principal, bug, subscription)
         key = (principal, bug)
         infos = self._principal_bug_to_infos.get(key)
@@ -180,8 +181,6 @@
         # membership) in a single query.
         store = Store.of(person)
         bug_id_options = [Bug.id == bug.id, Bug.duplicateofID == bug.id]
-        if bug.duplicateof is not None:
-            bug_id_options.append(Bug.id == bug.duplicateof.id)
         info = store.find(
             (BugSubscription, Bug, Person),
             BugSubscription.bug == Bug.id,
@@ -256,6 +255,7 @@
     def getDataForClient(self):
         reference_map = {}
         dest = {}
+
         def get_id(obj):
             "Get an id for the object so it can be shared."
             # We could leverage .id for most objects, but not pillars.
@@ -266,6 +266,7 @@
                 reference_map[obj] = identifier
                 dest[identifier] = obj
             return identifier
+
         def virtual_sub_data(info):
             return {
                 'principal': get_id(info.principal),
@@ -273,6 +274,7 @@
                 'pillar': get_id(info.pillar),
                 # We won't add bugtasks yet unless we need them.
                 }
+
         def real_sub_data(info):
             return {
                 'principal': get_id(info.principal),
@@ -302,18 +304,18 @@
             }
         for category, collection in ((as_owner, self.as_owner),
                                  (as_assignee, self.as_assignee)):
-            for name, inner in (('personal', collection.personal),
-                                ('as_team_admin', collection.as_team_admin),
-                                ('as_team_member', collection.as_team_member)
-                                ):
+            for name, inner in (
+                ('personal', collection.personal),
+                ('as_team_admin', collection.as_team_admin),
+                ('as_team_member', collection.as_team_member)):
                 category[name] = [virtual_sub_data(info) for info in inner]
             category['count'] = collection.count
         for category, collection in ((direct, self.direct),
                                      (from_duplicate, self.from_duplicate)):
-            for name, inner in (('personal', collection.personal),
-                                ('as_team_admin', collection.as_team_admin),
-                                ('as_team_member', collection.as_team_member)
-                                ):
+            for name, inner in (
+                ('personal', collection.personal),
+                ('as_team_admin', collection.as_team_admin),
+                ('as_team_member', collection.as_team_member)):
                 category[name] = [real_sub_data(info) for info in inner]
             category['count'] = collection.count
         return subscription_data, dest
 
=== modified file 'lib/lp/bugs/model/tests/test_personsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_personsubscriptioninfo.py	2011-04-13 10:01:27 +0000
+++ lib/lp/bugs/model/tests/test_personsubscriptioninfo.py	2011-04-15 10:49:53 +0000
@@ -5,35 +5,25 @@
 
 __metaclass__ = type
 
-from storm.store import Store
-from zope.security.interfaces import Unauthorized
-from zope.security.proxy import ProxyFactory, removeSecurityProxy
+from zope.security.proxy import removeSecurityProxy
 
-from canonical.launchpad import searchbuilder
-from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.testing import DatabaseFunctionalLayer
 from lp.bugs.enum import BugNotificationLevel
-from lp.bugs.interfaces.bugtask import (
-    BugTaskImportance,
-    BugTaskStatus,
-    )
 from lp.bugs.interfaces.personsubscriptioninfo import (
     IRealSubscriptionInfo,
     IRealSubscriptionInfoCollection,
     IVirtualSubscriptionInfo,
     IVirtualSubscriptionInfoCollection,
     )
-from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
 from lp.bugs.model.personsubscriptioninfo import PersonSubscriptions
 from lp.registry.interfaces.teammembership import TeamMembershipStatus
 from lp.testing import (
-    anonymous_logged_in,
-    login_person,
     person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.matchers import Provides
 
+
 class TestPersonSubscriptionInfo(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -312,20 +302,18 @@
 
     def test_duplicate_direct_reverse(self):
         # Subscribed directly to the primary bug, and a duplicate bug changes.
-        duplicate = self.factory.makeBug()
+        primary = self.factory.makeBug()
         with person_logged_in(self.subscriber):
-            self.bug.markAsDuplicate(duplicate)
-            duplicate.subscribe(self.subscriber, self.subscriber)
+            self.bug.markAsDuplicate(primary)
+            primary.subscribe(self.subscriber, self.subscriber)
         # Load a `PersonSubscriptionInfo`s for subscriber and a bug.
         self.subscriptions.reload()
 
-        self.assertCollectionsAreEmpty(except_='from_duplicate')
+        # This means no subscriptions on the duplicate bug.
+        self.assertCollectionsAreEmpty()
         self.failIf(self.subscriptions.muted)
         self.assertCollectionContents(
-            self.subscriptions.from_duplicate, personal=1)
-        self.assertRealSubscriptionInfoMatches(
-            self.subscriptions.from_duplicate.personal[0],
-            duplicate, self.subscriber, False, [], [])
+            self.subscriptions.from_duplicate, personal=0)
 
     def test_duplicate_multiple(self):
         # Subscribed directly to more than one duplicate bug.