launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+472898@xxxxxxxxxxxxxxxxxx>]

Jürgen Gmach has proposed merging ~jugmac00/launchpad:handle-rock-recipes-in-personmerge into launchpad:master with ~jugmac00/launchpad:add-basic-model-for-rock-recipe-builds-rework as a prerequisite.

Commit message:
Handle rock recipes in personmerge

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/472898

Similar to https://git.launchpad.net/launchpad/commit/?id=a98e2a935646ff7496c76c607d355815027c35a4, but without the API changes, they will be applied together with the other API changes.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:handle-rock-recipes-in-personmerge into launchpad:master.

diff --git a/lib/lp/registry/personmerge.py b/lib/lp/registry/personmerge.py
index 151ff30..0f535c3 100644
--- a/lib/lp/registry/personmerge.py
+++ b/lib/lp/registry/personmerge.py
@@ -24,6 +24,7 @@ from lp.registry.interfaces.teammembership import (
     ITeamMembershipSet,
     TeamMembershipStatus,
 )
+from lp.rocks.interfaces.rockrecipe import IRockRecipeSet
 from lp.services.database import postgresql
 from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import cursor, sqlvalues
@@ -936,6 +937,25 @@ def _mergeCharmRecipe(cur, from_person, to_person):
         IStore(recipes[0]).flush()
 
 
+def _mergeRockRecipe(cur, from_person, to_person):
+    # This shouldn't use removeSecurityProxy.
+    recipes = getUtility(IRockRecipeSet).findByOwner(from_person)
+    existing_names = [
+        r.name for r in getUtility(IRockRecipeSet).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.
@@ -1186,6 +1206,9 @@ def merge_people(from_person, to_person, reviewer, delete=False):
     _mergeCharmRecipe(cur, from_id, to_id)
     skip.append(("charmrecipe", "owner"))
 
+    _mergeRockRecipe(cur, from_id, to_id)
+    skip.append(("rockrecipe", "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 fcc5245..dd6a8dd 100644
--- a/lib/lp/registry/tests/test_personmerge.py
+++ b/lib/lp/registry/tests/test_personmerge.py
@@ -41,6 +41,10 @@ from lp.registry.personmerge import (
     merge_people,
 )
 from lp.registry.tests.test_person import KarmaTestMixin
+from lp.rocks.interfaces.rockrecipe import (
+    ROCK_RECIPE_ALLOW_CREATE,
+    IRockRecipeSet,
+)
 from lp.services.config import config
 from lp.services.database.sqlbase import cursor
 from lp.services.features.testing import FeatureFixture
@@ -914,6 +918,60 @@ class TestMergePeople(TestCaseWithFactory, KarmaTestMixin):
             ),
         )
 
+    def test_merge_moves_rock_recipes(self):
+        # When person/teams are merged, rock recipes owned by the from
+        # person are moved.
+        self.useFixture(FeatureFixture({ROCK_RECIPE_ALLOW_CREATE: "on"}))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        self.factory.makeRockRecipe(registrant=duplicate, owner=duplicate)
+        self._do_premerge(duplicate, mergee)
+        login_person(mergee)
+        duplicate, mergee = self._do_merge(duplicate, mergee)
+        self.assertEqual(
+            1, getUtility(IRockRecipeSet).findByOwner(mergee).count()
+        )
+
+    def test_merge_with_duplicated_rock_recipes(self):
+        # If both the from and to people have rock recipes with the same
+        # name, merging renames the duplicate from the from person's side.
+        self.useFixture(FeatureFixture({ROCK_RECIPE_ALLOW_CREATE: "on"}))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        [ref] = self.factory.makeGitRefs()
+        [ref2] = self.factory.makeGitRefs()
+        self.factory.makeRockRecipe(
+            registrant=duplicate, owner=duplicate, name="foo", git_ref=ref
+        )
+        self.factory.makeRockRecipe(
+            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(IRockRecipeSet).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
diff --git a/lib/lp/rocks/interfaces/rockrecipe.py b/lib/lp/rocks/interfaces/rockrecipe.py
index 4669930..f07e809 100644
--- a/lib/lp/rocks/interfaces/rockrecipe.py
+++ b/lib/lp/rocks/interfaces/rockrecipe.py
@@ -511,6 +511,9 @@ class IRockRecipeSet(Interface):
             these Git reference paths.
         """
 
+    def findByOwner(owner):
+        """Return all rock recipes with the given `owner`."""
+
     def detachFromGitRepository(repository):
         """Detach all rock recipes from the given Git repository.
 
diff --git a/lib/lp/rocks/model/rockrecipe.py b/lib/lp/rocks/model/rockrecipe.py
index 6e493e5..b4b85c1 100644
--- a/lib/lp/rocks/model/rockrecipe.py
+++ b/lib/lp/rocks/model/rockrecipe.py
@@ -474,6 +474,10 @@ class RockRecipeSet:
         # privacy infrastructure.
         return IStore(RockRecipe).find(RockRecipe, *clauses)
 
+    def findByOwner(self, owner):
+        """See `ICharmRecipeSet`."""
+        return IStore(RockRecipe).find(RockRecipe, owner=owner)
+
     def detachFromGitRepository(self, repository):
         """See `ICharmRecipeSet`."""
         self.findByGitRepository(repository).set(
diff --git a/lib/lp/rocks/tests/test_rockrecipe.py b/lib/lp/rocks/tests/test_rockrecipe.py
index 96f9846..91c3eb7 100644
--- a/lib/lp/rocks/tests/test_rockrecipe.py
+++ b/lib/lp/rocks/tests/test_rockrecipe.py
@@ -336,6 +336,20 @@ class TestRockRecipeSet(TestCaseWithFactory):
             ),
         )
 
+    def test_findByOwner(self):
+        # IRockRecipeSet.findByOwner returns all rock 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.makeRockRecipe(registrant=owner, owner=owner)
+                )
+        recipe_set = getUtility(IRockRecipeSet)
+        self.assertContentEqual(recipes[:2], recipe_set.findByOwner(owners[0]))
+        self.assertContentEqual(recipes[2:], recipe_set.findByOwner(owners[1]))
+
     def test_detachFromGitRepository(self):
         # IRockRecipeSet.detachFromGitRepository clears the given Git
         # repository from all rock recipes.