Merge lp:~kfogel/launchpad/505845-affects-person-from-dups into lp:launchpad

[Karl Fogel]

[Karl Fogel (kfogel)]

Fix bug #505845 by adding new properties Bug.users_affected_with_dupes and Bug.users_affected_count_with_dupes, so bug heat can do the correct calculations, and (eventually) so the web UI can show more accurate information.

Along the way, fix a long-standing bug in Bug.users_affected whereby it was counting users who had explicitly said "does not affect me" as being affected.

Note the one "XXX" still in the change.

[Graham Binns (gmb)]

Nice branch! Couple of nitpicks and a question about your XXX, but
otherwise nothing major. r=me with the fix for the XXX applied if
possible (or a promise that it will land shortly after this branch
does).

> === modified file 'lib/lp/bugs/doc/bug.txt'
> --- lib/lp/bugs/doc/bug.txt 2009-12-12 09:46:03 +0000
> +++ lib/lp/bugs/doc/bug.txt 2010-01-20 07:25:26 +0000
> @@ -1242,12 +1242,18 @@
> 0
> >>> test_bug.users_affected_count
> 3
> + >>> test_bug.markUserAffected(unaffected_user, affected=False)
> + >>> test_bug.isUserAffected(unaffected_user)
> + False
> + >>> test_bug.users_unaffected_count
> + 1
> + >>> test_bug.users_affected_count
> + 2
>
> We can also get the collection of users affected by a bug.
>
> >>> print '\n'.join(
> ... sorted(user.name for user in test_bug.users_affected))
> - blaze-bayley
> bruce-dickinson
> paul-dianno
>
> @@ -1255,6 +1261,144 @@
> >>> print list(unaffecting_bug.users_affected)
> [<Person at ...>]
>
> +Similarly, we can get the collection of users unaffected by a bug.
> +
> + >>> print '\n'.join(
> + ... sorted(user.name for user in test_bug.users_unaffected))
> + blaze-bayley
> +
> +If a user is marked as being affected by a bug (either by explicitly
> +marking it so, or by being the bug's owner), and then that bug is
> +marked as a duplicate of master bug, then the users_affected_count of
> +the master bug increases too.
> +
> + >>> dupe_affected_user = factory.makePerson(name='sheila-shakespeare')
> + >>> dupe_one = factory.makeBug(owner=dupe_affected_user)
> + >>> dupe_one.duplicateof = test_bug
> + >>> test_bug.users_affected_count_with_dupes
> + 3
> +
> +And the list of users the master bug affects includes that user.
> +
> + >>> print '\n'.join(
> + ... sorted(user.name for user in test_bug.users_affected_with_dupes))
> + bruce-dickinson
> + paul-dianno
> + sheila-shakespeare
> +
> +However, if the user was also marked as being affected by the master
> +bug, then the master bug's user_affected_count does *not* increment
> +just because she is also affected by the duplicate.
> +
> + >>> test_bug.markUserAffected(dupe_affected_user, affected=True)
> + >>> test_bug.users_affected_count_with_dupes
> + 3
> +
> +And the list of users that the master bug affects still includes the
> +user, of course.
> +
> + >>> print '\n'.join(
> + ... sorted(user.name for user in test_bug.users_affected_with_dupes))
> + bruce-dickinson
> + paul-dianno
> + sheila-shakespeare
> +
> +If there is another dup of the master bug, filed by someone else, the
> +master bug's affected count with dups increases.
> +
> + >>> dupe_affected_other_user = factory.makePerson(name='napoleon-bonaparte')
> + >>> dupe_three = factory.makeBug(owner=dupe_affected_other_user)
> + >>> dupe_three.duplicateof = test_bug
> + >>> test_bug.users_affected_count_with_dupes
> + 4
> +
> +If the user claims that two bugs both affect her, then if they are
> +both marked as duplicates of the master bugs, the master bug's
> +user_affected_count still only increments by 1 for that user.
> +
...

review: Approve (code)

[Karl Fogel (kfogel)]

Fixed typos, and took care of "XXX" comment with a surprisingly simple solution (well, surprising to me anyway).

review: Resubmit

[Graham Binns (gmb)]

Looks awesome.

review: Approve (code)

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2010-01-20 20:19:05 +0000
+++ lib/lp/bugs/configure.zcml	2010-01-22 23:54:16 +0000
@@ -629,6 +629,9 @@
                     users_unaffected_count
                     readonly_duplicateof
                     users_affected
+                    users_unaffected
+                    users_affected_count_with_dupes
+                    users_affected_with_dupes
                     bug_messages
                     isUserAffected
                     getHWSubmissions"/>

=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt	2009-12-12 09:46:03 +0000
+++ lib/lp/bugs/doc/bug.txt	2010-01-22 23:54:16 +0000
@@ -1242,12 +1242,18 @@
     0
     >>> test_bug.users_affected_count
     3
+    >>> test_bug.markUserAffected(unaffected_user, affected=False)
+    >>> test_bug.isUserAffected(unaffected_user)
+    False
+    >>> test_bug.users_unaffected_count
+    1
+    >>> test_bug.users_affected_count
+    2
 
 We can also get the collection of users affected by a bug.
 
     >>> print '\n'.join(
     ...     sorted(user.name for user in test_bug.users_affected))
-    blaze-bayley
     bruce-dickinson
     paul-dianno
 
@@ -1255,6 +1261,144 @@
     >>> print list(unaffecting_bug.users_affected)
     [<Person at ...>]
 
+Similarly, we can get the collection of users unaffected by a bug.
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in test_bug.users_unaffected))
+    blaze-bayley
+
+If a user is marked as being affected by a bug (either by explicitly
+marking it so, or by being the bug's owner), and then that bug is
+marked as a duplicate of master bug, then the users_affected_count of
+the master bug increases too.
+
+    >>> dupe_affected_user = factory.makePerson(name='sheila-shakespeare')
+    >>> dupe_one = factory.makeBug(owner=dupe_affected_user)
+    >>> dupe_one.duplicateof = test_bug
+    >>> test_bug.users_affected_count_with_dupes
+    3
+
+And the list of users the master bug affects includes that user.
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in test_bug.users_affected_with_dupes))
+    bruce-dickinson
+    paul-dianno
+    sheila-shakespeare
+
+However, if the user was also marked as being affected by the master
+bug, then the master bug's user_affected_count does *not* increment
+just because she is also affected by the duplicate.
+
+    >>> test_bug.markUserAffected(dupe_affected_user, affected=True)
+    >>> test_bug.users_affected_count_with_dupes
+    3
+
+And the list of users that the master bug affects still includes the
+user, of course.
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in test_bug.users_affected_with_dupes))
+    bruce-dickinson
+    paul-dianno
+    sheila-shakespeare
+
+If there is another dup of the master bug, filed by someone else, the
+master bug's affected count with dups increases.
+
+    >>> dupe_affected_other_user = factory.makePerson(name='napoleon-bonaparte')
+    >>> dupe_three = factory.makeBug(owner=dupe_affected_other_user)
+    >>> dupe_three.duplicateof = test_bug
+    >>> test_bug.users_affected_count_with_dupes
+    4
+
+If the user claims that two bugs both affect her, then if they are
+both marked as duplicates of the master bugs, the master bug's
+user_affected_count still only increments by 1 for that user.
+
+    >>> dupe_two = factory.makeBug(owner=dupe_affected_user)
+    >>> dupe_two.duplicateof = test_bug
+    >>> test_bug.users_affected_count_with_dupes
+    4
+
+Both duplicates claim to affect just that user:
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in dupe_one.users_affected))
+    sheila-shakespeare
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in dupe_two.users_affected))
+    sheila-shakespeare
+
+And the list of users that the master bug affects includes the user
+exactly once, of course.
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in test_bug.users_affected_with_dupes))
+    bruce-dickinson
+    napoleon-bonaparte
+    paul-dianno
+    sheila-shakespeare
+
+If the user marks the master bug as not affecting her, but the master
+bug still has a duplicate that she claims affects her, then that
+duplicate is also marked as not affecting her either.
+
+    >>> dupe_one.users_affected_count
+    1
+    >>> test_bug.markUserAffected(dupe_affected_user, affected=False)
+    >>> dupe_one.users_affected_count
+    0
+
+The master bug's affected count, with or without dups, is reduced by one:
+
+    >>> test_bug.users_affected_count
+    2
+    >>> test_bug.users_affected_count_with_dupes
+    3
+
+The dup user no longer appears as affected by the master bug nor
+either of the dups:
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in test_bug.users_affected_with_dupes))
+    bruce-dickinson
+    napoleon-bonaparte
+    paul-dianno
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in dupe_one.users_affected))
+    <BLANKLINE>
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in dupe_two.users_affected))
+    <BLANKLINE>
+
+Since the user who filed the first two dups had an entry explicitly
+saying she was affected, they now claim that she is unaffected.
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in dupe_one.users_unaffected))
+    sheila-shakespeare
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in dupe_two.users_unaffected))
+    sheila-shakespeare
+
+But she didn't file the third dup, so there was never any explicit
+record saying she was affected by it.  Thus she also does not appear
+as explicitly unaffected, even after marking the master bug as not
+affecting her.
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in dupe_three.users_unaffected))
+    <BLANKLINE>
+
+However, if a dup was not marked either way for that user, then do
+nothing to the dup when the master is marked as not affecting the
+user.
+
+    >>> print '\n'.join(
+    ...     sorted(user.name for user in dupe_three.users_affected))
+    napoleon-bonaparte
+
 
 Getting the distinct set of Bugs for a set of BugTasks
 ------------------------------------------------------

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2010-01-20 20:19:05 +0000
+++ lib/lp/bugs/interfaces/bug.py	2010-01-22 23:54:16 +0000
@@ -286,13 +286,33 @@
         title=_('The number of comments on this bug'),
         required=True, readonly=True)
     users_affected_count = exported(
-        Int(title=_('The number of users affected by this bug'),
+        Int(title=_('The number of users affected by this bug '
+                    '(not including duplicates)'),
             required=True, readonly=True))
     users_unaffected_count = exported(
+        # We don't say "(not including duplicates)" here because
+        # affected and unaffected are asymmetrical that way.  If a dup
+        # affects you, then the master bug affects you; but if a dup
+        # *doesn't* affect you, the master bug may or may not affect
+        # you, since a dup is often a specific symptom of a more
+        # general master bug.
         Int(title=_('The number of users unaffected by this bug'),
             required=True, readonly=True))
     users_affected = exported(CollectionField(
-            title=_('Users affected'),
+            title=_('Users affected (not including duplicates)'),
+            value_type=Reference(schema=IPerson),
+            readonly=True))
+    users_unaffected = exported(CollectionField(
+            title=_('Users explicitly marked as unaffected '
+                    '(not including duplicates)'),
+            value_type=Reference(schema=IPerson),
+            readonly=True))
+    users_affected_count_with_dupes = exported(
+      Int(title=_('The number of users affected by this bug '
+                  '(including duplicates)'),
+          required=True, readonly=True))
+    users_affected_with_dupes = exported(CollectionField(
+            title=_('Users affected (including duplicates)'),
             value_type=Reference(schema=IPerson),
             readonly=True))
 

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-01-22 17:31:00 +0000
+++ lib/lp/bugs/model/bug.py	2010-01-22 23:54:16 +0000
@@ -31,7 +31,8 @@
 from sqlobject import BoolCol, IntCol, ForeignKey, StringCol
 from sqlobject import SQLMultipleJoin, SQLRelatedJoin
 from sqlobject import SQLObjectNotFound
-from storm.expr import And, Count, In, LeftJoin, Select, SQLRaw, Func
+from storm.expr import (
+    And, Count, Func, In, LeftJoin, Not, Select, SQLRaw, Union)
 from storm.store import EmptyResultSet, Store
 
 from lazr.lifecycle.event import (
@@ -265,7 +266,46 @@
         """See `IBug`."""
         return Store.of(self).find(
             Person, BugAffectsPerson.person == Person.id,
-            BugAffectsPerson.bug == self)
+            BugAffectsPerson.affected,
+            BugAffectsPerson.bug == self)
+
+    @property
+    def users_unaffected(self):
+        """See `IBug`."""
+        return Store.of(self).find(
+            Person, BugAffectsPerson.person == Person.id,
+            Not(BugAffectsPerson.affected),
+            BugAffectsPerson.bug == self)
+
+    @property
+    def user_ids_affected_with_dupes(self):
+        """Return all IDs of Persons affected by this bug and its dupes.
+        The return value is a Storm expression.  Running a query with
+        this expression returns a result that may contain the same ID
+        multiple times, for example if that person is affected via
+        more than one duplicate."""
+        return Union(
+            Select(Person.id,
+                   And(BugAffectsPerson.person == Person.id,
+                       BugAffectsPerson.affected,
+                       BugAffectsPerson.bug == self)),
+            Select(Person.id,
+                   And(BugAffectsPerson.person == Person.id,
+                       BugAffectsPerson.bug == Bug.id,
+                       BugAffectsPerson.affected,
+                       Bug.duplicateof == self.id)))
+
+    @property
+    def users_affected_with_dupes(self):
+        """See `IBug`."""
+        return Store.of(self).find(
+            Person,
+            In(Person.id, self.user_ids_affected_with_dupes))
+
+    @property
+    def users_affected_count_with_dupes(self):
+        """See `IBug`."""
+        return self.users_affected_with_dupes.count()
 
     @property
     def indexed_messages(self):
@@ -1348,6 +1388,10 @@
             if bap.affected != affected:
                 bap.affected = affected
                 self._flushAndInvalidate()
+        # Loop over dupes.
+        for dupe in self.duplicates:
+            if dupe._getAffectedUser(user) is not None:
+                dupe.markUserAffected(user, affected)
 
     @property
     def readonly_duplicateof(self):

=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt	2009-11-20 04:21:24 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt	2010-01-22 23:54:16 +0000
@@ -42,6 +42,9 @@
   title: u'Make Jokosher use autoaudiosink'
   users_affected_collection_link: u'http://.../bugs/11/users_affected'
   users_affected_count: 0
+  users_affected_count_with_dupes: 0
+  users_affected_with_dupes_collection_link: u'http://.../bugs/11/users_affected_with_dupes'
+  users_unaffected_collection_link: u'http://.../bugs/11/users_unaffected'
   users_unaffected_count: 0
   who_made_private_link: None