Merge lp:~sinzui/launchpad/dsp-bug-counts-1 into lp:launchpad

[Curtis Hovey]

This is my branch to fix the DSP bug counts shown on +needs-packaging.

    lp:~sinzui/launchpad/dsp-bug-counts-1
    Diff size: 90
    Launchpad bug:
          https://bugs.launchpad.net/bugs/613610
    Test command: ./bin/test -vv -t test_bugheat
    Pre-implementation: deryck (my hero)
    Target release: 10.09

Fix the DSP bug counts shown on +needs-packaging
-------------------------------------------------

The bug counts shown on +needs-packaging are for all bugs reported against
the DSP. We want to show the open bugs that the user can see when viewing
the DSPs bug page.

The recent fix to exclude closed bugs was not enough to address this issue.
The rules must also ignore duplicate bugs. The call to recalculateBugHeatCache
should be added to updateHeat so that bugtargets are updated when their bugs
change.

Rules
-----

    * Add condition to exclude duplicate bugs from bug heat cache
      calculations.
    * Call recalculateBugHeatCache in updateHeat so that bugtarget caches
      update when their bugs change.

QA
--

    * Subscribe to flashplugin-nonfree on staging. Verify the DSP's bug
      count goes down.
          SELECT bug_count FROM distributionsourcepackage
          WHERE distribution = 1
            AND sourcepackagename in (
                SELECT id FROM sourcepackagename
                WHERE name = 'flashplugin-nonfree');

Lint
----

Linting changed files:
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/tests/test_bugheat.py
  lib/lp/registry/model/distributionsourcepackage.py

Test
----

    * lib/lp/bugs/tests/test_bugheat.py
      * Added a test to verify recalculateBugHeatCache is called during
        Bug.updateHeat. This may look evil, but messing with security
        proxies and monkey patching objects is more dangerous.
      * Add a duplicate bug to the test setup. The count (2) must not change.

Implementation
--------------

    * lib/lp/bugs/model/bug.py
      * Added a rule to iterate of the bug's tasks and call the
        task's recalculateBugHeatCache method. This is the same way
        setHeat works.
    * lib/lp/registry/model/distributionsourcepackage.py
      * Add a condition to ignore duplicate bugs in recalculateBugHeatCache.

[Brad Crittenden (bac)]

This fix is so simple -- let's hope it does the trick.

review: Approve (code)

[Robert Collins (lifeless)]

Could you expand on this? I seem to recall a bug saying that
duplicates *should* count towards the heat of a bug? or is this an
aggregate function saying that we shouldn't count the heat from
duplicates twice ?

[Curtis Hovey (sinzui)]

The duplicates heat is alredy counted on the bug it is a duplicate of. When a bug is marked a duplicate, its heat is set to 0. So it has no apparent affect when total heat or getting the max heat. It is however skewing the number of open bugs on the +needs-page. The user sees 1000 bugs, but discovers there is only 58. Most are duplicates, many are closed.

[Robert Collins (lifeless)]

Thanks for clarifying, I appreciate it.

[Curtis Hovey (sinzui)]

Hi.

I had a number of test failures caused by the change to updateHeat to also update the bug target caches. Many procs are changing bugs and thus are implicitly using SELECT and UPDATE to work with distros, DSPs, products, projects. Creating a DSP on demand also requires read SELECT on sourcepackagepublishinghistory and section. To pass the tests, I need to give bugnotification, checkwatches, queued, and uploading scripts access.

I also saw some very strange storm timing issues that were fixed by adding a call to flush() when we mutate the DSP to create a db instance on demand.

I hesitate to do this without your okay. If we do not do this, I think we need to rethink how bug targets keep that bug heat cache data in sync. We could reuse the old bug-heat script user to update thousands of targets every day, or consider a job.

