launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05214
[Merge] lp:~allenap/launchpad/bug-stats-key-error-bug-871076 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/bug-stats-key-error-bug-871076 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #871076 in Launchpad itself: "Product:+series OOPSes on INCOMPLETE_WITH_RESPONSE and INCOMPLETE_WITHOUT_RESPONSE bug tasks"
https://bugs.launchpad.net/launchpad/+bug/871076
For more details, see:
https://code.launchpad.net/~allenap/launchpad/bug-stats-key-error-bug-871076/+merge/78870
This problem was a palette of crates of tinned worms. I exaggerate,
but there was quite a lot of yak shaving involved in fixing this bug.
- There are three enums - BugTaskStatus, BugTaskStatusSearch,
BugTaskStatusSearchDisplay - that can be used somewhat
interchangeably. However, passing something other than a member of
BugTaskStatus into BugTask.canTransitionToStatus or
.transitionToStatus will not always work, so I've added a step to
normalize statuses, using a new function normalize_bugtask_status().
- I've changed the behaviour of getStatusCountsForProductSeries() so
that it returns a dict, with keys being enum members. Previously it
returned a list of 2-tuples, each containing a status *ID* and a
count. This required a new function, get_bugtask_status().
- I have updated the ProductSeries:+status view, and added a test for
bugtask_status_counts specifically.
--
https://code.launchpad.net/~allenap/launchpad/bug-stats-key-error-bug-871076/+merge/78870
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/bug-stats-key-error-bug-871076 into lp:launchpad.
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2011-10-05 18:24:09 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2011-10-11 12:09:42 +0000
@@ -20,6 +20,7 @@
'DB_INCOMPLETE_BUGTASK_STATUSES',
'DB_UNRESOLVED_BUGTASK_STATUSES',
'DEFAULT_SEARCH_BUGTASK_STATUSES_FOR_DISPLAY',
+ 'get_bugtask_status',
'IAddBugTaskForm',
'IAddBugTaskWithProductCreationForm',
'IBugTask',
@@ -28,12 +29,13 @@
'IBugTaskSet',
'ICreateQuestionFromBugTaskForm',
'IFrontPageBugTaskSearch',
+ 'IllegalRelatedBugTasksParams',
+ 'IllegalTarget',
'INominationsReviewTableBatchNavigator',
'IPersonBugTaskSearch',
'IRemoveQuestionFromBugTaskForm',
'IUpstreamProductBugTaskSearch',
- 'IllegalRelatedBugTasksParams',
- 'IllegalTarget',
+ 'normalize_bugtask_status',
'RESOLVED_BUGTASK_STATUSES',
'UNRESOLVED_BUGTASK_STATUSES',
'UserCannotEditBugTaskAssignee',
@@ -305,6 +307,33 @@
""")
+def get_bugtask_status(status_id):
+ """Get a member of `BugTaskStatus` or `BugTaskStatusSearch` by value.
+
+ `BugTaskStatus` and `BugTaskStatusSearch` intersect, but neither is a
+ subset of the other, so this searches first in `BugTaskStatus` then in
+ `BugTaskStatusSearch` for a member with the given ID.
+ """
+ try:
+ return BugTaskStatus.items[status_id]
+ except KeyError:
+ return BugTaskStatusSearch.items[status_id]
+
+
+def normalize_bugtask_status(status):
+ """Normalize `status`.
+
+ It might be a member of any of three related enums: `BugTaskStatus`,
+ `BugTaskStatusSearch`, or `BugTaskStatusSearchDisplay`. This tries to
+ normalize by value back to the first of those three enums in which the
+ status appears.
+ """
+ try:
+ return BugTaskStatus.items[status.value]
+ except KeyError:
+ return BugTaskStatusSearch.items[status.value]
+
+
class BugTagsSearchCombinator(EnumeratedType):
"""Bug Tags Search Combinator
=== added directory 'lib/lp/bugs/interfaces/tests'
=== added file 'lib/lp/bugs/interfaces/tests/__init__.py'
=== added file 'lib/lp/bugs/interfaces/tests/test_bugtask.py'
--- lib/lp/bugs/interfaces/tests/test_bugtask.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/interfaces/tests/test_bugtask.py 2011-10-11 12:09:42 +0000
@@ -0,0 +1,107 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for BugTask interfaces."""
+
+__metaclass__ = type
+
+from lp.testing import TestCase
+from lp.bugs.interfaces.bugtask import (
+ BugTaskStatus,
+ BugTaskStatusSearch,
+ BugTaskStatusSearchDisplay,
+ get_bugtask_status,
+ normalize_bugtask_status,
+ )
+
+
+class TestFunctions(TestCase):
+
+ def test_get_bugtask_status(self):
+ # Compose a map of BugTaskStatusSearch members from their values.
+ expected = dict(
+ (status.value, status)
+ for status in BugTaskStatusSearch.items)
+ # Update the expected status map - overwriting some entries - from
+ # BugTaskStatus members and their values.
+ expected.update(
+ (status.value, status)
+ for status in BugTaskStatus.items)
+ # Compose a map of statuses as discovered by value for each member of
+ # BugTaskStatusSearch.
+ observed = dict(
+ (status.value, get_bugtask_status(status.value))
+ for status in BugTaskStatusSearch.items)
+ # Update the expected status map with statuses discovered by value for
+ # each member of BugTaskStatus.
+ observed.update(
+ (status.value, get_bugtask_status(status.value))
+ for status in BugTaskStatus.items)
+ self.assertEqual(expected, observed)
+
+ def test_normalize_bugtask_status_from_BugTaskStatus(self):
+ expected = {
+ BugTaskStatus.CONFIRMED: BugTaskStatus.CONFIRMED,
+ BugTaskStatus.EXPIRED: BugTaskStatus.EXPIRED,
+ BugTaskStatus.FIXCOMMITTED: BugTaskStatus.FIXCOMMITTED,
+ BugTaskStatus.FIXRELEASED: BugTaskStatus.FIXRELEASED,
+ BugTaskStatus.INCOMPLETE: BugTaskStatus.INCOMPLETE,
+ BugTaskStatus.INPROGRESS: BugTaskStatus.INPROGRESS,
+ BugTaskStatus.INVALID: BugTaskStatus.INVALID,
+ BugTaskStatus.NEW: BugTaskStatus.NEW,
+ BugTaskStatus.OPINION: BugTaskStatus.OPINION,
+ BugTaskStatus.TRIAGED: BugTaskStatus.TRIAGED,
+ BugTaskStatus.UNKNOWN: BugTaskStatus.UNKNOWN,
+ BugTaskStatus.WONTFIX: BugTaskStatus.WONTFIX,
+ }
+ observed = dict(
+ (status, normalize_bugtask_status(status))
+ for status in BugTaskStatus.items)
+ self.assertEqual(expected, observed)
+
+ def test_normalize_bugtask_status_from_BugTaskStatusSearch(self):
+ expected = {
+ BugTaskStatusSearch.CONFIRMED: BugTaskStatus.CONFIRMED,
+ BugTaskStatusSearch.EXPIRED: BugTaskStatus.EXPIRED,
+ BugTaskStatusSearch.FIXCOMMITTED: BugTaskStatus.FIXCOMMITTED,
+ BugTaskStatusSearch.FIXRELEASED: BugTaskStatus.FIXRELEASED,
+ BugTaskStatusSearch.INCOMPLETE: BugTaskStatus.INCOMPLETE,
+ BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE:
+ BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE,
+ BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE:
+ BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE,
+ BugTaskStatusSearch.INPROGRESS: BugTaskStatus.INPROGRESS,
+ BugTaskStatusSearch.INVALID: BugTaskStatus.INVALID,
+ BugTaskStatusSearch.NEW: BugTaskStatus.NEW,
+ BugTaskStatusSearch.OPINION: BugTaskStatus.OPINION,
+ BugTaskStatusSearch.TRIAGED: BugTaskStatus.TRIAGED,
+ BugTaskStatusSearch.WONTFIX: BugTaskStatus.WONTFIX,
+ }
+ observed = dict(
+ (status, normalize_bugtask_status(status))
+ for status in BugTaskStatusSearch.items)
+ self.assertEqual(expected, observed)
+
+ def test_normalize_bugtask_status_from_BugTaskStatusSearchDisplay(self):
+ expected = {
+ BugTaskStatusSearchDisplay.CONFIRMED: BugTaskStatus.CONFIRMED,
+ BugTaskStatusSearchDisplay.EXPIRED: BugTaskStatus.EXPIRED,
+ BugTaskStatusSearchDisplay.FIXCOMMITTED:
+ BugTaskStatus.FIXCOMMITTED,
+ BugTaskStatusSearchDisplay.FIXRELEASED:
+ BugTaskStatus.FIXRELEASED,
+ BugTaskStatusSearchDisplay.INCOMPLETE_WITH_RESPONSE:
+ BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE,
+ BugTaskStatusSearchDisplay.INCOMPLETE_WITHOUT_RESPONSE:
+ BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE,
+ BugTaskStatusSearchDisplay.INPROGRESS: BugTaskStatus.INPROGRESS,
+ BugTaskStatusSearchDisplay.INVALID: BugTaskStatus.INVALID,
+ BugTaskStatusSearchDisplay.NEW: BugTaskStatus.NEW,
+ BugTaskStatusSearchDisplay.OPINION: BugTaskStatus.OPINION,
+ BugTaskStatusSearchDisplay.TRIAGED: BugTaskStatus.TRIAGED,
+ BugTaskStatusSearchDisplay.WONTFIX: BugTaskStatus.WONTFIX,
+ }
+ observed = dict(
+ (status, normalize_bugtask_status(status))
+ for status in BugTaskStatusSearchDisplay.items)
+ self.assertEqual(expected, observed)
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-10-10 09:04:37 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-10-11 12:09:42 +0000
@@ -116,11 +116,13 @@
BugTaskStatusSearch,
DB_INCOMPLETE_BUGTASK_STATUSES,
DB_UNRESOLVED_BUGTASK_STATUSES,
+ get_bugtask_status,
IBugTask,
IBugTaskDelta,
IBugTaskSet,
IllegalRelatedBugTasksParams,
IllegalTarget,
+ normalize_bugtask_status,
RESOLVED_BUGTASK_STATUSES,
UserCannotEditBugTaskAssignee,
UserCannotEditBugTaskImportance,
@@ -857,6 +859,7 @@
def canTransitionToStatus(self, new_status, user):
"""See `IBugTask`."""
+ new_status = normalize_bugtask_status(new_status)
celebrities = getUtility(ILaunchpadCelebrities)
if (self.status == BugTaskStatus.FIXRELEASED and
(user.id == self.bug.ownerID or user.inTeam(self.bug.owner))):
@@ -880,6 +883,8 @@
# testing the edit form.
return
+ new_status = normalize_bugtask_status(new_status)
+
if not self.canTransitionToStatus(new_status, user):
raise UserCannotEditBugTaskStatus(
"Only Bug Supervisors may change status to %s." % (
@@ -2804,13 +2809,12 @@
# since the get_bug_privacy_filter() check for non-admins is
# costly, don't filter those bugs at all.
bug_privacy_filter = ''
- cur = cursor()
-
# The union is actually much faster than a LEFT JOIN with the
# Milestone table, since postgres optimizes it to perform index
# scans instead of sequential scans on the BugTask table.
query = """
- SELECT status, count(*)
+ SELECT
+ status, COUNT(*)
FROM (
SELECT BugTask.status
FROM BugTask
@@ -2818,9 +2822,7 @@
WHERE
BugTask.productseries = %(series)s
%(privacy)s
-
UNION ALL
-
SELECT BugTask.status
FROM BugTask
JOIN Bug ON BugTask.bug = Bug.id
@@ -2831,11 +2833,15 @@
%(privacy)s
) AS subquery
GROUP BY status
- """ % dict(series=quote(product_series),
- privacy=bug_privacy_filter)
-
+ """
+ query %= dict(
+ series=quote(product_series),
+ privacy=bug_privacy_filter)
+ cur = cursor()
cur.execute(query)
- return cur.fetchall()
+ return dict(
+ (get_bugtask_status(status_id), count)
+ for (status_id, count) in cur.fetchall())
def findExpirableBugTasks(self, min_days_old, user,
bug=None, target=None, limit=None):
=== modified file 'lib/lp/bugs/model/tests/test_bugtask_status.py'
--- lib/lp/bugs/model/tests/test_bugtask_status.py 2011-09-28 14:36:34 +0000
+++ lib/lp/bugs/model/tests/test_bugtask_status.py 2011-10-11 12:09:42 +0000
@@ -10,8 +10,12 @@
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
-from lp.bugs.interfaces.bugtask import UserCannotEditBugTaskStatus
-from lp.bugs.model.bugtask import BugTaskStatus
+from lp.bugs.interfaces.bugtask import (
+ BugTaskStatus,
+ BugTaskStatusSearch,
+ BugTaskStatusSearchDisplay,
+ UserCannotEditBugTaskStatus,
+ )
from lp.testing import (
person_logged_in,
TestCaseWithFactory,
@@ -147,6 +151,35 @@
BugTaskStatus.NEW, self.user),
False)
+ def test_transitionToStatus_normalization(self):
+ # The new status is normalized using normalize_bugtask_status, so
+ # members of BugTaskStatusSearch or BugTaskStatusSearchDisplay can
+ # also be used.
+ with person_logged_in(self.user):
+ self.task.transitionToStatus(
+ BugTaskStatusSearch.CONFIRMED, self.user)
+ self.assertEqual(BugTaskStatus.CONFIRMED, self.task.status)
+ self.task.transitionToStatus(
+ BugTaskStatusSearchDisplay.CONFIRMED, self.user)
+ self.assertEqual(BugTaskStatus.CONFIRMED, self.task.status)
+
+ def test_canTransitionToStatus_normalization(self):
+ # The new status is normalized using normalize_bugtask_status, so
+ # members of BugTaskStatusSearch or BugTaskStatusSearchDisplay can
+ # also be used.
+ self.assertTrue(
+ self.task.canTransitionToStatus(
+ BugTaskStatusSearch.CONFIRMED, self.user))
+ self.assertFalse(
+ self.task.canTransitionToStatus(
+ BugTaskStatusSearch.WONTFIX, self.user))
+ self.assertTrue(
+ self.task.canTransitionToStatus(
+ BugTaskStatusSearchDisplay.CONFIRMED, self.user))
+ self.assertFalse(
+ self.task.canTransitionToStatus(
+ BugTaskStatusSearchDisplay.WONTFIX, self.user))
+
class TestBugTaskStatusTransitionForReporter(TestCaseWithFactory):
"""Tests for bug reporter status transitions."""
=== modified file 'lib/lp/bugs/tests/test_bugtaskset.py'
--- lib/lp/bugs/tests/test_bugtaskset.py 2011-03-23 16:28:51 +0000
+++ lib/lp/bugs/tests/test_bugtaskset.py 2011-10-11 12:09:42 +0000
@@ -7,9 +7,11 @@
from zope.component import getUtility
+from canonical.database.sqlbase import flush_database_updates
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.bugs.interfaces.bugtask import (
BugTaskStatus,
+ BugTaskStatusSearch,
IBugTaskSet,
)
from lp.testing import (
@@ -33,11 +35,8 @@
self.milestone = self.factory.makeMilestone(productseries=self.series)
def get_counts(self, user):
- counts = self.bugtask_set.getStatusCountsForProductSeries(
- user, self.series)
- return [
- (BugTaskStatus.items[status_id], count)
- for status_id, count in counts]
+ return self.bugtask_set.getStatusCountsForProductSeries(
+ user, self.series)
def test_privacy_and_counts_for_unauthenticated_user(self):
# An unauthenticated user should see bug counts for each status
@@ -47,7 +46,7 @@
self.factory.makeBug(series=self.series)
self.factory.makeBug(series=self.series, private=True)
self.assertEqual(
- [(BugTaskStatus.NEW, 2)],
+ {BugTaskStatus.NEW: 2},
self.get_counts(None))
def test_privacy_and_counts_for_owner(self):
@@ -58,7 +57,7 @@
self.factory.makeBug(series=self.series)
self.factory.makeBug(series=self.series, private=True)
self.assertEqual(
- [(BugTaskStatus.NEW, 4)],
+ {BugTaskStatus.NEW: 4},
self.get_counts(self.owner))
def test_privacy_and_counts_for_other_user(self):
@@ -72,22 +71,42 @@
self.factory.makeBug(series=self.series, private=True)
other = self.factory.makePerson()
self.assertEqual(
- [(BugTaskStatus.NEW, 4)],
+ {BugTaskStatus.NEW: 4},
self.get_counts(other))
def test_multiple_statuses(self):
# Test that separate counts are provided for each status that
# bugs are found in.
- for status in (BugTaskStatus.INVALID, BugTaskStatus.OPINION):
+ statuses = [
+ BugTaskStatus.INVALID,
+ BugTaskStatus.OPINION,
+ ]
+ for status in statuses:
self.factory.makeBug(milestone=self.milestone, status=status)
self.factory.makeBug(series=self.series, status=status)
for i in range(3):
self.factory.makeBug(series=self.series)
self.assertEqual(
- [(BugTaskStatus.INVALID, 2),
- (BugTaskStatus.OPINION, 2),
- (BugTaskStatus.NEW, 3),
- ],
+ {BugTaskStatus.INVALID: 2,
+ BugTaskStatus.OPINION: 2,
+ BugTaskStatus.NEW: 3},
+ self.get_counts(None))
+
+ def test_incomplete_status(self):
+ # INCOMPLETE is stored as either INCOMPLETE_WITH_RESPONSE or
+ # INCOMPLETE_WITHOUT_RESPONSE so the stats do not include a count of
+ # INCOMPLETE tasks.
+ statuses = [
+ BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE,
+ BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE,
+ BugTaskStatus.INCOMPLETE,
+ ]
+ for status in statuses:
+ self.factory.makeBug(series=self.series, status=status)
+ flush_database_updates()
+ self.assertEqual(
+ {BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE: 1,
+ BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE: 2},
self.get_counts(None))
=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py 2011-09-28 10:28:47 +0000
+++ lib/lp/registry/browser/productseries.py 2011-10-11 12:09:42 +0000
@@ -491,13 +491,14 @@
def bugtask_status_counts(self):
"""A list StatusCounts summarising the targeted bugtasks."""
bugtaskset = getUtility(IBugTaskSet)
- status_id_counts = bugtaskset.getStatusCountsForProductSeries(
+ status_counts = bugtaskset.getStatusCountsForProductSeries(
self.user, self.context)
- status_counts = dict([(BugTaskStatus.items[status_id], count)
- for status_id, count in status_id_counts])
- return [StatusCount(status, status_counts[status])
- for status in sorted(status_counts,
- key=attrgetter('sortkey'))]
+ # We sort by value before sortkey because the statuses returned can be
+ # from different (though related) enums.
+ statuses = sorted(status_counts, key=attrgetter('value', 'sortkey'))
+ return [
+ StatusCount(status, status_counts[status])
+ for status in statuses]
@cachedproperty
def specification_status_counts(self):
=== modified file 'lib/lp/registry/browser/tests/test_productseries_views.py'
--- lib/lp/registry/browser/tests/test_productseries_views.py 2011-07-29 14:26:49 +0000
+++ lib/lp/registry/browser/tests/test_productseries_views.py 2011-10-11 12:09:42 +0000
@@ -6,13 +6,17 @@
__metaclass__ = type
from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.interfaces.bugtask import (
+ BugTaskStatus,
+ BugTaskStatusSearch,
+ )
from lp.testing import (
BrowserTestCase,
+ person_logged_in,
TestCaseWithFactory,
- person_logged_in
)
+from lp.testing.matchers import Contains
from lp.testing.views import create_initialized_view
-from lp.testing.matchers import Contains
class TestProductSeriesHelp(TestCaseWithFactory):
@@ -40,3 +44,43 @@
"""Test that rendering the graph does not raise an exception."""
productseries = self.factory.makeProductSeries()
self.getViewBrowser(productseries, view_name='+timeline-graph')
+
+
+class TestProductSeriesStatus(TestCaseWithFactory):
+ """Tests for ProductSeries:+status."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_bugtask_status_counts(self):
+ """Test that `bugtask_status_counts` is sane."""
+ product = self.factory.makeProduct()
+ series = self.factory.makeProductSeries(product=product)
+ for status in BugTaskStatusSearch.items:
+ self.factory.makeBug(
+ series=series, status=status,
+ owner=product.owner)
+ self.factory.makeBug(
+ series=series, status=BugTaskStatus.UNKNOWN,
+ owner=product.owner)
+ with person_logged_in(product.owner):
+ view = create_initialized_view(series, '+status')
+ self.assertEqual(
+ [(BugTaskStatus.NEW, 1),
+ (BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE, 1),
+ # 2 because INCOMPLETE is stored as INCOMPLETE_WITH_RESPONSE
+ # or INCOMPLETE_WITHOUT_RESPONSE, and there was no response
+ # for the bug created as INCOMPLETE.
+ (BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE, 2),
+ (BugTaskStatus.OPINION, 1),
+ (BugTaskStatus.INVALID, 1),
+ (BugTaskStatus.WONTFIX, 1),
+ (BugTaskStatus.EXPIRED, 1),
+ (BugTaskStatus.CONFIRMED, 1),
+ (BugTaskStatus.TRIAGED, 1),
+ (BugTaskStatus.INPROGRESS, 1),
+ (BugTaskStatus.FIXCOMMITTED, 1),
+ (BugTaskStatus.FIXRELEASED, 1),
+ (BugTaskStatus.UNKNOWN, 1)],
+ [(status_count.status, status_count.count)
+ for status_count in view.bugtask_status_counts],
+ )
References
