launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/dsd-packagediff into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/dsd-packagediff/+merge/58070

When the update() method on DistroSeriesDifference is called, rather than blanking out any diffs, be a little smarter and see if there are any diffs that exactly fit our purpose and set the property. I also drive-by'd some lint that I noticed.
-- 
https://code.launchpad.net/~stevenk/launchpad/dsd-packagediff/+merge/58070
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/dsd-packagediff into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-04-06 15:35:05 +0000
+++ database/schema/security.cfg	2011-04-18 05:31:11 +0000
@@ -1076,6 +1076,7 @@
 public.job                                      = SELECT, UPDATE
 public.libraryfilealias                         = SELECT
 public.libraryfilecontent                       = SELECT
+public.packagediff                              = SELECT
 public.packageset                               = SELECT
 public.sourcepackagename                        = SELECT
 public.sourcepackagepublishinghistory           = SELECT

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-04-17 13:19:38 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-04-18 05:31:11 +0000
@@ -65,6 +65,7 @@
 from lp.soyuz.model.distroseriessourcepackagerelease import (
     DistroSeriesSourcePackageRelease,
     )
+from lp.soyuz.model.packagediff import PackageDiff
 
 
 class DistroSeriesDifference(Storm):
@@ -342,11 +343,9 @@
         clear_property_cache(self)
         self._updateType()
         updated = self._updateVersionsAndStatus()
-        # If the DSD has changed, we want to invalidate the diffs. The GC
-        # process for the Librarian will clean up after us.
+        # If the DSD has been updated, fiddle with package diffs.
         if updated is True:
-            self.package_diff = None
-            self.parent_package_diff = None
+            self._setPackageDiffs()
         return updated
 
     def _updateType(self):
@@ -434,6 +433,28 @@
                 return True
         return False
 
+    def _setPackageDiffs(self):
+        """Set package diffs if they exist."""
+        if self.base_version is None or self.base_source_pub is None:
+            self.package_diff = None
+            self.parent_package_diff = None
+            return
+        store = IStore(PackageDiff)
+        diffs = store.find(PackageDiff,
+            from_source=self.base_source_pub.sourcepackagerelease.id,
+            to_source=self.source_pub.sourcepackagerelease.id)
+        if diffs.is_empty():
+            self.package_diff = None
+        else:
+            self.package_diff = diffs[0]
+        parent_diffs = store.find(PackageDiff,
+            from_source=self.base_source_pub.sourcepackagerelease.id,
+            to_source=self.parent_source_pub.sourcepackagerelease.id)
+        if parent_diffs.is_empty():
+            self.parent_package_diff = None
+        else:
+            self.parent_package_diff = parent_diffs[0]
+
     def addComment(self, commenter, comment):
         """See `IDistroSeriesDifference`."""
         return getUtility(IDistroSeriesDifferenceCommentSource).new(

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2011-04-15 12:08:33 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2011-04-18 05:31:11 +0000
@@ -704,8 +704,45 @@
         naked_ds_diff.source_version = '1.4'
         naked_ds_diff.parent_source_version = '1.5'
 
-        self.assertEqual(ds_diff.source_package_release.version, '1.4')
-        self.assertEqual(ds_diff.parent_source_package_release.version, '1.5')
+        self.assertEqual('1.4', ds_diff.source_package_release.version)
+        self.assertEqual(
+            '1.5', ds_diff.parent_source_package_release.version)
+
+    def test_existing_packagediff_is_linked_when_dsd_created(self):
+        # When a relevant packagediff already exists, it is linked to the
+        # DSD when it is created.
+        derived_series = self.factory.makeDistroSeries(
+            parent_series=self.factory.makeDistroSeries())
+        spn = self.factory.getOrMakeSourcePackageName(
+            name=self.factory.getUniqueString())
+        parent_changelog_lfa = self.factory.makeChangelog(
+            spn.name, versions=['1.2-1', '1.0-1'])
+        changelog_lfa = self.factory.makeChangelog(
+            spn.name, versions=['1.1-1', '1.0-1'])
+        transaction.commit() # Yay, librarian.
+        parent_spr = self.factory.makeSourcePackageRelease(
+            sourcepackagename=spn, version='1.2-1',
+            changelog=parent_changelog_lfa,
+            distroseries=derived_series.parent_series)
+        parent_spph = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=derived_series.parent_series,
+            sourcepackagerelease=parent_spr)
+        spr = self.factory.makeSourcePackageRelease(
+            sourcepackagename=spn, version='1.1-1',
+            changelog=changelog_lfa, distroseries=derived_series)
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=derived_series, sourcepackagerelease=spr)
+        base_spph = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=derived_series,
+            sourcepackagename=spn, version='1.0-1',
+            status=PackagePublishingStatus.SUPERSEDED)
+        pd = self.factory.makePackageDiff(
+            from_source=base_spph.sourcepackagerelease, to_source=spr)
+        # factory.makeDistroSeriesDifference() will always create
+        # publications to be helpful. We don't need the help in this case.
+        dsd = getUtility(IDistroSeriesDifferenceSource).new(
+            derived_series, spn)
+        self.assertEqual(pd, dsd.package_diff)
 
     def _initDiffWithMultiplePendingPublications(self, versions, parent):
         ds_diff = self.factory.makeDistroSeriesDifference(versions=versions)

=== modified file 'lib/lp/soyuz/tests/test_distroseriesdifferencejob.py'
--- lib/lp/soyuz/tests/test_distroseriesdifferencejob.py	2011-04-13 16:03:28 +0000
+++ lib/lp/soyuz/tests/test_distroseriesdifferencejob.py	2011-04-18 05:31:11 +0000
@@ -561,7 +561,9 @@
             derived_series.parent_series)
         jobs = find_waiting_jobs(derived_series, source_package_name)
         self.runJob(jobs[0])
-        self.assertIs(None, ds_diff[0].package_diff)
+        # Since the diff showing the changes from 1.0-1 to 1.0-1derived1 is
+        # still valid, it isn't reset, but the parent diff is.
+        self.assertIsNot(None, ds_diff[0].package_diff)
         self.assertIs(None, ds_diff[0].parent_package_diff)
 
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-04-11 18:00:22 +0000
+++ lib/lp/testing/factory.py	2011-04-18 05:31:11 +0000
@@ -2305,8 +2305,7 @@
         versions=None,
         difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
         status=DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
-        changelogs=None,
-        set_base_version=False):
+        changelogs=None, set_base_version=False):
         """Create a new distro series source package difference."""
         if derived_series is None:
             parent_series = self.makeDistroSeries()