launchpad-reviewers team mailing list archive

[Quentin Debhi <mp+473917@xxxxxxxxxxxxxxxxxx>]

Quentin Debhi has proposed merging ~ruinedyourlife/launchpad:handle-craft-recipes-in-personmerge into launchpad:master with ~ruinedyourlife/launchpad:fix-craft-recipe-branchscanner-permissions as a prerequisite.

Commit message:
Handle craft recipes in personmerge

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ruinedyourlife/launchpad/+git/launchpad/+merge/473917
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ruinedyourlife/launchpad:handle-craft-recipes-in-personmerge into launchpad:master.

diff --git a/lib/lp/crafts/interfaces/craftrecipe.py b/lib/lp/crafts/interfaces/craftrecipe.py
index 1668752..da16d0b 100644
--- a/lib/lp/crafts/interfaces/craftrecipe.py
+++ b/lib/lp/crafts/interfaces/craftrecipe.py
@@ -506,6 +506,9 @@ class ICraftRecipeSet(Interface):
             these Git reference paths.
         """
 
+    def findByOwner(owner):
+        """Return all craft recipes for the given owner."""
+
     def detachFromGitRepository(repository):
         """Detach all craft recipes from the given Git repository.
 
diff --git a/lib/lp/crafts/model/craftrecipe.py b/lib/lp/crafts/model/craftrecipe.py
index ebbbf99..f95b377 100644
--- a/lib/lp/crafts/model/craftrecipe.py
+++ b/lib/lp/crafts/model/craftrecipe.py
@@ -382,6 +382,10 @@ class CraftRecipeSet:
         # privacy infrastructure.
         return IStore(CraftRecipe).find(CraftRecipe, *clauses)
 
+    def findByOwner(self, owner):
+        """See `ICraftRecipeSet`."""
+        return IStore(CraftRecipe).find(CraftRecipe, owner=owner)
+
     def detachFromGitRepository(self, repository):
         """See `ICraftRecipeSet`."""
         self.findByGitRepository(repository).set(
diff --git a/lib/lp/crafts/tests/test_craftrecipe.py b/lib/lp/crafts/tests/test_craftrecipe.py
index 87448f7..d6cf3ac 100644
--- a/lib/lp/crafts/tests/test_craftrecipe.py
+++ b/lib/lp/crafts/tests/test_craftrecipe.py
@@ -340,6 +340,20 @@ class TestCraftRecipeSet(TestCaseWithFactory):
             ),
         )
 
+    def test_findByOwner(self):
+        # ICraftRecipeSet.findByOwner returns all craft recipes with the
+        # given owner.
+        owners = [self.factory.makePerson() for i in range(2)]
+        recipes = []
+        for owner in owners:
+            for _ in range(2):
+                recipes.append(
+                    self.factory.makeCraftRecipe(registrant=owner, owner=owner)
+                )
+        recipe_set = getUtility(ICraftRecipeSet)
+        self.assertContentEqual(recipes[:2], recipe_set.findByOwner(owners[0]))
+        self.assertContentEqual(recipes[2:], recipe_set.findByOwner(owners[1]))
+
     def test_detachFromGitRepository(self):
         # ICraftRecipeSet.detachFromGitRepository clears the given Git
         # repository from all craft recipes.
diff --git a/lib/lp/registry/personmerge.py b/lib/lp/registry/personmerge.py
index 05f0ee2..22afa90 100644
--- a/lib/lp/registry/personmerge.py
+++ b/lib/lp/registry/personmerge.py
@@ -12,6 +12,7 @@ from zope.security.proxy import removeSecurityProxy
 from lp.charms.interfaces.charmrecipe import ICharmRecipeSet
 from lp.code.interfaces.branchcollection import IAllBranches, IBranchCollection
 from lp.code.interfaces.gitcollection import IGitCollection
