launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-preload-deleted-workitems into lp:launchpad.

Requested reviews:
  William Grant (wgrant): code
Related bugs:
  Bug #1215749 in Launchpad itself: "Work items preloading for specifications is too overzealous"
  https://bugs.launchpad.net/launchpad/+bug/1215749

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-preload-deleted-workitems/+merge/181708

Only preload non-deleted workitems in search_specifications.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-preload-deleted-workitems/+merge/181708
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/blueprints/model/specificationsearch.py'
--- lib/lp/blueprints/model/specificationsearch.py	2013-08-20 07:24:24 +0000
+++ lib/lp/blueprints/model/specificationsearch.py	2013-08-23 05:44:31 +0000
@@ -121,7 +121,8 @@
                 get_property_cache(spec).linked_branches = []
         if need_workitems:
             work_items = load_referencing(
-                SpecificationWorkItem, rows, ['specification_id'])
+                SpecificationWorkItem, rows, ['specification_id'],
+                extra_conditions=[SpecificationWorkItem.deleted == False])
             for workitem in work_items:
                 person_ids.add(workitem.assignee_id)
                 work_items_by_spec[workitem.specification_id].append(workitem)

=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py	2013-08-19 06:43:04 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2013-08-23 05:44:31 +0000
@@ -345,6 +345,18 @@
                 spec.workitems_text
         self.assertThat(recorder, HasQueryCount(Equals(4)))
 
+    def test_valid_specifications_preloading_excludes_deleted_workitems(self):
+        distroseries = self.factory.makeDistroSeries()
+        spec = self.factory.makeSpecification(
+            distribution=distroseries.distribution, goal=distroseries)
+        self.factory.makeSpecificationWorkItem(
+            specification=spec, deleted=True)
+        self.factory.makeSpecificationWorkItem(specification=spec)
+        workitems = [
+            s.workitems_text
+            for s in distroseries.api_valid_specifications]
+        self.assertContentEqual([spec.workitems_text], workitems)
+
 
 class TestDistroSeriesPackaging(TestCaseWithFactory):
 

=== modified file 'lib/lp/services/database/bulk.py'
--- lib/lp/services/database/bulk.py	2013-06-20 05:50:00 +0000
+++ lib/lp/services/database/bulk.py	2013-08-23 05:44:31 +0000
@@ -117,7 +117,8 @@
     return list(store.find(object_type, condition))
 
 
-def load_referencing(object_type, owning_objects, reference_keys):
+def load_referencing(object_type, owning_objects, reference_keys,
+                     extra_conditions=[]):
     """Load objects of object_type that reference owning_objects.
 
     Note that complex types like Person are best loaded through dedicated
@@ -130,6 +131,8 @@
         constraint could be lifted in future.
     :param reference_keys: A list of attributes that should be used to select
         object_type keys. e.g. ['branchID']
+    :param extra_conditions: A list of Storm clauses that will be used in the
+        final query.
     :return: A list of object_type where any of reference_keys refered to the
         primary key of any of owning_objects.
     """
@@ -147,7 +150,7 @@
     # clause.
     for column in map(partial(getattr, object_type), reference_keys):
         conditions.append(column.is_in(ids))
-    return list(store.find(object_type, Or(conditions)))
+    return list(store.find(object_type, Or(conditions), *extra_conditions))
 
 
 def load_related(object_type, owning_objects, foreign_keys):