launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1043319 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1043319 in Launchpad itself: "UnusedSharingPolicyPruner doesn't preserve permitted access policies"
  https://bugs.launchpad.net/launchpad/+bug/1043319

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1043319/+merge/121874

UnusedSharingPolicyPruner is meant to keep any access policies that are in use by artifacts, or allowed by the project sharing policy. The first bit works fine, but the sharing policy bit is buggy: it calculates the set difference between the extant access policies and the allowed types, but those are different types of object. It needs to instead find the access policies that have types not in the allowed set.

I rewrote the test to verify all three cases (deleted, kept by sharing policy, and kept by artifact), and renamed the pruner to UnusedAccessPolicyPruner to more accurately reflect its true purpose.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1043319/+merge/121874
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1043319 into lp:launchpad.

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-08-28 15:29:23 +0000
+++ lib/lp/scripts/garbo.py	2012-08-29 14:22:30 +0000
@@ -1067,13 +1067,13 @@
         transaction.commit()
 
 
-class UnusedSharingPolicyPruner(TunableLoop):
+class UnusedAccessPolicyPruner(TunableLoop):
     """Deletes unused AccessPolicy and AccessPolicyGrants for products."""
 
     maximum_chunk_size = 5000
 
     def __init__(self, log, abort_time=None):
-        super(UnusedSharingPolicyPruner, self).__init__(log, abort_time)
+        super(UnusedAccessPolicyPruner, self).__init__(log, abort_time)
         self.start_at = 1
         self.store = IMasterStore(Product)
 
@@ -1087,23 +1087,23 @@
     def __call__(self, chunk_size):
         products = list(self.findProducts()[:chunk_size])
         for product in products:
-            allowed_bug_policies = set(
+            allowed_bug_types = set(
                 BUG_POLICY_ALLOWED_TYPES.get(
                     product.bug_sharing_policy, FREE_INFORMATION_TYPES))
-            allowed_branch_policies = set(
+            allowed_branch_types = set(
                 BRANCH_POLICY_ALLOWED_TYPES.get(
                     product.branch_sharing_policy, FREE_INFORMATION_TYPES))
+            allowed_types = allowed_bug_types.union(allowed_branch_types)
             # Fetch all APs, and after filtering out ones that are forbidden
             # by the bug and branch policies, the APs that have no APAs are
             # unused and can be deleted.
             access_policies = set(
                 getUtility(IAccessPolicySource).findByPillar([product]))
-            candidate_aps = access_policies.difference(
-                allowed_bug_policies, allowed_branch_policies)
             apa_source = getUtility(IAccessPolicyArtifactSource)
             unused_aps = [
-                ap for ap in candidate_aps
-                if apa_source.findByPolicy([ap]).is_empty()]
+                ap for ap in access_policies
+                if ap.type not in allowed_types
+                and apa_source.findByPolicy([ap]).is_empty()]
             getUtility(IAccessPolicyGrantSource).revokeByPolicy(unused_aps)
             for ap in unused_aps:
                 self.store.remove(ap)
@@ -1398,8 +1398,8 @@
         SuggestiveTemplatesCacheUpdater,
         POTranslationPruner,
         UnlinkedAccountPruner,
+        UnusedAccessPolicyPruner,
         UnusedPOTMsgSetPruner,
-        UnusedSharingPolicyPruner,
         ]
     experimental_tunable_loops = [
         PersonPruner,

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2012-08-28 15:29:23 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2012-08-29 14:22:30 +0000
@@ -58,6 +58,7 @@
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
+    InformationType,
     )
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import IPersonSet
@@ -1084,20 +1085,39 @@
             self.assertEqual(
                 BugSharingPolicy.PUBLIC, product.bug_sharing_policy)
 
-    def test_UnusedSharingPolicyPruner(self):
-        # UnusedSharingPolicyPruner destroys the unused sharing details.
+    def getAccessPolicyTypes(self, pillar):
+        return [
+            ap.type
+            for ap in getUtility(IAccessPolicySource).findByPillar([pillar])]
+
+    def test_UnusedAccessPolicyPruner(self):
+        # UnusedAccessPolicyPruner removes access policies that aren't
+        # in use by artifacts or allowed by the project sharing policy.
         switch_dbuser('testadmin')
-        product = self.factory.makeProduct(
-            branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
-        ap = self.factory.makeAccessPolicy(pillar=product)
+        product = self.factory.makeProduct()
+        self.factory.makeCommercialSubscription(product=product)
+        with person_logged_in(product.owner):
+            product.setBugSharingPolicy(BugSharingPolicy.PROPRIETARY)
+            product.setBranchSharingPolicy(BranchSharingPolicy.PROPRIETARY)
+        [ap] = getUtility(IAccessPolicySource).find(
+            [(product, InformationType.PRIVATESECURITY)])
         self.factory.makeAccessPolicyArtifact(policy=ap)
-        self.factory.makeAccessPolicyGrant(policy=ap)
-        all_aps = getUtility(IAccessPolicySource).findByPillar([product])
-        # There are 3 because two are created implicitly when the product is.
-        self.assertEqual(3, all_aps.count())
+
+        # Private and Private Security were created with the project.
+        # Proprietary was created when the branch sharing policy was set.
+        self.assertContentEqual(
+            [InformationType.PRIVATESECURITY, InformationType.USERDATA,
+             InformationType.PROPRIETARY],
+            self.getAccessPolicyTypes(product))
+
         self.runDaily()
-        all_aps = getUtility(IAccessPolicySource).findByPillar([product])
-        self.assertEqual([ap], list(all_aps))
+
+        # Proprietary is permitted by the sharing policy, and there's a
+        # Private Security artifact. But Private isn't in use or allowed
+        # by a sharing policy, so garbo deleted it.
+        self.assertContentEqual(
+            [InformationType.PRIVATESECURITY, InformationType.PROPRIETARY],
+            self.getAccessPolicyTypes(product))
 
 
 class TestGarboTasks(TestCaseWithFactory):