launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05265
[Merge] lp:~benji/launchpad/bug-828572 into lp:launchpad
Benji York has proposed merging lp:~benji/launchpad/bug-828572 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #828572 in Launchpad itself: "bugs are marked incomplete_with_response if users or scripts change the status / tags immediately after setting the status"
https://bugs.launchpad.net/launchpad/+bug/828572
For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-828572/+merge/79733
We have transitioned from having a single INCOMPLETE bug status to
INCOMPLETE_WITHOUT_RESPONSE and INCOMPLETE_WITH_RESPONSE. During the
transition period the code that builds queries to find incomplete bugs
was using a heuristic based on date_last_message to deduce which bugs
had responses. Since we've finished transitioning to the new statuses,
we don't need that heuristic any more.
This "fixes" bug 828572 in that any newly-incomplete bugs will only
marked as having a response when an actual comment is added. However
there will be a few older bugs that were considered to have responses by
a garbo fixer will retain their erroneous response status until set to
incomplete anew.
I also gently reorganized lp.bugs.model.bug.setTags and added comments
there as a drive-by while trying to understand the code.
Tests: The _buildStatusClause method was somewhat under-tested so I
added several tests for it to ensure that my changes behaved the way I
expected them to (converting the instance method into a class method to
make testing easier).
The new tests can be run with this command:
bin/test -c -m lp.bugs.model.tests.test_bugtask \
-t TestBugTaskSetStatusSearchClauses
QA:
1) create a new bug
2) mark it incomplete
3) search for bugs that are incomplete without response
4) verify that the bug is in the list
5) add a tag to the bug
6) refresh the incomplete without response search and verify that the bug
is still listed
7) add a comment to the bug
8) refresh the incomplete without response search and verify that the bug
is no longer listed
9) search for bugs that are incomplete with a response
10) verify that the bug is listed in the search results
Lint: the "make lint" report doesn't show any new or existing lint.
--
https://code.launchpad.net/~benji/launchpad/bug-828572/+merge/79733
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-828572 into lp:launchpad.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2011-10-13 18:10:30 +0000
+++ lib/lp/bugs/model/bug.py 2011-10-18 19:15:59 +0000
@@ -1902,18 +1902,23 @@
def _setTags(self, tags):
"""Set the tags from a list of strings."""
- # In order to preserve the ordering of the tags, delete all tags
- # and insert the new ones.
+ # The cache will be stale after we add/remove tags, clear it.
+ del get_property_cache(self)._cached_tags
+ # Sets provide an easy way to get the difference between the old and
+ # new tags.
new_tags = set([tag.lower() for tag in tags])
old_tags = set(self.tags)
- del get_property_cache(self)._cached_tags
- added_tags = new_tags.difference(old_tags)
+ # Find the set of tags that are to be removed and remove them.
removed_tags = old_tags.difference(new_tags)
for removed_tag in removed_tags:
tag = BugTag.selectOneBy(bug=self, tag=removed_tag)
tag.destroySelf()
+ # Find the set of tags that are to be added and add them.
+ added_tags = new_tags.difference(old_tags)
for added_tag in added_tags:
BugTag(bug=self, tag=added_tag)
+ # Write all pending changes to the DB, including any pending non-tag
+ # changes.
Store.of(self).flush()
tags = property(_getTags, _setTags)
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-10-11 20:08:38 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-10-18 19:15:59 +0000
@@ -1726,62 +1726,30 @@
summary, Bug, ' AND '.join(constraint_clauses), ['BugTask'])
return self.search(search_params, _noprejoins=True)
- def _buildStatusClause(self, status):
+ @classmethod
+ def _buildStatusClause(cls, status):
"""Return the SQL query fragment for search by status.
Called from `buildQuery` or recursively."""
if zope_isinstance(status, any):
return '(' + ' OR '.join(
- self._buildStatusClause(dbitem)
+ cls._buildStatusClause(dbitem)
for dbitem
in status.query_values) + ')'
elif zope_isinstance(status, not_equals):
- return '(NOT %s)' % self._buildStatusClause(status.value)
+ return '(NOT %s)' % cls._buildStatusClause(status.value)
elif zope_isinstance(status, BaseItem):
- incomplete_response = (
- status == BugTaskStatus.INCOMPLETE)
- with_response = (
- status == BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE)
- without_response = (
- status == BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE)
- # TODO: bug 759467 tracks the migration of INCOMPLETE in the db to
- # INCOMPLETE_WITH_RESPONSE and INCOMPLETE_WITHOUT_RESPONSE. When
- # the migration is complete, we can convert status lookups to a
- # simple IN clause.
- if with_response or without_response:
- if with_response:
- return """(
- BugTask.status = %s OR
- (BugTask.status = %s
- AND (Bug.date_last_message IS NOT NULL
- AND BugTask.date_incomplete <=
- Bug.date_last_message)))
- """ % sqlvalues(
- BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE,
- BugTaskStatus.INCOMPLETE)
- elif without_response:
- return """(
- BugTask.status = %s OR
- (BugTask.status = %s
- AND (Bug.date_last_message IS NULL
- OR BugTask.date_incomplete >
- Bug.date_last_message)))
- """ % sqlvalues(
- BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE,
- BugTaskStatus.INCOMPLETE)
- assert with_response != without_response
- elif incomplete_response:
- # search for any of INCOMPLETE (being migrated from),
- # INCOMPLETE_WITH_RESPONSE or INCOMPLETE_WITHOUT_RESPONSE
- return 'BugTask.status %s' % search_value_to_where_condition(
- any(BugTaskStatus.INCOMPLETE,
- BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE,
- BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE))
+ # INCOMPLETE is not stored in the DB, instead one of
+ # DB_INCOMPLETE_BUGTASK_STATUSES is stored, so any request to
+ # search for INCOMPLETE should instead search for those values.
+ if status == BugTaskStatus.INCOMPLETE:
+ return ('(BugTask.status %s)'
+ % search_value_to_where_condition(
+ any(*DB_INCOMPLETE_BUGTASK_STATUSES)))
else:
return '(BugTask.status = %s)' % sqlvalues(status)
else:
- raise AssertionError(
- 'Unrecognized status value: %s' % repr(status))
+ raise ValueError('Unrecognized status value: %r' % (status,))
def _buildExcludeConjoinedClause(self, milestone):
"""Exclude bugtasks with a conjoined master.
=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py 2011-10-03 20:08:04 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2011-10-18 19:15:59 +0000
@@ -8,6 +8,7 @@
from lazr.lifecycle.event import ObjectModifiedEvent
from lazr.lifecycle.snapshot import Snapshot
+from testtools.testcase import ExpectedException
from testtools.matchers import Equals
from zope.component import getUtility
from zope.event import notify
@@ -18,6 +19,7 @@
from canonical.launchpad.searchbuilder import (
all,
any,
+ not_equals,
)
from canonical.launchpad.webapp.interfaces import ILaunchBag
from canonical.testing.layers import (
@@ -41,6 +43,7 @@
from lp.bugs.model.bugtask import (
bug_target_from_key,
bug_target_to_key,
+ BugTaskSet,
build_tag_search_clause,
IllegalTarget,
validate_new_target,
@@ -192,6 +195,71 @@
new=bug_task.importance))
+class TestBugTaskSetStatusSearchClauses(TestCase):
+ # BugTaskSets contain a utility function that generates SQL WHERE clauses
+ # used to find sets of bugs. These tests exercise that utility function.
+
+ def searchClause(self, status_spec):
+ return BugTaskSet._buildStatusClause(status_spec)
+
+ def test_simple_queries(self):
+ # WHERE clauses for simple status values are straightforward.
+ self.assertEqual(
+ '(BugTask.status = 10)',
+ self.searchClause(BugTaskStatus.NEW))
+ self.assertEqual(
+ '(BugTask.status = 16)',
+ self.searchClause(BugTaskStatus.OPINION))
+ self.assertEqual(
+ '(BugTask.status = 22)',
+ self.searchClause(BugTaskStatus.INPROGRESS))
+
+ def test_INCOMPLETE_query(self):
+ # Since we don't really store INCOMPLETE in the DB but instead store
+ # values with finer shades of meaning, asking for INCOMPLETE will
+ # result in a clause that actually matches multiple statuses.
+ self.assertEqual(
+ '(BugTask.status IN (13,14))',
+ self.searchClause(BugTaskStatus.INCOMPLETE))
+
+ def test_negative_query(self):
+ # If a negative is requested then the WHERE clause is simply wrapped
+ # in a "NOT".
+ status = BugTaskStatus.INCOMPLETE
+ base_query = self.searchClause(status)
+ expected_negative_query = '(NOT {0})'.format(base_query)
+ self.assertEqual(
+ expected_negative_query,
+ self.searchClause(not_equals(status)))
+
+ def test_any_query(self):
+ # An "any" object may be passed in containing a set of statuses to
+ # return.
+ self.assertEqual(
+ '((BugTask.status = 10) OR (BugTask.status = 16))',
+ self.searchClause(any(BugTaskStatus.NEW, BugTaskStatus.OPINION)))
+
+ def test_any_query_with_INCOMPLETE(self):
+ # Since INCOMPLETE is not a single-value status (see above) an "any"
+ # query causes a slightly more complex query to be generated.
+ self.assertEqual(
+ '((BugTask.status = 10) OR (BugTask.status IN (13,14)))',
+ self.searchClause(
+ any(BugTaskStatus.NEW, BugTaskStatus.INCOMPLETE)))
+
+ def test_all_query(self):
+ # Since status is single-valued, asking for "all" statuses in a set
+ # doesn't make any sense.
+ with ExpectedException(ValueError):
+ self.searchClause(
+ all(BugTaskStatus.NEW, BugTaskStatus.INCOMPLETE))
+
+ def test_bad_value(self):
+ # If an unrecognized status is provided then an error is raised.
+ with ExpectedException(ValueError):
+ self.searchClause('this-is-not-a-status')
+
+
class TestBugTaskTagSearchClauses(TestCase):
def searchClause(self, tag_spec):
