Merge lp:~bac/launchpad/bug-520476 into lp:launchpad

[Brad Crittenden]

Throw and UnexpectedFormData exception when +editpgpkeys has no action.

= Summary =

Somehow (dumb browser, network fart, who knows?) the +editpgpkeys form
has been submitted with the form action set to None, which causes the
call to getattr to fail. Add defensive programming to not allow that to
happen and produce a better failure.

== Proposed fix ==

Remove the test for non-None before throwing UnexpectedFormData.

== Pre-implementation notes ==

Talk with Curtis.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt gpg-views.txt

== 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/registry/browser/tests/gpg-views.txt
  lib/lp/registry/browser/person.py

== Pylint notices ==

lib/lp/registry/browser/person.py
    1944: [C0301] Line too long (83/78)

Pre-existing lint problem. Will fix before submitting.

[Curtis Hovey (sinzui)]

Thanks for doing this. As we discussed, the root cause was probably on the client, which is still an oops, but one that the client needs to address, not us.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-05-19 17:03:40 +0000
+++ lib/lp/registry/browser/person.py	2010-05-20 19:00:56 +0000
@@ -1941,7 +1941,7 @@
             extra_params=critical_bugs_params)
 
     def getHighBugsURL(self, distributionsourcepackage):
-        """Return the URL for high importance bugs on distributionsourcepackage."""
+        """Return URL for high importance bugs on distributionsourcepackage."""
         high_bugs_params = {
             'field.status': [], 'field.importance': "High"}
 
@@ -3658,13 +3658,17 @@
             IGPGHandler).getURLForKeyInServer(self.fingerprint, public=True)
 
     def form_action(self):
-        permitted_actions = ['claim_gpg', 'deactivate_gpg',
-                             'remove_gpgtoken', 'reactivate_gpg']
+        permitted_actions = [
+            'claim_gpg',
+            'deactivate_gpg',
+            'remove_gpgtoken',
+            'reactivate_gpg',
+            ]
         if self.request.method != "POST":
             return ''
         action = self.request.form.get('action')
-        if action and (action not in permitted_actions):
-            raise UnexpectedFormData("Action was not defined")
+        if action not in permitted_actions:
+            raise UnexpectedFormData("Action not permitted: %s" % action)
         getattr(self, action)()
 
     def claim_gpg(self):

=== modified file 'lib/lp/registry/browser/tests/gpg-views.txt'
--- lib/lp/registry/browser/tests/gpg-views.txt	2010-03-11 16:09:29 +0000
+++ lib/lp/registry/browser/tests/gpg-views.txt	2010-05-20 19:00:56 +0000
@@ -28,10 +28,10 @@
     >>> revoked     = "84D2 05F0 3E1E 6709 6CB5  4E26 2BE8 3793 AACC D97C"
     >>> expired     = "ECA5 B797 586F 2E27 381A  16CF DE6C 9167 046C 6D63"
 
-    >>> def post_fingerprint(fingerprint):
+    >>> def post_fingerprint(fingerprint, action='claim_gpg'):
     ...     request = LaunchpadTestRequest(form={
     ...         'fingerprint': fingerprint,
-    ...         'action': 'claim_gpg',
+    ...         'action': action,
     ...         'import': 'Import Key'})
     ...     request.method = "POST"
     ...     view = getMultiAdapter((sample_user, request), name="+editpgpkeys")
@@ -128,8 +128,12 @@
     >>> view.context.pending_gpg_keys
     []
 
-
-And knock the server out, finally:
-
-  >>> z.tearDown()
-
+In some unknown way, the action sent to the form can be None (see bug 520476).
+
+    >>> view = post_fingerprint(good_fingerprint, action=None)
+    Traceback (most recent call last):
+     ...
+    UnexpectedFormData: Action not permitted: None
+
+Tear down the key server.
+    >>> z.tearDown()