launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-723168 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #723168 POFile stats verifier lacks db privilege
  https://bugs.launchpad.net/bugs/723168

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-723168/+merge/50808

= 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.
-- 
https://code.launchpad.net/~jtv/launchpad/bug-723168/+merge/50808
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-723168 into lp:launchpad.

=== 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 @@
 groups=script
 public.language                         = SELECT
 public.pofile                           = SELECT, UPDATE
-public.potemplate                       = SELECT
+public.potemplate                       = SELECT, UPDATE
 public.potmsgset                        = SELECT
 public.translationmessage               = SELECT
 public.translationtemplateitem          = SELECT

=== 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 @@
     @property
     def translation_side(self):
         """See `IPOTemplate`."""
-        if self.productseries is not None:
+        if self.productseriesID is not None:
             return TranslationSide.UPSTREAM
         else:
             return TranslationSide.UBUNTU

=== 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 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Integration-test POFile statistics verification script."""
+
+__metaclass__ = type
+
+import transaction
+
+from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.testing import TestCaseWithFactory
+from lp.translations.interfaces.side import TranslationSide
+
+
+class TestVerifyPOFileStats(TestCaseWithFactory):
+    layer = LaunchpadZopelessLayer
+
+    def _makeNonemptyPOFile(self, side):
+        pofile = self.factory.makePOFile(side=side)
+        self.factory.makePOTMsgSet(potemplate=pofile.potemplate, sequence=1)
+        return pofile
+
+    def test_database_permissions(self):
+        # The script has sufficient database privileges to do its job.
+        sides = [TranslationSide.UPSTREAM, TranslationSide.UBUNTU]
+        pofiles = [
+            self._makeNonemptyPOFile(side) for side in sides]
+        transaction.commit()
+        self.layer.switchDbUser(dbuser='pofilestats')
+
+        for pofile in pofiles:
+            pofile.updateStatistics()