Launchpad itself

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

  1. dsp-bug-counts-1
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 11442
Proposed branch: lp:~sinzui/launchpad/dsp-bug-counts-1
Merge into: lp:launchpad
Diff against target: 232 lines (+59/-14)
5 files modified
database/schema/security.cfg (+19/-11)
lib/lp/bugs/doc/bug-heat.txt (+0/-2)
lib/lp/bugs/model/bug.py (+3/-1)
lib/lp/bugs/tests/test_bugheat.py (+35/-0)
lib/lp/registry/model/distributionsourcepackage.py (+2/-0)
To merge this branch: bzr merge lp:~sinzui/launchpad/dsp-bug-counts-1
Reviewer Review Type Date Requested Status
Deryck Hodge (community) code Approve
Brad Crittenden (community) code Approve
Review via email: mp+33257@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

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

review: Approve (code)
Revision history for this message
Robert Collins (lifeless) wrote :

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 ?

Revision history for this message
Curtis Hovey (sinzui) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

Thanks for clarifying, I appreciate it.

Revision history for this message
Curtis Hovey (sinzui) wrote :
Download full text (7.2 KiB)

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...

Read more...

Revision history for this message
Deryck Hodge (deryck) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
535public.bugwatchactivity = SELECT, INSERT, UPDATE535public.bugwatchactivity = SELECT, INSERT, UPDATE
536public.cve = SELECT, INSERT, UPDATE536public.cve = SELECT, INSERT, UPDATE
537public.cvereference = SELECT, INSERT, UPDATE537public.cvereference = SELECT, INSERT, UPDATE
538public.distribution = SELECT538public.distribution = SELECT, UPDATE
539public.distributionsourcepackage = SELECT, INSERT, UPDATE
539public.distroseries = SELECT540public.distroseries = SELECT
540public.emailaddress = SELECT, INSERT541public.emailaddress = SELECT, INSERT
541public.job = SELECT, INSERT, UPDATE542public.job = SELECT, INSERT, UPDATE
@@ -548,13 +549,16 @@
548public.packagebugsupervisor = SELECT549public.packagebugsupervisor = SELECT
549public.person = SELECT, INSERT, UPDATE550public.person = SELECT, INSERT, UPDATE
550public.personlanguage = SELECT551public.personlanguage = SELECT
551public.product = SELECT552public.product = SELECT, UPDATE
552public.productseries = SELECT553public.productseries = SELECT
553public.project = SELECT554public.project = SELECT, UPDATE
554public.questionbug = SELECT555public.questionbug = SELECT
555public.question = SELECT556public.question = SELECT
556public.questionsubscription = SELECT557public.questionsubscription = SELECT
558public.section = SELECT
559public.sourcepackagepublishinghistory = SELECT
557public.sourcepackagename = SELECT560public.sourcepackagename = SELECT
561public.sourcepackagerelease = SELECT
558public.structuralsubscription = SELECT562public.structuralsubscription = SELECT
559public.teammembership = SELECT563public.teammembership = SELECT
560public.teamparticipation = SELECT, INSERT564public.teamparticipation = SELECT, INSERT
@@ -1083,7 +1087,7 @@
1083# to insert it.1087# to insert it.
1084public.gpgkey = SELECT, INSERT1088public.gpgkey = SELECT, INSERT
1085public.signedcodeofconduct = SELECT1089public.signedcodeofconduct = SELECT
1086public.distribution = SELECT1090public.distribution = SELECT, UPDATE
1087public.distroseries = SELECT, UPDATE1091public.distroseries = SELECT, UPDATE
1088public.distroarchseries = SELECT1092public.distroarchseries = SELECT
1089public.sourcepackagepublishinghistory = SELECT1093public.sourcepackagepublishinghistory = SELECT
@@ -1140,8 +1144,8 @@
1140public.bugnotificationrecipient = SELECT, INSERT1144public.bugnotificationrecipient = SELECT, INSERT
1141public.bugnomination = SELECT1145public.bugnomination = SELECT
1142public.bugtask = SELECT, UPDATE1146public.bugtask = SELECT, UPDATE
1143public.product = SELECT1147public.product = SELECT, UPDATE
1144public.project = SELECT1148public.project = SELECT, UPDATE
1145public.bugmessage = SELECT, INSERT1149public.bugmessage = SELECT, INSERT
1146public.message = SELECT, INSERT1150public.message = SELECT, INSERT
1147public.messagechunk = SELECT, INSERT1151public.messagechunk = SELECT, INSERT
@@ -1163,6 +1167,7 @@
1163public.questionsubscription = SELECT1167public.questionsubscription = SELECT
1164public.answercontact = SELECT1168public.answercontact = SELECT
1165public.personlanguage = SELECT1169public.personlanguage = SELECT
1170public.section = SELECT
1166public.structuralsubscription = SELECT1171public.structuralsubscription = SELECT
11671172
1168# Diffing against ancestry and maintenance tasks.1173# Diffing against ancestry and maintenance tasks.
@@ -1190,7 +1195,7 @@
1190public.archive = SELECT, UPDATE1195public.archive = SELECT, UPDATE
1191public.archivearch = SELECT, UPDATE1196public.archivearch = SELECT, UPDATE
1192public.archivepermission = SELECT1197public.archivepermission = SELECT
1193public.distribution = SELECT1198public.distribution = SELECT, UPDATE
1194public.distroseries = SELECT1199public.distroseries = SELECT
1195public.distroarchseries = SELECT1200public.distroarchseries = SELECT
1196public.processor = SELECT1201public.processor = SELECT
@@ -1241,8 +1246,8 @@
1241public.bugnotificationrecipient = SELECT, INSERT1246public.bugnotificationrecipient = SELECT, INSERT
1242public.bugnomination = SELECT1247public.bugnomination = SELECT
1243public.bugtask = SELECT, UPDATE1248public.bugtask = SELECT, UPDATE
1244public.product = SELECT1249public.product = SELECT, UPDATE
1245public.project = SELECT1250public.project = SELECT, UPDATE
1246public.bugmessage = SELECT, INSERT1251public.bugmessage = SELECT, INSERT
1247public.message = SELECT, INSERT1252public.message = SELECT, INSERT
1248public.messagechunk = SELECT, INSERT1253public.messagechunk = SELECT, INSERT
@@ -1264,6 +1269,7 @@
1264public.questionsubscription = SELECT1269public.questionsubscription = SELECT
1265public.answercontact = SELECT1270public.answercontact = SELECT
1266public.personlanguage = SELECT1271public.personlanguage = SELECT
1272public.section = SELECT
1267public.structuralsubscription = SELECT1273public.structuralsubscription = SELECT
1268public.packageset = SELECT1274public.packageset = SELECT
1269public.packagesetgroup = SELECT1275public.packagesetgroup = SELECT
@@ -1312,13 +1318,14 @@
1312public.bugtag = SELECT1318public.bugtag = SELECT
1313public.bugtask = SELECT, INSERT, UPDATE1319public.bugtask = SELECT, INSERT, UPDATE
1314public.bugwatch = SELECT1320public.bugwatch = SELECT
1321public.distribution = SELECT, UPDATE
1315public.job = SELECT, INSERT, UPDATE1322public.job = SELECT, INSERT, UPDATE
1316public.component = SELECT1323public.component = SELECT
1317public.packagebugsupervisor = SELECT1324public.packagebugsupervisor = SELECT
1318public.person = SELECT1325public.person = SELECT
1319public.personlanguage = SELECT1326public.personlanguage = SELECT
1320public.product = SELECT1327public.product = SELECT, UPDATE
1321public.project = SELECT1328public.project = SELECT, UPDATE
1322public.productseries = SELECT1329public.productseries = SELECT
1323public.question = SELECT1330public.question = SELECT
1324public.questionbug = SELECT1331public.questionbug = SELECT
@@ -1326,6 +1333,7 @@
1326public.distribution = SELECT1333public.distribution = SELECT
1327public.distributionsourcepackage = SELECT, INSERT, UPDATE1334public.distributionsourcepackage = SELECT, INSERT, UPDATE
1328public.distroseries = SELECT1335public.distroseries = SELECT
1336public.section = SELECT
1329public.sourcepackagename = SELECT1337public.sourcepackagename = SELECT
1330public.sourcepackagerelease = SELECT1338public.sourcepackagerelease = SELECT
1331public.sourcepackagepublishinghistory = SELECT1339public.sourcepackagepublishinghistory = SELECT
13321340
=== 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 @@
287287
288 >>> product = factory.makeProduct()288 >>> product = factory.makeProduct()
289 >>> bug = factory.makeBug(product=product)289 >>> bug = factory.makeBug(product=product)
290 >>> print product.max_bug_heat
291 None
292 >>> bug.setHeat(123)290 >>> bug.setHeat(123)
293 >>> print product.max_bug_heat291 >>> print product.max_bug_heat
294 123292 123
295293
=== 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 @@
16331633
1634 def userCanView(self, user):1634 def userCanView(self, user):
1635 """See `IBug`.1635 """See `IBug`.
1636 1636
1637 Note that Editing is also controlled by this check,1637 Note that Editing is also controlled by this check,
1638 because we permit editing of any bug one can see.1638 because we permit editing of any bug one can see.
1639 """1639 """
@@ -1724,6 +1724,8 @@
17241724
1725 self.heat = SQL("calculate_bug_heat(%s)" % sqlvalues(self))1725 self.heat = SQL("calculate_bug_heat(%s)" % sqlvalues(self))
1726 self.heat_last_updated = UTC_NOW1726 self.heat_last_updated = UTC_NOW
1727 for task in self.bugtasks:
1728 task.target.recalculateBugHeatCache()
1727 store.flush()1729 store.flush()
17281730
1729 @property1731 @property
17301732
=== 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 @@
77
8import unittest8import unittest
99
10from zope.interface import implements
11
10from storm.store import Store12from storm.store import Store
1113
14from lazr.delegates import delegates
15
12from canonical.testing import LaunchpadZopelessLayer16from canonical.testing import LaunchpadZopelessLayer
13from lp.bugs.interfaces.bugtask import BugTaskStatus17from lp.bugs.interfaces.bugtask import BugTaskStatus
18from lp.registry.interfaces.distributionsourcepackage import (
19 IDistributionSourcePackage)
14from lp.testing import TestCaseWithFactory20from lp.testing import TestCaseWithFactory
15from lp.testing.factory import LaunchpadObjectFactory21from lp.testing.factory import LaunchpadObjectFactory
1622
1723
24class BugUpdateHeat(TestCaseWithFactory):
25
26 layer = LaunchpadZopelessLayer
27
28 def test_updateHeat_calls_recalculateBugHeatCache(self):
29 # This requires some instrumentation. The updateHeat() method is
30 # called by many methods that may be involved in setting up a bug
31 # target.
32 class TestTarget:
33 implements(IDistributionSourcePackage)
34 delegates(IDistributionSourcePackage, context='target')
35
36 def __init__(self, target):
37 self.target = target
38 self.called = False
39
40 def recalculateBugHeatCache(self):
41 self.called = True
42
43 self.target = TestTarget(
44 self.factory.makeDistributionSourcePackage(with_db=True))
45 self.bugtask = self.factory.makeBugTask(target=self.target)
46 self.bugtask.bug.updateHeat()
47 self.assertTrue(self.target.called)
48
49
18class MaxHeatByTargetBase:50class MaxHeatByTargetBase:
19 """Base class for testing a bug target's max_bug_heat attribute."""51 """Base class for testing a bug target's max_bug_heat attribute."""
2052
@@ -114,9 +146,12 @@
114 self.bugtask1 = self.factory.makeBugTask(target=self.target)146 self.bugtask1 = self.factory.makeBugTask(target=self.target)
115 self.bugtask2 = self.factory.makeBugTask(target=self.target)147 self.bugtask2 = self.factory.makeBugTask(target=self.target)
116 self.bugtask3 = self.factory.makeBugTask(target=self.target)148 self.bugtask3 = self.factory.makeBugTask(target=self.target)
149 self.bugtask4 = self.factory.makeBugTask(target=self.target)
117 # A closed bug is not include in DSP bug heat calculations.150 # A closed bug is not include in DSP bug heat calculations.
118 self.bugtask3.transitionToStatus(151 self.bugtask3.transitionToStatus(
119 BugTaskStatus.FIXRELEASED, self.target.distribution.owner)152 BugTaskStatus.FIXRELEASED, self.target.distribution.owner)
153 # A duplicate bug is not include in DSP bug heat calculations.
154 self.bugtask4.bug.markAsDuplicate(self.bugtask1.bug)
120 # Bug heat gets calculated by complicated rules in a db155 # Bug heat gets calculated by complicated rules in a db
121 # stored procedure. We will override them here to avoid156 # stored procedure. We will override them here to avoid
122 # testing inconsitencies if those values are calculated157 # testing inconsitencies if those values are calculated
123158
=== 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 @@
224 BugTask.bug == Bug.id,224 BugTask.bug == Bug.id,
225 BugTask.distributionID == self.distribution.id,225 BugTask.distributionID == self.distribution.id,
226 BugTask.sourcepackagenameID == self.sourcepackagename.id,226 BugTask.sourcepackagenameID == self.sourcepackagename.id,
227 Bug.duplicateof == None,
227 BugTask.status.is_in(UNRESOLVED_BUGTASK_STATUSES)).one()228 BugTask.status.is_in(UNRESOLVED_BUGTASK_STATUSES)).one()
228229
229 # Aggregate functions return NULL if zero rows match.230 # Aggregate functions return NULL if zero rows match.
@@ -565,6 +566,7 @@
565 dsp.sourcepackagename = sourcepackagename566 dsp.sourcepackagename = sourcepackagename
566 dsp.is_upstream_link_allowed = is_upstream_link_allowed567 dsp.is_upstream_link_allowed = is_upstream_link_allowed
567 Store.of(distribution).add(dsp)568 Store.of(distribution).add(dsp)
569 Store.of(distribution).flush()
568 return dsp570 return dsp
569571
570 @classmethod572 @classmethod