Merge lp:~salgado/launchpad/breadcrumbs-for-leafs into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

According to <https://wiki.canonical.com/Launchpad/UI/Navigation>, we'll
want breadcrumbs for the current page when it's not the default one for
the context, so this branch adds that.

== Proposed fix ==

Add a new method to the Hierarchy view, which builds an extra breadcrumb
for the requested page. This breadcrumb is then appended to the list of
breadcrumbs returned by Hierarchy.items

== Implementation details ==

We should be using the page's title as the breadcrumb's text, but that's
not easy to get programmatically as we need the template name to look up
the page title in pagetitles.py. Once our views have a page_title
attribute and we get rid of pagetitles.py, it'll be easy to change the
breadcrumb to use the page's title instead of its name.

== Tests ==

./bin/tests -vvt test_breadcrumbs

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/stories/branches/xx-branch-deletion.txt
  lib/canonical/launchpad/webapp/tests/test_breadcrumbs.py
  lib/lp/bugs/stories/bugs/xx-link-bug-to-branch.txt
  lib/lp/bugs/stories/bug-release-management/nomination-navigation.txt
  lib/canonical/launchpad/browser/launchpad.py
  lib/canonical/launchpad/webapp/breadcrumb.py
  lib/lp/bugs/stories/bugs/xx-bug-activity.txt
  lib/lp/registry/stories/foaf/xx-admin-person-review.txt
  lib/lp/registry/stories/product/xx-product-files.txt
  lib/lp/soyuz/stories/soyuz/xx-distribution-archives.txt
  lib/lp/bugs/browser/tests/test_breadcrumbs.py

== Pylint notices ==

lib/canonical/launchpad/browser/launchpad.py
    106: [F0401] Unable to import 'lazr.uri' (No module named uri)

[Brad Crittenden (bac)]

This change looks great Salgado.

review: Approve (code)

[Brad Crittenden (bac)]

This change looks great Salgado.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/browser/launchpad.py'
--- lib/canonical/launchpad/browser/launchpad.py	2009-08-26 07:59:49 +0000
+++ lib/canonical/launchpad/browser/launchpad.py	2009-09-01 18:06:25 +0000
@@ -34,6 +34,7 @@
 import urllib
 from datetime import timedelta, datetime
 
+from zope.app import zapi
 from zope.datetime import parseDatetimetz, tzinfo, DateTimeError
 from zope.component import getUtility, queryAdapter
 from zope.interface import implements
@@ -96,6 +97,7 @@
     LaunchpadFormView, LaunchpadView, Link, Navigation,
     StandardLaunchpadFacets, canonical_name, canonical_url, custom_widget,
     stepto)
+from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp.interfaces import (
     IBreadcrumb, ILaunchBag, ILaunchpadRoot, INavigationMenu,
     NotFoundError, POSTToNonCanonicalURL)
@@ -234,25 +236,48 @@
                 breadcrumbs.append(breadcrumb)
 
         host = URI(self.request.getURL()).host
-        if (len(breadcrumbs) == 0
-            or host == allvhosts.configs['mainsite'].hostname):
-            return breadcrumbs
-
-        # If we got this far it means we have breadcrumbs and we're not on the
-        # mainsite, so we'll sneak an extra breadcrumb for the vhost we're on.
-        vhost = host.split('.')[0]
-
-        # Iterate over the context of our breadcrumbs in reverse order and for
-        # the first one we find an adapter named after the vhost we're on,
-        # generate an extra breadcrumb and insert it in our list.
-        for idx, breadcrumb in reversed(list(enumerate(breadcrumbs))):
-            extra_breadcrumb = queryAdapter(
-                breadcrumb.context, IBreadcrumb, name=vhost)
-            if extra_breadcrumb is not None:
-                breadcrumbs.insert(idx + 1, extra_breadcrumb)
-                break
+        mainhost = allvhosts.configs['mainsite'].hostname
+        if len(breadcrumbs) != 0 and host != mainhost:
+            # We have breadcrumbs and we're not on the mainsite, so we'll
+            # sneak an extra breadcrumb for the vhost we're on.
+            vhost = host.split('.')[0]
+
+            # Iterate over the context of our breadcrumbs in reverse order and
+            # for the first one we find an adapter named after the vhost we're
+            # on, generate an extra breadcrumb and insert it in our list.
+            for idx, breadcrumb in reversed(list(enumerate(breadcrumbs))):
+                extra_breadcrumb = queryAdapter(
+                    breadcrumb.context, IBreadcrumb, name=vhost)
+                if extra_breadcrumb is not None:
+                    breadcrumbs.insert(idx + 1, extra_breadcrumb)
+                    break
+        if len(breadcrumbs):
+            page_crumb = self.makeBreadcrumbForRequestedPage()
+            if page_crumb:
+                breadcrumbs.append(page_crumb)
         return breadcrumbs
 
