Launchpad itself

Merge lp:~benji/launchpad/bug-388997 into lp:launchpad/db-devel

  1. bug-388997
  2. Merge into db-devel
Proposed by Benji York
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merge reported by: Benji York
Merged at revision: not available
Proposed branch: lp:~benji/launchpad/bug-388997
Merge into: lp:launchpad/db-devel
Diff against target: 136 lines (+78/-4)
4 files modified
lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py (+28/-0)
lib/canonical/launchpad/webapp/vocabulary.py (+21/-1)
lib/lp/translations/browser/hastranslationimports.py (+7/-3)
lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt (+22/-0)
To merge this branch: bzr merge lp:~benji/launchpad/bug-388997
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Review via email: mp+29851@code.launchpad.net

Description of the change

Pre-implementation discussion:

Yesterday Jeroen (jtv) and I discussed bug 388997
(https://bugs.edge.launchpad.net/rosetta/+bug/388997).

Jeroen proposed that for the argument in question (filter_extension) the most
appropriate response to invalid input is to ignore it and use a default.

Implementation details:

The implementation is a straight-forward subclass of SimpleVocabulary (named
ForgivingSimpleVocabulary) that catches LookupErrors and returns the default
instead.

After looking for an already-existing implementation we settled on
lib/canonical/launchpad/webapp/vocabulary.py as the best place for the new
vocabulary to live.

The tests for ForgivingSimpleVocabulary live in
lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py.

I also changed lib/lp/translations/browser/hastranslationimports.py to use the
new vocabulary for filter_extension and added a test to
lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt which
demonstrate that unrecognized filter_extension values don't generate errors.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Yay! Thanks for fixing this for us. The way you implemented this made me sort of expect SimpleVocabulary to have it built in: lookup ought to fail only if you have not set a default term.

Some review notes:

=== added file 'lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py'
--- lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py 2010-07-14 10:07:44 +0000
@@ -0,0 +1,28 @@
+# Copyright 2009 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).

Could you update the copyright date?

+ def test_normal_lookup(self):
+ """Test that lookups for proper values succeed."""
+ self.assertIs(self.vocabulary.getTerm('term-1'), self.term_1)
+
+ def test_errant_lookup(self):
+ """Test that lookups for invalid values return the default."""
+ self.assertIs(self.vocabulary.getTerm('does-not-exist'), self.term_2)

It may be a matter of personal style but I find that "Test that" doesn't carry its weight in a test description. An assertive "Lookups for proper values succeed" would do for me. In doctests we like to talk about a "narrative" that, instead of circling around the test code pointing at the things it does, describes the same thing that the code illustrates.

--- lib/lp/translations/browser/hastranslationimports.py 2009-11-20 14:15:34 +0000
+++ lib/lp/translations/browser/hastranslationimports.py 2010-07-14 10:07:44 +0000
@@ -391,10 +392,13 @@

[...]

+ # We use a ForgivingSimpleVocabulary because we don't care if a user
+ # provides an invalid value, if they do we just ignore it and show
+ # them all files.
+ return ForgivingSimpleVocabulary(terms, default_term=all_files)

In the comment, the comma before that second "if" is really a full stop.

--- lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt 2010-04-26 16:00:31 +0000
+++ lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt 2010-07-14 10:07:44 +0000
@@ -449,3 +449,23 @@
   1
   >>> print msgs[0]
   Upload ignored. The tarball you uploaded did not contain...
+
+Bad filter_extension
+~~~~~~~~~~~~~~~~~~~~
+
+Very often robots attempt to request URLs with garbage appended to the end.
+Presumably they scraped these URLs from the web and couldn't quite tell where
+the query string ended and the surrounding text began. Here we'll simulate

In our case, it seems to have happened because someone on IRC closed a parenthesis right after the URL, and an IRC log site linkified the URL with the erroneous parenthesis included. And then I suppose the bots kept coming back because the oops pages they get are not marked as errors and are different every time. Which means they must be interesting and "hot," right?

All small stuff. Go land that baby!

Jeroen

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py'
2--- lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py 1970-01-01 00:00:00 +0000
3+++ lib/canonical/launchpad/webapp/tests/test_forgiving_vocabulary.py 2010-07-14 13:33:55 +0000
4@@ -0,0 +1,28 @@
5+# Copyright 2009 Canonical Ltd. This software is licensed under the
6+# GNU Affero General Public License version 3 (see the file LICENSE).
7+
8+__metaclass__ = type
9+
10+from zope.schema.vocabulary import SimpleTerm
11+
12+from canonical.launchpad.webapp.vocabulary import ForgivingSimpleVocabulary
13+from lp.testing import TestCase
14+
15+
16+class TestForgivingSimpleVocabulary(TestCase):
17+ """Tests for ForgivingSimpleVocabulary."""
18+
19+ def setUp(self):
20+ super(TestForgivingSimpleVocabulary, self).setUp()
21+ self.term_1 = SimpleTerm('term-1', 'term-1', 'My first term')
22+ self.term_2 = SimpleTerm('term-2', 'term-2', 'My second term')
23+ self.vocabulary = ForgivingSimpleVocabulary(
24+ terms=[self.term_1, self.term_2], default_term=self.term_2)
25+
26+ def test_normal_lookup(self):
27+ """Lookups for proper values succeed."""
28+ self.assertIs(self.vocabulary.getTerm('term-1'), self.term_1)
29+
30+ def test_errant_lookup(self):
31+ """Lookups for invalid values return the default."""
32+ self.assertIs(self.vocabulary.getTerm('does-not-exist'), self.term_2)
33
34=== modified file 'lib/canonical/launchpad/webapp/vocabulary.py'
35--- lib/canonical/launchpad/webapp/vocabulary.py 2010-01-22 16:09:02 +0000
36+++ lib/canonical/launchpad/webapp/vocabulary.py 2010-07-14 13:33:55 +0000
37@@ -12,6 +12,7 @@
38 __metaclass__ = type
39
40 __all__ = [
41+ 'ForgivingSimpleVocabulary',
42 'IHugeVocabulary',
43 'SQLObjectVocabularyBase',
44 'NamedSQLObjectVocabulary',
45@@ -24,12 +25,31 @@
46
47 from zope.interface import implements, Attribute, Interface
48 from zope.schema.interfaces import IVocabulary, IVocabularyTokenized
49-from zope.schema.vocabulary import SimpleTerm
50+from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
51 from zope.security.proxy import isinstance as zisinstance
52
53 from canonical.database.sqlbase import SQLBase
54
55
56+class ForgivingSimpleVocabulary(SimpleVocabulary):
57+ """A vocabulary that returns a default term for unrecognized values."""
58+
59+ def __init__(self, *args, **kws):
60+ missing = object()
61+ self._default_term = kws.pop('default_term', missing)
62+ if self._default_term is missing:
63+ raise TypeError('required argument "default_term" not provided')
64+ return super(ForgivingSimpleVocabulary, self).__init__(*args, **kws)
65+
66+
67+ def getTerm(self, value):
68+ """Look up a value, returning the default if it is not found."""
69+ try:
70+ return super(ForgivingSimpleVocabulary, self).getTerm(value)
71+ except LookupError:
72+ return self._default_term
73+
74+
75 class IHugeVocabulary(IVocabulary, IVocabularyTokenized):
76 """Interface for huge vocabularies.
77
78
79=== modified file 'lib/lp/translations/browser/hastranslationimports.py'
80--- lib/lp/translations/browser/hastranslationimports.py 2009-11-20 14:15:34 +0000
81+++ lib/lp/translations/browser/hastranslationimports.py 2010-07-14 13:33:55 +0000
82@@ -25,6 +25,7 @@
83 from canonical.cachedproperty import cachedproperty
84 from canonical.launchpad import _
85 from canonical.launchpad.webapp.interfaces import UnexpectedFormData
86+from canonical.launchpad.webapp.vocabulary import ForgivingSimpleVocabulary
87 from lp.registry.interfaces.distribution import IDistribution
88 from lp.registry.interfaces.pillar import IPillarNameSet
89 from lp.translations.interfaces.translationimportqueue import (
90@@ -391,10 +392,13 @@
91
92 def __call__(self, context):
93 file_extensions = ('po', 'pot')
94-
95- terms = [SimpleTerm('all', 'all', 'All files')]
96+ all_files = SimpleTerm('all', 'all', 'All files')
97+ terms = [all_files]
98 for extension in file_extensions:
99 title = 'Only %s files' % extension
100 terms.append(SimpleTerm(extension, extension, title))
101- return SimpleVocabulary(terms)
102
103+ # We use a ForgivingSimpleVocabulary because we don't care if a user
104+ # provides an invalid value. If they do we just ignore it and show
105+ # them all files.
106+ return ForgivingSimpleVocabulary(terms, default_term=all_files)
107
108=== modified file 'lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt'
109--- lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt 2010-04-26 16:00:31 +0000
110+++ lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt 2010-07-14 13:33:55 +0000
111@@ -449,3 +449,25 @@
112 1
113 >>> print msgs[0]
114 Upload ignored. The tarball you uploaded did not contain...
115+
116+Bad filter_extension
117+~~~~~~~~~~~~~~~~~~~~
118+
119+Very often robots attempt to request URLs with garbage appended to the end.
120+In at least one case it seems to have happened because someone on IRC closed a
121+parenthesis right after the URL, and an IRC log site linkified the URL with
122+the erroneous parenthesis included.
123+
124+Here we'll simulate such a request and show that the resulting unrecognized
125+filter_extension values do not generate an error. See bug 388997.
126+
127+ >>> post_data = urllib.urlencode(
128+ ... {
129+ ... 'field.filter_target': 'all',
130+ ... 'field.filter_status': 'all',
131+ ... 'field.filter_extension': 'potlksajflkasj',
132+ ... 'field.actions.change_status': 'Change status',
133+ ... })
134+ >>> user_browser.open(
135+ ... 'http://translations.launchpad.dev/+imports',
136+ ... data=post_data)

Subscribers

People subscribed via source and target branches

to status/vote changes: