launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19560
[Merge] lp:~cjwatson/launchpad/singleton-listing-no-redirect into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/singleton-listing-no-redirect into lp:launchpad.
Commit message:
Link directly to the recipe/snap in the one-recipe/snap case, rather than redirecting from +recipes/+snaps listing views.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #891918 in Launchpad itself: "redirect from /~/+recipes to recipe when only one recipe can be confusing"
https://bugs.launchpad.net/launchpad/+bug/891918
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/singleton-listing-no-redirect/+merge/273405
Link directly to the recipe/snap in the one-recipe/snap case, rather than redirecting from +recipes/+snaps listing views. Listing views should show listings even if they have only one item.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/singleton-listing-no-redirect into lp:launchpad.
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py 2015-09-29 18:02:29 +0000
+++ lib/lp/code/browser/branch.py 2015-10-05 13:41:26 +0000
@@ -523,14 +523,22 @@
if check_permission('launchpad.View', proposal)]
@property
- def recipe_count_text(self):
+ def recipes_link(self):
+ """A link to recipes for this branch."""
count = self.context.recipes.count()
if count == 0:
- return 'No recipes'
+ # Nothing to link to.
+ return 'No recipes using this branch.'
elif count == 1:
- return '1 recipe'
+ # Link to the single recipe.
+ return structured(
+ '<a href="%s">1 recipe</a> using this branch.',
+ canonical_url(self.context.recipes.one())).escapedtext
else:
- return '%s recipes' % count
+ # Link to a recipe listing.
+ return structured(
+ '<a href="+recipes">%s recipes</a> using this branch.',
+ count).escapedtext
@property
def is_import_branch_with_no_landing_candidates(self):
=== modified file 'lib/lp/code/browser/sourcepackagerecipelisting.py'
--- lib/lp/code/browser/sourcepackagerecipelisting.py 2015-02-24 13:46:46 +0000
+++ lib/lp/code/browser/sourcepackagerecipelisting.py 2015-10-05 13:41:26 +0000
@@ -16,7 +16,6 @@
from lp.code.browser.decorations import DecoratedBranch
from lp.services.feeds.browser import FeedsMixin
from lp.services.webapp import (
- canonical_url,
LaunchpadView,
Link,
)
@@ -46,13 +45,6 @@
return 'Source Package Recipes for %(displayname)s' % {
'displayname': self.context.displayname}
- def initialize(self):
- super(RecipeListingView, self).initialize()
- recipes = self.context.recipes
- if recipes.count() == 1:
- recipe = recipes.one()
- self.request.response.redirect(canonical_url(recipe))
-
class BranchRecipeListingView(RecipeListingView):
=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py 2015-09-18 10:15:54 +0000
+++ lib/lp/code/browser/tests/test_branch.py 2015-10-05 13:41:26 +0000
@@ -266,6 +266,32 @@
login_person(branch.owner)
self.assertFalse(view.user_can_upload)
+ def test_recipes_link_no_recipes(self):
+ # A branch with no recipes does not show a recipes link.
+ branch = self.factory.makeAnyBranch()
+ view = create_initialized_view(branch, '+index')
+ self.assertEqual('No recipes using this branch.', view.recipes_link)
+
+ def test_recipes_link_one_recipe(self):
+ # A branch with one recipe shows a link to that recipe.
+ branch = self.factory.makeAnyBranch()
+ recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
+ view = create_initialized_view(branch, '+index')
+ expected_link = (
+ '<a href="%s">1 recipe</a> using this branch.' %
+ canonical_url(recipe))
+ self.assertEqual(expected_link, view.recipes_link)
+
+ def test_recipes_link_more_recipes(self):
+ # A branch with more than one recipe shows a link to a listing.
+ branch = self.factory.makeAnyBranch()
+ self.factory.makeSourcePackageRecipe(branches=[branch])
+ self.factory.makeSourcePackageRecipe(branches=[branch])
+ view = create_initialized_view(branch, '+index')
+ self.assertEqual(
+ '<a href="+recipes">2 recipes</a> using this branch.',
+ view.recipes_link)
+
def _addBugLinks(self, branch):
for status in BugTaskStatus.items:
bug = self.factory.makeBug(status=status)
=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py 2015-02-24 13:46:46 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py 2015-10-05 13:41:26 +0000
@@ -22,11 +22,9 @@
layer = DatabaseFunctionalLayer
def test_project_branch_recipe_listing(self):
- # We can see recipes for the project. We need to create two, since
- # only one will redirect to that recipe.
+ # We can see recipes for the project.
branch = self.factory.makeProductBranch()
recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
- self.factory.makeSourcePackageRecipe(branches=[branch])
text = self.getMainText(recipe.base_branch, '+recipes')
self.assertTextMatchesExpressionIgnoreWhitespace("""
Source Package Recipes for lp:.*
@@ -34,11 +32,9 @@
spr-name.* Person-name""", text)
def test_package_branch_recipe_listing(self):
- # We can see recipes for the package. We need to create two, since
- # only one will redirect to that recipe.
+ # We can see recipes for the package.
branch = self.factory.makePackageBranch()
recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
- self.factory.makeSourcePackageRecipe(branches=[branch])
text = self.getMainText(recipe.base_branch, '+recipes')
self.assertTextMatchesExpressionIgnoreWhitespace("""
Source Package Recipes for lp:.*
=== modified file 'lib/lp/code/templates/branch-recipes.pt'
--- lib/lp/code/templates/branch-recipes.pt 2012-06-15 16:23:50 +0000
+++ lib/lp/code/templates/branch-recipes.pt 2015-10-05 13:41:26 +0000
@@ -10,15 +10,7 @@
<div id="recipe-links" class="actions">
<div id="recipe-summary">
<img src="/@@/source-package-recipe" />
- <tal:no-recipes condition="not: view/context/recipes/count">
- No recipes
- </tal:no-recipes>
- <tal:recipes condition="view/context/recipes/count">
- <a href="+recipes" tal:content="structure view/recipe_count_text">
- 1 branch
- </a>
- </tal:recipes>
- using this branch.
+ <tal:recipes replace="structure view/recipes_link" />
<a href="/+help-code/related-recipes.html" target="help"
class="sprite maybe action-icon">(?)</a>
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2015-10-01 10:25:19 +0000
+++ lib/lp/registry/browser/person.py 2015-10-05 13:41:26 +0000
@@ -274,10 +274,7 @@
from lp.services.webapp.publisher import LaunchpadView
from lp.services.worlddata.interfaces.country import ICountry
from lp.services.worlddata.interfaces.language import ILanguageSet
-from lp.snappy.browser.hassnaps import (
- HasSnapsMenuMixin,
- HasSnapsViewMixin,
- )
+from lp.snappy.browser.hassnaps import HasSnapsMenuMixin
from lp.snappy.interfaces.snap import ISnapSet
from lp.soyuz.browser.archivesubscription import (
traverse_archive_subscription_for_subscriber,
@@ -1653,8 +1650,7 @@
raise AssertionError('Unknown group to contact.')
-class PersonView(LaunchpadView, FeedsMixin, ContactViaWebLinksMixin,
- HasSnapsViewMixin):
+class PersonView(LaunchpadView, FeedsMixin, ContactViaWebLinksMixin):
"""A View class used in almost all Person's pages."""
@property
=== modified file 'lib/lp/snappy/browser/hassnaps.py'
--- lib/lp/snappy/browser/hassnaps.py 2015-09-18 10:38:27 +0000
+++ lib/lp/snappy/browser/hassnaps.py 2015-10-05 13:41:26 +0000
@@ -12,9 +12,13 @@
from zope.component import getUtility
from lp.code.browser.decorations import DecoratedBranch
+from lp.code.interfaces.gitrepository import IGitRepository
from lp.services.features import getFeatureFlag
-from lp.services.propertycache import cachedproperty
-from lp.services.webapp import Link
+from lp.services.webapp import (
+ canonical_url,
+ Link,
+ )
+from lp.services.webapp.escaping import structured
from lp.snappy.interfaces.snap import (
ISnapSet,
SNAP_FEATURE_FLAG,
@@ -45,24 +49,38 @@
class HasSnapsViewMixin:
"""A view mixin for objects that implement IHasSnaps."""
- @cachedproperty
- def snap_count(self):
+ @property
+ def snaps(self):
context = self.context
if isinstance(context, DecoratedBranch):
context = context.branch
return getUtility(ISnapSet).findByContext(
- context, visible_by_user=self.user).count()
+ context, visible_by_user=self.user)
@property
def show_snap_information(self):
- return bool(getFeatureFlag(SNAP_FEATURE_FLAG)) or self.snap_count != 0
+ return (
+ bool(getFeatureFlag(SNAP_FEATURE_FLAG)) or
+ not self.snaps.is_empty())
@property
- def snap_count_text(self):
- count = self.snap_count
+ def snaps_link(self):
+ """A link to snap packages for this object."""
+ count = self.snaps.count()
+ if IGitRepository.providedBy(self.context):
+ context_type = 'repository'
+ else:
+ context_type = 'branch'
if count == 0:
- return 'No snap packages'
+ # Nothing to link to.
+ return 'No snap packages using this %s.' % context_type
elif count == 1:
- return '1 snap package'
+ # Link to the single snap package.
+ return structured(
+ '<a href="%s">1 snap package</a> using this %s.',
+ canonical_url(self.snaps.one()), context_type).escapedtext
else:
- return '%s snap packages' % count
+ # Link to a snap package listing.
+ return structured(
+ '<a href="+snaps">%s snap packages</a> using this %s.',
+ count, context_type).escapedtext
=== modified file 'lib/lp/snappy/browser/snaplisting.py'
--- lib/lp/snappy/browser/snaplisting.py 2015-09-17 08:52:02 +0000
+++ lib/lp/snappy/browser/snaplisting.py 2015-10-05 13:41:26 +0000
@@ -18,10 +18,7 @@
from lp.code.browser.decorations import DecoratedBranch
from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.feeds.browser import FeedsMixin
-from lp.services.webapp import (
- canonical_url,
- LaunchpadView,
- )
+from lp.services.webapp import LaunchpadView
from lp.snappy.interfaces.snap import ISnapSet
@@ -48,9 +45,6 @@
loader = partial(
getUtility(ISnapSet).preloadDataForSnaps, user=self.user)
self.snaps = DecoratedResultSet(snaps, pre_iter_hook=loader)
- if self.snaps.count() == 1:
- snap = self.snaps.one()
- self.request.response.redirect(canonical_url(snap))
class BranchSnapListingView(SnapListingView):
=== added file 'lib/lp/snappy/browser/tests/test_hassnaps.py'
--- lib/lp/snappy/browser/tests/test_hassnaps.py 1970-01-01 00:00:00 +0000
+++ lib/lp/snappy/browser/tests/test_hassnaps.py 2015-10-05 13:41:26 +0000
@@ -0,0 +1,86 @@
+# Copyright 2015 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test views for objects that have snap packages."""
+
+__metaclass__ = type
+
+from lp.services.features.testing import FeatureFixture
+from lp.services.webapp import canonical_url
+from lp.snappy.interfaces.snap import SNAP_FEATURE_FLAG
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.views import create_initialized_view
+
+
+class TestRelatedSnapsMixin:
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestRelatedSnapsMixin, self).setUp()
+ self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+
+ def test_snaps_link_no_snaps(self):
+ # An object with no snap packages does not show a snap packages link.
+ context = self.makeContext()
+ view = create_initialized_view(context, "+index")
+ self.assertEqual(
+ "No snap packages using this %s." % self.context_type,
+ view.snaps_link)
+
+ def test_snaps_link_one_snap(self):
+ # An object with one snap package shows a link to that snap package.
+ context = self.makeContext()
+ snap = self.makeSnap(context)
+ view = create_initialized_view(context, "+index")
+ expected_link = (
+ '<a href="%s">1 snap package</a> using this %s.' %
+ (canonical_url(snap), self.context_type))
+ self.assertEqual(expected_link, view.snaps_link)
+
+ def test_snaps_link_more_snaps(self):
+ # An object with more than one snap package shows a link to a listing.
+ context = self.makeContext()
+ self.makeSnap(context)
+ self.makeSnap(context)
+ view = create_initialized_view(context, "+index")
+ expected_link = (
+ '<a href="+snaps">2 snap packages</a> using this %s.' %
+ self.context_type)
+ self.assertEqual(expected_link, view.snaps_link)
+
+
+class TestRelatedSnapsBranch(TestRelatedSnapsMixin, TestCaseWithFactory):
+
+ context_type = "branch"
+
+ def makeContext(self):
+ return self.factory.makeAnyBranch()
+
+ def makeSnap(self, context):
+ return self.factory.makeSnap(branch=context)
+
+
+class TestRelatedSnapsGitRepository(
+ TestRelatedSnapsMixin, TestCaseWithFactory):
+
+ context_type = "repository"
+
+ def makeContext(self):
+ return self.factory.makeGitRepository()
+
+ def makeSnap(self, context):
+ [ref] = self.factory.makeGitRefs(repository=context)
+ return self.factory.makeSnap(git_ref=ref)
+
+
+class TestRelatedSnapsGitRef(TestRelatedSnapsMixin, TestCaseWithFactory):
+
+ context_type = "branch"
+
+ def makeContext(self):
+ return self.factory.makeGitRefs()[0]
+
+ def makeSnap(self, context):
+ return self.factory.makeSnap(git_ref=context)
=== modified file 'lib/lp/snappy/browser/tests/test_snaplisting.py'
--- lib/lp/snappy/browser/tests/test_snaplisting.py 2015-09-16 13:30:33 +0000
+++ lib/lp/snappy/browser/tests/test_snaplisting.py 2015-10-05 13:41:26 +0000
@@ -55,20 +55,21 @@
self.assertThat(self.getViewBrowser(context).contents, Not(matcher))
login(ANONYMOUS)
self.makeSnap(**kwargs)
+ self.makeSnap(**kwargs)
self.assertThat(self.getViewBrowser(context).contents, matcher)
def test_branch_links_to_snaps(self):
branch = self.factory.makeAnyBranch()
- self.assertSnapsLink(branch, "1 snap package", branch=branch)
+ self.assertSnapsLink(branch, "2 snap packages", branch=branch)
def test_git_repository_links_to_snaps(self):
repository = self.factory.makeGitRepository()
[ref] = self.factory.makeGitRefs(repository=repository)
- self.assertSnapsLink(repository, "1 snap package", git_ref=ref)
+ self.assertSnapsLink(repository, "2 snap packages", git_ref=ref)
def test_git_ref_links_to_snaps(self):
[ref] = self.factory.makeGitRefs()
- self.assertSnapsLink(ref, "1 snap package", git_ref=ref)
+ self.assertSnapsLink(ref, "2 snap packages", git_ref=ref)
def test_person_links_to_snaps(self):
person = self.factory.makePerson()
@@ -83,54 +84,38 @@
project, "View snap packages", link_has_context=True, git_ref=ref)
def test_branch_snap_listing(self):
- # We can see snap packages for a Bazaar branch. We need to create
- # two, since if there's only one then +snaps will redirect to that
- # package.
+ # We can see snap packages for a Bazaar branch.
branch = self.factory.makeAnyBranch()
- for _ in range(2):
- self.makeSnap(branch=branch)
+ self.makeSnap(branch=branch)
text = self.getMainText(branch, "+snaps")
self.assertTextMatchesExpressionIgnoreWhitespace("""
Snap packages for lp:.*
Name Owner Registered
- snap-name.* Team Name.* .*
snap-name.* Team Name.* .*""", text)
def test_git_repository_snap_listing(self):
- # We can see snap packages for a Git repository. We need to create
- # two, since if there's only one then +snaps will redirect to that
- # package.
+ # We can see snap packages for a Git repository.
repository = self.factory.makeGitRepository()
- ref1, ref2 = self.factory.makeGitRefs(
- repository=repository,
- paths=[u"refs/heads/branch-1", u"refs/heads/branch-2"])
- for ref in ref1, ref2:
- self.makeSnap(git_ref=ref)
+ [ref] = self.factory.makeGitRefs(repository=repository)
+ self.makeSnap(git_ref=ref)
text = self.getMainText(repository, "+snaps")
self.assertTextMatchesExpressionIgnoreWhitespace("""
Snap packages for lp:~.*
Name Owner Registered
- snap-name.* Team Name.* .*
snap-name.* Team Name.* .*""", text)
def test_git_ref_snap_listing(self):
- # We can see snap packages for a Git reference. We need to create
- # two, since if there's only one then +snaps will redirect to that
- # package.
+ # We can see snap packages for a Git reference.
[ref] = self.factory.makeGitRefs()
- for _ in range(2):
- self.makeSnap(git_ref=ref)
+ self.makeSnap(git_ref=ref)
text = self.getMainText(ref, "+snaps")
self.assertTextMatchesExpressionIgnoreWhitespace("""
Snap packages for ~.*:.*
Name Owner Registered
- snap-name.* Team Name.* .*
snap-name.* Team Name.* .*""", text)
def test_person_snap_listing(self):
- # We can see snap packages for a person. We need to create two,
- # since if there's only one then +snaps will redirect to that
- # package.
+ # We can see snap packages for a person.
owner = self.factory.makePerson(displayname="Snap Owner")
self.makeSnap(
registrant=owner, owner=owner, branch=self.factory.makeAnyBranch(),
@@ -146,9 +131,7 @@
snap-name.* lp:.* .*""", text)
def test_project_snap_listing(self):
- # We can see snap packages for a project. We need to create two,
- # since if there's only one then +snaps will redirect to that
- # package.
+ # We can see snap packages for a project.
project = self.factory.makeProduct(displayname="Snappable")
self.makeSnap(
branch=self.factory.makeProductBranch(product=project),
=== modified file 'lib/lp/snappy/templates/snap-macros.pt'
--- lib/lp/snappy/templates/snap-macros.pt 2015-09-18 10:15:54 +0000
+++ lib/lp/snappy/templates/snap-macros.pt 2015-10-05 13:41:26 +0000
@@ -14,13 +14,7 @@
<div id="snap-links" class="actions">
<div id="snap-summary">
- <tal:no-snaps condition="not: view/snap_count">
- No snap packages
- </tal:no-snaps>
- <tal:snaps condition="view/snap_count">
- <a href="+snaps" tal:content="view/snap_count_text">1 snap package</a>
- </tal:snaps>
- using this <metal:slot define-slot="context_type">branch</metal:slot>.
+ <tal:snaps replace="structure view/snaps_link" />
</div>
</div>
Follow ups
