Code review comment for lp:~salgado/launchpad/breadcrumbs-for-leafs

Revision history for this message
Guilherme Salgado (salgado) wrote :

= Summary =

Change text of leaf breadcrumbs from the page name (+export, etc) to its
title, fixing https://bugs.edge.launchpad.net/bugs/423691

== Proposed fix ==

Previously, only objects which had a Navigation class would be appended
to request.traversed_objects upon traversal. From now on, any traversed
objects will be appended there, including the views. This fixes
https://bugs.edge.launchpad.net/bugs/423898

Since we now have the view in traversed_objects, we can just use
its .page_title (falling back to its entry on pagetitles.py) as the text
of the leaf breadcrumb. Like before, though, this leaf breadcrumb is
only added when the view is not the default one for the context.

== Implementation details ==

MultiStepViews don't define a .page_title because what actually gets
rendered is the (step) view that they contain, and those define
a .page_title. However, we don't want to do that when looking up the
text for the breadcrumbs, so MultiStepViews are now required to provide
a .page_title.

Some tests from doc/navigation.txt where moved to
webapp/tests/test_publication.py and others to
webapp/tests/test_servers.py

== Tests ==

./bin/test -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/canonical/launchpad/webapp/publication.py
  lib/canonical/launchpad/webapp/tests/test_publication.py
  lib/lp/translations/browser/tests/test_breadcrumbs.py
  lib/canonical/launchpad/webapp/tests/breadcrumbs.py
  lib/canonical/launchpad/webapp/publisher.py
  lib/canonical/launchpad/webapp/tests/test_breadcrumbs.py
  lib/canonical/launchpad/browser/multistep.py
  lib/lp/registry/browser/product.py
  lib/lp/soyuz/stories/soyuz/xx-builder-page.txt
  lib/canonical/launchpad/browser/launchpad.py
  lib/lp/bugs/browser/bugalsoaffects.py
  lib/lp/translations/browser/pofile.py
  lib/lp/soyuz/stories/ppa/xx-ppa-navigation.txt
  lib/lp/registry/stories/distribution/xx-distribution-packages.txt
  lib/lp/bugs/browser/cve.py
  lib/canonical/lazr/testing/menus.py
  lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt
  lib/lp/registry/browser/branding.py
  lib/canonical/launchpad/webapp/tales.py
  lib/canonical/launchpad/doc/navigation.txt
  lib/canonical/launchpad/doc/hierarchical-menu.txt
  lib/canonical/launchpad/webapp/tests/test_servers.py
  lib/lp/bugs/browser/tests/test_breadcrumbs.py
  lib/lp/soyuz/stories/soyuz/xx-buildfarm-index.txt

== Pylint notices ==

lib/canonical/launchpad/webapp/publication.py
    551: [E1002,
LaunchpadBrowserPublication.beginErrorHandlingTransaction] Use super on
an old style class

lib/lp/registry/browser/product.py
    57: [F0401] Unable to import 'lazr.delegates' (No module named
delegates)

lib/lp/bugs/browser/bugalsoaffects.py
    21: [F0401] Unable to import 'lazr.enum' (No module named enum)
    22: [F0401] Unable to import 'lazr.lifecycle.event' (No module named
lifecycle)

lib/canonical/launchpad/webapp/tales.py
    23: [F0401] Unable to import 'lazr.enum' (No module named enum)

« Back to merge proposal