launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/incomplete-api into lp:launchpad.

Commit message:
Restore API support for searching for Incomplete bug tasks.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #872496 in Launchpad itself: "All package stats now report zero "Incomplete" bug reports"
  https://bugs.launchpad.net/launchpad/+bug/872496

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/incomplete-api/+merge/125515

This is possibly a regression from the change to store the actual distinct
incomplete enum in the DB.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Create an API test, verify it fails, then use the debugger to
      find where INCOMPLETE fails to become the other two completes.
    * Ah, the _build_status_clause() does the adaption, but does not
      because it is checking for BugTaskStatus.INCOMPLETE, but the
      API sends BugTaskStatusSearch.INCOMPLETE.
      * Both internal and external search use BugTaskStatusSearch, but our
        view know that the model uses BugTaskStatus so it is adapted
        early before going to the model. API goes directly to the model
        and is not adapted, _build_status_clause() needs to know about both.


QA

    * Run this script and verify more that 20 bugs are found:

    {{{
    from launchpadlib.launchpad import Launchpad
    lp = Launchpad.login_with(
        'testing', service_root='https://api.qastaging.launchpad.net',
        version='devel')
    project = lp.projects['launchpad']
    tasks = project.searchTasks(status="Incomplete")
    for task in tasks:
        print task.title
    print '%d bug tasks found' % len(tasks)
    }}}


LINT

    lib/lp/bugs/model/bugtasksearch.py
    lib/lp/bugs/model/tests/test_bugtasksearch.py
    lib/lp/bugs/tests/test_searchtasks_webservice.py


LoC

    I have a 3000+ line credit as of this week.


TEST

    ./bin/test -vvc lp.bugs.tests.test_searchtasks_webservice
    ./bin/test -vvc -t SetStatusSearch lp.bugs.model.tests.test_bugtasksearch


IMPLEMENTATION

I updated _build_status_clause() to make the API integration test pass, but
then saw the method handles single and list values. I found the tests for
the method and add two new tests based on the existing INCOMPLETE tests
to verify both enums are supported.
    lib/lp/bugs/model/bugtasksearch.py
    lib/lp/bugs/model/tests/test_bugtasksearch.py
    lib/lp/bugs/tests/test_searchtasks_webservice.py
-- 
https://code.launchpad.net/~sinzui/launchpad/incomplete-api/+merge/125515
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/incomplete-api into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-09-17 07:50:51 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-09-20 15:53:24 +0000
@@ -894,13 +894,16 @@
     """Return the SQL query fragment for search by status.
 
     Called from `_build_query` or recursively."""
+
     if zope_isinstance(status, any):
         values = list(status.query_values)
-        # Since INCOMPLETE isn't stored as a single value we need to
+        # Since INCOMPLETE isn't stored as a single value any more, we need to
         # expand it before generating the SQL.
-        if BugTaskStatus.INCOMPLETE in values:
-            values.remove(BugTaskStatus.INCOMPLETE)
-            values.extend(DB_INCOMPLETE_BUGTASK_STATUSES)
+        old = set([BugTaskStatus.INCOMPLETE, BugTaskStatusSearch.INCOMPLETE])
+        accepted_values = list(set(values) - old)
+        if len(accepted_values) < len(values):
+            accepted_values.extend(DB_INCOMPLETE_BUGTASK_STATUSES)
+            values = accepted_values
         return search_value_to_storm_where_condition(col, any(*values))
     elif zope_isinstance(status, not_equals):
         return Not(_build_status_clause(col, status.value))
@@ -908,7 +911,10 @@
         # 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:
+        # BugTaskStatus is used internally, BugTaskStatusSearch is used
+        # externally, such as API.
+        if (status == BugTaskStatus.INCOMPLETE
+            or status == BugTaskStatusSearch.INCOMPLETE):
             status = any(*DB_INCOMPLETE_BUGTASK_STATUSES)
         return search_value_to_storm_where_condition(col, status)
     else:

=== modified file 'lib/lp/bugs/model/tests/test_bugtasksearch.py'
--- lib/lp/bugs/model/tests/test_bugtasksearch.py	2012-09-18 04:30:42 +0000
+++ lib/lp/bugs/model/tests/test_bugtasksearch.py	2012-09-20 15:53:24 +0000
@@ -25,6 +25,7 @@
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance,
     BugTaskStatus,
+    BugTaskStatusSearch,
     IBugTaskSet,
     )
 from lp.bugs.interfaces.bugtasksearch import (
@@ -1773,6 +1774,13 @@
             'BugTask.status IN (13, 14)',
             self.searchClause(BugTaskStatus.INCOMPLETE))
 
+    def test_BugTaskStatusSearch_INCOMPLETE_query(self):
+        # BugTaskStatusSearch.INCOMPLETE is treated as
+        # BugTaskStatus.INCOMPLETE.
+        self.assertEqual(
+            'BugTask.status IN (13, 14)',
+            self.searchClause(BugTaskStatusSearch.INCOMPLETE))
+
     def test_negative_query(self):
         # If a negative is requested then the WHERE clause is simply wrapped
         # in a "NOT".
@@ -1801,6 +1809,14 @@
             self.searchClause(
                 any(BugTaskStatus.NEW, BugTaskStatus.INCOMPLETE)))
 
+    def test_any_query_with_BugTaskStatusSearch_INCOMPLETE(self):
+        # BugTaskStatusSearch.INCOMPLETE is treated as
+        # BugTaskStatus.INCOMPLETE.
+        self.assertEqual(
+            'BugTask.status IN (10, 13, 14)',
+            self.searchClause(
+                any(BugTaskStatus.NEW, BugTaskStatusSearch.INCOMPLETE)))
+
     def test_all_query(self):
         # Since status is single-valued, asking for "all" statuses in a set
         # doesn't make any sense.

=== modified file 'lib/lp/bugs/tests/test_searchtasks_webservice.py'
--- lib/lp/bugs/tests/test_searchtasks_webservice.py	2012-09-17 08:15:11 +0000
+++ lib/lp/bugs/tests/test_searchtasks_webservice.py	2012-09-20 15:53:24 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from lp.bugs.interfaces.bugtask import BugTaskStatusSearch
 from lp.registry.enums import InformationType
 from lp.testing import (
     person_logged_in,
@@ -107,6 +108,19 @@
             ValueError, "Unrecognized order_by: u'date_created'",
             response.jsonBody)
 
+    def test_search_incomplete_status_results(self):
+        # The Incomplete status matches Incomplete with response and
+        # Incomplete without response bug tasks.
+        with person_logged_in(self.owner):
+            self.factory.makeBug(
+                target=self.product,
+                status=BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE)
+            self.factory.makeBug(
+                target=self.product,
+                status=BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE)
+        response = self.search("devel", status="Incomplete")
+        self.assertEqual(response['total_size'], 2)
+
 
 class TestGetBugData(TestCaseWithFactory):
     """Tests for the /bugs getBugData operation."""