launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/teach-rasj-about-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/teach-rasj-about-branches/+merge/114106

Teach RemoveArtifactSubscriptionsJob about branches.
-- 
https://code.launchpad.net/~stevenk/launchpad/teach-rasj-about-branches/+merge/114106
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/teach-rasj-about-branches into lp:launchpad.

=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py	2012-06-29 02:27:31 +0000
+++ lib/lp/registry/model/sharingjob.py	2012-07-10 06:43:24 +0000
@@ -41,10 +41,20 @@
     implements,
     )
 
-from lp.bugs.interfaces.bug import IBugSet
+from lp.bugs.interfaces.bug import (
+    IBug,
+    IBugSet,
+    )
 from lp.bugs.model.bugsubscription import BugSubscription
 from lp.bugs.model.bugtaskflat import BugTaskFlat
 from lp.bugs.model.bugtasksearch import get_bug_privacy_filter_terms
+from lp.code.interfaces.branch import IBranch
+from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.code.model.branch import (
+    Branch,
+    get_branch_privacy_filter,
+    )
+from lp.code.model.branchsubscription import BranchSubscription
 from lp.registry.enums import InformationType
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProduct
@@ -253,12 +263,20 @@
                information_types=None):
         """See `IRemoveArtifactSubscriptionsJob`."""
 
-        bug_ids = [bug.id for bug in artifacts or []]
+        bug_ids = []
+        branch_ids = []
+        if artifacts:
+            for artifact in artifacts:
+                if IBug.providedBy(artifact):
+                    bug_ids.append(artifact.id)
+                elif IBranch.providedBy(artifact):
+                    branch_ids.append(artifact.id)
         information_types = [
             info_type.value for info_type in information_types or []
         ]
         metadata = {
             'bug_ids': bug_ids,
+            'branch_ids': branch_ids,
             'information_types': information_types,
             'requestor.id': requestor.id
         }
@@ -284,6 +302,16 @@
         return getUtility(IBugSet).getByNumbers(self.bug_ids)
 
     @property
+    def branch_ids(self):
+        if not 'branch_ids' in self.metadata:
+            return []
+        return self.metadata['branch_ids']
+
+    @property
+    def branches(self):
+        return [getUtility(IBranchLookup).get(id) for id in self.branch_ids]
+
+    @property
     def information_types(self):
         if not 'information_types' in self.metadata:
             return []
@@ -307,6 +335,7 @@
             'information_types': [t.name for t in self.information_types],
             'requestor': self.requestor.name,
             'bug_ids': self.bug_ids,
+            'branch_ids': self.branch_ids,
             'pillar': getattr(self.pillar, 'name', None),
             'grantee': getattr(self.grantee, 'name', None)
             }
@@ -321,33 +350,54 @@
 
         # Find all bug subscriptions for which the subscriber cannot see the
         # bug.
-        invisible_filter = (
-            Not(Or(*get_bug_privacy_filter_terms(BugSubscription.person_id))))
-        filters = [invisible_filter]
+        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]
 
         if self.bug_ids:
