Merge lp:~mbp/launchpad/678090-affected-count into lp:launchpad

[Martin Pool]

[r=mbp][bug=678090] bug page shows count of all affected people across dupes

This changes the "affects N people" on a bug page to show the total people affected across all bugs.

It does this by actually counting them, which may or may not be a performance problem. I have checked there is only one query, and actually by rearranging the code and using a cachedproperty it now does fewer queries than previously.

Robert suggested just adding up the cached affected-user-count across all duplicates, but that's incorrect in a way that's likely to generate knock-on bugs, and it wasn't obvious how to implement it.

[Martin Pool (mbp)]

This branch is currently incomplete but I'd still appreciate pre-review on the general approach. Specifically before it can land:

 * it needs the javascript version updated
 * perhaps the js and server-side forms can be unified using Moustache?
 * it needs tests

[Robert Collins (lifeless)]

This looks wrong:
38 + if total_affected == 1:
39 + return "This bug affects you and 1 other person"

surely thats 'This bug affects you', and a count of 2 is you and 1 other person, and 3+ is you and %d other people.

I think the approach is reasonable but you should consider feature flagging it in case it is slower. Also changing templates etc would just messy up the clarity of this change.

[Martin Pool (mbp)]

On 3 November 2011 15:59, Robert Collins <email address hidden> wrote:
> This looks wrong:
> 38      + if total_affected == 1:
> 39      + return "This bug affects you and 1 other person"
>
> surely thats 'This bug affects you', and a count of 2 is you and 1 other person, and 3+ is you and %d other people.

thanks.

>
> I think the approach is reasonable but you should consider feature flagging it in case it is slower.

good idea, and that will actually give good assurance that it's no
slower when the feature is off.

> Also changing templates etc would just messy up the clarity of this change.

k

[Martin Pool (mbp)]

This adds a feature flag checked on the view. It will cost us one more query, so the query count tests are pushed up, but only once per view.

Also some decent tests.

It turns out if I override the view properly this does not need js changes, which is nice.

[Martin Pool (mbp)]

I've added a flag, more tests, and made it more correct in cases like the current user being affected by a dupe. Please re-review.

[Martin Pool (mbp)]

<wallyworld_> poolie: looks ok to me, my misreading of the fflag aside. my personal preference is not to set the flag if the test is expecting the default behaviour

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-11-11 18:36:39 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-11-14 05:10:29 +0000
@@ -3626,59 +3626,76 @@
         else:
             return 'false'
 
-    @property
+    @cachedproperty
     def other_users_affected_count(self):
-        """The number of other users affected by this bug."""
-        if self.current_user_affected_status:
-            return self.context.users_affected_count - 1
+        """The number of other users affected by this bug.
+        """
+        if getFeatureFlag('bugs.affected_count_includes_dupes.disabled'):
+            if self.current_user_affected_status:
+                return self.context.users_affected_count - 1
+            else:
+                return self.context.users_affected_count
         else:
+            return self.context.other_users_affected_count_with_dupes
+
+    @cachedproperty
+    def total_users_affected_count(self):
+        """The number of affected users, typically across all users.
+
+        Counting across duplicates may be disabled at run time.
+        """
+        if getFeatureFlag('bugs.affected_count_includes_dupes.disabled'):
             return self.context.users_affected_count
+        else:
+            return self.context.users_affected_count_with_dupes
 
-    @property
+    @cachedproperty
     def affected_statement(self):
         """The default "this bug affects" statement to show.
 
         The outputs of this method should be mirrored in
         MeTooChoiceSource._getSourceNames() (Javascript).
         """
-        if self.other_users_affected_count == 1:
-            if self.current_user_affected_status is None:
+        me_affected = self.current_user_affected_status
+        other_affected = self.other_users_affected_count
+        if me_affected is None:
+            if other_affected == 1:
                 return "This bug affects 1 person. Does this bug affect you?"
-            elif self.current_user_affected_status:
-                return "This bug affects you and 1 other person"
-            else:
-                return "This bug affects 1 person, but not you"
-        elif self.other_users_affected_count > 1:
-            if self.current_user_affected_status is None:
+            elif other_affected > 1:
                 return (
                     "This bug affects %d people. Does this bug "
-                    "affect you?" % (self.other_users_affected_count))
-            elif self.current_user_affected_status:
-                return "This bug affects you and %d other people" % (
-                    self.other_users_affected_count)
+                    "affect you?" % (other_affected))
             else:
-                return "This bug affects %d people, but not you" % (
-                    self.other_users_affected_count)
-        else:
-            if self.current_user_affected_status is None:
                 return "Does this bug affect you?"
-            elif self.current_user_affected_status:
+        elif me_affected is True:
+            if other_affected == 0:
                 return "This bug affects you"
+            elif other_affected == 1:
+                return "This bug affects you and 1 other person"
             else:
+                return "This bug affects you and %d other people" % (
+                    other_affected)
+        else:
+            if other_affected == 0:
                 return "This bug doesn't affect you"
