launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/revision-author-none into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #718849 oopse branch/revision_author can be None
  https://bugs.launchpad.net/bugs/718849

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/revision-author-none/+merge/49731

Do not escape revision author if the name and email are None.

    Launchpad bug: https://bugs.launchpad.net/bugs/718849
    Pre-implementation: no one
    Test command: ./bin/test -vv -t TestRevisionAuthorFormatterAPI

OOPS-1871A1382 demonstrates that the branch revision author can be None.

My initial assumption was that this was bad data introduced from a merge.
This is not the case here. The old tales code worked because it just
rendered the email address, and tales knows that the value of None does
not need escaping. the new tales formatter also escapes the content, but
if does not check that there is something to escape.

--------------------------------------------------------------------

RULES

    * Update RevisionAuthorFormatterAPI to only escape values if they
      are not None.


QA

    * Visit https://code.launchpad.net/~registry/+branches
    * Verify that the autumn-engine development branch is listed
      (https://code.launchpad.net/~registry/autumn-engine/development)


LINT

    lib/lp/app/browser/tales.py
    lib/lp/code/browser/tests/test_revisionauthor.py


IMPLEMENTATION

Added a test to reproduce the case where RevisionAuthor name and email are
None. Updated the switch in the formatter to return an empty string when
there is no email to render.
    lib/lp/app/browser/tales.py
    lib/lp/code/browser/tests/test_revisionauthor.py
-- 
https://code.launchpad.net/~sinzui/launchpad/revision-author-none/+merge/49731
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/revision-author-none into lp:launchpad.

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2011-02-09 00:29:50 +0000
+++ lib/lp/app/browser/tales.py	2011-02-14 23:28:00 +0000
@@ -2266,10 +2266,13 @@
                 view_name, rootsite)
         elif context.name_without_email:
             return cgi.escape(context.name_without_email)
-        elif getUtility(ILaunchBag).user is not None:
+        elif context.email and getUtility(ILaunchBag).user is not None:
             return cgi.escape(context.email)
-        else:
+        elif context.email:
             return "<email address hidden>"
+        else:
+            # The RevisionAuthor name and email is None.
+            return ''
 
 
 def clean_path_segments(request):

=== modified file 'lib/lp/code/browser/tests/test_revisionauthor.py'
--- lib/lp/code/browser/tests/test_revisionauthor.py	2011-02-09 00:29:50 +0000
+++ lib/lp/code/browser/tests/test_revisionauthor.py	2011-02-14 23:28:00 +0000
@@ -67,6 +67,13 @@
         self.assertNotIn(account, self._formatAuthorLink(revision))
         self.assertNotIn('@', self._formatAuthorLink(revision))
 
+    def test_empty_string_when_name_and_email_are_none(self):
+        # When the RevisionAuthor name and email attrs are None, an
+        # empty string is returned.
+        revision = self.factory.makeRevision(author='')
+        login(USER_EMAIL)
+        self.assertEqual('', self._formatAuthorLink(revision))
+
     def test_name_is_escaped(self):
         # The author's name is HTML-escaped.
         revision = self.factory.makeRevision(author="apples & pears")