Merge lp:~sinzui/launchpad/oopsless-repr into lp:launchpad

[Curtis Hovey]

This is my branch to make __repr__ ascii safe for several classes.

    lp:~sinzui/launchpad/oopsless-repr
    Diff size: 169
    Launchpad bug: https://bugs.launchpad.net/bugs/575976
    Test command: ./bin/test -vv \
        -t test_person_repr -t test_distribution_repr -t test_account_repr
    Pre-implementation: matsubara
    Target release: 10.05

Make __repr__ ascii safe for several classes
---------------------------------------------

As seen on OOPS-1582EB1587 a UnicodeEncodeError was raised while GET'ing the
+reviewaccount page for a person and the person display name contains non-
ascii characters. The oops was created *after* the canonical_urldata_iterator
raised its own error because henninge hacked the URL to non-existent person.
The error message wanted to tell henninge to be a good boy, but the
NoCanonicalUrl uses the __repr__ of the object, which for many launchpad
objects, will contain Unicode.

This affects the following models:
    Account
    Distribution
    Person

Rules
-----

    * Use use .encode('ASCII', 'backslashreplace') to make the displayname
      safe.

QA
--

This is difficult to test since we only see this issue during oopses.
One oops that be reproduced is
    * Visit https://edge.launchpad.net/~ayrton.araujo/+reviewaccount
    * Verify the oops is not about:
      UnicodeEncodeError: 'ascii' codec can't encode character u'\xfa'
    * Verify the oops is about NoCanonicalUrl

Lint
----

Linting changed files:
  lib/canonical/launchpad/database/account.py
  lib/canonical/launchpad/tests/test_account.py
  lib/lp/registry/model/distribution.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_distribution.py
  lib/lp/registry/tests/test_person.py

Test
----

    * lib/canonical/launchpad/tests/test_account.py
      Added tests for ANSI and Unicode __repr__
    * lib/lp/registry/tests/test_distribution.py
      Added tests for ANSI and Unicode __repr__
    * lib/lp/registry/tests/test_person.py
      Added tests for ANSI and Unicode __repr__

Implementation
--------------

    * lib/canonical/launchpad/database/account.py
      Escaped the displayname.
    * lib/lp/registry/model/distribution.py
      Escaped the displayname and fixed two lint issues.
    * lib/lp/registry/model/person.py
      Escaped the displayname.

[Eleanor Berger (intellectronica)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/database/account.py'
--- lib/canonical/launchpad/database/account.py	2010-04-06 19:51:22 +0000
+++ lib/canonical/launchpad/database/account.py	2010-05-12 18:05:45 +0000
@@ -48,8 +48,9 @@
         dbName='openid_identifier', notNull=True, default=DEFAULT)
 
     def __repr__(self):
+        displayname = self.displayname.encode('ASCII', 'backslashreplace')
         return "<%s '%s' (%s)>" % (
-            self.__class__.__name__, self.displayname, self.status)
+            self.__class__.__name__, displayname, self.status)
 
     def _getEmails(self, status):
         """Get related `EmailAddress` objects with the given status."""

=== renamed file 'lib/canonical/launchpad/tests/test_personless_accounts.py' => 'lib/canonical/launchpad/tests/test_account.py'
--- lib/canonical/launchpad/tests/test_personless_accounts.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/tests/test_account.py	2010-05-12 18:05:45 +0000
@@ -8,6 +8,23 @@
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.testing import DatabaseFunctionalLayer
 
