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/potemplate.py (+1/-1)
lib/lp/translations/scripts/tests/test_verify_pofile_stats.py (+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: mp+50808@code.launchpad.net

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/rosetta-pofile-stats.py. 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:
  database/schema/security.cfg
  lib/lp/translations/scripts/tests/test_verify_pofile_stats.py
  lib/lp/translations/model/potemplate.py

./database/schema/security.cfg
     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
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2011-02-07 16:46:35 +0000
+++ database/schema/security.cfg 2011-02-22 21:02:23 +0000
@@ -454,7 +454,7 @@
454groups=script454groups=script
455public.language = SELECT455public.language = SELECT
456public.pofile = SELECT, UPDATE456public.pofile = SELECT, UPDATE
457public.potemplate = SELECT457public.potemplate = SELECT, UPDATE
458public.potmsgset = SELECT458public.potmsgset = SELECT
459public.translationmessage = SELECT459public.translationmessage = SELECT
460public.translationtemplateitem = SELECT460public.translationtemplateitem = SELECT
461461
=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py 2011-02-21 19:34:59 +0000
+++ lib/lp/translations/model/potemplate.py 2011-02-22 21:02:23 +0000
@@ -1023,7 +1023,7 @@
1023 @property1023 @property
1024 def translation_side(self):1024 def translation_side(self):
1025 """See `IPOTemplate`."""1025 """See `IPOTemplate`."""
1026 if self.productseries is not None:1026 if self.productseriesID is not None:
1027 return TranslationSide.UPSTREAM1027 return TranslationSide.UPSTREAM
1028 else:1028 else:
1029 return TranslationSide.UBUNTU1029 return TranslationSide.UBUNTU
10301030
=== added file 'lib/lp/translations/scripts/tests/test_verify_pofile_stats.py'
--- lib/lp/translations/scripts/tests/test_verify_pofile_stats.py 1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/tests/test_verify_pofile_stats.py 2011-02-22 21:02:23 +0000
@@ -0,0 +1,32 @@
1# Copyright 2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Integration-test POFile statistics verification script."""
5
6__metaclass__ = type
7
8import transaction
9
10from canonical.testing.layers import LaunchpadZopelessLayer
11from lp.testing import TestCaseWithFactory
12from lp.translations.interfaces.side import TranslationSide
13
14
15class TestVerifyPOFileStats(TestCaseWithFactory):
16 layer = LaunchpadZopelessLayer
17
18 def _makeNonemptyPOFile(self, side):
19 pofile = self.factory.makePOFile(side=side)
20 self.factory.makePOTMsgSet(potemplate=pofile.potemplate, sequence=1)
21 return pofile
22
23 def test_database_permissions(self):
24 # The script has sufficient database privileges to do its job.
25 sides = [TranslationSide.UPSTREAM, TranslationSide.UBUNTU]
26 pofiles = [
27 self._makeNonemptyPOFile(side) for side in sides]
28 transaction.commit()
29 self.layer.switchDbUser(dbuser='pofilestats')
30
31 for pofile in pofiles:
32 pofile.updateStatistics()