launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/observer-merging/+merge/82952

Building on the recently landed access policy model, this branch adds support to the branch deleter and person merger. Once these this lands, lp:~wgrant/launchpad/observer-db-2 can land to add the foreign key constraints.

This 4-stage process is necessary due to the requirement that schema changes be deployable without code changes.
-- 
https://code.launchpad.net/~wgrant/launchpad/observer-merging/+merge/82952
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/observer-merging into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-11-17 03:30:34 +0000
+++ database/schema/security.cfg	2011-11-22 00:22:24 +0000
@@ -1980,6 +1980,7 @@
 
 [person-merge-job]
 groups=script
+public.accesspolicygrant                = SELECT, UPDATE, DELETE
 public.account                          = SELECT, UPDATE
 public.announcement                     = SELECT, UPDATE
 public.answercontact                    = SELECT, UPDATE, DELETE

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2011-09-27 00:57:13 +0000
+++ lib/lp/code/model/branch.py	2011-11-22 00:22:24 +0000
@@ -141,6 +141,7 @@
     )
 from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
 from lp.codehosting.safe_open import safe_open
+from lp.registry.interfaces.accesspolicy import IAccessPolicyArtifactSource
 from lp.registry.interfaces.person import (
     validate_person,
     validate_public_person,
@@ -1125,6 +1126,7 @@
 
         self._deleteBranchSubscriptions()
         self._deleteJobs()
+        getUtility(IAccessPolicyArtifactSource).delete(self)
 
         # Now destroy the branch.
         branch_id = self.id

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2011-11-14 20:14:44 +0000
+++ lib/lp/code/model/tests/test_branch.py	2011-11-22 00:22:24 +0000
@@ -114,6 +114,7 @@
 from lp.codehosting.safe_open import BadUrl
 from lp.registry.interfaces.person import PersonVisibility
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.model.accesspolicy import AccessPolicyArtifact
 from lp.registry.model.sourcepackage import SourcePackage
 from lp.services.osutils import override_environ
 from lp.services.propertycache import clear_property_cache
@@ -1138,6 +1139,12 @@
             branch.canBeDeleted(), True,
             "A branch that has a import is deletable.")
 
+    def test_accessPolicyArtifactDoesntDisableDeletion(self):
+        """A branch referenced by an AccessPolicyArtifact can be deleted."""
+        artifact = self.factory.makeAccessPolicyArtifact(self.branch)
+        self.factory.makeAccessPolicyGrant(object=artifact)
+        self.assertEqual(True, self.branch.canBeDeleted())
+
     def test_bugBranchLinkDisablesDeletion(self):
         """A branch linked to a bug cannot be deleted."""
         params = CreateBugParams(
@@ -1272,6 +1279,16 @@
             BuildQueue, BuildQueue.job == other_job.job)
         self.assertNotEqual([], list(other_buildqueue))
 
+    def test_AccessPolicyArtifact_deleted(self):
+        # Any AccessPolicyArtifact referencing the branch is removed.
+        branch = self.factory.makeAnyBranch()
+        artifact = self.factory.makeAccessPolicyArtifact(branch)
+        self.factory.makeAccessPolicyGrant(object=artifact)
+        store = Store.of(branch)
+        branch.destroySelf()
+        self.assertContentEqual(
+            [], store.find(AccessPolicyArtifact, branch=branch))
+
     def test_createsJobToReclaimSpace(self):
         # When a branch is deleted from the database, a job to remove the
         # branch from disk as well.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-11-14 12:23:59 +0000
+++ lib/lp/registry/model/person.py	2011-11-22 00:22:24 +0000
@@ -3656,6 +3656,31 @@
         skip.append(
             (decorator_table.lower(), person_pointer_column.lower()))
 
+    def _mergeAccessPolicyGrant(self, cur, from_id, to_id):
+        # Update only the AccessPolicyGrants that will not conflict.
+        cur.execute('''
+            UPDATE AccessPolicyGrant
+            SET grantee=%(to_id)d
+            WHERE grantee = %(from_id)d AND (
+                policy NOT IN
+                    (
+                    SELECT policy
+                    FROM AccessPolicyGrant
+                    WHERE grantee = %(to_id)d
+                    )
+                OR artifact NOT IN
+                    (
+                    SELECT artifact
+                    FROM AccessPolicyGrant
+                    WHERE grantee = %(to_id)d
+                    )
+                )
+            ''' % vars())
+        # and delete those left over.
+        cur.execute('''
+            DELETE FROM AccessPolicyGrant WHERE grantee = %(from_id)d
+            ''' % vars())
+
     def _mergeBranches(self, from_person, to_person):
         # This shouldn't use removeSecurityProxy.
         branches = getUtility(IBranchCollection).ownedBy(from_person)
@@ -4209,6 +4234,9 @@
             % vars())
         skip.append(('gpgkey', 'owner'))
 
+        self._mergeAccessPolicyGrant(cur, from_id, to_id)
+        skip.append(('accesspolicygrant', 'grantee'))
+
         # Update the Branches that will not conflict, and fudge the names of
         # ones that *do* conflict.
         self._mergeBranches(from_person, to_person)

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2011-11-21 04:08:34 +0000
+++ lib/lp/registry/tests/test_person.py	2011-11-22 00:22:24 +0000
@@ -61,6 +61,7 @@
 from lp.registry.interfaces.personnotification import IPersonNotificationSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.product import IProductSet
+from lp.registry.model.accesspolicy import AccessPolicyGrant
 from lp.registry.model.karma import (
     KarmaCategory,
     KarmaTotalCache,
@@ -1185,6 +1186,41 @@
         dsp = self.factory.makeDistributionSourcePackage()
         self.assertConflictingSubscriptionDeletes(dsp)
 
+    def test_merge_accesspolicygrants(self):
+        # AccessPolicyGrants are transferred from the duplicate.
+        person = self.factory.makePerson()
+        grant = self.factory.makeAccessPolicyGrant()
+        self._do_premerge(grant.grantee, person)
+        with person_logged_in(person):
+            self._do_merge(grant.grantee, person)
+        self.assertEqual(person, grant.grantee)
+
+    def test_merge_accesspolicygrants_conflicts(self):
+        # Conflicting AccessPolicyGrants are deleted.
+        policy = self.factory.makeAccessPolicy()
+
+        person = self.factory.makePerson()
+        person_grantor = self.factory.makePerson()
+        person_grant = self.factory.makeAccessPolicyGrant(
+            grantee=person, grantor=person_grantor, object=policy)
+
+        duplicate = self.factory.makePerson()
+        duplicate_grantor = self.factory.makePerson()
+        duplicate_grant = self.factory.makeAccessPolicyGrant(
+            grantee=duplicate, grantor=duplicate_grantor, object=policy)
+
+        self._do_premerge(duplicate, person)
+        with person_logged_in(person):
+            self._do_merge(duplicate, person)
+        transaction.commit()
+
+        self.assertEqual(person, person_grant.grantee)
+        self.assertEqual(person_grantor, person_grant.grantor)
+        self.assertIs(
+            None,
+            IStore(AccessPolicyGrant).get(
+                AccessPolicyGrant, duplicate_grant.id))
+
     def test_mergeAsync(self):
         # mergeAsync() creates a new `PersonMergeJob`.
         from_person = self.factory.makePerson()