Merge lp:~jelmer/launchpad/bug113563 into lp:launchpad

[Jelmer Vernooij]

ADd IDistribution.getSeriesByStatus()

This fixes an old tech-debt bug (bug 113563) about having a IDistribution.getSeriesByStatus() method.

[Henning Eggers (henninge)]

Just a quick look before I turn to your other proposal ;-)

 - Please use storm for queries, not SQLobject (DistroSeries.selectBy is not storm).
 - Please test None explicitely, as per our style guide.

No review yet, so somebody else can pick it up later, as I won't be around too long.

[Jelmer Vernooij (jelmer)]

Thanks, I've fixed those issues at least.

[Gavin Panella (allenap)]

Looking at the diff I saw a problem; getSeriesByStatus() calls distro.getSeries() with no arguments, but arguments are required. Weird. Then I realised that SeriesByStatusTests was never being run because it hadn't been added to test_suite(). Sadly, but not unexpectedly, it doesn't pass :) I'm sure it's not much work to sort out though.

review: Needs Fixing

[Eleanor Berger (intellectronica)]

review: Approve (code)

[Gavin Panella (allenap)]

Ah, I see Tom's reviewed it. Sorry for not getting to this sooner Jelmer.

review: Abstain

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2010-05-27 11:06:25 +0000
+++ lib/lp/registry/interfaces/distribution.py	2010-06-21 13:37:31 +0000
@@ -385,6 +385,13 @@
         """Return a (distroseries,pocket) tuple which is the given textual
         distroseriesname in this distribution."""
 