+from lp.crafts.interfaces.craftrecipe import ICraftRecipeSet
 from lp.oci.interfaces.ocirecipe import IOCIRecipeSet
 from lp.registry.interfaces.mailinglist import (
     PURGE_STATES,
@@ -956,6 +957,25 @@ def _mergeRockRecipe(cur, from_person, to_person):
         IStore(recipes[0]).flush()
 
 
+def _mergeCraftRecipe(cur, from_person, to_person):
+    # This shouldn't use removeSecurityProxy.
+    recipes = getUtility(ICraftRecipeSet).findByOwner(from_person)
+    existing_names = [
+        r.name for r in getUtility(ICraftRecipeSet).findByOwner(to_person)
+    ]
+    for recipe in recipes:
+        naked_recipe = removeSecurityProxy(recipe)
+        new_name = naked_recipe.name
+        count = 1
+        while new_name in existing_names:
+            new_name = "%s-%s" % (recipe.name, count)
+            count += 1
+        naked_recipe.owner = to_person
+        naked_recipe.name = new_name
+    if not recipes.is_empty():
+        IStore(recipes[0]).flush()
+
+
 def _purgeUnmergableTeamArtifacts(from_team, to_team, reviewer):
     """Purge team artifacts that cannot be merged, but can be removed."""
     # A team cannot have more than one mailing list.
@@ -1215,6 +1235,9 @@ def merge_people(from_person, to_person, reviewer, delete=False):
     _mergeRockRecipe(cur, from_id, to_id)
     skip.append(("rockrecipe", "owner"))
 
+    _mergeCraftRecipe(cur, from_id, to_id)
+    skip.append(("craftrecipe", "owner"))
+
     _mergeVulnerabilitySubscription(cur, from_id, to_id)
     skip.append(("vulnerabilitysubscription", "person"))
 
diff --git a/lib/lp/registry/tests/test_personmerge.py b/lib/lp/registry/tests/test_personmerge.py
index dd6a8dd..7acaa67 100644
--- a/lib/lp/registry/tests/test_personmerge.py
+++ b/lib/lp/registry/tests/test_personmerge.py
@@ -23,6 +23,10 @@ from lp.charms.interfaces.charmrecipe import (
     ICharmRecipeSet,
 )
 from lp.code.interfaces.gitrepository import IGitRepositorySet
+from lp.crafts.interfaces.craftrecipe import (
+    CRAFT_RECIPE_ALLOW_CREATE,
+    ICraftRecipeSet,
+)
 from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE, IOCIRecipeSet
 from lp.registry.enums import BugSharingPolicy
 from lp.registry.interfaces.accesspolicy import (
@@ -972,6 +976,60 @@ class TestMergePeople(TestCaseWithFactory, KarmaTestMixin):
             ),
         )
 
+    def test_merge_moves_craft_recipes(self):
+        # When person/teams are merged, craft recipes owned by the from
+        # person are moved.
+        self.useFixture(FeatureFixture({CRAFT_RECIPE_ALLOW_CREATE: "on"}))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        self.factory.makeCraftRecipe(registrant=duplicate, owner=duplicate)
+        self._do_premerge(duplicate, mergee)
+        login_person(mergee)
+        duplicate, mergee = self._do_merge(duplicate, mergee)
+        self.assertEqual(
+            1, getUtility(ICraftRecipeSet).findByOwner(mergee).count()
+        )
+
+    def test_merge_with_duplicated_craft_recipes(self):
+        # If both the from and to people have craft recipes with the same
+        # name, merging renames the duplicate from the from person's side.
+        self.useFixture(FeatureFixture({CRAFT_RECIPE_ALLOW_CREATE: "on"}))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        [ref] = self.factory.makeGitRefs()
+        [ref2] = self.factory.makeGitRefs()
+        self.factory.makeCraftRecipe(
+            registrant=duplicate, owner=duplicate, name="foo", git_ref=ref
+        )
+        self.factory.makeCraftRecipe(
+            registrant=mergee, owner=mergee, name="foo", git_ref=ref2
+        )
+        self._do_premerge(duplicate, mergee)
+        login_person(mergee)
+        duplicate, mergee = self._do_merge(duplicate, mergee)
+        recipes = sorted(
+            getUtility(ICraftRecipeSet).findByOwner(mergee),
+            key=attrgetter("name"),
+        )
+        self.assertEqual(2, len(recipes))
+        self.assertThat(
+            recipes,
+            MatchesListwise(
+                [
+                    MatchesStructure.byEquality(
+                        git_repository=ref2.repository,
+                        git_path=ref2.path,
+                        name="foo",
+                    ),
+                    MatchesStructure.byEquality(
+                        git_repository=ref.repository,
+                        git_path=ref.path,
+                        name="foo-1",
+                    ),
+                ]
+            ),
+        )
+
 
 class TestMergeMailingListSubscriptions(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer