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
1=== modified file 'database/schema/security.cfg'
2--- database/schema/security.cfg 2010-08-15 11:29:23 +0000
3+++ database/schema/security.cfg 2010-08-25 14:45:55 +0000
4@@ -535,7 +535,8 @@
5 public.bugwatchactivity = SELECT, INSERT, UPDATE
6 public.cve = SELECT, INSERT, UPDATE
7 public.cvereference = SELECT, INSERT, UPDATE
8-public.distribution = SELECT
9+public.distribution = SELECT, UPDATE
10+public.distributionsourcepackage = SELECT, INSERT, UPDATE
11 public.distroseries = SELECT
12 public.emailaddress = SELECT, INSERT
13 public.job = SELECT, INSERT, UPDATE
14@@ -548,13 +549,16 @@
15 public.packagebugsupervisor = SELECT
16 public.person = SELECT, INSERT, UPDATE
17 public.personlanguage = SELECT
18-public.product = SELECT
19+public.product = SELECT, UPDATE
20 public.productseries = SELECT
21-public.project = SELECT
22+public.project = SELECT, UPDATE
23 public.questionbug = SELECT
24 public.question = SELECT
25 public.questionsubscription = SELECT
26+public.section = SELECT
27+public.sourcepackagepublishinghistory = SELECT
28 public.sourcepackagename = SELECT
29+public.sourcepackagerelease = SELECT
30 public.structuralsubscription = SELECT
31 public.teammembership = SELECT
32 public.teamparticipation = SELECT, INSERT
33@@ -1083,7 +1087,7 @@
34 # to insert it.
35 public.gpgkey = SELECT, INSERT
36 public.signedcodeofconduct = SELECT
37-public.distribution = SELECT
38+public.distribution = SELECT, UPDATE
39 public.distroseries = SELECT, UPDATE
40 public.distroarchseries = SELECT
41 public.sourcepackagepublishinghistory = SELECT
42@@ -1140,8 +1144,8 @@
43 public.bugnotificationrecipient = SELECT, INSERT
44 public.bugnomination = SELECT
45 public.bugtask = SELECT, UPDATE
46-public.product = SELECT
47-public.project = SELECT
48+public.product = SELECT, UPDATE
49+public.project = SELECT, UPDATE
50 public.bugmessage = SELECT, INSERT
51 public.message = SELECT, INSERT
52 public.messagechunk = SELECT, INSERT
53@@ -1163,6 +1167,7 @@
54 public.questionsubscription = SELECT
55 public.answercontact = SELECT
56 public.personlanguage = SELECT
57+public.section = SELECT
58 public.structuralsubscription = SELECT
59
60 # Diffing against ancestry and maintenance tasks.
61@@ -1190,7 +1195,7 @@
62 public.archive = SELECT, UPDATE
63 public.archivearch = SELECT, UPDATE
64 public.archivepermission = SELECT
65-public.distribution = SELECT
66+public.distribution = SELECT, UPDATE
67 public.distroseries = SELECT
68 public.distroarchseries = SELECT
69 public.processor = SELECT
70@@ -1241,8 +1246,8 @@
71 public.bugnotificationrecipient = SELECT, INSERT
72 public.bugnomination = SELECT
73 public.bugtask = SELECT, UPDATE
74-public.product = SELECT
75-public.project = SELECT
76+public.product = SELECT, UPDATE
77+public.project = SELECT, UPDATE
78 public.bugmessage = SELECT, INSERT
79 public.message = SELECT, INSERT
80 public.messagechunk = SELECT, INSERT
81@@ -1264,6 +1269,7 @@
82 public.questionsubscription = SELECT
83 public.answercontact = SELECT
84 public.personlanguage = SELECT
85+public.section = SELECT
86 public.structuralsubscription = SELECT
87 public.packageset = SELECT
88 public.packagesetgroup = SELECT
89@@ -1312,13 +1318,14 @@
90 public.bugtag = SELECT
91 public.bugtask = SELECT, INSERT, UPDATE
92 public.bugwatch = SELECT
93+public.distribution = SELECT, UPDATE
94 public.job = SELECT, INSERT, UPDATE
95 public.component = SELECT
96 public.packagebugsupervisor = SELECT
97 public.person = SELECT
98 public.personlanguage = SELECT
99-public.product = SELECT
100-public.project = SELECT
101+public.product = SELECT, UPDATE
102+public.project = SELECT, UPDATE
103 public.productseries = SELECT
104 public.question = SELECT
105 public.questionbug = SELECT
106@@ -1326,6 +1333,7 @@
107 public.distribution = SELECT
108 public.distributionsourcepackage = SELECT, INSERT, UPDATE
109 public.distroseries = SELECT
110+public.section = SELECT
111 public.sourcepackagename = SELECT
112 public.sourcepackagerelease = SELECT
113 public.sourcepackagepublishinghistory = SELECT
114
115=== modified file 'lib/lp/bugs/doc/bug-heat.txt'
116--- lib/lp/bugs/doc/bug-heat.txt 2010-06-22 16:08:05 +0000
117+++ lib/lp/bugs/doc/bug-heat.txt 2010-08-25 14:45:55 +0000
118@@ -287,8 +287,6 @@
119
120 >>> product = factory.makeProduct()
121 >>> bug = factory.makeBug(product=product)
122- >>> print product.max_bug_heat
123- None
124 >>> bug.setHeat(123)
125 >>> print product.max_bug_heat
126 123
127
128=== modified file 'lib/lp/bugs/model/bug.py'
129--- lib/lp/bugs/model/bug.py 2010-08-22 18:31:30 +0000
130+++ lib/lp/bugs/model/bug.py 2010-08-25 14:45:55 +0000
131@@ -1633,7 +1633,7 @@
132
133 def userCanView(self, user):
134 """See `IBug`.
135-
136+
137 Note that Editing is also controlled by this check,
138 because we permit editing of any bug one can see.
139 """
140@@ -1724,6 +1724,8 @@
141
142 self.heat = SQL("calculate_bug_heat(%s)" % sqlvalues(self))
143 self.heat_last_updated = UTC_NOW
144+ for task in self.bugtasks:
145+ task.target.recalculateBugHeatCache()
146 store.flush()
147
148 @property
149
150=== modified file 'lib/lp/bugs/tests/test_bugheat.py'
151--- lib/lp/bugs/tests/test_bugheat.py 2010-08-20 20:31:18 +0000
152+++ lib/lp/bugs/tests/test_bugheat.py 2010-08-25 14:45:55 +0000
153@@ -7,14 +7,46 @@
154
155 import unittest
156
157+from zope.interface import implements
158+
159 from storm.store import Store
160
161+from lazr.delegates import delegates
162+
163 from canonical.testing import LaunchpadZopelessLayer
164 from lp.bugs.interfaces.bugtask import BugTaskStatus
165+from lp.registry.interfaces.distributionsourcepackage import (
166+ IDistributionSourcePackage)
167 from lp.testing import TestCaseWithFactory
168 from lp.testing.factory import LaunchpadObjectFactory
169
170
171+class BugUpdateHeat(TestCaseWithFactory):
172+
173+ layer = LaunchpadZopelessLayer
174+
175+ def test_updateHeat_calls_recalculateBugHeatCache(self):
176+ # This requires some instrumentation. The updateHeat() method is
177+ # called by many methods that may be involved in setting up a bug
178+ # target.
179+ class TestTarget:
180+ implements(IDistributionSourcePackage)
181+ delegates(IDistributionSourcePackage, context='target')
182+
183+ def __init__(self, target):
184+ self.target = target
185+ self.called = False
186+
187+ def recalculateBugHeatCache(self):
188+ self.called = True
189+
190+ self.target = TestTarget(
191+ self.factory.makeDistributionSourcePackage(with_db=True))
192+ self.bugtask = self.factory.makeBugTask(target=self.target)
193+ self.bugtask.bug.updateHeat()
194+ self.assertTrue(self.target.called)
195+
196+
197 class MaxHeatByTargetBase:
198 """Base class for testing a bug target's max_bug_heat attribute."""
199
200@@ -114,9 +146,12 @@
201 self.bugtask1 = self.factory.makeBugTask(target=self.target)
202 self.bugtask2 = self.factory.makeBugTask(target=self.target)
203 self.bugtask3 = self.factory.makeBugTask(target=self.target)
204+ self.bugtask4 = self.factory.makeBugTask(target=self.target)
205 # A closed bug is not include in DSP bug heat calculations.
206 self.bugtask3.transitionToStatus(
207 BugTaskStatus.FIXRELEASED, self.target.distribution.owner)
208+ # A duplicate bug is not include in DSP bug heat calculations.
209+ self.bugtask4.bug.markAsDuplicate(self.bugtask1.bug)
210 # Bug heat gets calculated by complicated rules in a db
211 # stored procedure. We will override them here to avoid
212 # testing inconsitencies if those values are calculated
213
214=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
215--- lib/lp/registry/model/distributionsourcepackage.py 2010-08-20 20:31:18 +0000
216+++ lib/lp/registry/model/distributionsourcepackage.py 2010-08-25 14:45:55 +0000
217@@ -224,6 +224,7 @@
218 BugTask.bug == Bug.id,
219 BugTask.distributionID == self.distribution.id,
220 BugTask.sourcepackagenameID == self.sourcepackagename.id,
221+ Bug.duplicateof == None,
222 BugTask.status.is_in(UNRESOLVED_BUGTASK_STATUSES)).one()
223
224 # Aggregate functions return NULL if zero rows match.
225@@ -565,6 +566,7 @@
226 dsp.sourcepackagename = sourcepackagename
227 dsp.is_upstream_link_allowed = is_upstream_link_allowed
228 Store.of(distribution).add(dsp)
229+ Store.of(distribution).flush()
230 return dsp
231
232 @classmethod