launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/fix-rasj-and-branches into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1012448 in Launchpad itself: "Revoking access to a branch doesn't remove subscriptions"
  https://bugs.launchpad.net/launchpad/+bug/1012448

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/fix-rasj-and-branches/+merge/114779

While QAing the changes introduced in https://code.launchpad.net/~stevenk/launchpad/teach-rasj-about-branches/+merge/114106, William and I discovered that RASJ misbehaves if you don't pass any branches -- it will correctly deal with bug subscriptions, but then constructs a query that will take approximately three eons to complete on QAS, and then remove all subscriptions for that user from any branch. This is obviously bad.

I have reflowed the run() method to boot so that it won't deal with branches unless it needs to, and same with bugs, as well as sprinkling a branch into the bugs test and a bug into the branches test.

I have also noticed that IBranch.transitionToInformationType() does not create a job, so have added that in along with fixing some lint.
-- 
https://code.launchpad.net/~stevenk/launchpad/fix-rasj-and-branches/+merge/114779
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/fix-rasj-and-branches into lp:launchpad.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-07-13 00:17:32 +0000
+++ lib/lp/code/model/branch.py	2012-07-13 04:58:27 +0000
@@ -2,7 +2,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212,W0141,F0401
-from lp.registry.interfaces.product import IProduct
 
 __metaclass__ = type
 __all__ = [
@@ -145,10 +144,12 @@
     IAccessArtifactSource,
     )
 from lp.registry.interfaces.person import (
-    PersonVisibility,
     validate_person,
     validate_public_person,
     )
+from lp.registry.interfaces.sharingjob import (
+    IRemoveArtifactSubscriptionsJobSource,
+    )
 from lp.registry.model.accesspolicy import (
     AccessPolicyGrant,
     reconcile_access_for_artifact,
@@ -172,6 +173,7 @@
     ArrayAgg,
     ArrayIntersects,
     )
+from lp.services.features import getFeatureFlag
 from lp.services.helpers import shortlist
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
@@ -271,6 +273,13 @@
                 service.ensureAccessGrants(
                     blind_subscribers, who, branches=[self],
                     ignore_permissions=True)
+        flag = 'disclosure.unsubscribe_jobs.enabled'
+        if bool(getFeatureFlag(flag)):
+            # As a result of the transition, some subscribers may no longer
+            # have access to the branch. We need to run a job to remove any
+            # such subscriptions.
+            getUtility(IRemoveArtifactSubscriptionsJobSource).create(
+                who, [self])
 
     registrant = ForeignKey(
         dbName='registrant', foreignKey='Person',

=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py	2012-07-11 01:24:30 +0000
+++ lib/lp/registry/model/sharingjob.py	2012-07-13 04:58:27 +0000
@@ -344,19 +344,13 @@
         logger = logging.getLogger()
         logger.info(self.getOperationDescription())
 
-        # Find all bug subscriptions for which the subscriber cannot see the
-        # bug.
-        bug_invisible_filter = Not(
-            Or(*get_bug_privacy_filter_terms(BugSubscription.person_id)))
-        bug_filters = [bug_invisible_filter]
-        branch_invisible_filter = Not(
-            Or(*get_branch_privacy_filter(BranchSubscription.personID)))
-        branch_filters = [branch_invisible_filter]
+        bug_filters = []
+        branch_filters = []
 
+        if self.branch_ids:
+            branch_filters.append(Branch.id.is_in(self.branch_ids))
         if self.bug_ids:
             bug_filters.append(BugTaskFlat.bug_id.is_in(self.bug_ids))
-        elif self.branch_ids:
-            branch_filters.append(Branch.id.is_in(self.branch_ids))
         else:
             if self.information_types:
                 bug_filters.append(
@@ -385,15 +379,26 @@
                         TeamParticipation.personID,
                         where=TeamParticipation.team == self.grantee)))
 
