launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/soyuz/browser/distributionsourcepackagerelease.py'
> --- lib/lp/soyuz/browser/distributionsourcepackagerelease.py	2014-05-06 09:11:14 +0000
> +++ lib/lp/soyuz/browser/distributionsourcepackagerelease.py	2014-12-18 12:02:47 +0000
> @@ -21,6 +21,7 @@
>  from lp.services.librarian.browser import ProxiedLibraryFileAlias
>  from lp.services.propertycache import cachedproperty
>  from lp.services.webapp import (
> +    canonical_url,
>      LaunchpadView,
>      Navigation,
>      stepthrough,
> @@ -56,6 +57,13 @@
>              return None
>          return build
>  
> +    @stepthrough('+latestbuild')
> +    def redirect_latestbuild(self, name):
> +        build = self.context.getBuildsByArchTag(name).first()
> +        if build is not None:
> +            return self.redirectSubTree(canonical_url(build))
> +        return self.redirectSubTree(canonical_url(self.context))

The latest build can change, so this should be a 303 rather than the default 301.

> +
>  
>  class DistributionSourcePackageReleaseView(LaunchpadView):
>      """View logic for `DistributionSourcePackageRelease` objects. """
> 
> === modified file 'lib/lp/soyuz/browser/tests/test_distributionsourcepackagerelease.py'
> --- lib/lp/soyuz/browser/tests/test_distributionsourcepackagerelease.py	2014-11-09 01:07:27 +0000
> +++ lib/lp/soyuz/browser/tests/test_distributionsourcepackagerelease.py	2014-12-18 12:02:47 +0000
> @@ -10,11 +10,55 @@
>  
>  from zope.security.proxy import removeSecurityProxy
>  
> +from lp.services.webapp import canonical_url
>  from lp.testing import TestCaseWithFactory
> -from lp.testing.layers import LaunchpadFunctionalLayer
> +from lp.testing.layers import (
> +    DatabaseFunctionalLayer,
> +    LaunchpadFunctionalLayer,
> +    )
> +from lp.testing.publication import test_traverse
>  from lp.testing.views import create_initialized_view
>  
>  
> +class TestDistributionSourcePackageReleaseNavigation(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUpLatestBuildTests(self):
> +        distribution = self.factory.makeDistribution()
> +        dses = [
> +            self.factory.makeDistroSeries(distribution=distribution)
> +            for i in range(2)]
> +        spr = self.factory.makeSourcePackageRelease(distroseries=dses[0])
> +        proc = self.factory.makeProcessor()
> +        builds = []
> +        for i in range(2):
> +            self.factory.makeSourcePackagePublishingHistory(
> +                distroseries=dses[0], sourcepackagerelease=spr)
> +            das = self.factory.makeDistroArchSeries(
> +                distroseries=dses[i], architecturetag="arch", processor=proc)
> +            builds.append(self.factory.makeBinaryPackageBuild(
> +                source_package_release=spr, distroarchseries=das))
> +        builds.append(
> +            self.factory.makeBinaryPackageBuild(source_package_release=spr))
> +        return canonical_url(distribution.getSourcePackageRelease(spr)), builds
> +
> +    def test_latestbuild_known_arch(self):
> +        # +latestbuild traverses to the most recent build for the requested
> +        # architecture.

I'd say "redirects" rather than "traverses", for clarity.

> +        dspr_url, builds = self.setUpLatestBuildTests()
> +        _, view, _ = test_traverse("%s/+latestbuild/arch" % dspr_url)
> +        self.assertEqual(
> +            canonical_url(builds[1]), removeSecurityProxy(view).target)
> +
> +    def test_latestbuild_unknown_arch(self):
> +        # If there is no build for the requested architecture, +latestbuild
> +        # traverses to the context DSPR.

"redirects" here too.

> +        dspr_url, _ = self.setUpLatestBuildTests()
> +        obj, _, _ = test_traverse("%s/+latestbuild/unknown" % dspr_url)
> +        self.assertEqual(dspr_url, canonical_url(obj))
> +
> +
>  class TestDistributionSourcePackageReleaseFiles(TestCaseWithFactory):
>      """Source package release files are rendered correctly."""
>  
> 
> === modified file 'lib/lp/soyuz/interfaces/distributionsourcepackagerelease.py'
> --- lib/lp/soyuz/interfaces/distributionsourcepackagerelease.py	2014-11-09 23:28:27 +0000
> +++ lib/lp/soyuz/interfaces/distributionsourcepackagerelease.py	2014-12-18 12:02:47 +0000
> @@ -39,6 +39,9 @@
>          "be inherited from a parent distribution, not necessarily built "
>          "here, but must be published in a main archive.")
>  
> +    def getBuildsByArchTag(arch_tag):
> +        """Return the builds for this SPR on the given architecture."""
> +
>      sample_binary_packages = Attribute("A single binary package of each "
>          "named package produced from this source package in this "
>          "distribution. The are each of form DistroSeriesBinaryPackage.")
> 
> === modified file 'lib/lp/soyuz/model/distributionsourcepackagerelease.py'
> --- lib/lp/soyuz/model/distributionsourcepackagerelease.py	2014-11-27 20:52:37 +0000
> +++ lib/lp/soyuz/model/distributionsourcepackagerelease.py	2014-12-18 12:02:47 +0000
> @@ -30,6 +30,7 @@
>  from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
>  from lp.soyuz.model.binarypackagename import BinaryPackageName
>  from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
> +from lp.soyuz.model.distroarchseries import DistroArchSeries
>  from lp.soyuz.model.distroseriesbinarypackage import DistroSeriesBinaryPackage
>  from lp.soyuz.model.publishing import (
>      BinaryPackagePublishingHistory,
> @@ -90,9 +91,7 @@
>          return self.getPublishingHistories(
>              self.distribution, [self.sourcepackagerelease])
>  
> -    @property
> -    def builds(self):
> -        """See IDistributionSourcePackageRelease."""
> +    def _getBuilds(self, clauses=[]):
>          # First, get all the builds built in a main archive (this will
>          # include new and failed builds.)
>          builds_built_in_main_archives = Store.of(self.distribution).find(
> @@ -100,7 +99,8 @@
>              BinaryPackageBuild.source_package_release ==
>                  self.sourcepackagerelease,
>              BinaryPackageBuild.archive_id.is_in(
> -                self.distribution.all_distro_archive_ids))
> +                self.distribution.all_distro_archive_ids),
> +            *clauses)
>  
>          # Next get all the builds that have a binary published in the
>          # main archive... this will include many of those in the above
> @@ -114,14 +114,27 @@
>              BinaryPackagePublishingHistory.binarypackagerelease ==
>                  BinaryPackageRelease.id,
>              BinaryPackagePublishingHistory.archiveID.is_in(
> -                self.distribution.all_distro_archive_ids)).config(
> -                    distinct=True)
> +                self.distribution.all_distro_archive_ids),
> +            *clauses).config(distinct=True)
>  
>          return builds_built_in_main_archives.union(
>              builds_published_in_main_archives).order_by(
>                  Desc(BinaryPackageBuild.id))
>  
>      @property
> +    def builds(self):
> +        """See IDistributionSourcePackageRelease."""
> +        return self._getBuilds()
> +
> +    def getBuildsByArchTag(self, arch_tag):
> +        """See IDistributionSourcePackageRelease."""
> +        clauses = [
> +            BinaryPackageBuild.distro_arch_series_id == DistroArchSeries.id,
> +            DistroArchSeries.architecturetag == arch_tag,
> +            ]
> +        return self._getBuilds(clauses)
> +
> +    @property
>      def sample_binary_packages(self):
>          """See IDistributionSourcePackageRelease."""
>          #avoid circular imports.
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/dspr-latestbuild-archtag/+merge/245101
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.