+    def makeBreadcrumbForRequestedPage(self):
+        """Return an `IBreadcrumb` for the requested page.
+
+        The `IBreadcrumb` for the requested page is created using the current
+        URL and the page's name (i.e. the last path segment of the URL).
+
+        If the requested page (as specified in self.request) is the default
+        one for the last traversed object, return None.
+        """
+        url = self.request.getURL()
+        last_segment = URI(url).path.split('/')[-1]
+        default_view_name = zapi.getDefaultViewName(
+            self.request.traversed_objects[-1], self.request)
+        if last_segment.startswith('+') and last_segment != default_view_name:
+            breadcrumb = Breadcrumb(None)
+            breadcrumb._url = url
+            breadcrumb.text = last_segment
+            return breadcrumb
+        else:
+            return None
+
     @property
     def display_breadcrumbs(self):
         """Return whether the breadcrumbs should be displayed."""
@@ -260,6 +285,7 @@
         # to display it as it will simply repeat the context.title.
         return len(self.items) > 1
 
+
 class MaintenanceMessage:
     """Display a maintenance message if the control file is present and
     it contains a valid iso format time.

=== modified file 'lib/canonical/launchpad/webapp/breadcrumb.py'
--- lib/canonical/launchpad/webapp/breadcrumb.py	2009-08-25 12:35:23 +0000
+++ lib/canonical/launchpad/webapp/breadcrumb.py	2009-08-31 17:40:56 +0000
@@ -27,6 +27,7 @@
     implements(IBreadcrumb)
 
     text = None
+    _url = None
 
     def __init__(self, context):
         self.context = context
@@ -46,7 +47,10 @@
 
     @property
     def url(self):
-        return canonical_url(self.context, rootsite=self.rootsite)
+        if self._url is None:
+            return canonical_url(self.context, rootsite=self.rootsite)
+        else:
+            return self._url
 
     @property
     def icon(self):

=== modified file 'lib/canonical/launchpad/webapp/tests/test_breadcrumbs.py'
--- lib/canonical/launchpad/webapp/tests/test_breadcrumbs.py	2009-08-25 12:35:23 +0000
+++ lib/canonical/launchpad/webapp/tests/test_breadcrumbs.py	2009-08-31 17:40:56 +0000
@@ -37,6 +37,29 @@
         self.assertEquals(Breadcrumb(cookbook).rootsite, 'cooking')
 
 
+class TestExtraBreadcrumbForLeafPageOnHierarchyView(BaseBreadcrumbTestCase):
+    """When the current page is not the object's default one (+index), we add
+    an extra breadcrumb for it.
+    """
+
+    def setUp(self):
+        super(TestExtraBreadcrumbForLeafPageOnHierarchyView, self).setUp()
+        login('[e-mail]')
+        self.product = self.factory.makeProduct(name='crumb-tester')
+        self.product_url = canonical_url(self.product)
+
+    def test_default_page(self):
+        urls = self._getBreadcrumbsURLs(
+            self.product_url, [self.root, self.product])
+        self.assertEquals(urls, [self.product_url])
+
+    def test_non_default_page(self):
+        downloads_url = "%s/+download" % self.product_url
+        urls = self._getBreadcrumbsURLs(
+            downloads_url, [self.root, self.product])
+        self.assertEquals(urls, [self.product_url, downloads_url])
+
+
 class TestExtraVHostBreadcrumbsOnHierarchyView(BaseBreadcrumbTestCase):
     """How our breadcrumbs behave when using a vhost other than the main one?
 

=== modified file 'lib/lp/bugs/browser/tests/test_breadcrumbs.py'
--- lib/lp/bugs/browser/tests/test_breadcrumbs.py	2009-08-24 20:28:33 +0000
+++ lib/lp/bugs/browser/tests/test_breadcrumbs.py	2009-08-31 17:40:56 +0000
@@ -35,9 +35,11 @@
         url = canonical_url(
             self.bug.default_bugtask, rootsite='bugs', view_name='+activity')
         urls = self._getBreadcrumbsURLs(url, self.traversed_objects)
-        self.assertEquals(urls[-1], self.bugtask_url)
+        self.assertEquals(urls[-1], "%s/+activity" % self.bugtask_url)
+        self.assertEquals(urls[-2], self.bugtask_url)
         texts = self._getBreadcrumbsTexts(url, self.traversed_objects)
-        self.assertEquals(texts[-1], "Bug #%d" % self.bug.id)
+        self.assertEquals(texts[-1], "+activity")
+        self.assertEquals(texts[-2], "Bug #%d" % self.bug.id)
 
     def test_bugtask_private_bug(self):
         # A breadcrumb is not generated for a bug that the user does
@@ -45,8 +47,7 @@
         login('[e-mail]')
         self.bug.setPrivate(True, self.bug.owner)
         login(ANONYMOUS)