-        bug_subscriptions = IStore(BugSubscription).using(
-            BugSubscription,
-            Join(BugTaskFlat, BugTaskFlat.bug_id == BugSubscription.bug_id)
-            ).find(BugSubscription, *bug_filters).config(distinct=True)
-        branch_subscriptions = IStore(BranchSubscription).find(
-            BranchSubscription, *branch_filters).config(distinct=True)
-        for sub in bug_subscriptions:
-            sub.bug.unsubscribe(
-                sub.person, self.requestor, ignore_permissions=True)
-        for sub in branch_subscriptions:
-            sub.branch.unsubscribe(
-                sub.person, self.requestor, ignore_permissions=True)
+        if bug_filters:
+            bug_filters.append(Not(
+                Or(*get_bug_privacy_filter_terms(
+                    BugSubscription.person_id))))
+            bug_subscriptions = IStore(BugSubscription).using(
+                BugSubscription,
+                Join(BugTaskFlat,
+                    BugTaskFlat.bug_id == BugSubscription.bug_id)
+                ).find(BugSubscription, *bug_filters).config(distinct=True)
+            for sub in bug_subscriptions:
+                sub.bug.unsubscribe(
+                    sub.person, self.requestor, ignore_permissions=True)
+        if branch_filters:
+            branch_filters.append(Not(
+                Or(*get_branch_privacy_filter(BranchSubscription.personID))))
+            branch_subscriptions = IStore(BranchSubscription).using(
+                BranchSubscription,
+                Join(Branch, Branch.id == BranchSubscription.branchID)
+                ).find(BranchSubscription, *branch_filters).config(
+                    distinct=True)
+            for sub in branch_subscriptions:
+                sub.branch.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-07-10 06:35:48 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2012-07-13 04:58:27 +0000
@@ -293,6 +293,9 @@
         bug = self.factory.makeBug(
             owner=owner, product=product,
             information_type=InformationType.USERDATA)
+        branch = self.factory.makeBranch(
+            owner=owner, product=product,
+            information_type=InformationType.USERDATA)
 
         # The artifact grantees will not lose access when the job is run.
         artifact_indirect_grantee = self.factory.makePerson()
@@ -304,6 +307,9 @@
         bug.subscribe(policy_indirect_grantee, owner)
         bug.subscribe(artifact_team_grantee, owner)
         bug.subscribe(artifact_indirect_grantee, owner)
+        branch.subscribe(artifact_indirect_grantee,
+            BranchSubscriptionNotificationLevel.NOEMAIL, None,
+            CodeReviewNotificationLevel.NOEMAIL, owner)
         # Subscribing policy_team_grantee has created an artifact grant so we
         # need to revoke that to test the job.
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
@@ -332,6 +338,7 @@
         self.assertNotIn(policy_indirect_grantee, subscribers)
         self.assertIn(artifact_team_grantee, subscribers)
         self.assertIn(artifact_indirect_grantee, subscribers)
+        self.assertIn(artifact_indirect_grantee, branch.subscribers)
 
     def _assert_branch_change_unsubscribes(self, change_callback):
         product = self.factory.makeProduct()
@@ -346,6 +353,9 @@
 
         self.factory.makeAccessPolicyGrant(policy, policy_team_grantee, owner)
         login_person(owner)
+        bug = self.factory.makeBug(
+            owner=owner, product=product,
+            information_type=InformationType.USERDATA)
         branch = self.factory.makeBranch(
             owner=owner, product=product,
             information_type=InformationType.USERDATA)
@@ -371,6 +381,7 @@
             artifact_indirect_grantee,
             BranchSubscriptionNotificationLevel.NOEMAIL, None,
             CodeReviewNotificationLevel.NOEMAIL, owner)
+        bug.subscribe(artifact_indirect_grantee, owner)
         # Subscribing policy_team_grantee has created an artifact grant so we
         # need to revoke that to test the job.
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
@@ -398,6 +409,7 @@
         self.assertNotIn(policy_indirect_grantee, branch.subscribers)
         self.assertIn(artifact_team_grantee, branch.subscribers)
         self.assertIn(artifact_indirect_grantee, branch.subscribers)
+        self.assertIn(artifact_indirect_grantee, bug.getDirectSubscribers())
 
     def test_change_information_type_branch(self):
         def change_information_type(branch):