Merge lp:~stevenk/launchpad/dsd-base_source_pub-search-parent into lp:launchpad

[Steve Kowalik]

[r=henninge][bug=734719] DSD.base_source_pub now looks in the parent series first, and then the child distroseries.

DSD.base_source_pub should look in the parent series first, since that is most likely to contain the base version publication in the real world.

[Robert Collins (lifeless)]

This does two queries always - the count() (expensive) and then a first.

Better to just do a first:
result = pubs.first()
if result is None:
    result = ... other side query ... .first()
return result

[Henning Eggers (henninge)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Steven,
thanks for the quick chat and explanation. I should have looked at more
context but I had hoped to do it quickly.

 review approve code

This looks fine to me, only the two issues mentioned on mumble. Please have a
look below.

Cheers,
Henning

Am 14.03.2011 06:23, schrieb Steve Kowalik:
> === modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
> === modified file 'lib/lp/registry/model/distroseriesdifference.py'
> === modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
> --- lib/lp/registry/tests/test_distroseriesdifference.py 2011-03-10 11:11:50 +0000
> +++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-03-14 05:22:57 +0000
> @@ -474,7 +474,8 @@
[...]

> @@ -515,6 +517,36 @@
> self.assertEqual('1.0', ds_diff.base_version)
> self.assertIs(None, ds_diff.base_source_pub)
>
> + def test_base_source_pub_only_in_child(self):
> + # If the base version is only published in the child distroseries,
> + # the base source publication is still located and returned.
> + derived_changelog = self.factory.makeChangelog(
> + versions=['1.0', '1.2'])
> + parent_changelog = self.factory.makeChangelog(
> + versions=['1.0', '1.3'])
> + transaction.commit() # Yay, librarian.
> +
> + ds_diff = self.factory.makeDistroSeriesDifference(versions={
> + 'derived': '1.2',
> + 'parent': '1.3',
> + },
> + changelogs={
> + 'derived': derived_changelog,
> + 'parent': parent_changelog,
> + })

This is formatted a bit inconsistently that makes it harder to read. This
would be better, I think.

        ds_diff = self.factory.makeDistroSeriesDifference(
            versions={
                'derived': '1.2',
                'parent': '1.3',
                },
            changelogs={
                'derived': derived_changelog,
                'parent': parent_changelog,
                })

I *think* it is python style to place closing braces on the same level as the
content?

> +
> + # Passing in a base version to makeDistroSeriesDifference() creates
> + # it in both distroseries, so we need to do it ourselves.

So, the comment would be much clearer to me like this but maybe that's too
verbose?

# We do not pass in a 'base' version to makeDistroSeriesDifference()
# because it would be created in both distroseries and we don't want
# that. We rather create it ourselves in just the child series.

> + spr = self.factory.makeSourcePackageRelease(
> + sourcepackagename=ds_diff.source_package_name, version='1.0')
> + self.factory.makeSourcePackagePublishingHistory(
> + distroseries=ds_diff.derived_series, sourcepackagerelease=spr,
> + status=PackagePublishingStatus.SUPERSEDED)
> + ds_diff.update()
> + self.assertEqual('1.0', ds_diff.base_version)
> + self.assertEqual(
> + ds_diff.derived_series, ds_diff.base_source_pub.distroseries)
> +
> def test_requestPackageDiffs(self):
> # IPackageDiffs are created f...

review: Approve (code)

=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py	2010-10-07 14:59:21 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py	2011-03-14 09:44:28 +0000
@@ -132,7 +132,8 @@
         ISourcePackagePublishingHistory,
         title=_("Base source pub"), readonly=True,
         description=_(
-            "The common base version published in the derived series."))
+            "The common base version published in the parent or the "
+            "derived series."))
 
     owner = Reference(
         IPerson, title=_("Owning team of the derived series"), readonly=True,

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-03-10 04:00:08 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-03-14 09:44:28 +0000
@@ -29,7 +29,6 @@
     IMasterStore,
     IStore,
     )
-from lp.archivepublisher.debversion import Version
 from lp.registry.enum import (
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
@@ -154,14 +153,18 @@
     def base_source_pub(self):
         """See `IDistroSeriesDifference`."""
         if self.base_version is not None:
-            pubs = self.derived_series.main_archive.getPublishedSources(
+            parent = self.derived_series.parent_series
+            result = parent.main_archive.getPublishedSources(
                 name=self.source_package_name.name,
-                version=self.base_version,
-                distroseries=self.derived_series)
-            # We know there is a base version published in the distroseries'
-            # main archive.
-            return pubs.first()
-
+                version=self.base_version, distroseries=parent).first()
+            if result is None:
+                # If the base version isn't in the parent, it may be
+                # published in the child distroseries.
+                child = self.derived_series
+                result = child.main_archive.getPublishedSources(
+                    name=self.source_package_name.name,
+                    version=self.base_version, distroseries=child).first()
+            return result
         return None
 
     @property

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2011-03-10 11:11:50 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2011-03-14 09:44:28 +0000
@@ -474,7 +474,8 @@
         self.assertIs(None, ds_diff.base_source_pub)
 
     def test_base_source_pub(self):
-        # The publishing in the derived series with base_version is returned.
+        # The publication in the parent series with the base version is
+        # returned.
         derived_changelog = self.factory.makeChangelog(
             versions=['1.0', '1.2'])
         parent_changelog = self.factory.makeChangelog(
@@ -493,7 +494,8 @@
 
         base_pub = ds_diff.base_source_pub
         self.assertEqual('1.0', base_pub.source_package_version)
-        self.assertEqual(ds_diff.derived_series, base_pub.distroseries)
+        self.assertEqual(
+            ds_diff.derived_series.parent_series, base_pub.distroseries)
 
     def test_base_source_pub_not_published(self):
         # If the base version isn't published, the base version is
@@ -515,6 +517,37 @@
         self.assertEqual('1.0', ds_diff.base_version)
         self.assertIs(None, ds_diff.base_source_pub)
 
+    def test_base_source_pub_only_in_child(self):
+        # If the base version is only published in the child distroseries,
+        # the base source publication is still located and returned.
+        derived_changelog = self.factory.makeChangelog(
+            versions=['1.0', '1.2'])
+        parent_changelog = self.factory.makeChangelog(
+            versions=['1.0', '1.3'])
+        transaction.commit() # Yay, librarian.
+
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            versions={
+                'derived': '1.2',
+                'parent': '1.3',
+            },
+            changelogs={
+                'derived': derived_changelog,
+                'parent': parent_changelog,
+            })
+
+        # Passing in a base version to makeDistroSeriesDifference() creates
+        # it in both distroseries, which we don't want, so we need to do it
+        # ourselves.
+        spr = self.factory.makeSourcePackageRelease(
+            sourcepackagename=ds_diff.source_package_name, version='1.0')
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=ds_diff.derived_series, sourcepackagerelease=spr,
+            status=PackagePublishingStatus.SUPERSEDED)
+        self.assertEqual('1.0', ds_diff.base_version)
+        self.assertEqual(
+            ds_diff.derived_series, ds_diff.base_source_pub.distroseries)
+
     def test_requestPackageDiffs(self):
         # IPackageDiffs are created for the corresponding versions.
         dervied_changelog = self.factory.makeChangelog(