launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/merge-and-private-branches into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #918754 in Launchpad itself: "cannot delete/merge a team with private branches"
  https://bugs.launchpad.net/launchpad/+bug/918754

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/merge-and-private-branches/+merge/115229

I tried to delete a team that had private branches in projects with
branch visibility rules. Merge transfers artefacts that must have
owners, such as as branches to ~registry. Merge oopses because ~registry
does not have a policy for the project.

While this bug will disappear when BVP is removed. We do not want
~registry to own private branches. The delete page must explain that the
team cannot be deleted until the team's private branches are deleted or
the ownership is transferred to another person.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Find or write a method that can lookup a person's private branches
      without being constrained by viability.
    * Update delete and merge page to explain that Launchpad does not know
      what to do with the team's private branches. Someone must either change
      the owner of each private branch or delete it so that confidential
      information is neither lost or disclosed.
      * ValidatingMergeView.validate() must look for private branches.
    * Add an assert to check for private branches in PersonSet.Merge()
      as is done for other conditions conditions where we want the views
      to validate first.


QA

    * Visit https://qastaging.launchpad.net/~canonical-mq/+delete
    * Verify the page states that the team cannot be deleted
    * Delete or change the owners of the private branches
    * Visit https://qastaging.launchpad.net/~canonical-mq/+delete
    * Verify the Delete action available; use it
    * Verify the team is deleted after 10 minutes.
      * Otherwise check for an email reporting an error.


LINT

    lib/lp/code/interfaces/branchcollection.py
    lib/lp/code/model/branchcollection.py
    lib/lp/code/model/tests/test_branchcollection.py
    lib/lp/registry/browser/peoplemerge.py
    lib/lp/registry/browser/tests/test_peoplemerge.py
    lib/lp/registry/model/person.py



TEST

    ./bin/test -vvc lp.code.model.tests.test_branchcollection
    ./bin/test -vvc lp.registry.browser.tests.test_peoplemerge
    ./bin/test -vvc lp.registry.tests.test_personset


IMPLEMENTATION

Added isPrivate() to the IALlBranches collection that will be used
in conjunction with ownedBy() to get all private branches a person owns
without being constrained by the normal visibility checks.
    lib/lp/code/interfaces/branchcollection.py
    lib/lp/code/model/branchcollection.py
    lib/lp/code/model/tests/test_branchcollection.py

Check for private branches and advise the user to delete or transfer
them.
    lib/lp/registry/browser/tests/test_peoplemerge.py
    lib/lp/registry/model/person.py

Add an assert to ensure callsites check for private branches before
calling merge.
    lib/lp/registry/browser/peoplemerge.py
