Launchpad itself

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://staging/~benji/launchpad/bug-388997

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jeroen T. Vermeulen (community)	code	2010-07-14	Approve on 2010-07-14
Review via email: mp+29851@code.staging.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.

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-07-14:

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

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)