Merge lp:~wgrant/launchpad/better-createManyTasks into lp:launchpad

[William Grant]

Rewrite BugTask.createManyTasks to be a bit cleaner, and reimplement createTask on top of it.

BugTask.createManyTasks was added yesterday to more efficiently approve distroseries nominations. But it's pretty badly duplicated with createTask, and roughly twice as long as it needs to be.

This branch rewrites both of them to share code, be cleaner, and be slightly faster.

[Ian Booth (wallyworld)]

Looks ok to me. Net LOC reduction as well.

review: Approve

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2012-05-10 13:13:05 +0000
+++ lib/lp/bugs/interfaces/bug.py	2012-05-11 02:47:18 +0000
@@ -779,9 +779,6 @@
     def removeWatch(bug_watch, owner):
         """Remove a bug watch from the bug."""
 
-    def addManyTasks(owners, targets):
-        """Create multiple bug tasks on this bug."""
-
     @call_with(owner=REQUEST_USER)
     @operation_parameters(target=copy_field(IBugTask['target']))
     @export_factory_operation(IBugTask, [])

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-05-10 17:18:14 +0000
+++ lib/lp/bugs/model/bug.py	2012-05-11 02:47:18 +0000
@@ -1237,10 +1237,6 @@
         """See `IBug`."""
         return getUtility(IBugTaskSet).createTask(self, owner, target)
 
-    def addManyTasks(self, owner, targets):
-        """See `IBug`."""
-        return getUtility(IBugTaskSet).createManyTasks(self, owner, targets)
-
     def addWatch(self, bugtracker, remotebug, owner):
         """See `IBug`."""
         # We shouldn't add duplicate bug watches.

=== modified file 'lib/lp/bugs/model/bugnomination.py'
--- lib/lp/bugs/model/bugnomination.py	2012-05-10 13:13:05 +0000
+++ lib/lp/bugs/model/bugnomination.py	2012-05-11 02:47:18 +0000
@@ -22,6 +22,7 @@
     ForeignKey,
     SQLObjectNotFound,
     )
+from zope.component import getUtility
 from zope.interface import implements
 
 from lp.app.errors import NotFoundError
@@ -32,6 +33,7 @@
     IBugNomination,
     IBugNominationSet,
     )
+from lp.bugs.interfaces.bugtask import IBugTaskSet
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
@@ -91,7 +93,7 @@
                     targets.append(distroseries)
         else:
             targets.append(self.productseries)
-        bugtasks = self.bug.addManyTasks(approver, targets)
+        bugtasks = getUtility(IBugTaskSet).createManyTasks(self.bug, approver, targets)
         for bug_task in bugtasks:
             self.bug.addChange(BugTaskAdded(UTC_NOW, approver, bug_task))
 

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2012-05-10 23:14:38 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-05-11 02:47:18 +0000
@@ -1611,109 +1611,64 @@
         params = BugTaskSearchParams(user, **kwargs)
         return self.search(params)
 
-    def _initNewTask(self, bug, owner, target, status, importance, assignee,
-                       milestone):
-        if not status:
+    def createManyTasks(self, bug, owner, targets, status=None,
+                        importance=None, assignee=None, milestone=None):
+        """See `IBugTaskSet`."""
+        if status is None:
             status = IBugTask['status'].default
-        if not importance:
+        if importance is None:
             importance = IBugTask['importance'].default