+            elif other_affected == 1:
+                return "This bug affects 1 person, but not you"
+            elif other_affected > 1:
+                return "This bug affects %d people, but not you" % (
+                    other_affected)
 
-    @property
+    @cachedproperty
     def anon_affected_statement(self):
         """The "this bug affects" statement to show to anonymous users.
 
         The outputs of this method should be mirrored in
         MeTooChoiceSource._getSourceNames() (Javascript).
         """
-        if self.context.users_affected_count == 1:
+        affected = self.total_users_affected_count
+        if affected == 1:
             return "This bug affects 1 person"
-        elif self.context.users_affected_count > 1:
-            return "This bug affects %d people" % (
-                self.context.users_affected_count)
+        elif affected > 1:
+            return "This bug affects %d people" % affected
         else:
             return None
 

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-11-11 18:36:39 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-11-14 05:10:29 +0000
@@ -122,7 +122,7 @@
         self.getUserBrowser(url, person_no_teams)
         # This may seem large: it is; there is easily another 30% fat in
         # there.
-        self.assertThat(recorder, HasQueryCount(LessThan(84)))
+        self.assertThat(recorder, HasQueryCount(LessThan(85)))
         count_with_no_teams = recorder.count
         # count with many teams
         self.invalidate_caches(task)
@@ -138,7 +138,7 @@
     def test_rendered_query_counts_constant_with_attachments(self):
         with celebrity_logged_in('admin'):
             browses_under_limit = BrowsesWithQueryLimit(
-                86, self.factory.makePerson())
+                88, self.factory.makePerson())
 
             # First test with a single attachment.
             task = self.factory.makeBugTask()
@@ -192,7 +192,7 @@
         # Ideally this should be much fewer, but this tries to keep a win of
         # removing more than half of these.
         self.assertThat(recorder, HasQueryCount(
-            LessThan(count_with_no_branches + 45),
+            LessThan(count_with_no_branches + 46),
             ))
 
     def test_interesting_activity(self):
@@ -335,19 +335,122 @@
             1, self.view.other_users_affected_count)
         other_user_1 = self.factory.makePerson()
         self.bug.markUserAffected(other_user_1, True)
+        self.refresh()
         self.failUnlessEqual(
             2, self.view.other_users_affected_count)
         other_user_2 = self.factory.makePerson()
         self.bug.markUserAffected(other_user_2, True)
+        self.refresh()
         self.failUnlessEqual(
             3, self.view.other_users_affected_count)
         self.bug.markUserAffected(other_user_1, False)
