launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/archive-build-navigation into lp:launchpad.

Commit message:
Drop BuildNavigationMixin, in favour of more correct navigations on Archive, BuilderSet and DistributionSourcePackageRelease. BuilderSet now always redirects to the real URL, while the other two only traverse to builds that are legitimately in that context.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/archive-build-navigation/+merge/218363

Drop BuildNavigationMixin, in favour of more correct navigations on Archive, BuilderSet and DistributionSourcePackageRelease. BuilderSet now always redirects to the real URL, while the other two only traverse to builds that are legitimately in that context.

BuilderSet would previously traverse to recipe builds directly, and one could traverse to any binary or recipe build under any Archive or DSPR, or any TTB under any Branch.
-- 
https://code.launchpad.net/~wgrant/launchpad/archive-build-navigation/+merge/218363
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/archive-build-navigation into lp:launchpad.

=== modified file 'lib/lp/buildmaster/browser/builder.py'
--- lib/lp/buildmaster/browser/builder.py	2014-02-26 03:05:44 +0000
+++ lib/lp/buildmaster/browser/builder.py	2014-05-06 08:39:51 +0000
@@ -38,6 +38,9 @@
     IBuilderSet,
     )
 from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.code.interfaces.sourcepackagerecipebuild import (
+    ISourcePackageRecipeBuildSource,
+    )
 from lp.services.database.interfaces import IStore
 from lp.services.helpers import english_list
 from lp.services.propertycache import (
@@ -57,22 +60,29 @@
 from lp.services.webapp.batching import StormRangeFactory
 from lp.services.webapp.breadcrumb import Breadcrumb
 from lp.soyuz.browser.build import (
-    BuildNavigationMixin,
     BuildRecordsView,
+    get_build_by_name,
     )
-
-
-class BuilderSetNavigation(GetitemNavigation, BuildNavigationMixin):
+from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
+
+
+class BuilderSetNavigation(GetitemNavigation):
     """Navigation methods for IBuilderSet."""
     usedfor = IBuilderSet
 
     @stepthrough('+build')
     def traverse_build(self, name):
-        build = super(BuilderSetNavigation, self).traverse_build(name)
-        if build is None:
-            return None
-        else:
-            return self.redirectSubTree(canonical_url(build))
+        build = get_build_by_name(IBinaryPackageBuildSet, name)
+        if build is None:
+            return None
+        return self.redirectSubTree(canonical_url(build))
+
+    @stepthrough('+recipebuild')
+    def traverse_recipebuild(self, name):
+        build = get_build_by_name(ISourcePackageRecipeBuildSource, name)
+        if build is None:
+            return None
+        return self.redirectSubTree(canonical_url(build))
 
 
 class BuilderSetBreadcrumb(Breadcrumb):

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2014-04-02 22:43:37 +0000
+++ lib/lp/code/browser/branch.py	2014-05-06 08:39:51 +0000
@@ -83,7 +83,6 @@
     )
 from lp.app.browser.lazrjs import EnumChoiceWidget
 from lp.app.enums import InformationType
-from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.vocabularies import InformationTypeVocabulary
 from lp.app.widgets.itemswidgets import LaunchpadRadioWidgetWithDescription
@@ -107,7 +106,6 @@
     BranchType,
     CodeImportResultStatus,
     CodeImportReviewStatus,
-    RevisionControlSystems,
     )
 from lp.code.errors import (
     BranchCreationForbidden,
@@ -256,12 +254,11 @@
     @stepthrough("+translation-templates-build")
     def traverse_translation_templates_build(self, id_string):
         """Traverses to a `TranslationTemplatesBuild`."""
-        try:
-            ttbj_id = int(id_string)
-        except ValueError:
-            raise NotFoundError(id_string)
-        source = getUtility(ITranslationTemplatesBuildSource)
-        return source.getByID(ttbj_id)
+        from lp.soyuz.browser.build import get_build_by_name
+        build = get_build_by_name(ITranslationTemplatesBuildSource, id_string)
+        if build is None or build.branch != self.context:
+            return None
+        return build
 
 
 class BranchEditMenu(NavigationMenu):

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2013-09-12 05:19:43 +0000
+++ lib/lp/soyuz/browser/archive.py	2014-05-06 08:39:51 +0000
@@ -80,6 +80,9 @@
     )
 from lp.app.widgets.textwidgets import StrippedTextWidget
 from lp.buildmaster.enums import BuildStatus
+from lp.code.interfaces.sourcepackagerecipebuild import (
+    ISourcePackageRecipeBuildSource,
+    )
 from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -120,8 +123,8 @@
     ArchiveSourcePublications,
     )
 from lp.soyuz.browser.build import (
-    BuildNavigationMixin,
     BuildRecordsView,
+    get_build_by_name,
     )
 from lp.soyuz.browser.sourceslist import SourcesListEntriesWidget
 from lp.soyuz.browser.widgets.archive import PPANameWidget
@@ -142,7 +145,10 @@
     )
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
-from lp.soyuz.interfaces.binarypackagebuild import BuildSetStatus
+from lp.soyuz.interfaces.binarypackagebuild import (
+    BuildSetStatus,
+    IBinaryPackageBuildSet,
+    )
 from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packagecopyjob import IPlainPackageCopyJobSource
@@ -224,12 +230,25 @@
         return u"+archive/%s" % self.context.name
 
 