-- 
https://code.launchpad.net/~sinzui/launchpad/merge-and-private-branches/+merge/115229
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/merge-and-private-branches into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py	2012-07-14 07:50:13 +0000
+++ lib/lp/code/interfaces/branchcollection.py	2012-07-16 21:17:23 +0000
@@ -157,6 +157,9 @@
         with a sourcepackage.
         """
 
+    def isPrivate():
+        """Restrict the collection to private branches."""
+
     def ownedBy(person):
         """Restrict the collection to branches owned by 'person'."""
 

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2012-07-14 07:50:13 +0000
+++ lib/lp/code/model/branchcollection.py	2012-07-16 21:17:23 +0000
@@ -61,7 +61,10 @@
 from lp.code.model.codereviewvote import CodeReviewVoteReference
 from lp.code.model.revision import Revision
 from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
-from lp.registry.enums import PUBLIC_INFORMATION_TYPES
+from lp.registry.enums import (
+    PRIVATE_INFORMATION_TYPES,
+    PUBLIC_INFORMATION_TYPES,
+    )
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.person import (
@@ -607,6 +610,11 @@
             Branch.product == None,
             Branch.sourcepackagename == None])
 
+    def isPrivate(self):
+        """See `IBranchCollection`."""
+        return self._filterBy([
+            Branch.information_type.is_in(PRIVATE_INFORMATION_TYPES)])
+
     def ownedBy(self, person):
         """See `IBranchCollection`."""
         return self._filterBy([Branch.owner == person], symmetric=False)

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2012-07-14 07:50:13 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2012-07-16 21:17:23 +0000
@@ -303,11 +303,25 @@
         self.factory.makeAnyBranch(owner=person)
         self.factory.makeProductBranch(product=product)
         collection = self.all_branches.inProduct(product).ownedBy(person)
-        self.all_branches.inProduct(product).ownedBy(person)
         self.assertEqual([branch], list(collection.getBranches()))
         collection = self.all_branches.ownedBy(person).inProduct(product)
         self.assertEqual([branch], list(collection.getBranches()))
 
+    def test_ownedBy_and_isPrivate(self):
+        # 'ownedBy' and 'isPrivate' can combine to form a collection that is
+        # restricted to private branches owned by a particular person.
+        person = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        branch = self.factory.makeProductBranch(
+            product=product, owner=person,
+            information_type=InformationType.USERDATA)
+        self.factory.makeAnyBranch(owner=person)
+        self.factory.makeProductBranch(product=product)
+        collection = self.all_branches.isPrivate().ownedBy(person)
+        self.assertEqual([branch], list(collection.getBranches()))
+        collection = self.all_branches.ownedBy(person).isPrivate()
+        self.assertEqual([branch], list(collection.getBranches()))
+
     def test_ownedByTeamMember_and_inProduct(self):
         # 'ownedBy' and 'inProduct' can combine to form a collection that is
         # restricted to branches of a particular product owned by a particular

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py	2012-07-07 19:30:24 +0000
+++ lib/lp/registry/browser/peoplemerge.py	2012-07-16 21:17:23 +0000
@@ -24,6 +24,7 @@
     LaunchpadFormView,
     )
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.code.interfaces.branchcollection import IAllBranches
 from lp.registry.interfaces.mailinglist import (
     MailingListStatus,
     PURGE_STATES,
@@ -73,6 +74,12 @@
                     "can be merged. It may take ten minutes to remove the "
                     "deleted PPA's files.",
                     mapping=dict(name=dupe_person.name)))
+            all_branches = getUtility(IAllBranches)
+            if all_branches.ownedBy(dupe_person).isPrivate().count() != 0:
+                self.addError(
+                    _("${name} owns private branches that must be "
+                      "deleted or transferred to another owner first.",
+                    mapping=dict(name=dupe_person.name)))
             if dupe_person.isMergePending():
                 self.addError(_("${name} is already queued for merging.",
                       mapping=dict(name=dupe_person.name)))

=== modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py	2012-07-16 21:17:23 +0000
@@ -6,6 +6,7 @@
 
 from zope.component import getUtility
 
+from lp.registry.enums import InformationType
 from lp.registry.interfaces.person import (
     IPersonSet,
     TeamSubscriptionPolicy,
@@ -46,7 +47,7 @@
     def test_cannot_merge_person_with_ppas(self):
         # A team with a PPA cannot be merged.
         login_celebrity('admin')
-        archive = self.dupe.createPPA()
+        self.dupe.createPPA()
         login_celebrity('registry_experts')
         view = create_initialized_view(
             self.person_set, '+requestmerge', form=self.getForm())
@@ -56,6 +57,18 @@
               "files."],
             view.errors)
 
+    def test_cannot_merge_person_with_private_branches(self):
+        # A team or user with a private branches cannot be merged.
+        self.factory.makeBranch(
+            owner=self.dupe, information_type=InformationType.USERDATA)
+        login_celebrity('registry_experts')
+        view = create_initialized_view(
+            self.person_set, '+requestmerge', form=self.getForm())
+        self.assertEqual(
+            [u"dupe owns private branches that must be deleted or "
+              "transferred to another owner first."],
+            view.errors)
+
     def test_cannot_merge_person_with_itself(self):
         # A IPerson cannot be merged with itself.
         login_person(self.target)
@@ -69,7 +82,7 @@
         # A merge cannot be requested for an IPerson if it there is a job
         # queued to merge it into another IPerson.
         job_source = getUtility(IPersonMergeJobSource)
-        duplicate_job = job_source.create(
+        job_source.create(
             from_person=self.dupe, to_person=self.target)
         login_person(self.target)
         view = create_initialized_view(
@@ -81,7 +94,7 @@
         # A merge cannot be requested for an IPerson if it there is a job
         # queued to merge it into another IPerson.
         job_source = getUtility(IPersonMergeJobSource)
-        duplicate_job = job_source.create(
+        job_source.create(
             from_person=self.target, to_person=self.dupe)
         login_person(self.target)
         view = create_initialized_view(
@@ -173,7 +186,7 @@
         # A team with a PPA cannot be merged.
         login_celebrity('admin')
         self.dupe_team.subscriptionpolicy = TeamSubscriptionPolicy.MODERATED
-        archive = self.dupe_team.createPPA()
+        self.dupe_team.createPPA()
         login_celebrity('registry_experts')
         view = self.getView()
         self.assertEqual(
@@ -209,7 +222,7 @@
     def test_cannot_merge_person_with_ppa(self):
         # A person with a PPA cannot be merged.
         login_celebrity('admin')
-        archive = self.dupe_person.createPPA()
+        self.dupe_person.createPPA()
         view = self.getView()
         self.assertEqual(
             [u"dupe-person has a PPA that must be deleted before it can be "

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-07-07 19:30:24 +0000
+++ lib/lp/registry/model/person.py	2012-07-16 21:17:23 +0000
@@ -141,7 +141,10 @@
 from lp.bugs.model.bugtarget import HasBugsBase
 from lp.bugs.model.bugtask import get_related_bugtasks_search_params
 from lp.bugs.model.structuralsubscription import StructuralSubscription
-from lp.code.interfaces.branchcollection import IBranchCollection
+from lp.code.interfaces.branchcollection import (
+    IAllBranches,
+    IBranchCollection,
+    )
 from lp.code.model.hasbranches import (
     HasBranchesMixin,
     HasMergeProposalsMixin,
@@ -4186,6 +4189,9 @@
                                    ArchiveStatus.DELETING]) is not None:
             raise AssertionError(
                 'from_person has a ppa in ACTIVE or DELETING status')
+        from_person_branches = getUtility(IAllBranches).ownedBy(from_person)
+        if from_person_branches.isPrivate().count() != 0:
+            raise AssertionError('from_person has private branches.')
         if from_person.is_team:
             self._purgeUnmergableTeamArtifacts(
                 from_person, to_person, reviewer)