Merge lp:~julian-edwards/launchpad/set-previous-series-bug-805913 into lp:launchpad

[Julian Edwards]

[r=jtv][bug=805913] Make DistroSeries.previous_series get auto-populated when a new series is created.

= Summary =
Make DistroSeries.previous_series get auto-populated when a new series is created.

== Implementation details ==
1. rename prior_series to priorReleasedSeries and convert to a method (only one place was using it and it didn't need to be a cachedproperty)
2. refactor the code in priorReleasedSeries so it's also in IDistroSeriesSet.
3. Stormify the query.
4. Add tests for it! (there were none before)
5. Make the browser code for adding a new series call the utility method to work out what the previous_series should be.
6. Tests for that.

ADDENDUM!

It turns out that going back to the prior released is not the right thing since there can be many unreleased series, so the code now searches back to the most recent series and uses that (if there is one).

== Tests ==
bin/test -cvvt TestDistroSeriesAddView -t test_priorReleasedSeries

== Demo and Q/A ==
+initseries currently blows up when initialising because of the missing data,
this will fix it and easily checked.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/tests/test_distroseries.py
  lib/lp/registry/interfaces/distroseries.py
  lib/lp/registry/browser/tests/test_distroseries.py
  lib/lp/registry/model/sourcepackage.py
  lib/lp/registry/model/distroseries.py
  lib/lp/registry/browser/distroseries.py

[Jeroen T. Vermeulen (jtv)]

Some definite improvements here, even without considering the fixed bug. I do have some problems with the code though: you seem to test for the presence of a release date as an implicit side effect of SQL comparison semantics for nulls (whereas, on a side note, doing the same comparison in python would include unreleased series!). Please make this explicit: add a check or a comment, and test that the right thing happens with unreleased series.

Also, what about createAndAdd in the view code? It uses self.context.series[0] for previous_series. Should that be self.context.priorReleasedSeries()[0]?

(I can't say that priorReleasedSeries works for me as a name, by the way: I'd pronounce it with emphasis on the one word that wasn't capitalized. That's rarely an issue with the start-with-a-verb style we used to require.)

Jeroen

review: Approve

[Jeroen T. Vermeulen (jtv)]

review: Needs Fixing

[Julian Edwards (julian-edwards)]

Thanks for the review, let me know how it looks now.

On Wednesday 06 July 2011 14:14:24 you wrote:
> Review: Approve
> Some definite improvements here, even without considering the fixed bug. I
> do have some problems with the code though: you seem to test for the
> presence of a release date as an implicit side effect of SQL comparison
> semantics for nulls (whereas, on a side note, doing the same comparison in
> python would include unreleased series!). Please make this explicit: add
> a check or a comment, and test that the right thing happens with
> unreleased series.

Right, thanks for spotting that. I've changed it to check that datereleased is
not NULL.

> Also, what about createAndAdd in the view code? It uses
> self.context.series[0] for previous_series. Should that be
> self.context.priorReleasedSeries()[0]?

No, it's correct, you should have paid attention to the addendum :)

> (I can't say that priorReleasedSeries works for me as a name, by the way:
> I'd pronounce it with emphasis on the one word that wasn't capitalized.
> That's rarely an issue with the start-with-a-verb style we used to
> require.)

There's no accounting for taste!

[Jeroen T. Vermeulen (jtv)]

This is a little bit clearer, thanks.

review: Approve

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-07-05 15:51:35 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-07-07 08:59:47 +0000
@@ -93,7 +93,9 @@
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
     )
-from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.distroseries import (
+    IDistroSeries,
+    )
 from lp.registry.interfaces.distroseriesdifference import (
     IDistroSeriesDifferenceSource,
     )
@@ -620,6 +622,12 @@
     @action(_('Add Series'), name='create')
     def createAndAdd(self, action, data):
         """Create and add a new Distribution Series"""
+        # 'series' is a cached property so this won't issue 2 queries.
+        if self.context.series:
+            previous_series = self.context.series[0]
+        else:
+            previous_series = None
+        # previous_series will be None if there isn't one.
         distroseries = self.context.newSeries(
             name=data['name'],
             displayname=data['displayname'],
@@ -627,7 +635,7 @@
             summary=data['summary'],
             description=u"",
             version=data['version'],
-            previous_series=None,
+            previous_series=previous_series,
             registrant=self.user)
         notify(ObjectCreatedEvent(distroseries))
         self.next_url = canonical_url(distroseries)

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-07-05 15:51:35 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-07-07 08:59:47 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from datetime import timedelta
 import difflib
 import re
 from textwrap import TextWrapper
@@ -62,6 +63,7 @@
     getFeatureFlag,
     )
 from lp.services.features.testing import FeatureFixture
+from lp.services.utils import utc_now
 from lp.soyuz.enums import (
     ArchivePermissionType,
     PackagePublishingStatus,
@@ -469,13 +471,12 @@
 
     layer = DatabaseFunctionalLayer
 
-    def test_submit(self):
-        # When creating a new DistroSeries via DistroSeriesAddView, the title
-        # is set to the same as the displayname (title is, in any case,
-        # deprecated), the description is left empty, and previous_series is
-        # None (DistroSeriesInitializeView takes care of setting that).
-        user = self.factory.makePerson()
-        distribution = self.factory.makeDistribution(owner=user)
+    def setUp(self):
+        super(TestDistroSeriesAddView, self).setUp()
+        self.user = self.factory.makePerson()
+        self.distribution = self.factory.makeDistribution(owner=self.user)
+
+    def createNewDistroseries(self):
         form = {
             "field.name": u"polished",
             "field.version": u"12.04",
@@ -483,17 +484,41 @@
             "field.summary": u"Even The Register likes it.",
             "field.actions.create": u"Add Series",
             }
-        with person_logged_in(user):
-            create_initialized_view(distribution, "+addseries", form=form)
-        distroseries = distribution.getSeries(u"polished")
+        with person_logged_in(self.user):
+            create_initialized_view(self.distribution, "+addseries",
+                                    form=form)
+        distroseries = self.distribution.getSeries(u"polished")
+        return distroseries
+
+    def assertCreated(self, distroseries):
         self.assertEqual(u"polished", distroseries.name)
         self.assertEqual(u"12.04", distroseries.version)
         self.assertEqual(u"Polished Polecat", distroseries.displayname)
         self.assertEqual(u"Polished Polecat", distroseries.title)
         self.assertEqual(u"Even The Register likes it.", distroseries.summary)
         self.assertEqual(u"", distroseries.description)
+        self.assertEqual(self.user, distroseries.owner)
+
+    def test_plain_submit(self):
+        # When creating a new DistroSeries via DistroSeriesAddView, the title
+        # is set to the same as the displayname (title is, in any case,
+        # deprecated), the description is left empty, and previous_series is
+        # None (DistroSeriesInitializeView takes care of setting that).
+        distroseries = self.createNewDistroseries()
+        self.assertCreated(distroseries)
         self.assertIs(None, distroseries.previous_series)
-        self.assertEqual(user, distroseries.owner)
+
+    def test_submit_sets_previous_series(self):
+        # Creating a new series when one already exists should set the
+        # previous_series.
+        old_series = self.factory.makeDistroSeries(self.distribution,
+                                                   version='11.10')
+        older_series = self.factory.makeDistroSeries(self.distribution,
+                                                     version='11.04')
+        old_time = utc_now() - timedelta(days=5)
+        removeSecurityProxy(old_series).datereleased = old_time
+        distroseries = self.createNewDistroseries()
+        self.assertEqual(old_series, distroseries.previous_series)
 
 
 class TestDistroSeriesInitializeView(TestCaseWithFactory):

=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2011-07-05 10:47:28 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2011-07-07 08:59:47 +0000
@@ -360,9 +360,8 @@
             automatically upgrade within backports, but not into it.
             """))
 
-    # other properties
-    prior_series = Attribute(
-        "Prior series *by date* from the same distribution.")
+    def priorReleasedSeries():
+        """Prior series *by date* from the same distribution."""
 
     main_archive = exported(
         Reference(
@@ -1070,6 +1069,18 @@
         released == None will do no filtering on status.
         """
 
+    def priorReleasedSeries(self, distribution, prior_to_date):
+        """Find distroseries for the supplied distro  released before a
+        certain date.
+
+        :param distribution: An `IDistribution` in which to search for its
+            series.
+        :param prior_to_date: A `datetime`
+
+        :return: `IResultSet` of `IDistroSeries` that were released before
+            prior_to_date, ordered in increasing order of age.
+        """
+
 
 @error_status(httplib.BAD_REQUEST)
 class DerivationError(Exception):

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-07-04 16:01:04 +0000
+++ lib/lp/registry/model/distroseries.py	2011-07-07 08:59:47 +0000
@@ -688,22 +688,14 @@
             orderBy=["Language.englishname"])
         return result
 
-    @cachedproperty
-    def prior_series(self):
+    def priorReleasedSeries(self):
         """See `IDistroSeries`."""
-        # This property is cached because it is used intensely inside
-        # sourcepackage.py; avoiding regeneration reduces a lot of
-        # count(*) queries.
         datereleased = self.datereleased
         # if this one is unreleased, use the last released one
         if not datereleased:
