launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/add-link-to-noneformatter into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #714519 LocationError: 'link' on SourcePackageRecipeBuild:+index 
  https://bugs.launchpad.net/bugs/714519

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/add-link-to-noneformatter/+merge/50088

See bug 714519. The tales link formatter should be able to render objects with value None.

== Implementation ==

Enhance NoneFormatter to support traversing the "link" attribute of None objects. Returns "None".
Enhance format_link() helper to handle None objects, allowing a specified "None" value to be returned. Default is "None".

== Tests ==

There were no tests for NoneFormatter so I added these. Also added a test_none() to TestXHTMLRepresentations

bin/test -vvt test_noneformatter
bin/test -vvt test_web_service

== Lint ==

Linting changed files:
  lib/lp/app/browser/tales.py
  lib/lp/app/browser/webservice.py
  lib/lp/app/browser/tests/test_noneformatter.py
  lib/lp/app/browser/tests/test_webservice.py
  lib/lp/code/browser/sourcepackagerecipe.py

./lib/lp/app/browser/tests/test_webservice.py
      37: E501 line too long (80 characters)
      37: Line exceeds 78 characters.



-- 
https://code.launchpad.net/~wallyworld/launchpad/add-link-to-noneformatter/+merge/50088
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/add-link-to-noneformatter into lp:launchpad.

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2011-02-14 23:20:14 +0000
+++ lib/lp/app/browser/tales.py	2011-02-17 02:02:16 +0000
@@ -75,8 +75,10 @@
 SEPARATOR = ' : '
 
 
-def format_link(obj, view_name=None):
+def format_link(obj, view_name=None, empty_value='None'):
     """Return the equivalent of obj/fmt:link as a string."""
+    if obj is None:
+        return empty_value
     adapter = queryAdapter(obj, IPathAdapter, 'fmt')
     link = getattr(adapter, 'link', None)
     if link is None:
@@ -487,6 +489,8 @@
             # and not another traversal command.
             furtherPath.pop()
             return ''
+        elif name == 'link':
+            return 'None'
         elif name in self.allowed_names:
             return ''
         else:

=== added file 'lib/lp/app/browser/tests/test_noneformatter.py'
--- lib/lp/app/browser/tests/test_noneformatter.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/tests/test_noneformatter.py	2011-02-17 02:02:16 +0000
@@ -0,0 +1,65 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for lp.registry.browser.tales.NoneFormatter"""
+
+__metaclass__ = type
+
+
+from zope.component import queryAdapter
+from zope.traversing.interfaces import (
+    IPathAdapter,
+    TraversalError,
+    )
+
+from canonical.testing.layers import FunctionalLayer
+from lp.testing import TestCaseWithFactory
+
+
+class TestXHTMLRepresentations(TestCaseWithFactory):
+
+    layer = FunctionalLayer
+
+    def test_valid_traversal(self):
+        adapter = queryAdapter(None, IPathAdapter, 'fmt')
+        traverse = getattr(adapter, 'traverse', None)
+
+        allowed_names = set([
+            'approximatedate',
+            'approximateduration',
+            'break-long-words',
+            'date',
+            'datetime',
+            'displaydate',
+            'isodate',
+            'email-to-html',
+            'exactduration',
+            'lower',
+            'nice_pre',
+            'nl_to_br',
+            'pagetitle',
+            'rfc822utcdatetime',
+            'text-to-html',
+            'time',
+            'url',
+            ])
+
+        for name in allowed_names:
+            self.assertEqual('', traverse(name, []))
+
+    def test_invalid_traversal(self):
+        adapter = queryAdapter(None, IPathAdapter, 'fmt')
+        traverse = getattr(adapter, 'traverse', None)
+        self.failUnlessRaises(TraversalError, traverse, "foo", [])
+
+    def test_link(self):
+        adapter = queryAdapter(None, IPathAdapter, 'fmt')
+        traverse = getattr(adapter, 'traverse', None)
+        self.assertEqual('None', traverse('link', []))
+
+    def test_shorten_traversal(self):
+        adapter = queryAdapter(None, IPathAdapter, 'fmt')
+        traverse = getattr(adapter, 'traverse', None)
+        extra = ['1', '2']
+        self.assertEqual('', traverse('shorten', extra))
+        self.assertEqual(['1'], extra)

=== modified file 'lib/lp/app/browser/tests/test_webservice.py'
--- lib/lp/app/browser/tests/test_webservice.py	2011-01-20 22:46:06 +0000
+++ lib/lp/app/browser/tests/test_webservice.py	2011-02-17 02:02:16 +0000
@@ -20,6 +20,11 @@
 
     layer = DatabaseFunctionalLayer
 
+    def test_none(self):
+        # Test the XHTML representation of None
+        self.assertEqual(format_link(None), 'None')
+        self.assertEqual(format_link(None, empty_value=''), '')
+
     def test_person(self):
         # Test the XHTML representation of a person.
         eric = self.factory.makePerson()

=== modified file 'lib/lp/app/browser/webservice.py'
--- lib/lp/app/browser/webservice.py	2011-01-27 20:55:40 +0000
+++ lib/lp/app/browser/webservice.py	2011-02-17 02:02:16 +0000
@@ -30,13 +30,10 @@
     def render(value):
         # The value is a webservice link to the object, we want field value.
         obj = getattr(context, field.__name__, None)
-        if obj is None:
-            return ''
-        else:
-            try:
-                return format_link(obj)
-            except NotImplementedError:
-                return value
+        try:
+            return format_link(obj, empty_value='')
+        except NotImplementedError:
+            return value
     return render
 
 

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2011-02-15 11:31:56 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-02-17 02:02:16 +0000
@@ -253,10 +253,7 @@
     @property
     def archive_picker(self):
         ppa = self.context.daily_build_archive
-        if ppa is None:
-            initial_html = 'None'
-        else:
-            initial_html = format_link(ppa)
+        initial_html = format_link(ppa)
         field = ISourcePackageEditSchema['daily_build_archive']
         return InlineEditPickerWidget(
             self.context, field, initial_html,