Merge lp:~cjwatson/launchpad/singleton-listing-no-redirect into lp:launchpad

[Colin Watson]

Link directly to the recipe/snap in the one-recipe/snap case, rather than redirecting from +recipes/+snaps listing views.

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.

[William Grant (wgrant)]

Yes please.

review: Approve (code)

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