-        url = canonical_url(
-            self.bug.default_bugtask, rootsite='bugs', view_name='+activity')
+        url = canonical_url(self.bug.default_bugtask, rootsite='bugs')
         self.assertEquals(
             ['http://launchpad.dev/crumb-tester',
              'http://bugs.launchpad.dev/crumb-tester'],

=== modified file 'lib/lp/bugs/stories/bug-release-management/nomination-navigation.txt'
--- lib/lp/bugs/stories/bug-release-management/nomination-navigation.txt	2009-08-27 08:32:39 +0000
+++ lib/lp/bugs/stories/bug-release-management/nomination-navigation.txt	2009-09-01 17:31:05 +0000
@@ -12,7 +12,7 @@
     ... '/nominations/2/+editstatus')
     >>> print_location(admin_browser.contents)
     Hierarchy: Ubuntu > ?mozilla-firefox? package > 
-        Bugs on mozilla-firefox > Bug #1
+        Bugs on mozilla-firefox > Bug #1...
     Tabs:
     * Overview - http://launchpad.dev/ubuntu/+source/mozilla-firefox
     * Code - http://code.launchpad.dev/ubuntu/+source/mozilla-firefox

=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-activity.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-activity.txt	2009-08-28 14:31:15 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-activity.txt	2009-09-01 17:31:05 +0000
@@ -7,13 +7,12 @@
     ...     'mozilla-firefox/+bug/3/+activity')
     >>> main_content = find_main_content(anon_browser.contents)
 
-
 The page contains a link back to the bug page in the breadcrumbs and
 the main heading repeats the bug number for clarity:
 
     >>> print_location(anon_browser.contents)
     Hierarchy: Debian > ?mozilla-firefox? package >
-        Bugs on mozilla-firefox > Bug #3
+        Bugs on mozilla-firefox > Bug #3...
     Tabs:
     * Overview - http://launchpad.dev/debian/+source/mozilla-firefox
     * Code - http://code.launchpad.dev/debian/+source/mozilla-firefox

=== modified file 'lib/lp/bugs/stories/bugs/xx-link-bug-to-branch.txt'
--- lib/lp/bugs/stories/bugs/xx-link-bug-to-branch.txt	2009-08-28 11:12:06 +0000
+++ lib/lp/bugs/stories/bugs/xx-link-bug-to-branch.txt	2009-09-01 17:31:05 +0000
@@ -57,6 +57,7 @@
     >>> link.click()
     >>> print extract_text(find_tag_by_id(
     ...     user_browser.contents, 'maincontent'))
+    Mozilla Firefox...
     Remove bug branch link
     Are you sure you want to remove the link between Bug #1: Firefox does
     not support SVG and the branch lp://dev/~name12/firefox/main?

=== modified file 'lib/lp/code/stories/branches/xx-branch-deletion.txt'
--- lib/lp/code/stories/branches/xx-branch-deletion.txt	2009-08-12 00:58:24 +0000
+++ lib/lp/code/stories/branches/xx-branch-deletion.txt	2009-09-01 17:31:05 +0000
@@ -25,6 +25,7 @@
     >>> delete_link.click()
     >>> print extract_text(find_main_content(browser.contents))
     Delete branch
+    Mozilla Firefox...
     Branch deletion is permanent.
     or Cancel
 
@@ -68,7 +69,7 @@
     >>> admin_browser.open(branch_location)
     >>> admin_browser.getLink('Delete branch').click()
     >>> print extract_text(find_main_content(admin_browser.contents))
-    Delete branch
+    Delete branch...
     This branch cannot be deleted as it has 1 branch sharing revisions.
 
 However, you can delete a branch that's the official branch of a source

=== modified file 'lib/lp/registry/stories/foaf/xx-admin-person-review.txt'
--- lib/lp/registry/stories/foaf/xx-admin-person-review.txt	2009-08-01 00:02:55 +0000
+++ lib/lp/registry/stories/foaf/xx-admin-person-review.txt	2009-09-01 17:31:05 +0000
@@ -47,7 +47,7 @@
 
 The page also contains a link back to the +review page.
 
-    >>> link = admin_browser.getLink(url='+review')
+    >>> link = admin_browser.getLink(url='+review', index=1)
     >>> print link.text
     edit[IMG] Review the user's Launchpad information
 

=== modified file 'lib/lp/registry/stories/product/xx-product-files.txt'
--- lib/lp/registry/stories/product/xx-product-files.txt	2009-08-27 17:50:21 +0000
+++ lib/lp/registry/stories/product/xx-product-files.txt	2009-09-01 17:31:05 +0000
@@ -98,6 +98,7 @@
     >>> main_content = find_main_content(tbird_owner.contents)
     >>> print extract_text(main_content)
     Download project files
+    Mozilla Thunderbird...
     No download files exist for this project...
     Add download file for release: 0.8
     >>> tbird_owner.getControl('Delete Files')

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-distribution-archives.txt'
--- lib/lp/soyuz/stories/soyuz/xx-distribution-archives.txt	2009-08-12 13:00:09 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-distribution-archives.txt	2009-09-01 17:31:05 +0000
@@ -52,6 +52,7 @@
     >>> main_content = find_main_content(anon_browser.contents)
     >>> print extract_text(main_content)
     Copy Archives related to Ubuntu Linux
+    Ubuntu...
     'Copy' archives containing packages copied from other archives
     (the main archive or PPAs) for a distribution.
     ...