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
=== 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 13:33:55 +0000
@@ -0,0 +1,28 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4__metaclass__ = type
5
6from zope.schema.vocabulary import SimpleTerm
7
8from canonical.launchpad.webapp.vocabulary import ForgivingSimpleVocabulary
9from lp.testing import TestCase
10
11
12class TestForgivingSimpleVocabulary(TestCase):
13 """Tests for ForgivingSimpleVocabulary."""
14
15 def setUp(self):
16 super(TestForgivingSimpleVocabulary, self).setUp()
17 self.term_1 = SimpleTerm('term-1', 'term-1', 'My first term')
18 self.term_2 = SimpleTerm('term-2', 'term-2', 'My second term')
19 self.vocabulary = ForgivingSimpleVocabulary(
20 terms=[self.term_1, self.term_2], default_term=self.term_2)
21
22 def test_normal_lookup(self):
23 """Lookups for proper values succeed."""
24 self.assertIs(self.vocabulary.getTerm('term-1'), self.term_1)
25
26 def test_errant_lookup(self):
27 """Lookups for invalid values return the default."""
28 self.assertIs(self.vocabulary.getTerm('does-not-exist'), self.term_2)
029
=== modified file 'lib/canonical/launchpad/webapp/vocabulary.py'
--- lib/canonical/launchpad/webapp/vocabulary.py 2010-01-22 16:09:02 +0000
+++ lib/canonical/launchpad/webapp/vocabulary.py 2010-07-14 13:33:55 +0000
@@ -12,6 +12,7 @@
12__metaclass__ = type12__metaclass__ = type
1313
14__all__ = [14__all__ = [
15 'ForgivingSimpleVocabulary',
15 'IHugeVocabulary',16 'IHugeVocabulary',
16 'SQLObjectVocabularyBase',17 'SQLObjectVocabularyBase',
17 'NamedSQLObjectVocabulary',18 'NamedSQLObjectVocabulary',
@@ -24,12 +25,31 @@
2425
25from zope.interface import implements, Attribute, Interface26from zope.interface import implements, Attribute, Interface
26from zope.schema.interfaces import IVocabulary, IVocabularyTokenized27from zope.schema.interfaces import IVocabulary, IVocabularyTokenized
27from zope.schema.vocabulary import SimpleTerm28from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
28from zope.security.proxy import isinstance as zisinstance29from zope.security.proxy import isinstance as zisinstance
2930
30from canonical.database.sqlbase import SQLBase31from canonical.database.sqlbase import SQLBase
3132
3233
34class ForgivingSimpleVocabulary(SimpleVocabulary):
35 """A vocabulary that returns a default term for unrecognized values."""
36
37 def __init__(self, *args, **kws):
38 missing = object()
39 self._default_term = kws.pop('default_term', missing)
40 if self._default_term is missing:
41 raise TypeError('required argument "default_term" not provided')
42 return super(ForgivingSimpleVocabulary, self).__init__(*args, **kws)
43
44
45 def getTerm(self, value):
46 """Look up a value, returning the default if it is not found."""
47 try:
48 return super(ForgivingSimpleVocabulary, self).getTerm(value)
49 except LookupError:
50 return self._default_term
51
52
33class IHugeVocabulary(IVocabulary, IVocabularyTokenized):53class IHugeVocabulary(IVocabulary, IVocabularyTokenized):
34 """Interface for huge vocabularies.54 """Interface for huge vocabularies.
3555
3656
=== modified file 'lib/lp/translations/browser/hastranslationimports.py'
--- lib/lp/translations/browser/hastranslationimports.py 2009-11-20 14:15:34 +0000
+++ lib/lp/translations/browser/hastranslationimports.py 2010-07-14 13:33:55 +0000
@@ -25,6 +25,7 @@
25from canonical.cachedproperty import cachedproperty25from canonical.cachedproperty import cachedproperty
26from canonical.launchpad import _26from canonical.launchpad import _
27from canonical.launchpad.webapp.interfaces import UnexpectedFormData27from canonical.launchpad.webapp.interfaces import UnexpectedFormData
28from canonical.launchpad.webapp.vocabulary import ForgivingSimpleVocabulary
28from lp.registry.interfaces.distribution import IDistribution29from lp.registry.interfaces.distribution import IDistribution
29from lp.registry.interfaces.pillar import IPillarNameSet30from lp.registry.interfaces.pillar import IPillarNameSet
30from lp.translations.interfaces.translationimportqueue import (31from lp.translations.interfaces.translationimportqueue import (
@@ -391,10 +392,13 @@
391392
392 def __call__(self, context):393 def __call__(self, context):
393 file_extensions = ('po', 'pot')394 file_extensions = ('po', 'pot')
394395 all_files = SimpleTerm('all', 'all', 'All files')
395 terms = [SimpleTerm('all', 'all', 'All files')]396 terms = [all_files]
396 for extension in file_extensions:397 for extension in file_extensions:
397 title = 'Only %s files' % extension398 title = 'Only %s files' % extension
398 terms.append(SimpleTerm(extension, extension, title))399 terms.append(SimpleTerm(extension, extension, title))
399 return SimpleVocabulary(terms)
400400
401 # We use a ForgivingSimpleVocabulary because we don't care if a user
402 # provides an invalid value. If they do we just ignore it and show
403 # them all files.
404 return ForgivingSimpleVocabulary(terms, default_term=all_files)
401405
=== modified file 'lib/lp/translations/stories/importqueue/xx-translation-import-queue.txt'
--- 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 13:33:55 +0000
@@ -449,3 +449,25 @@
449 1449 1
450 >>> print msgs[0]450 >>> print msgs[0]
451 Upload ignored. The tarball you uploaded did not contain...451 Upload ignored. The tarball you uploaded did not contain...
452
453Bad filter_extension
454~~~~~~~~~~~~~~~~~~~~
455
456Very often robots attempt to request URLs with garbage appended to the end.
457In at least one case it seems to have happened because someone on IRC closed a
458parenthesis right after the URL, and an IRC log site linkified the URL with
459the erroneous parenthesis included.
460
461Here we'll simulate such a request and show that the resulting unrecognized
462filter_extension values do not generate an error. See bug 388997.
463
464 >>> post_data = urllib.urlencode(
465 ... {
466 ... 'field.filter_target': 'all',
467 ... 'field.filter_status': 'all',
468 ... 'field.filter_extension': 'potlksajflkasj',
469 ... 'field.actions.change_status': 'Change status',
470 ... })
471 >>> user_browser.open(
472 ... 'http://translations.launchpad.dev/+imports',
473 ... data=post_data)

Subscribers

People subscribed via source and target branches

to status/vote changes: