launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/set-previous-series-bug-805913 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #805913 in Launchpad itself: "previous_series (parent_series in DB) is no longer set when opening a new distroseries"
  https://bugs.launchpad.net/launchpad/+bug/805913

For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/set-previous-series-bug-805913/+merge/66942

= 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.

== 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
-- 
https://code.launchpad.net/~julian-edwards/launchpad/set-previous-series-bug-805913/+merge/66942
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/set-previous-series-bug-805913 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-06-28 17:57:34 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-07-05 17:49:25 +0000
@@ -93,7 +93,10 @@
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
     )
-from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.distroseries import (
+    IDistroSeries,
+    IDistroSeriesSet,
+    )
 from lp.registry.interfaces.distroseriesdifference import (
     IDistroSeriesDifferenceSource,
     )
@@ -620,6 +623,10 @@
     @action(_('Add Series'), name='create')
     def createAndAdd(self, action, data):
         """Create and add a new Distribution Series"""
+        all_previous_series = getUtility(
+            IDistroSeriesSet).priorReleasedSeries(self.context, UTC_NOW)
+        previous_series = all_previous_series.first()
+        # previous_series will be None if there isn't one.
         distroseries = self.context.newSeries(
             name=data['name'],
             displayname=data['displayname'],
@@ -627,7 +634,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-06-28 17:57:34 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-07-05 17:49:25 +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,38 @@
             "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)
+        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-06-20 16:04:15 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2011-07-05 17:49:25 +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(
@@ -1046,6 +1045,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-06-17 14:23:52 +0000
+++ lib/lp/registry/model/distroseries.py	2011-07-05 17:49:25 +0000
@@ -685,22 +685,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):
@@ -2021,4 +2013,16 @@
         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
+            ).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-05 17:49:25 +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,7 +427,7 @@
         # if we are an ubuntu sourcepackage, try the previous series of
         # ubuntu
         if self.distribution == ubuntu:
-            ubuntuseries = self.distroseries.prior_series
+            ubuntuseries = self.distroseries.priorReleasedSeries()
             if ubuntuseries:
                 previous_ubuntu_series = ubuntuseries[0]
                 sp = SourcePackage(sourcepackagename=self.sourcepackagename,
@@ -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-06-14 19:49:18 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2011-07-05 17:49:25 +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,29 @@
             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()
+        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(ds4).datereleased = newer
+
+        prior = ds3.priorReleasedSeries()
+        self.assertEqual(
+            [ds2, ds1],
+            list(prior))
+
 
 class TestDistroSeriesPackaging(TestCaseWithFactory):