-            datereleased = 'NOW'
-        results = DistroSeries.select('''
-                distribution = %s AND
-                datereleased < %s
-                ''' % sqlvalues(self.distribution.id, datereleased),
-                orderBy=['-datereleased'])
-        return list(results)
+            datereleased = UTC_NOW
+        return getUtility(IDistroSeriesSet).priorReleasedSeries(
+            self.distribution, datereleased)
 
     @property
     def bug_reporting_guidelines(self):
@@ -2030,4 +2022,17 @@
         if orderBy is not None:
             return DistroSeries.select(where_clause, orderBy=orderBy)
         else:
+
             return DistroSeries.select(where_clause)
+
+    def priorReleasedSeries(self, distribution, prior_to_date):
+            """See `IDistroSeriesSet`."""
+            store = Store.of(distribution)
+            results = store.find(
+                DistroSeries,
+                DistroSeries.distributionID == distribution.id,
+                DistroSeries.datereleased < prior_to_date,
+                DistroSeries.datereleased != None
+            ).order_by(Desc(DistroSeries.datereleased))
+
+            return results

=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py	2011-06-08 15:00:44 +0000
+++ lib/lp/registry/model/sourcepackage.py	2011-07-07 08:59:47 +0000
@@ -44,7 +44,6 @@
     BugTargetBase,
     HasBugHeatMixin,
     )
-from lp.bugs.model.bugtask import BugTask
 from lp.buildmaster.enums import BuildStatus
 from lp.code.model.seriessourcepackagebranch import (
     SeriesSourcePackageBranchSet,
@@ -428,9 +427,9 @@
         # if we are an ubuntu sourcepackage, try the previous series of
         # ubuntu
         if self.distribution == ubuntu:
-            ubuntuseries = self.distroseries.prior_series
-            if ubuntuseries:
-                previous_ubuntu_series = ubuntuseries[0]
+            ubuntuseries = self.distroseries.priorReleasedSeries()
+            previous_ubuntu_series = ubuntuseries.first()
+            if previous_ubuntu_series is not None:
                 sp = SourcePackage(sourcepackagename=self.sourcepackagename,
                                    distroseries=previous_ubuntu_series)
                 return sp.packaging
@@ -500,7 +499,8 @@
         """See `IBugTarget`."""
         return self.distroseries.getUsedBugTags()
 
-    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None):
+    def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0,
+                                     include_tags=None):
         """See IBugTarget."""
         # Circular fail.
         from lp.bugs.model.bugsummary import BugSummary

=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py	2011-07-04 15:40:04 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2011-07-07 08:59:47 +0000
@@ -5,6 +5,9 @@
 
 __metaclass__ = type
 
+from datetime import (
+    timedelta,
+)
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -20,6 +23,7 @@
 from lp.registry.errors import NoSuchDistroSeries
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.utils import utc_now
 from lp.soyuz.enums import (
     ArchivePurpose,
     PackagePublishingStatus,
@@ -256,6 +260,35 @@
             distroseries=distroseries, archive=distroseries.main_archive)
         self.assertTrue(distroseries.isInitialized())
 
+    def test_priorReleasedSeries(self):
+        # Make sure that previousReleasedSeries returns all series with a
+        # release date less than the contextual series,
+        # ordered by descending date.
+        distro = self.factory.makeDistribution()
+        unreleased = self.factory.makeDistroSeries(distribution=distro)
+        ds1 = self.factory.makeDistroSeries(distribution=distro)
+        ds2 = self.factory.makeDistroSeries(distribution=distro)
+        ds3 = self.factory.makeDistroSeries(distribution=distro)
+        ds4 = self.factory.makeDistroSeries(distribution=distro)
+
+        now = utc_now()
+        older = now - timedelta(days=5)
+        oldest = now - timedelta(days=10)
+        newer = now + timedelta(days=15)
+        removeSecurityProxy(ds1).datereleased = oldest
+        removeSecurityProxy(ds2).datereleased = older
+        removeSecurityProxy(ds3).datereleased = now
+        removeSecurityProxy(ds4).datereleased = newer
+
+        # The data set up here is 5 distroseries. where one is unreleased,
+        # ds1 and ds2 are released and in the past and ds4 is released but
+        # in the future compared to ds3.
+
+        prior = ds3.priorReleasedSeries()
+        self.assertEqual(
+            [ds2, ds1],
+            list(prior))
+
     def test_getDifferenceComments_gets_DistroSeriesDifferenceComments(self):
         distroseries = self.factory.makeDistroSeries()
         dsd = self.factory.makeDistroSeriesDifference(