launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-personmerge into lp:launchpad with lp:~cjwatson/launchpad/git-finish-sharing as a prerequisite.

Commit message:
Handle Git repositories during person merging.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1032731 in Launchpad itself: "Support for Launchpad-hosted Git repositories"
  https://bugs.launchpad.net/launchpad/+bug/1032731

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-personmerge/+merge/250669

Handle Git repositories during person merging.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-personmerge into lp:launchpad.

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py	2013-05-02 00:40:14 +0000
+++ lib/lp/registry/browser/peoplemerge.py	2015-02-23 19:46:34 +0000
@@ -25,6 +25,7 @@
     )
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.code.interfaces.branchcollection import IAllBranches
+from lp.code.interfaces.gitcollection import IAllGitRepositories
 from lp.registry.interfaces.mailinglist import (
     MailingListStatus,
     PURGE_STATES,
@@ -79,6 +80,13 @@
                     _("${name} owns private branches that must be "
                       "deleted or transferred to another owner first.",
                     mapping=dict(name=dupe_person.name)))
+            all_repositories = getUtility(IAllGitRepositories)
+            if not all_repositories.ownedBy(
+                    dupe_person).isPrivate().is_empty():
+                self.addError(
+                    _("${name} owns private Git repositories 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	2013-02-06 06:32:24 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py	2015-02-23 19:46:34 +0000
@@ -286,7 +286,7 @@
             view.errors)
 
     def test_cannot_merge_person_with_private_branches(self):
-        # A team or user with a private branches cannot be merged.
+        # A team or user with a private branch cannot be merged.
         self.factory.makeBranch(
             owner=self.dupe, information_type=InformationType.USERDATA)
         login_celebrity('registry_experts')
@@ -297,6 +297,18 @@
               "transferred to another owner first."],
             view.errors)
 
+    def test_cannot_merge_person_with_private_git_repositories(self):
+        # A team or user with a private Git repository cannot be merged.
+        self.factory.makeGitRepository(
+            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 Git repositories 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)

=== modified file 'lib/lp/registry/personmerge.py'
--- lib/lp/registry/personmerge.py	2015-02-10 00:46:18 +0000
+++ lib/lp/registry/personmerge.py	2015-02-23 19:46:34 +0000
@@ -14,6 +14,7 @@
     IAllBranches,
     IBranchCollection,
     )
+from lp.code.interfaces.gitcollection import IGitCollection
 from lp.registry.interfaces.mailinglist import (
     IMailingListSet,
     MailingListStatus,
@@ -152,6 +153,13 @@
         %(from_id)s''', dict(to_id=to_id, from_id=from_id))
 
 
+def _mergeGitRepositories(from_person, to_person):
+    # This shouldn't use removeSecurityProxy.
+    repositories = getUtility(IGitCollection).ownedBy(from_person)
+    for repository in repositories.getRepositories():
+        removeSecurityProxy(repository).setOwner(to_person, to_person)
+
+
 def _mergeSourcePackageRecipes(from_person, to_person):
     # This shouldn't use removeSecurityProxy.
     recipes = from_person.recipes
@@ -691,9 +699,6 @@
         ('bugsummaryjournal', 'viewed_by'),
         ('latestpersonsourcepackagereleasecache', 'creator'),
         ('latestpersonsourcepackagereleasecache', 'maintainer'),
-        # This needs handling before we deploy the git code, but can be
-        # ignored for the purpose of deploying the database tables.
-        ('gitrepository', 'owner'),
         ]
 
     references = list(postgresql.listReferences(cur, 'person', 'id'))
@@ -744,6 +749,11 @@
     _mergeBranchMergeQueues(cur, from_id, to_id)
     skip.append(('branchmergequeue', 'owner'))
 
+    # Update the GitRepositories that will not conflict, and fudge the names
+    # of ones that *do* conflict.
+    _mergeGitRepositories(from_person, to_person)
+    skip.append(('gitrepository', 'owner'))
+
     _mergeSourcePackageRecipes(from_person, to_person)
     skip.append(('sourcepackagerecipe', 'owner'))
 

=== modified file 'lib/lp/registry/tests/test_personmerge.py'
--- lib/lp/registry/tests/test_personmerge.py	2014-06-18 18:29:13 +0000
+++ lib/lp/registry/tests/test_personmerge.py	2015-02-23 19:46:34 +0000
@@ -270,6 +270,35 @@
         self.assertEqual(2, len(branches))
         self.assertContentEqual([u'foo', u'foo-1'], branches)
 
+    def test_merge_moves_git_repositories(self):
+        # When person/teams are merged, Git repositories owned by the from
+        # person are moved.
+        person = self.factory.makePerson()
+        repository = self.factory.makeGitRepository()
+        duplicate = repository.owner
+        self._do_premerge(repository.owner, person)
+        login_person(person)
+        duplicate, person = self._do_merge(duplicate, person)
+        repositories = person.getGitRepositories()
+        self.assertEqual(1, repositories.count())
+
+    def test_merge_with_duplicated_git_repositories(self):
+        # If both the from and to people have Git repositories with the same
+        # name, merging renames the duplicate from the from person's side.
+        project = self.factory.makeProduct()
+        from_repository = self.factory.makeGitRepository(
+            target=project, name=u'foo')
+        to_repository = self.factory.makeGitRepository(
+            target=project, name=u'foo')
+        mergee = to_repository.owner
+        duplicate = from_repository.owner
+        self._do_premerge(duplicate, mergee)
+        login_person(mergee)
+        duplicate, mergee = self._do_merge(duplicate, mergee)
+        repositories = [r.name for r in mergee.getGitRepositories()]
+        self.assertEqual(2, len(repositories))
+        self.assertContentEqual([u'foo', u'foo-1'], repositories)
+
     def test_merge_moves_recipes(self):
         # When person/teams are merged, recipes owned by the from person are
         # moved.