-        if not assignee:
-            assignee = None
-        if not milestone:
-            milestone = None
-
-        # Make sure there's no task for this bug already filed
-        # against the target.
-        validate_new_target(bug, target)
-
-        target_key = bug_target_to_key(target)
-        if not bug.private and bug.security_related:
-            product = target_key['product']
-            distribution = target_key['distribution']
-            if product and product.security_contact:
-                bug.subscribe(product.security_contact, owner)
-            elif distribution and distribution.security_contact:
-                bug.subscribe(distribution.security_contact, owner)
-
-        non_target_create_params = dict(
-            bug=bug,
-            _status=status,
-            importance=importance,
-            assignee=assignee,
-            owner=owner,
-            milestone=milestone)
-        create_params = non_target_create_params.copy()
-        create_params.update(target_key)
-        return create_params, non_target_create_params
-
-    def createManyTasks(self, bug, owner, targets,
-                        status=IBugTask['status'].default,
-                        importance=IBugTask['importance'].default,
-                        assignee=None, milestone=None):
-        """See `IBugTaskSet`."""
-        params = [self._initNewTask(bug, owner, target, status, importance,
-            assignee, milestone) for target in targets]
-
-        fieldnames = (
-            'assignee',
-            'bug',
-            'distribution',
-            'distroseries',
-            'importance',
-            'milestone',
-            'owner',
-            'product',
-            'productseries',
-            'sourcepackagename',
-            '_status',
-        )
-
-        fields = [getattr(BugTask, field) for field in fieldnames]
-        # Values need to be a list of tuples in the order we're declaring our
-        # fields.
-        values = [[p[0].get(field) for field in fieldnames] for p in params]
-
-        taskset = create(fields, values, get_objects=True)
+        target_keys = []
+        pillars = set()
+        for target in targets:
+            validate_new_target(bug, target)
+            pillars.add(target.pillar)
+            target_keys.append(bug_target_to_key(target))
+        if bug.information_type == InformationType.UNEMBARGOEDSECURITY:
+            for pillar in pillars:
+                if pillar.security_contact:
+                    bug.subscribe(pillar.security_contact, owner)
+
+        values = [
+            (bug, owner, key['product'], key['productseries'],
+             key['distribution'], key['distroseries'],
+             key['sourcepackagename'], status, importance, assignee,
+             milestone)
+            for key in target_keys]
+        tasks = create(
+            (BugTask.bug, BugTask.owner, BugTask.product,
+             BugTask.productseries, BugTask.distribution,
+             BugTask.distroseries, BugTask.sourcepackagename, BugTask._status,
+             BugTask.importance, BugTask.assignee, BugTask.milestone),
+            values, get_objects=True)
+
         del get_property_cache(bug).bugtasks
-        for bugtask in taskset:
+        for bugtask in tasks:
             bugtask.updateTargetNameCache()
             if bugtask.conjoined_slave:
                 bugtask._syncFromConjoinedSlave()
-        return taskset
+        return tasks
 
-    def createTask(self, bug, owner, target,
-                   status=IBugTask['status'].default,
-                   importance=IBugTask['importance'].default,
+    def createTask(self, bug, owner, target, status=None, importance=None,
                    assignee=None, milestone=None):
         """See `IBugTaskSet`."""
-        create_params, non_target_create_params = self._initNewTask(bug,
-            owner, target, status, importance, assignee, milestone)
-        bugtask = BugTask(**create_params)
-
-        if create_params['distribution']:
-            # Create tasks for accepted nominations if this is a source
-            # package addition.
-            accepted_nominations = [
-                nomination for nomination in
-                bug.getNominations(create_params['distribution'])
-                if nomination.isApproved()]
-            for nomination in accepted_nominations:
-                accepted_series_task = BugTask(
-                    distroseries=nomination.distroseries,
-                    sourcepackagename=create_params['sourcepackagename'],
-                    **non_target_create_params)
-                accepted_series_task.updateTargetNameCache()
-
-        if bugtask.conjoined_slave:
-            bugtask._syncFromConjoinedSlave()
-
-        bugtask.updateTargetNameCache()
-        del get_property_cache(bug).bugtasks
-        # Because of block_implicit_flushes, it is possible for a new bugtask
-        # to be queued in appropriately, which leads to Bug.bugtasks not
-        # finding the bugtask.
-        Store.of(bugtask).flush()
-        return bugtask
+        # Create tasks for accepted nominations if this is a source
+        # package addition. Distribution nominations are for all the
+        # tasks.
+        targets = [target]
+        key = bug_target_to_key(target)
+        if key['distribution'] is not None:
+            for nomination in bug.getNominations(key['distribution']):
+                if not nomination.isApproved():
+                    continue
+                targets.append(
+                    nomination.distroseries.getSourcePackage(
+                        key['sourcepackagename']))
+
+        tasks = self.createManyTasks(
+            bug, owner, targets, status=status, importance=importance,
+            assignee=assignee, milestone=milestone)
+        return [task for task in tasks if task.target == target][0]
 
     def getStatusCountsForProductSeries(self, user, product_series):
         """See `IBugTaskSet`."""