launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/improved-bugremovesubscriptions-job into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/improved-bugremovesubscriptions-job/+merge/110480

This branch makes two quick fixes to RemoveBugSubscriptionsJob:

 - Change the core query to JOIN BugTaskFlat rather than using an IN subquery. Much more optimisable.
 - Improve the operation description and repr to include all relevant information, and use unique names rather than displaynames.
-- 
https://code.launchpad.net/~wgrant/launchpad/improved-bugremovesubscriptions-job/+merge/110480
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/improved-bugremovesubscriptions-job into lp:launchpad.

=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py	2012-06-15 02:27:19 +0000
+++ lib/lp/registry/model/sharingjob.py	2012-06-15 07:23:30 +0000
@@ -24,6 +24,7 @@
 from storm.expr import (
     And,
     In,
+    Join,
     Not,
     Or,
     Select,
@@ -168,15 +169,9 @@
         self.context = job
 
     def __repr__(self):
-        if self.grantee:
-            return '<%(job_type)s job for %(grantee)s and %(pillar)s>' % {
-                'job_type': self.context.job_type.name,
-                'grantee': self.grantee.displayname,
-                'pillar': self.pillar_text,
-                }
-        else:
-            return '<%(job_type)s job>' % {
-                'job_type': self.context.job_type.name,
+        return '<%(job_type)s job %(desc)s>' % {
+            'job_type': self.context.job_type.name,
+            'desc': self.getOperationDescription(),
             }
 
     @property
@@ -305,7 +300,16 @@
         return list(result)
 
     def getOperationDescription(self):
-        return 'removing subscriptions for bugs %s' % self.bug_ids
+        info = {
+            'information_types': [t.name for t in self.information_types],
+            'requestor': self.requestor.name,
+            'bug_ids': self.bug_ids,
+            'pillar': getattr(self.pillar, 'name', None),
+            'grantee': getattr(self.grantee, 'name', None)
+            }
+        return (
+            'reconciling subscriptions for %s' % ', '.join(
+                '%s=%s' % (k, v) for (k, v) in sorted(info.items()) if v))
 
     def run(self):
         """See `IRemoveBugSubscriptionsJob`."""
@@ -332,20 +336,16 @@
                 filters.append(
                     BugTaskFlat.distribution == self.distro)
 
-        subscriptions_filter = [
-            In(BugSubscription.bug_id,
-                Select(BugTaskFlat.bug_id, where=And(*filters)))]
         if self.grantee:
-            subscriptions_filter.append(
+            filters.append(
                 In(BugSubscription.person_id,
                     Select(
                         TeamParticipation.personID,
-                        where=TeamParticipation.team == self.grantee))
-            )
-        subscriptions = IStore(BugSubscription).find(
+                        where=TeamParticipation.team == self.grantee)))
+        subscriptions = IStore(BugSubscription).using(
             BugSubscription,
-            *subscriptions_filter
-        )
+            Join(BugTaskFlat, BugTaskFlat.bug_id == BugSubscription.bug_id)
+            ).find(BugSubscription, *filters).config(distinct=True)
         for sub in subscriptions:
             sub.bug.unsubscribe(
                 sub.person, self.requestor, ignore_permissions=True)

=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py	2012-06-15 00:42:38 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2012-06-15 07:23:30 +0000
@@ -91,16 +91,18 @@
     layer = DatabaseFunctionalLayer
 
     def _makeJob(self):
-        bug = self.factory.makeBug()
-        requestor = self.factory.makePerson()
+        self.bug = self.factory.makeBug()
+        self.requestor = self.factory.makePerson()
         job = getUtility(IRemoveBugSubscriptionsJobSource).create(
-            requestor, bugs=[bug])
+            self.requestor, bugs=[self.bug])
         return job
 
     def test_repr(self):
         job = self._makeJob()
         self.assertEqual(
-            '<REMOVE_BUG_SUBSCRIPTIONS job>',
+            '<REMOVE_BUG_SUBSCRIPTIONS job reconciling subscriptions for '
+            'bug_ids=[%d], requestor=%s>'
+            % (self.bug.id, self.requestor.name),
             repr(job))
 
     def test_create_success(self):