+class TestAccount(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_account_repr_ansii(self):
+        # Verify that ANSI displayname is ascii safe.
+        distro = self.factory.makeAccount(u'\xdc-account')
+        ignore, displayname, status_1, status_2 = repr(distro).rsplit(' ', 3)
+        self.assertEqual("'\\xdc-account'", displayname)
+        self.assertEqual('(Active account)>', '%s %s' % (status_1, status_2))
+
+    def test_account_repr_unicode(self):
+        # Verify that Unicode displayname is ascii safe.
+        distro = self.factory.makeAccount(u'\u0170-account')
+        ignore, displayname, status_1, status_2 = repr(distro).rsplit(' ', 3)
+        self.assertEqual("'\\u0170-account'", displayname)
+
 
 class TestPersonlessAccountPermissions(TestCaseWithFactory):
     """In order for Person-less accounts to see their non-public details and

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2010-04-13 13:38:41 +0000
+++ lib/lp/registry/model/distribution.py	2010-05-12 18:05:45 +0000
@@ -185,8 +185,9 @@
     max_bug_heat = Int()
 
     def __repr__(self):
+        displayname = self.displayname.encode('ASCII', 'backslashreplace')
         return "<%s '%s' (%s)>" % (
-            self.__class__.__name__, self.displayname, self.name)
+            self.__class__.__name__, displayname, self.name)
 
     def _init(self, *args, **kw):
         """Initialize an `IBaseDistribution` or `IDerivativeDistribution`."""
@@ -1530,7 +1531,7 @@
         """See `IHasBugSupervisor`."""
         self.bug_supervisor = bug_supervisor
         if bug_supervisor is not None:
-            subscription = self.addBugSubscription(bug_supervisor, user)
+            self.addBugSubscription(bug_supervisor, user)
 
     def userCanEdit(self, user):
         """See `IDistribution`."""
@@ -1634,6 +1635,6 @@
             mugshot=mugshot,
             logo=logo,
             icon=icon)
-        archive = getUtility(IArchiveSet).new(distribution=distro,
+        getUtility(IArchiveSet).new(distribution=distro,
             owner=owner, purpose=ArchivePurpose.PRIMARY)
         return distro

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-05-07 20:28:47 +0000
+++ lib/lp/registry/model/person.py	2010-05-12 18:05:45 +0000
@@ -263,8 +263,8 @@
                      storm_validator=_validate_name)
 
     def __repr__(self):
-        return '<Person at 0x%x %s (%s)>' % (
-            id(self), self.name, self.displayname)
+        displayname = self.displayname.encode('ASCII', 'backslashreplace')
+        return '<Person at 0x%x %s (%s)>' % (id(self), self.name, displayname)
 
     displayname = StringCol(dbName='displayname', notNull=True)
 

=== modified file 'lib/lp/registry/tests/test_distribution.py'
--- lib/lp/registry/tests/test_distribution.py	2009-12-13 11:55:40 +0000
+++ lib/lp/registry/tests/test_distribution.py	2010-05-12 18:05:45 +0000
@@ -14,6 +14,32 @@
 from lp.soyuz.interfaces.distributionsourcepackagerelease import (
     IDistributionSourcePackageRelease)
 from lp.registry.interfaces.series import SeriesStatus
+from lp.testing import TestCaseWithFactory
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
+
+
+class TestDistribution(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestDistribution, self).setUp('[e-mail]')
+
+    def test_distribution_repr_ansii(self):
+        # Verify that ANSI displayname is ascii safe.
+        distro = self.factory.makeDistribution(
+            name="distro", displayname=u'\xdc-distro')
+        ignore, displayname, name = repr(distro).rsplit(' ', 2)
+        self.assertEqual("'\\xdc-distro'", displayname)
+        self.assertEqual('(distro)>', name)
+
+    def test_distribution_repr_unicode(self):
+        # Verify that Unicode displayname is ascii safe.
+        distro = self.factory.makeDistribution(
+            name="distro", displayname=u'\u0170-distro')
+        ignore, displayname, name = repr(distro).rsplit(' ', 2)
+        self.assertEqual("'\\u0170-distro'", displayname)
 
 
 class TestDistributionCurrentSourceReleases(
@@ -25,6 +51,7 @@
     for the latest published source across multiple distro series.
     """
 
+    layer = LaunchpadFunctionalLayer
     release_interface = IDistributionSourcePackageRelease
 
     @property
@@ -75,7 +102,4 @@
 
 
 def test_suite():
-    suite = unittest.TestSuite()
-    suite.addTest(unittest.makeSuite(TestDistributionCurrentSourceReleases))
-    return suite
-
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-04-23 14:22:30 +0000
+++ lib/lp/registry/tests/test_person.py	2010-05-12 18:05:45 +0000
@@ -435,6 +435,21 @@
                 hasattr(snap, name),
                 "%s should be omitted from the snapshot but is not." % name)
 
+    def test_person_repr_ansii(self):
+        # Verify that ANSI displayname is ascii safe.
+        person = self.factory.makePerson(
+            name="user", displayname=u'\xdc-tester')
+        ignore, name, displayname = repr(person).rsplit(' ', 2)
+        self.assertEqual('user', name)
+        self.assertEqual('(\\xdc-tester)>', displayname)
+
+    def test_person_repr_unicode(self):
+        # Verify that Unicode displayname is ascii safe.
+        person = self.factory.makePerson(
+            name="user", displayname=u'\u0170-tester')
+        ignore, displayname = repr(person).rsplit(' ', 1)
+        self.assertEqual('(\\u0170-tester)>', displayname)
+
 
 class TestPersonSet(unittest.TestCase):
     """Test `IPersonSet`."""