+    def getSeriesByStatus(status):
+        """Query context distribution for distroseries with a given status.
+
+        :param status: Series status to look for
+        :return: list of `IDistroSeries`
+        """
+
     def getSourcePackageCaches(archive=None):
         """The set of all source package info caches for this distribution.
 

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2010-06-21 07:26:51 +0000
+++ lib/lp/registry/model/distribution.py	2010-06-21 13:37:31 +0000
@@ -13,7 +13,7 @@
 
 from sqlobject import BoolCol, ForeignKey, SQLObjectNotFound, StringCol
 from sqlobject.sqlbuilder import SQLConstant
-from storm.locals import Desc, In, Int, Join, SQL
+from storm.locals import Desc, In, Int, Join, Or, SQL
 from storm.store import Store
 from zope.component import getUtility
 from zope.interface import alsoProvides, implements
@@ -524,13 +524,12 @@
 
     def getSeries(self, name_or_version):
         """See `IDistribution`."""
-        distroseries = DistroSeries.selectOneBy(
-            distribution=self, name=name_or_version)
-        if distroseries is None:
-            distroseries = DistroSeries.selectOneBy(
-                distribution=self, version=name_or_version)
-            if distroseries is None:
-                raise NoSuchDistroSeries(name_or_version)
+        distroseries = Store.of(self).find(DistroSeries,
+               Or(DistroSeries.name == name_or_version,
+               DistroSeries.version == name_or_version),
+            DistroSeries.distribution == self).one()
+        if not distroseries:
+            raise NoSuchDistroSeries(name_or_version)
         return distroseries
 
     def getDevelopmentSeries(self):
@@ -773,6 +772,12 @@
 
         raise NotFoundError(distroseries_name)
 
+    def getSeriesByStatus(self, status):
+        """See `IDistribution`."""
+        return Store.of(self).find(DistroSeries, 
+            DistroSeries.distribution == self,
+            DistroSeries.status == status)
+
     def getFileByName(self, filename, archive=None, source=True, binary=True):
         """See `IDistribution`."""
         assert (source or binary), "searching in an explicitly empty " \

=== modified file 'lib/lp/registry/tests/test_distribution.py'
--- lib/lp/registry/tests/test_distribution.py	2010-05-12 18:03:14 +0000
+++ lib/lp/registry/tests/test_distribution.py	2010-06-21 13:37:31 +0000
@@ -11,9 +11,10 @@
 
 from lp.registry.tests.test_distroseries import (
     TestDistroSeriesCurrentSourceReleases)
+from lp.registry.interfaces.distroseries import NoSuchDistroSeries
+from lp.registry.interfaces.series import SeriesStatus
 from lp.soyuz.interfaces.distributionsourcepackagerelease import (
     IDistributionSourcePackageRelease)
-from lp.registry.interfaces.series import SeriesStatus
 from lp.testing import TestCaseWithFactory
 from canonical.testing.layers import (
     DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
@@ -101,5 +102,47 @@
         self.assertTrue(series is distribution._cached_series)
 
 
+class SeriesByStatusTests(TestCaseWithFactory):
+    """Test IDistribution.getSeriesByStatus().
+    """
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_get_none(self):
+        distro = self.factory.makeDistribution()
+        self.assertEquals([],
+            list(distro.getSeriesByStatus(SeriesStatus.FROZEN)))
+
+    def test_get_current(self):
+        distro = self.factory.makeDistribution()
+        series = self.factory.makeDistroSeries(distribution=distro, 
+            status=SeriesStatus.CURRENT)
+        self.assertEquals([series],
+            list(distro.getSeriesByStatus(SeriesStatus.CURRENT)))
+
+
+class SeriesTests(TestCaseWithFactory):
+    """Test IDistribution.getSeries().
+    """
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_get_none(self):
+        distro = self.factory.makeDistribution()
+        self.assertRaises(NoSuchDistroSeries, distro.getSeries, "astronomy")
+
+    def test_get_by_name(self):
+        distro = self.factory.makeDistribution()
+        series = self.factory.makeDistroSeries(distribution=distro, 
+            name="dappere")
+        self.assertEquals(series, distro.getSeries("dappere"))
+
+    def test_get_by_version(self):
+        distro = self.factory.makeDistribution()
+        series = self.factory.makeDistroSeries(distribution=distro, 
+            name="dappere", version="42.6")
+        self.assertEquals(series, distro.getSeries("42.6"))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/soyuz/scripts/ftpmaster.py'
--- lib/lp/soyuz/scripts/ftpmaster.py	2010-02-15 12:59:55 +0000
+++ lib/lp/soyuz/scripts/ftpmaster.py	2010-06-21 13:37:31 +0000
@@ -1036,23 +1036,6 @@
             raise LaunchpadScriptFailure(
                 "Action does not accept defined suite.")
 
-    # XXX cprov 2007-04-20 bug=113563.: Should be implemented in
-    # IDistribution.
-    def getSeriesByStatus(self, status):
-        """Query context distribution for a distroseries in a given status.
-
-        I may raise LaunchpadScriptError if no suitable distroseries in a
-        given status was found.
-        """
-        # XXX sabdfl 2007-05-27: Isn't this a bit risky, if there are
-        # multiple series with the desired status?
-        for series in self.location.distribution.series:
-            if series.status == status:
-                return series
-        raise NotFoundError(
-                "Could not find a %s distroseries in %s"
-                % (status.name, self.location.distribution.name))
-
     @property
     def current(self):
         """Return the name of the CURRENT distroseries.
@@ -1063,12 +1046,12 @@
         command-line or if not CURRENT distroseries was found.
         """
         self.checkNoSuiteDefined()
-        try:
-            series = self.getSeriesByStatus(SeriesStatus.CURRENT)
-        except NotFoundError, err:
-            raise LaunchpadScriptFailure(err)
+        series = self.location.distribution.getSeriesByStatus(
+            SeriesStatus.CURRENT)
+        if not series:
+            raise LaunchpadScriptFailure("No CURRENT series.")
 
-        return series.name
+        return series[0].name
 
     @property
     def development(self):
@@ -1090,17 +1073,14 @@
         wanted_status = (SeriesStatus.DEVELOPMENT,
                          SeriesStatus.FROZEN)
         for status in wanted_status:
-            try:
-                series = self.getSeriesByStatus(status)
-            except NotFoundError:
-                pass
-
-        if series is None:
+            series = self.location.distribution.getSeriesByStatus(status)
+            if series.count() > 0:
+                break
+        else:
             raise LaunchpadScriptFailure(
                 'There is no DEVELOPMENT distroseries for %s' %
                 self.location.distribution.name)
-
-        return series.name
+        return series[0].name
 
     @property
     def supported(self):