launchpad-reviewers team mailing list archive

[Colin Watson <mp+409494@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:charm-recipe-person-merge into launchpad:master.

Commit message:
Handle charm recipes in personmerge

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/409494
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:charm-recipe-person-merge into launchpad:master.

diff --git a/lib/lp/charms/interfaces/charmrecipe.py b/lib/lp/charms/interfaces/charmrecipe.py
index 8b7e9d5..20eebc8 100644
--- a/lib/lp/charms/interfaces/charmrecipe.py
+++ b/lib/lp/charms/interfaces/charmrecipe.py
@@ -48,6 +48,7 @@ from lazr.restful.declarations import (
     exported_as_webservice_entry,
     operation_for_version,
     operation_parameters,
+    operation_returns_collection_of,
     operation_returns_entry,
     REQUEST_USER,
     )
@@ -614,6 +615,14 @@ class ICharmRecipeSet(Interface):
     def exists(owner, project, name):
         """Check to see if a matching charm recipe exists."""
 
+    @operation_parameters(
+        owner=Reference(IPerson, title=_("Owner"), required=True))
+    @operation_returns_collection_of(ICharmRecipe)
+    @export_read_operation()
+    @operation_for_version("devel")
+    def findByOwner(owner):
+        """Return all charm recipes with the given `owner`."""
+
     def findByPerson(person, visible_by_user=None):
         """Return all charm recipes relevant to `person`.
 
diff --git a/lib/lp/charms/model/charmrecipe.py b/lib/lp/charms/model/charmrecipe.py
index ebd765a..e7d43d0 100644
--- a/lib/lp/charms/model/charmrecipe.py
+++ b/lib/lp/charms/model/charmrecipe.py
@@ -837,6 +837,10 @@ class CharmRecipeSet:
         expressions.append(get_charm_recipe_privacy_filter(visible_by_user))
         return IStore(CharmRecipe).find(CharmRecipe, *expressions)
 
+    def findByOwner(self, owner):
+        """See `ICharmRecipeSet`."""
+        return IStore(CharmRecipe).find(CharmRecipe, owner=owner)
+
     def findByPerson(self, person, visible_by_user=None):
         """See `ICharmRecipeSet`."""
         def _getRecipes(collection):
diff --git a/lib/lp/charms/tests/test_charmrecipe.py b/lib/lp/charms/tests/test_charmrecipe.py
index a399743..44bf444 100644
--- a/lib/lp/charms/tests/test_charmrecipe.py
+++ b/lib/lp/charms/tests/test_charmrecipe.py
@@ -1052,6 +1052,19 @@ class TestCharmRecipeSet(TestCaseWithFactory):
             getUtility(ICharmRecipeSet).getByName(
                 owner, project, "proj-charm"))
 
+    def test_findByOwner(self):
+        # ICharmRecipeSet.findByOwner returns all charm recipes with the
+        # given owner.
+        owners = [self.factory.makePerson() for i in range(2)]
+        recipes = []
+        for owner in owners:
+            for i in range(2):
+                recipes.append(self.factory.makeCharmRecipe(
+                    registrant=owner, owner=owner))
+        recipe_set = getUtility(ICharmRecipeSet)
+        self.assertContentEqual(recipes[:2], recipe_set.findByOwner(owners[0]))
+        self.assertContentEqual(recipes[2:], recipe_set.findByOwner(owners[1]))
+
     def test_findByPerson(self):
         # ICharmRecipeSet.findByPerson returns all charm recipes with the
         # given owner or based on repositories with the given owner.
diff --git a/lib/lp/registry/personmerge.py b/lib/lp/registry/personmerge.py
index 1767c8c..b2e61d9 100644
--- a/lib/lp/registry/personmerge.py
+++ b/lib/lp/registry/personmerge.py
@@ -10,6 +10,7 @@ from storm.store import Store
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.charms.interfaces.charmrecipe import ICharmRecipeSet
 from lp.code.interfaces.branchcollection import (
     IAllBranches,
     IBranchCollection,
@@ -727,6 +728,24 @@ def _mergeOCIRecipeSubscription(cur, from_id, to_id):
         ''' % vars())
 
 
+def _mergeCharmRecipe(cur, from_person, to_person):
+    # This shouldn't use removeSecurityProxy.
+    recipes = getUtility(ICharmRecipeSet).findByOwner(from_person)
+    existing_names = [
+        r.name for r in getUtility(ICharmRecipeSet).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.
@@ -824,14 +843,6 @@ def merge_people(from_person, to_person, reviewer, delete=False):
         ('latestpersonsourcepackagereleasecache', 'maintainer'),
         # Obsolete table.
         ('branchmergequeue', 'owner'),
-        # XXX cjwatson 2020-02-05: This needs handling before we deploy the
-        # OCI recipe code, but can be ignored for the purpose of deploying
-        # the database tables.
-        ('ocirecipe', 'owner'),
-        # XXX cjwatson 2021-05-24: This needs handling before we deploy the
-        # charm recipe code, but can be ignored for the purpose of deploying
-        # the database tables.
-        ('charmrecipe', 'owner'),
         ]
 
     references = list(postgresql.listReferences(cur, 'person', 'id'))
@@ -967,6 +978,9 @@ def merge_people(from_person, to_person, reviewer, delete=False):
     _mergeOCIRecipeSubscription(cur, from_id, to_id)
     skip.append(('ocirecipesubscription', 'person'))
 
+    _mergeCharmRecipe(cur, from_id, to_id)
+    skip.append(('charmrecipe', 'owner'))
+
     # Sanity check. If we have a reference that participates in a
     # UNIQUE index, it must have already been handled by this point.
     # We can tell this by looking at the skip list.
diff --git a/lib/lp/registry/tests/test_personmerge.py b/lib/lp/registry/tests/test_personmerge.py
index 8194a63..0b5e264 100644
--- a/lib/lp/registry/tests/test_personmerge.py
+++ b/lib/lp/registry/tests/test_personmerge.py
@@ -9,6 +9,7 @@ from operator import attrgetter
 import pytz
 from testtools.matchers import (
     Equals,
+    MatchesListwise,
     MatchesSetwise,
     MatchesStructure,
     )
@@ -18,6 +19,10 @@ from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.charms.interfaces.charmrecipe import (
+    CHARM_RECIPE_ALLOW_CREATE,
+    ICharmRecipeSet,
+    )
 from lp.code.interfaces.gitrepository import IGitRepositorySet
 from lp.oci.interfaces.ocirecipe import (
     IOCIRecipeSet,
@@ -741,6 +746,47 @@ class TestMergePeople(TestCaseWithFactory, KarmaTestMixin):
         self.assertEqual(ref.path, oci_recipes[1].git_path)
         self.assertEqual(u'foo-1', oci_recipes[1].name)
 
+    def test_merge_moves_charm_recipes(self):
+        # When person/teams are merged, charm recipes owned by the from
+        # person are moved.
+        self.useFixture(FeatureFixture({CHARM_RECIPE_ALLOW_CREATE: 'on'}))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        self.factory.makeCharmRecipe(registrant=duplicate, owner=duplicate)
+        self._do_premerge(duplicate, mergee)
+        login_person(mergee)
+        duplicate, mergee = self._do_merge(duplicate, mergee)
+        self.assertEqual(
+            1, getUtility(ICharmRecipeSet).findByOwner(mergee).count())
+
+    def test_merge_with_duplicated_charm_recipes(self):
+        # If both the from and to people have charm recipes with the same
+        # name, merging renames the duplicate from the from person's side.
+        self.useFixture(FeatureFixture({CHARM_RECIPE_ALLOW_CREATE: 'on'}))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        [ref] = self.factory.makeGitRefs()
+        [ref2] = self.factory.makeGitRefs()
+        self.factory.makeCharmRecipe(
+            registrant=duplicate, owner=duplicate, name='foo', git_ref=ref)
+        self.factory.makeCharmRecipe(
+            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(ICharmRecipeSet).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):