launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
Handle LiveFS.owner during person merging.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1247461 in Launchpad itself: "Move live filesystem building into Launchpad"
  https://bugs.launchpad.net/launchpad/+bug/1247461

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

Handle LiveFS.owner during person merging.  This reverts the temporary hack from https://code.launchpad.net/~cjwatson/launchpad/livefs-personmerge-whitelist/+merge/223537.

The main thing I was unsure about here was the manual flush I had to do at the end of _mergeLiveFS.  It makes sense that I should have to flush database updates, given that merge_people calls store.invalidate() not long afterwards, and my tests failed when I didn't do that; but how come _mergeSourcePackageRecipes apparently doesn't need to do the same?
-- 
https://code.launchpad.net/~cjwatson/launchpad/livefs-personmerge/+merge/223654
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/livefs-personmerge into lp:launchpad.

=== modified file 'lib/lp/registry/personmerge.py'
--- lib/lp/registry/personmerge.py	2014-06-18 10:52:12 +0000
+++ lib/lp/registry/personmerge.py	2014-06-18 22:20:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Person/team merger implementation."""
@@ -34,6 +34,7 @@
 from lp.services.mail.helpers import get_email_template
 from lp.soyuz.enums import ArchiveStatus
 from lp.soyuz.interfaces.archive import IArchiveSet
+from lp.soyuz.interfaces.livefs import ILiveFSSet
 
 
 def _merge_person_decoration(to_person, from_person, skip,
@@ -577,6 +578,24 @@
     ''' % params)
 
 
+def _mergeLiveFS(cur, from_person, to_person):
+    # This shouldn't use removeSecurityProxy.
+    livefses = getUtility(ILiveFSSet).getByPerson(from_person)
+    existing_names = [
+        l.name for l in getUtility(ILiveFSSet).getByPerson(to_person)]
+    for livefs in livefses:
+        new_name = livefs.name
+        count = 1
+        while new_name in existing_names:
+            new_name = '%s-%s' % (livefs.name, count)
+            count += 1
+        naked_livefs = removeSecurityProxy(livefs)
+        naked_livefs.owner = to_person
+        naked_livefs.name = new_name
+    if not livefses.is_empty():
+        IStore(livefses[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.
@@ -672,9 +691,6 @@
         ('bugsummaryjournal', 'viewed_by'),
         ('latestpersonsourcepackagereleasecache', 'creator'),
         ('latestpersonsourcepackagereleasecache', 'maintainer'),
-        # This needs handling before we deploy the livefs code, but can be
-        # ignored for the purpose of deploying the database tables.
-        ('livefs', 'owner'),
         ]
 
     references = list(postgresql.listReferences(cur, 'person', 'id'))
@@ -799,6 +815,9 @@
     _mergeCodeReviewInlineCommentDraft(cur, from_id, to_id)
     skip.append(('codereviewinlinecommentdraft', 'person'))
 
+    _mergeLiveFS(cur, from_person, to_person)
+    skip.append(('livefs', '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.

=== modified file 'lib/lp/registry/tests/test_personmerge.py'
--- lib/lp/registry/tests/test_personmerge.py	2013-06-21 02:39:33 +0000
+++ lib/lp/registry/tests/test_personmerge.py	2014-06-18 22:20:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for merge_people."""
@@ -34,11 +34,16 @@
 from lp.registry.tests.test_person import KarmaTestMixin
 from lp.services.config import config
 from lp.services.database.sqlbase import cursor
+from lp.services.features.testing import FeatureFixture
 from lp.services.identity.interfaces.emailaddress import (
     EmailAddressStatus,
     IEmailAddressSet,
     )
 from lp.soyuz.enums import ArchiveStatus
