launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/launchpad/sync-packages-bug-880246 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #880246 in Launchpad itself: "+synchronised-packages showing syncs to distros derived past the one I sync'd to"
  https://bugs.launchpad.net/launchpad/+bug/880246

For more details, see:
https://code.launchpad.net/~rvb/launchpad/sync-packages-bug-880246/+merge/80889

This branch fixes the publication code so that the default is now to create new publications with creator=None.  It's the responsibility of the call sites to set the right creator if needed.  This might look like a frightening change but the creator field on SPPH objects was created very recently and is currently used only by the copier code (and that behavior is properly tested).

Note: this won't fix the publishings already created with a creator instead of None (the ones mentioned on the bug report).

= Pre implementation =

Talked to bigjools about setting the default SPPH.creator to None.

= Tests =

./bin/test -vvc test_person test_getLatestSynchronisedPublishings_derived_series
./bin/test -vvc test_initialize_distroseries test_copied_publishings_creator_None_cloner
./bin/test -vvc test_initialize_distroseries test_copied_publishings_creator_None_copier

= Q/A =

Create a package in distroseries A, copy it to distroseries B and make sure you get credited for that on the +synchronised-packages page.  Then initialize a new series from B and make sure you don't see the newly created packages on the +synchronised-packages page.
-- 
https://code.launchpad.net/~rvb/launchpad/sync-packages-bug-880246/+merge/80889
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/sync-packages-bug-880246 into lp:launchpad.

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2011-09-28 23:26:44 +0000
+++ lib/lp/registry/tests/test_person.py	2011-11-02 15:14:31 +0000
@@ -38,6 +38,7 @@
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
+    LaunchpadZopelessLayer,
     reconnect_stores,
     )
 from lp.answers.model.answercontact import AnswerContact
@@ -77,6 +78,10 @@
     ArchivePurpose,
     ArchiveStatus,
     )
+from lp.soyuz.scripts.initialize_distroseries import InitializeDistroSeries
+from lp.soyuz.scripts.tests.test_initialize_distroseries import (
+    InitializationHelperTestCase,
+    )
 from lp.testing import (
     ANONYMOUS,
     celebrity_logged_in,
@@ -439,14 +444,14 @@
         self.assertThat(recorder, HasQueryCount(Equals(1)))
 
     def createCopiedPackage(self, spph, copier, dest_distroseries=None,
-                            dest_archive=None):
+                            dest_archive=None,
+                            dest_pocket=PackagePublishingPocket.UPDATES):
         if dest_distroseries is None:
             dest_distroseries = self.factory.makeDistroSeries()
         if dest_archive is None:
             dest_archive = dest_distroseries.main_archive
         return spph.copyTo(
-            dest_distroseries, creator=copier,
-            pocket=PackagePublishingPocket.UPDATES,
+            dest_distroseries, creator=copier, pocket=dest_pocket,
             archive=dest_archive)
 
     def test_getLatestSynchronisedPublishings_most_recent_first(self):
@@ -521,6 +526,38 @@
             synchronised_spphs.count())
 
 
+class TestPersonInitializedSeries(TestPerson, InitializationHelperTestCase):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_getLatestSynchronisedPublishings_derived_series(self):
+        # The publishings copied over when a new series is initialized
+        # are not picked up by getLatestSynchronisedPublishings.
+        copier = self.factory.makePerson()
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            pocket=PackagePublishingPocket.RELEASE, creator=copier)
+        parent, unused = self.setupParent(
+            packages={}, arch_tag='i386')
+        self.createCopiedPackage(spph, copier, parent)
+        copied_spph2 = self.createCopiedPackage(
+            spph, copier, parent,
+            dest_pocket=PackagePublishingPocket.RELEASE)
+
+        # Initialize a new series from 'parent'.
+        child = self.factory.makeDistroSeries(previous_series=parent)
+        ids = InitializeDistroSeries(child, [parent.id])
+        ids.check()
+        ids.initialize()
+        synchronised_spphs = copier.getLatestSynchronisedPublishings()
+
+        # The publishings copied from the parent are not picked up by
+        # getLatestSynchronisedPublishings because creator=None for
+        # these new publishings.
+        self.assertContentEqual(
+            [copied_spph2],
+            synchronised_spphs)
+
+
 class TestPersonStates(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2011-10-23 02:58:56 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-11-02 15:14:31 +0000
@@ -1567,9 +1567,6 @@
         from lp.registry.model.distributionsourcepackage import (
             DistributionSourcePackage)
 
-        if creator is None:
-            creator = sourcepackagerelease.creator
-
         pub = SourcePackagePublishingHistory(
             distroseries=distroseries,
             pocket=pocket,

=== modified file 'lib/lp/soyuz/scripts/initialize_distroseries.py'
--- lib/lp/soyuz/scripts/initialize_distroseries.py	2011-10-13 13:02:27 +0000
+++ lib/lp/soyuz/scripts/initialize_distroseries.py	2011-11-02 15:14:31 +0000
@@ -560,7 +560,8 @@
                             sources, target_archive, self.distroseries,
                             target_pocket, include_binaries=not self.rebuild,
                             check_permissions=False, strict_binaries=False,
-                            close_bugs=False, create_dsd_job=False)
+                            close_bugs=False, create_dsd_job=False,
+                            person=None)
                         if self.rebuild:
                             rebuilds = []
                             for pubrec in sources_published:

=== modified file 'lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2011-10-13 10:45:09 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2011-11-02 15:14:31 +0000
@@ -1249,6 +1249,31 @@
         self.assertFalse(
             ids._use_cloner(target_archive, target_archive))
 
+    def test_copied_publishings_creator_None_cloner(self):
+        # The new publishings, copied over from the parents, have their
+        # 'creator' field set to None.  This tests that behaviour when
+        # the cloner is used to perform the initialization.
+        parent, unused = self.setupParent(packages={u'p1': u'1.2'})
+        child = self.setUpSeriesWithPreviousSeries(previous_parents=[parent])
+        self.factory.makeSourcePackagePublishingHistory(distroseries=child)
+        self._fullInitialize([parent], child=child)
+
+        published_sources = child.main_archive.getPublishedSources(
+            distroseries=child)
+        self.assertEqual(None, published_sources[0].creator)
+
+    def test_copied_publishings_creator_None_copier(self):
+        # The new publishings, copied over from the parents, have their
+        # 'creator' field set to None.  This tests that behaviour when
+        # the copier is used to perform the initialization.
+        parent, unused = self.setupParent(packages={u'p1': u'1.2'})
+        child = self.setUpSeriesWithPreviousSeries(previous_parents=[parent])
+        self._fullInitialize([parent], child=child)
+
+        published_sources = child.main_archive.getPublishedSources(
+            distroseries=child)
+        self.assertEqual(None, published_sources[0].creator)
+
     def test__has_same_parents_as_previous_series_explicit(self):
         # IDS._has_same_parents_as_previous_series returns True if the
         # parents for the series to be initialized are the same as