Merge lp:~jtv/launchpad/bug-723168 into lp:launchpad

Proposed by Jeroen T. Vermeulen on 2011-02-22
Status: Merged
Approved by: Jeroen T. Vermeulen on 2011-02-22
Approved revision: no longer in the source branch.
Merged at revision: 12439
Proposed branch: lp:~jtv/launchpad/bug-723168
Merge into: lp:launchpad
Diff against target: 62 lines (+34/-2)
3 files modified
database/schema/security.cfg (+1/-1)
lib/lp/translations/model/ (+1/-1)
lib/lp/translations/scripts/tests/ (+32/-0)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-723168
Reviewer Review Type Date Requested Status
Leonard Richardson (community) 2011-02-22 Approve on 2011-02-22
Review via email:

Commit message

[r=leonardr][bug=723168] Fix database permissions error (and some of the slowness) in POFile stats verifier.

Description of the change

= Summary =

The POFile statistics verifier script was failing in production because it was selecting from the ProductSeries table, and it had no permission to.

Thing is, there's no reason why it should. The only reason it's accessing Product is because POTemplate.translation_side evaluates whether self.productseries is None. Just that turned out to be enough to make the ORM fetch the productseries, which is neither allowed nor desired. The errors got swallowed by the script, but eventually it did break things.

== Proposed fix ==

Check the productseriesID foreign-key attribute for Noneness, rather than the productseries reference attribute. Presto: no need for further queries, and a faster script to boot.

Also, I noticed that whereas POFile.updateStatistics may refresh the template's message count, it doesn't actually have permission to. Just in case it helps I gave it permission to. This may have been failing anyway, but again the errors would have been swallowed by the script.

== Pre-implementation notes ==

I did not have a proper pre-imp for this, though I worked on it with a LOSA and informed the Translations veterans. It's just too basic.

== Implementation details ==

I added a test that reproduces the problem. This is a style of testing I've gotten into recently: unit-test scripts in lightweight layers with no concern for permissions, then have a separate test case that just exercises database permissions. With the database commits (and also the layer setup, if you're running isolated tests) it's just a waste of time to run everything in a heavyweight layer.

Note that the test exercises POFile.updateStatistics, rather than running the full script. The reason for this is not just that I wanted to save time. It's also to get around the script swallowing errors.

== Tests ==

./bin/test -vvc lp.translations.scripts.test_verify_pofile_stats

== Demo and Q/A ==

Run cronsctipts/ With the bug, it'll start failing quite early on and keep doing so. Without the bug, it should run for hours with few or (ideally) no errors.

= Launchpad lint =

Ignore the security.cfg lint. We really shouldn't be checking line lengths in that file at all, but changes to the file are rare enough that nobody bothers to fix this.

Checking for conflicts and issues in changed files.

Linting changed files:

     726: Line exceeds 78 characters.
     727: Line exceeds 78 characters.
     728: Line exceeds 78 characters.
     754: Line exceeds 78 characters.
     758: Line exceeds 78 characters.
     813: Line exceeds 78 characters.
     826: Line exceeds 78 characters.
     827: Line exceeds 78 characters.
     843: Line exceeds 78 characters.
     844: Line exceeds 78 characters.
     845: Line exceeds 78 characters.
     846: Line exceeds 78 characters.
     847: Line exceeds 78 characters.
     899: Line exceeds 78 characters.
     900: Line exceeds 78 characters.
     901: Line exceeds 78 characters.
     931: Line exceeds 78 characters.
    1012: Line exceeds 78 characters.
    1022: Line exceeds 78 characters.
    1023: Line exceeds 78 characters.

To post a comment you must log in.
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'database/schema/security.cfg'
2--- database/schema/security.cfg 2011-02-07 16:46:35 +0000
3+++ database/schema/security.cfg 2011-02-22 21:02:23 +0000
4@@ -454,7 +454,7 @@
5 groups=script
6 public.language = SELECT
7 public.pofile = SELECT, UPDATE
8-public.potemplate = SELECT
9+public.potemplate = SELECT, UPDATE
10 public.potmsgset = SELECT
11 public.translationmessage = SELECT
12 public.translationtemplateitem = SELECT
14=== modified file 'lib/lp/translations/model/'
15--- lib/lp/translations/model/ 2011-02-21 19:34:59 +0000
16+++ lib/lp/translations/model/ 2011-02-22 21:02:23 +0000
17@@ -1023,7 +1023,7 @@
18 @property
19 def translation_side(self):
20 """See `IPOTemplate`."""
21- if self.productseries is not None:
22+ if self.productseriesID is not None:
23 return TranslationSide.UPSTREAM
24 else:
25 return TranslationSide.UBUNTU
27=== added file 'lib/lp/translations/scripts/tests/'
28--- lib/lp/translations/scripts/tests/ 1970-01-01 00:00:00 +0000
29+++ lib/lp/translations/scripts/tests/ 2011-02-22 21:02:23 +0000
30@@ -0,0 +1,32 @@
31+# Copyright 2011 Canonical Ltd. This software is licensed under the
32+# GNU Affero General Public License version 3 (see the file LICENSE).
34+"""Integration-test POFile statistics verification script."""
36+__metaclass__ = type
38+import transaction
40+from canonical.testing.layers import LaunchpadZopelessLayer
41+from lp.testing import TestCaseWithFactory
42+from lp.translations.interfaces.side import TranslationSide
45+class TestVerifyPOFileStats(TestCaseWithFactory):
46+ layer = LaunchpadZopelessLayer
48+ def _makeNonemptyPOFile(self, side):
49+ pofile = self.factory.makePOFile(side=side)
50+ self.factory.makePOTMsgSet(potemplate=pofile.potemplate, sequence=1)
51+ return pofile
53+ def test_database_permissions(self):
54+ # The script has sufficient database privileges to do its job.
55+ sides = [TranslationSide.UPSTREAM, TranslationSide.UBUNTU]
56+ pofiles = [
57+ self._makeNonemptyPOFile(side) for side in sides]
58+ transaction.commit()
59+ self.layer.switchDbUser(dbuser='pofilestats')
61+ for pofile in pofiles:
62+ pofile.updateStatistics()