{{{

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2010-08-15 11:29:23 +0000
+++ database/schema/security.cfg 2010-08-23 21:17:14 +0000
@@ -535,7 +535,8 @@
 public.bugwatchactivity = SELECT, INSERT, UPDATE
 public.cve = SELECT, INSERT, UPDATE
 public.cvereference = SELECT, INSERT, UPDATE
-public.distribution = SELECT
+public.distribution = SELECT, UPDATE
+public.distributionsourcepackage = SELECT, INSERT, UPDATE
 public.distroseries = SELECT
 public.emailaddress = SELECT, INSERT
 public.job = SELECT, INSERT, UPDATE
@@ -548,13 +549,16 @@
 public.packagebugsupervisor = SELECT
 public.person = SELECT, INSERT, UPDATE
 public.personlanguage = SELECT
-public.product = SELECT
+public.product = SELECT, UPDATE
 public.productseries = SELECT
-public.project = SELECT
+public.project = SELECT, UPDATE
 public.questionbug = SELECT
 public.question = SELECT
 public.questionsubscription = SELECT
+public.section = SELECT
+public.sourcepackagepublishinghistory = SELECT
 public.sourcepackagename = SELECT
+public.sourcepackagerelease = SELECT
 public.structuralsubscription = SELECT
 public.teammembership = SELECT
 public.teamparticipation = SELECT, INSERT
@@ -1083,7 +1087,7 @@
 # to insert it.
 public.gpgkey = SELECT, INSERT
 public.signedcodeofconduct = SELECT
-public.distribution = SELECT
+public.distribution = SELECT, UPDATE
 public.distroseries = SELECT, UPDATE
 public.distroarchseries = SELECT
 public.sourcepackagepublishinghistory = SELECT
@@ -1140,8 +1144,8 @@
 public.bugnotificationrecipient = SELECT, INSERT
 public.bugnomination = SELECT
 public.bugtask = SELECT, UPDATE
-public.product = SELECT
-public.p...

[Deryck Hodge (deryck)]

The changes here look good to me. Curtis and I chatted a bit on IRC about the changes to security.cfg, and I'm comfortable with them and have no concerns.

Cheers,
deryck

review: Approve (code)

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-08-15 11:29:23 +0000
+++ database/schema/security.cfg	2010-08-25 14:45:55 +0000
@@ -535,7 +535,8 @@
 public.bugwatchactivity                 = SELECT, INSERT, UPDATE
 public.cve                              = SELECT, INSERT, UPDATE
 public.cvereference                     = SELECT, INSERT, UPDATE
-public.distribution                     = SELECT
+public.distribution                     = SELECT, UPDATE
+public.distributionsourcepackage        = SELECT, INSERT, UPDATE
 public.distroseries                     = SELECT
 public.emailaddress                     = SELECT, INSERT
 public.job                              = SELECT, INSERT, UPDATE
@@ -548,13 +549,16 @@
 public.packagebugsupervisor             = SELECT
 public.person                           = SELECT, INSERT, UPDATE
 public.personlanguage                   = SELECT
-public.product                          = SELECT
+public.product                          = SELECT, UPDATE
 public.productseries                    = SELECT
-public.project                          = SELECT
+public.project                          = SELECT, UPDATE
 public.questionbug                      = SELECT
 public.question                         = SELECT
 public.questionsubscription             = SELECT
+public.section                          = SELECT
+public.sourcepackagepublishinghistory   = SELECT
 public.sourcepackagename                = SELECT
+public.sourcepackagerelease             = SELECT
 public.structuralsubscription           = SELECT
 public.teammembership                   = SELECT
 public.teamparticipation                = SELECT, INSERT
@@ -1083,7 +1087,7 @@
 # to insert it.
 public.gpgkey                           = SELECT, INSERT
 public.signedcodeofconduct              = SELECT
-public.distribution                     = SELECT
+public.distribution                     = SELECT, UPDATE
 public.distroseries                     = SELECT, UPDATE
 public.distroarchseries                 = SELECT
 public.sourcepackagepublishinghistory   = SELECT
@@ -1140,8 +1144,8 @@
 public.bugnotificationrecipient         = SELECT, INSERT
 public.bugnomination                    = SELECT
 public.bugtask                          = SELECT, UPDATE
-public.product                          = SELECT
-public.project                          = SELECT
+public.product                          = SELECT, UPDATE
+public.project                          = SELECT, UPDATE
 public.bugmessage                       = SELECT, INSERT
 public.message                          = SELECT, INSERT
 public.messagechunk                     = SELECT, INSERT
@@ -1163,6 +1167,7 @@
 public.questionsubscription             = SELECT
 public.answercontact                    = SELECT
 public.personlanguage                   = SELECT
+public.section                          = SELECT
 public.structuralsubscription           = SELECT
 
 # Diffing against ancestry and maintenance tasks.
@@ -1190,7 +1195,7 @@
 public.archive                          = SELECT, UPDATE
 public.archivearch                      = SELECT, UPDATE
 public.archivepermission                = SELECT
-public.distribution                     = SELECT
+public.distribution                     = SELECT, UPDATE
 public.distroseries                     = SELECT
 public.distroarchseries                 = SELECT
 public.processor                        = SELECT
@@ -1241,8 +1246,8 @@
 public.bugnotificationrecipient         = SELECT, INSERT
 public.bugnomination                    = SELECT
 public.bugtask                          = SELECT, UPDATE
-public.product                          = SELECT
-public.project                          = SELECT
+public.product                          = SELECT, UPDATE
+public.project                          = SELECT, UPDATE
 public.bugmessage                       = SELECT, INSERT
 public.message                          = SELECT, INSERT
 public.messagechunk                     = SELECT, INSERT
@@ -1264,6 +1269,7 @@
 public.questionsubscription             = SELECT
 public.answercontact                    = SELECT
 public.personlanguage                   = SELECT
+public.section                          = SELECT
 public.structuralsubscription           = SELECT
 public.packageset                       = SELECT
 public.packagesetgroup                  = SELECT
@@ -1312,13 +1318,14 @@
 public.bugtag                           = SELECT
 public.bugtask                          = SELECT, INSERT, UPDATE
 public.bugwatch                         = SELECT
+public.distribution                     = SELECT, UPDATE
 public.job                              = SELECT, INSERT, UPDATE
 public.component                        = SELECT
 public.packagebugsupervisor             = SELECT
 public.person                           = SELECT
 public.personlanguage                   = SELECT
-public.product                          = SELECT
-public.project                          = SELECT
+public.product                          = SELECT, UPDATE
+public.project                          = SELECT, UPDATE
 public.productseries                    = SELECT
 public.question                         = SELECT
 public.questionbug                      = SELECT
@@ -1326,6 +1333,7 @@
 public.distribution                     = SELECT
 public.distributionsourcepackage        = SELECT, INSERT, UPDATE
 public.distroseries                     = SELECT
+public.section                          = SELECT
 public.sourcepackagename                = SELECT
 public.sourcepackagerelease             = SELECT
 public.sourcepackagepublishinghistory   = SELECT

=== modified file 'lib/lp/bugs/doc/bug-heat.txt'
--- lib/lp/bugs/doc/bug-heat.txt	2010-06-22 16:08:05 +0000
+++ lib/lp/bugs/doc/bug-heat.txt	2010-08-25 14:45:55 +0000
@@ -287,8 +287,6 @@
 
     >>> product = factory.makeProduct()
     >>> bug = factory.makeBug(product=product)
-    >>> print product.max_bug_heat
-    None
     >>> bug.setHeat(123)
     >>> print product.max_bug_heat
     123

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-08-22 18:31:30 +0000
+++ lib/lp/bugs/model/bug.py	2010-08-25 14:45:55 +0000
@@ -1633,7 +1633,7 @@
 
     def userCanView(self, user):
         """See `IBug`.
-        
+
         Note that Editing is also controlled by this check,
         because we permit editing of any bug one can see.
         """
@@ -1724,6 +1724,8 @@
 
         self.heat = SQL("calculate_bug_heat(%s)" % sqlvalues(self))
         self.heat_last_updated = UTC_NOW
+        for task in self.bugtasks:
+            task.target.recalculateBugHeatCache()
         store.flush()
 
     @property

=== modified file 'lib/lp/bugs/tests/test_bugheat.py'
--- lib/lp/bugs/tests/test_bugheat.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/tests/test_bugheat.py	2010-08-25 14:45:55 +0000
@@ -7,14 +7,46 @@
 
 import unittest
 
+from zope.interface import implements
+
 from storm.store import Store
 
+from lazr.delegates import delegates
+
 from canonical.testing import LaunchpadZopelessLayer
 from lp.bugs.interfaces.bugtask import BugTaskStatus
+from lp.registry.interfaces.distributionsourcepackage import (
+    IDistributionSourcePackage)
 from lp.testing import TestCaseWithFactory
 from lp.testing.factory import LaunchpadObjectFactory
 
 
+class BugUpdateHeat(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_updateHeat_calls_recalculateBugHeatCache(self):
+        # This requires some instrumentation. The updateHeat() method is
+        # called by many methods that may be involved in setting up a bug
+        # target.
+        class TestTarget:
+            implements(IDistributionSourcePackage)
+            delegates(IDistributionSourcePackage, context='target')
+
+            def __init__(self, target):
+                self.target = target
+                self.called = False
+
+            def recalculateBugHeatCache(self):
+                self.called = True
+
+        self.target = TestTarget(
+            self.factory.makeDistributionSourcePackage(with_db=True))
+        self.bugtask = self.factory.makeBugTask(target=self.target)
+        self.bugtask.bug.updateHeat()
+        self.assertTrue(self.target.called)
+
+
 class MaxHeatByTargetBase:
     """Base class for testing a bug target's max_bug_heat attribute."""
 
@@ -114,9 +146,12 @@
         self.bugtask1 = self.factory.makeBugTask(target=self.target)
         self.bugtask2 = self.factory.makeBugTask(target=self.target)
         self.bugtask3 = self.factory.makeBugTask(target=self.target)
+        self.bugtask4 = self.factory.makeBugTask(target=self.target)
         # A closed bug is not include in DSP bug heat calculations.
         self.bugtask3.transitionToStatus(
             BugTaskStatus.FIXRELEASED, self.target.distribution.owner)
+        # A duplicate bug is not include in DSP bug heat calculations.
+        self.bugtask4.bug.markAsDuplicate(self.bugtask1.bug)
         # Bug heat gets calculated by complicated rules in a db
         # stored procedure. We will override them here to avoid
         # testing inconsitencies if those values are calculated

=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
--- lib/lp/registry/model/distributionsourcepackage.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/distributionsourcepackage.py	2010-08-25 14:45:55 +0000
@@ -224,6 +224,7 @@
             BugTask.bug == Bug.id,
             BugTask.distributionID == self.distribution.id,
             BugTask.sourcepackagenameID == self.sourcepackagename.id,
+            Bug.duplicateof == None,
             BugTask.status.is_in(UNRESOLVED_BUGTASK_STATUSES)).one()
 
         # Aggregate functions return NULL if zero rows match.
@@ -565,6 +566,7 @@
         dsp.sourcepackagename = sourcepackagename
         dsp.is_upstream_link_allowed = is_upstream_link_allowed
         Store.of(distribution).add(dsp)
+        Store.of(distribution).flush()
         return dsp
 
     @classmethod