+from lp.soyuz.interfaces.livefs import (
+    ILiveFSSet,
+    LIVEFS_FEATURE_FLAG,
+    )
 from lp.testing import (
     celebrity_logged_in,
     login_person,
@@ -488,6 +493,39 @@
             self.assertEqual(0, inviting_team.invited_member_count)
             self.assertEqual(0, proposed_team.proposed_member_count)
 
+    def test_merge_moves_livefses(self):
+        # When person/teams are merged, live filesystems owned by the from
+        # person are moved.
+        self.useFixture(FeatureFixture({LIVEFS_FEATURE_FLAG: u"on"}))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        self.factory.makeLiveFS(registrant=duplicate, owner=duplicate)
+        self._do_premerge(duplicate, mergee)
+        login_person(mergee)
+        duplicate, mergee = self._do_merge(duplicate, mergee)
+        self.assertEqual(1, getUtility(ILiveFSSet).getByPerson(mergee).count())
+
+    def test_merge_with_duplicated_livefses(self):
+        # If both the from and to people have live filesystems with the same
+        # name, merging renames the duplicate from the from person's side.
+        self.useFixture(FeatureFixture({LIVEFS_FEATURE_FLAG: u"on"}))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        self.factory.makeLiveFS(
+            registrant=duplicate, owner=duplicate, name=u'foo',
+            metadata={'project': 'FROM'})
+        self.factory.makeLiveFS(
+            registrant=mergee, owner=mergee, name=u'foo',
+            metadata={'project': 'TO'})
+        self._do_premerge(duplicate, mergee)
+        login_person(mergee)
+        duplicate, mergee = self._do_merge(duplicate, mergee)
+        livefses = getUtility(ILiveFSSet).getByPerson(mergee)
+        self.assertEqual(2, livefses.count())
+        project_names = [l.metadata['project'] for l in livefses]
+        self.assertEqual(['TO', 'FROM'], project_names)
+        self.assertEqual(u'foo-1', livefses[1].name)
+
 
 class TestMergeMailingListSubscriptions(TestCaseWithFactory):
 

=== modified file 'lib/lp/soyuz/interfaces/livefs.py'
--- lib/lp/soyuz/interfaces/livefs.py	2014-06-18 22:20:52 +0000
+++ lib/lp/soyuz/interfaces/livefs.py	2014-06-18 22:20:53 +0000
@@ -280,6 +280,9 @@
     def interpret(owner_name, distribution_name, distro_series_name, name):
         """Like `getByName`, but takes names of objects."""
 
+    def getByPerson(owner):
+        """Return all live filesystems with the given `owner`."""
+
     @collection_default_content()
     def getAll():
         """Return all of the live filesystems in Launchpad.

=== modified file 'lib/lp/soyuz/model/livefs.py'
--- lib/lp/soyuz/model/livefs.py	2014-06-18 22:20:52 +0000
+++ lib/lp/soyuz/model/livefs.py	2014-06-18 22:20:53 +0000
@@ -283,6 +283,10 @@
             getUtility(IDistroSeriesSet).queryByName, distribution)
         return self.getByName(owner, distro_series, name)
 
+    def getByPerson(self, owner):
+        """See `ILiveFSSet`."""
+        return IStore(LiveFS).find(LiveFS, LiveFS.owner == owner)
+
     def getAll(self):
         """See `ILiveFSSet`."""
         user = getUtility(ILaunchBag).user

=== modified file 'lib/lp/soyuz/tests/test_livefs.py'
--- lib/lp/soyuz/tests/test_livefs.py	2014-06-18 22:20:52 +0000
+++ lib/lp/soyuz/tests/test_livefs.py	2014-06-18 22:20:53 +0000
@@ -301,6 +301,19 @@
             getUtility(ILiveFSSet).exists(
                 livefs.owner, livefs.distro_series, u"different"))
 
+    def test_getByPerson(self):
+        # ILiveFSSet.getByPerson returns all LiveFSes with the given owner.
+        owners = [self.factory.makePerson() for i in range(2)]
+        livefses = []
+        for owner in owners:
+            for i in range(2):
+                livefses.append(self.factory.makeLiveFS(
+                    registrant=owner, owner=owner))
+        self.assertContentEqual(
+            livefses[:2], getUtility(ILiveFSSet).getByPerson(owners[0]))
+        self.assertContentEqual(
+            livefses[2:], getUtility(ILiveFSSet).getByPerson(owners[1]))
+
     def test_getAll(self):
         # ILiveFSSet.getAll returns all LiveFSes.
         livefses = [self.factory.makeLiveFS() for i in range(3)]