-class ArchiveNavigation(Navigation, FileNavigationMixin,
-                        BuildNavigationMixin):
+class ArchiveNavigation(Navigation, FileNavigationMixin):
     """Navigation methods for IArchive."""
 
     usedfor = IArchive
 
+    @stepthrough('+build')
+    def traverse_build(self, name):
+        build = get_build_by_name(IBinaryPackageBuildSet, name)
+        if build is None or build.archive != self.context:
+            return None
+        return build
+
+    @stepthrough('+recipebuild')
+    def traverse_recipebuild(self, name):
+        build = get_build_by_name(ISourcePackageRecipeBuildSource, name)
+        if build is None or build.archive != self.context:
+            return None
+        return build
+
     @stepthrough('+sourcepub')
     def traverse_sourcepub(self, name):
         return self._traverse_publication(name, source=True)
@@ -1052,7 +1071,7 @@
         count = job_source.getIncompleteJobsForArchive(self.context).count()
         if count > 5:
             return 'Showing 5 of %s' % count
-    
+
     @cachedproperty
     def has_append_perm(self):
         return check_permission('launchpad.Append', self.context)

=== modified file 'lib/lp/soyuz/browser/build.py'
--- lib/lp/soyuz/browser/build.py	2014-02-26 03:05:44 +0000
+++ lib/lp/soyuz/browser/build.py	2014-05-06 08:39:51 +0000
@@ -10,12 +10,12 @@
     'BuildCancelView',
     'BuildContextMenu',
     'BuildNavigation',
-    'BuildNavigationMixin',
     'BuildRecordsView',
     'BuildRescoringView',
     'BuildUrl',
     'BuildView',
     'DistributionBuildRecordsView',
+    'get_build_by_name',
     ]
 
 
@@ -54,9 +54,6 @@
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
-from lp.code.interfaces.sourcepackagerecipebuild import (
-    ISourcePackageRecipeBuildSource,
-    )
 from lp.services.librarian.browser import (
     FileNavigationMixin,
     ProxiedLibraryFileAlias,
@@ -69,7 +66,6 @@
     GetitemNavigation,
     LaunchpadView,
     Link,
-    stepthrough,
     )
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.batching import (
@@ -81,11 +77,27 @@
 from lp.soyuz.enums import PackageUploadStatus
 from lp.soyuz.interfaces.binarypackagebuild import (
     IBinaryPackageBuild,
-    IBinaryPackageBuildSet,
     IBuildRescoreForm,
     )
 
 
+def get_build_by_name(utility, name):
+    """Find a build by a utility interface and ID string.
+
+    Designed for Navigation implementations.
+
+    Returns None if the ID doesn't match a build.
+    """
+    try:
+        build_id = int(name)
+    except ValueError:
+        return None
+    try:
+        return getUtility(utility).getByID(build_id)
+    except NotFoundError:
+        return None
+
+
 class BuildUrl:
     """Dynamic URL declaration for IBinaryPackageBuild.
 
@@ -123,33 +135,6 @@
     usedfor = IBinaryPackageBuild
 
 
-class BuildNavigationMixin:
-    """Provide a simple way to traverse to builds."""
-
-    @stepthrough('+build')
-    def traverse_build(self, name):
-        try:
-            build_id = int(name)
-        except ValueError:
-            return None
-        try:
-            return getUtility(IBinaryPackageBuildSet).getByID(build_id)
-        except NotFoundError:
-            return None
-
-    @stepthrough('+recipebuild')
-    def traverse_recipebuild(self, name):
-        try:
-            build_id = int(name)
-        except ValueError:
-            return None
-        try:
-            return getUtility(ISourcePackageRecipeBuildSource).getByID(
-                build_id)
-        except NotFoundError:
-            return None
-
-
 class BuildContextMenu(ContextMenu):
     """Overview menu for build records """
     usedfor = IBinaryPackageBuild

=== modified file 'lib/lp/soyuz/browser/distributionsourcepackagerelease.py'
--- lib/lp/soyuz/browser/distributionsourcepackagerelease.py	2012-11-21 07:49:12 +0000
+++ lib/lp/soyuz/browser/distributionsourcepackagerelease.py	2014-05-06 08:39:51 +0000
@@ -23,10 +23,12 @@
 from lp.services.webapp import (
     LaunchpadView,
     Navigation,
+    stepthrough,
     )
 from lp.services.webapp.breadcrumb import Breadcrumb
-from lp.soyuz.browser.build import BuildNavigationMixin
+from lp.soyuz.browser.build import get_build_by_name
 from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.soyuz.interfaces.distributionsourcepackagerelease import (
     IDistributionSourcePackageRelease,
     )
@@ -40,10 +42,20 @@
         return self.context.version
 
 
-class DistributionSourcePackageReleaseNavigation(Navigation,
-                                                 BuildNavigationMixin):
+class DistributionSourcePackageReleaseNavigation(Navigation):
     usedfor = IDistributionSourcePackageRelease
 
+    @stepthrough('+build')
+    def traverse_build(self, name):
+        build = get_build_by_name(IBinaryPackageBuildSet, name)
+        if (build is None
+            or build.archive not in
+                self.context.distribution.all_distro_archives
+            or build.source_package_release !=
+                self.context.sourcepackagerelease):
+            return None
+        return build
+
 
 class DistributionSourcePackageReleaseView(LaunchpadView):
     """View logic for `DistributionSourcePackageRelease` objects. """