Merge lp:~michael.nelson/launchpad/bug-235279-old-series-in-sources-list into lp:launchpad

[Michael Nelson]

[Michael Nelson (michael.nelson)]

= Summary =

Currently the distroseries selector, used on the PPA index page,
presents as an option a distroseries for which all packages have been
deleted in that PPA.

See https://bugs.edge.launchpad.net/soyuz/+bug/235279
for further explanation and an example.

== Proposed fix ==

It turned out that the underlying Archive.series_with_sources method was
not checking the status of the published sources.

This fix ensures that non-active publications (ie. deleted or superseded
publications) are ignored when determining an archive's series with sources.

== Pre-implementation notes ==

None.

== Implementation details ==

I took the chance to convert the query to Storm, and reduced it to one
single query rather than two separate ones.

I know we have a policy against lambda functions, but I just used the
same sorting that is already present on Distribution.serieses. I'll be
happy to convert it to a normal function if that's preferable.

== Tests ==

bin/test -vvt TestSeriesWithSources

== Demo and Q/A ==

There is an example linked from the bug that can be used for QA.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/tests/test_archive.py
  lib/lp/soyuz/model/archive.py

--
Michael

[Deryck Hodge (deryck)]

Hi, Michael.

This looks good to me. I'll defer to your own judgment about the lambda. I'm not personally bothered by it, especially since this is like another section of the code.

Otherwise, the changes and the test look good.

Cheers,
deryck

review: Approve

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2009-07-18 21:19:37 +0000
+++ lib/lp/soyuz/model/archive.py	2009-07-27 14:45:01 +0000
@@ -23,6 +23,7 @@
 from zope.interface import alsoProvides, implements
 from zope.security.interfaces import Unauthorized
 
+from lp.archivepublisher.debversion import Version
 from lp.archiveuploader.utils import re_issource, re_isadeb
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
@@ -80,7 +81,8 @@
 from lp.soyuz.interfaces.packagecopyrequest import (
     IPackageCopyRequestSet)
 from lp.soyuz.interfaces.publishing import (
-    PackagePublishingPocket, PackagePublishingStatus, IPublishingSet)
+    active_publishing_status, PackagePublishingPocket,
+    PackagePublishingStatus, IPublishingSet)
 from lp.registry.interfaces.sourcepackagename import (
     ISourcePackageNameSet)
 from lp.soyuz.scripts.packagecopier import (
@@ -211,14 +213,25 @@
     @property
     def series_with_sources(self):
         """See `IArchive`."""
-        cur = cursor()
-        query = """SELECT DISTINCT distroseries FROM
-                      SourcePackagePublishingHistory WHERE
-                      SourcePackagePublishingHistory.archive = %s"""
-        cur.execute(query % self.id)
-        published_series_ids = [int(row[0]) for row in cur.fetchall()]
-        return [s for s in self.distribution.serieses if s.id in
-                published_series_ids]
+        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+
+        # Import DistroSeries here to avoid circular imports.
+        from lp.registry.model.distroseries import DistroSeries
+
+        distro_serieses = store.find(
+            DistroSeries,
+            DistroSeries.distribution == self.distribution,
+            SourcePackagePublishingHistory.distroseries == DistroSeries.id,
+            SourcePackagePublishingHistory.archive == self,
+            SourcePackagePublishingHistory.status.is_in(
+                active_publishing_status))
+
+        distro_serieses.config(distinct=True)
+
+        # Ensure the ordering is the same as presented by
+        # Distribution.serieses
+        return sorted(
+            distro_serieses, key=lambda a: Version(a.version), reverse=True)
 
     @property
     def dependencies(self):

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2009-07-17 18:46:25 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2009-07-27 14:45:01 +0000
@@ -1,7 +1,7 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Test Build features."""
+"""Test Archive features."""
 
 from datetime import datetime
 import pytz
@@ -168,5 +168,80 @@
             self.publisher.ubuntutest.main_archive.binaries_size)
 
 
+class TestSeriesWithSources(TestCaseWithFactory):
+    """Create some sources in different series."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestSeriesWithSources, self).setUp()
+        self.publisher = SoyuzTestPublisher()
+        self.publisher.prepareBreezyAutotest()
+
+        # Create three sources for the two different distroseries.
+        breezy_autotest = self.publisher.distroseries
+        ubuntu_test = breezy_autotest.distribution
+        self.serieses = [breezy_autotest]
+        self.serieses.append(self.factory.makeDistroRelease(
+            distribution=ubuntu_test, name="foo-series"))
+
+        self.sources = []
+        gedit_src_hist = self.publisher.getPubSource(
+            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED)
+        self.sources.append(gedit_src_hist)
+
+        firefox_src_hist = self.publisher.getPubSource(
+            sourcename="firefox", status=PackagePublishingStatus.PUBLISHED,
+            distroseries=self.serieses[1])
+        self.sources.append(firefox_src_hist)
+
+        gtg_src_hist = self.publisher.getPubSource(
+            sourcename="getting-things-gnome",
+            status=PackagePublishingStatus.PUBLISHED,
+            distroseries=self.serieses[1])
+        self.sources.append(gtg_src_hist)
+
+        # Shortcuts for test readability.
+        self.archive = self.serieses[0].main_archive
+
+    def test_series_with_sources_returns_all_series(self):
+        # Calling series_with_sources returns all series with publishings.
+        serieses = self.archive.series_with_sources
+        serieses_names = [series.displayname for series in serieses]
+
+        self.assertContentEqual(
+            [u'Breezy Badger Autotest', u'Foo-series'],
+            serieses_names)
+
+    def test_series_with_sources_ignore_non_published_records(self):
+        # If all publishings in a series are deleted or superseded
+        # the series will not be returned.
+        self.sources[0].secure_record.status = (
+            PackagePublishingStatus.DELETED)
+
+        serieses = self.archive.series_with_sources
+        serieses_names = [series.displayname for series in serieses]
+
+        self.assertContentEqual([u'Foo-series'], serieses_names)
+
+    def test_series_with_sources_ordered_by_version(self):
+        # The returned series are ordered by the distroseries version.
+        serieses = self.archive.series_with_sources
+        versions = [series.version for series in serieses]
+
+        # Latest version should be first
+        self.assertEqual(
+            [u'6.6.6', u'1.0'], versions,
+            "The latest version was not first.")
+
+        # Update the version of breezyautotest and ensure that the
+        # latest version is still first.
+        self.serieses[0].version = u'0.5'
+        serieses = self.archive.series_with_sources
+        versions = [series.version for series in serieses]
+        self.assertEqual(
+            [u'1.0', u'0.5'], versions,
+            "The latest version was not first.")
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)