-            filters.append(BugTaskFlat.bug_id.is_in(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:
-                filters.append(
-                    BugTaskFlat.information_type.is_in(self.information_types))
+                bug_filters.append(
+                    BugTaskFlat.information_type.is_in(
+                        self.information_types))
+                branch_filters.append(
+                    Branch.information_type.is_in(self.information_types))
             if self.product:
-                filters.append(
+                bug_filters.append(
                     BugTaskFlat.product == self.product)
+                branch_filters.append(Branch.product == self.product)
             if self.distro:
-                filters.append(
+                bug_filters.append(
                     BugTaskFlat.distribution == self.distro)
+                branch_filters.append(Branch.distribution == self.distro)
 
         if self.grantee:
-            filters.append(
+            bug_filters.append(
                 In(BugSubscription.person_id,
                     Select(
                         TeamParticipation.personID,
                         where=TeamParticipation.team == self.grantee)))
-        subscriptions = IStore(BugSubscription).using(
+            branch_filters.append(
+                In(BranchSubscription.person_id,
+                    Select(
+                        TeamParticipation.personID,
+                        where=TeamParticipation.team == self.grantee)))
+
+        bug_subscriptions = IStore(BugSubscription).using(
             BugSubscription,
             Join(BugTaskFlat, BugTaskFlat.bug_id == BugSubscription.bug_id)
-            ).find(BugSubscription, *filters).config(distinct=True)
-        for sub in subscriptions:
+            ).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)

=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py	2012-06-29 02:15:05 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2012-07-10 06:43:24 +0000
@@ -101,7 +101,7 @@
             self.requestor, artifacts=[self.bug])
         return job
 
-    def test_repr(self):
+    def test_repr_bugs(self):
         job = self._makeJob()
         self.assertEqual(
             '<REMOVE_ARTIFACT_SUBSCRIPTIONS job reconciling subscriptions '
@@ -109,6 +109,16 @@
             % (self.bug.id, self.requestor.name),
             repr(job))
 
+    def test_repr_branches(self):
+        requestor = self.factory.makePerson()
+        branch = self.factory.makeBranch()
+        job = getUtility(IRemoveArtifactSubscriptionsJobSource).create(
+            requestor, artifacts=[branch])
+        self.assertEqual(
+            '<REMOVE_ARTIFACT_SUBSCRIPTIONS job reconciling subscriptions '
+            'for branch_ids=[%d], requestor=%s>'
+            % (branch.id, requestor.name), repr(job))
+
     def test_create_success(self):
         # Create an instance of SharingJobDerived that delegates to SharingJob.
         self.assertIs(True, ISharingJobSource.providedBy(SharingJobDerived))
@@ -323,6 +333,79 @@
         self.assertIn(artifact_team_grantee, subscribers)
         self.assertIn(artifact_indirect_grantee, subscribers)
 
+    def _assert_branch_change_unsubscribes(self, change_callback):
+        product = self.factory.makeProduct()
+        owner = self.factory.makePerson()
+        [policy] = getUtility(IAccessPolicySource).find(
+            [(product, InformationType.USERDATA)])
+        # The policy grantees will lose access.
+        policy_indirect_grantee = self.factory.makePerson()
+        policy_team_grantee = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED,
+            members=[policy_indirect_grantee])
+
+        self.factory.makeAccessPolicyGrant(policy, policy_team_grantee, owner)
+        login_person(owner)
+        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()
+        artifact_team_grantee = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED,
+            members=[artifact_indirect_grantee])
+
+        branch.subscribe(
+            policy_team_grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
+            None, CodeReviewNotificationLevel.NOEMAIL, owner)
+        branch.subscribe(
+            policy_indirect_grantee,
+            BranchSubscriptionNotificationLevel.NOEMAIL, None,
+            CodeReviewNotificationLevel.NOEMAIL, owner)
+        branch.subscribe(
+            artifact_team_grantee,
+            BranchSubscriptionNotificationLevel.NOEMAIL, None,
+            CodeReviewNotificationLevel.NOEMAIL, 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(
+            getUtility(IAccessArtifactSource).find(
+                [branch]), [policy_team_grantee])
+
+        # policy grantees are subscribed because the job has not been run yet.
+        #subscribers = removeSecurityProxy(branch).subscribers
+        self.assertIn(policy_team_grantee, branch.subscribers)
+        self.assertIn(policy_indirect_grantee, branch.subscribers)
+
+        # Change branch attributes so that it can become inaccessible for
+        # some users.
+        change_callback(branch)
+        reconcile_access_for_artifact(
+            branch, branch.information_type, [branch.product])
+
+        getUtility(IRemoveArtifactSubscriptionsJobSource).create(
+            owner, [branch])
+        with block_on_job(self):
+            transaction.commit()
+
+        # Check the result. Policy grantees will be unsubscribed.
+        self.assertNotIn(policy_team_grantee, branch.subscribers)
+        self.assertNotIn(policy_indirect_grantee, branch.subscribers)
+        self.assertIn(artifact_team_grantee, branch.subscribers)
+        self.assertIn(artifact_indirect_grantee, branch.subscribers)
+
+    def test_change_information_type_branch(self):
+        def change_information_type(branch):
+            removeSecurityProxy(branch).information_type = (
+                InformationType.EMBARGOEDSECURITY)
+
+        self._assert_branch_change_unsubscribes(change_information_type)
+
     def test_change_information_type(self):
         # Changing the information type of a bug unsubscribes users who can no
         # longer see the bug.