-        self.failUnlessEqual(
-            2, self.view.other_users_affected_count)
-        self.bug.markUserAffected(self.view.user, True)
-        self.refresh()
-        self.failUnlessEqual(
-            2, self.view.other_users_affected_count)
+        self.refresh()
+        self.failUnlessEqual(
+            2, self.view.other_users_affected_count)
+        self.bug.markUserAffected(self.view.user, True)
+        self.refresh()
+        self.failUnlessEqual(
+            2, self.view.other_users_affected_count)
+
+    def makeDuplicate(self):
+        user2 = self.factory.makePerson()
+        self.bug2 = self.factory.makeBug()
+        self.bug2.markUserAffected(user2, True)
+        self.assertEqual(
+            2, self.bug2.users_affected_count)
+        self.bug2.markAsDuplicate(self.bug)
+        # After this there are three users already affected: the creators of
+        # the two bugs, plus user2.  The current user is not yet affected by
+        # any of them.
+
+    def test_counts_user_unaffected(self):
+        self.useFixture(FeatureFixture(
+            {'bugs.affected_count_includes_dupes.disabled': ''}))
+        self.makeDuplicate()
+        self.assertEqual(
+            3, self.view.total_users_affected_count)
+        self.assertEqual(
+            "This bug affects 3 people. Does this bug affect you?",
+            self.view.affected_statement)
+        self.assertEqual(
+            "This bug affects 3 people",
+            self.view.anon_affected_statement)
+        self.assertEqual(
+            self.view.other_users_affected_count,
+            3)
+
+    def test_counts_affected_by_duplicate(self):
+        self.useFixture(FeatureFixture(
+            {'bugs.affected_count_includes_dupes.disabled': ''}))
+        self.makeDuplicate()
+        # Now with you affected by the duplicate, but not the master.
+        self.bug2.markUserAffected(self.view.user, True)
+        self.refresh()
+        self.assertEqual(
+            "This bug affects 3 people. Does this bug affect you?",
+            self.view.affected_statement)
+        self.assertEqual(
+            "This bug affects 4 people",
+            self.view.anon_affected_statement)
+        self.assertEqual(
+            self.view.other_users_affected_count,
+            3)
+
+    def test_counts_affected_by_master(self):
+        self.useFixture(FeatureFixture(
+            {'bugs.affected_count_includes_dupes.disabled': ''}))
+        self.makeDuplicate()
+        # And now with you also affected by the master.
+        self.bug.markUserAffected(self.view.user, True)
+        self.refresh()
+        self.assertEqual(
+            "This bug affects you and 3 other people",
+            self.view.affected_statement)
+        self.assertEqual(
+            "This bug affects 4 people",
+            self.view.anon_affected_statement)
+        self.assertEqual(
+            self.view.other_users_affected_count,
+            3)
+
+    def test_counts_affected_by_duplicate_not_by_master(self):
+        self.useFixture(FeatureFixture(
+            {'bugs.affected_count_includes_dupes.disabled': ''}))
+        self.makeDuplicate()
+        self.bug2.markUserAffected(self.view.user, True)
+        self.bug.markUserAffected(self.view.user, False)
+        # You're not included in this count, even though you are affected by
+        # the dupe.
+        self.assertEqual(
+            "This bug affects 3 people, but not you",
+            self.view.affected_statement)
+        # It would be reasonable for Anon to see this bug cluster affecting
+        # either 3 or 4 people.  However at the moment the "No" answer on the
+        # master is more authoritative than the "Yes" on the dupe.
+        self.assertEqual(
+            "This bug affects 3 people",
+            self.view.anon_affected_statement)
+        self.assertEqual(
+            self.view.other_users_affected_count,
+            3)
+
+    def test_total_users_affected_count_without_dupes(self):
+        self.useFixture(FeatureFixture(
+            {'bugs.affected_count_includes_dupes.disabled': 'on'}))
+        self.makeDuplicate()
+        self.refresh()
+        # Does not count the two users of bug2, so just 1.
+        self.assertEqual(
+            1, self.view.total_users_affected_count)
+        self.assertEqual(
+            "This bug affects 1 person. Does this bug affect you?",
+            self.view.affected_statement)
+        self.assertEqual(
+            "This bug affects 1 person",
+            self.view.anon_affected_statement)
+        self.assertEqual(
+            1,
+            self.view.other_users_affected_count)
 
     def test_affected_statement_no_one_affected(self):
         self.bug.markUserAffected(self.bug.owner, False)

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-10-27 23:32:17 +0000
+++ lib/lp/bugs/configure.zcml	2011-11-14 05:10:29 +0000
@@ -818,6 +818,7 @@
                     users_affected
                     users_unaffected
                     users_affected_count_with_dupes
+                    other_users_affected_count_with_dupes
                     users_affected_with_dupes
                     bug_messages
                     isUserAffected

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-10-25 02:12:44 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-11-14 05:10:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213,E0602
@@ -374,13 +374,17 @@
             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))
+        Int(title=_('The number of users affected by this bug '
+            '(including duplicates)'),
+        required=True, readonly=True))
+    other_users_affected_count_with_dupes = exported(
+        Int(title=_('The number of users affected by this bug '
+            '(including duplicates), excluding the current user'),
+        required=True, readonly=True))
     users_affected_with_dupes = exported(doNotSnapshot(CollectionField(
-            title=_('Users affected (including duplicates)'),
-            value_type=Reference(schema=IPerson),
-            readonly=True)))
+        title=_('Users affected (including duplicates)'),
+        value_type=Reference(schema=IPerson),
+        readonly=True)))
 
     heat = exported(
         Int(title=_("The 'heat' of the bug"),

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-11-07 21:25:46 +0000
+++ lib/lp/bugs/model/bug.py	2011-11-14 05:10:29 +0000
@@ -481,6 +481,15 @@
         return self.users_affected_with_dupes.count()
 
     @property
+    def other_users_affected_count_with_dupes(self):
+        """See `IBug`."""
+        current_user = getUtility(ILaunchBag).user
+        if not current_user:
+            return self.users_affected_count_with_dupes
+        return self.users_affected_with_dupes.find(
+            Person.id != current_user.id).count()
+
+    @property
     def indexed_messages(self):
         """See `IMessageTarget`."""
         # Note that this is a decorated result set, so will cache its

=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt	2011-09-30 16:05:20 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt	2011-11-14 05:10:29 +0000
@@ -39,6 +39,7 @@
     messages_collection_link: u'http://.../bugs/11/messages'
     name: None
     number_of_duplicates: 0
+    other_users_affected_count_with_dupes: 0
     owner_link: u'http://.../~name16'
     private: False
     resource_type_link: u'http://.../#bug'

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-11-03 13:58:07 +0000
+++ lib/lp/services/features/flags.py	2011-11-14 05:10:29 +0000
@@ -47,6 +47,12 @@
      '',
      '',
      ''),
+    ('bugs.affected_count_includes_dupes.disabled',
+     'boolean',
+     ("Disable adding up affected users across all duplicate bugs."),
+     '',
+     '',
+     'https://bugs.launchpad.net/launchpad/+bug/678090'),
     ('bugs.bugtracker_components.enabled',
      'boolean',
      ('Enables the display of bugtracker components.'),