Merge lp:~jtv/launchpad/recife-pofile-owner-privs into lp:~launchpad/launchpad/recife

[Jeroen T. Vermeulen]

Give POFile.owner no special privileges, and always set it to the creator.

Strip POFile.owner of its privileges
====================================

For the Recife feature branch, this branch implements a change to the privileges model. The change pertains to who gets edit rights to a translation as embodied by a POFile.

The details of that are quite complex. I leave most for my next branch. But one thing that bestows edit privileges to a user is ownership: the user who is (a member of) a POFile.owner can edit that POFile. But ownership of a POFile is somewhat arbitrary: just submitting a suggestion for instance can implicitly create a POFile in the database. Anyone can do it in almost anyone's project or package.

Having submitted the first suggestion to a translation should not make you an editor, and that is where the existing code does a little weasel dance: *if* you currently have edit rights, then you also become the owner of the POFile that you cause to be created. If you don't, ownership defaults to the Rosetta experts team. Nobody will ever notice… unless you later lose your edit rights for whatever reason. Then you'll arbitrarily stay able to edit those POFiles that you caused to be created, but not the others that you lost edit rights to. It's both complicated and pointless.

After discussion with all members of the Translations team at various times, this branch streamlines POFile.owner:
 * Implies no special privileges on the POFile.
 * Always gets set to whoever causes it to be created.
 * Is initialized during POFile construction, not with a separate method.
 * Effectively means "creator" (and may be renamed later).

It's not obvious but there were actually two places in the code that gave special rights to POFile.owner: the security.py class built on EditByOwnersOrAdmins (implicitly authorizing the context's owner) and POFile.canEditTranslations checked for it. My branch also removes duplication from those checks, concentrating them in POFile. Not ideal, but the checking code is complex enough that hiding it in security.py might hide performance problems.

I hit some circular imports between pofile and person, so you'll see me concentrate references to DummyPOFile here and there and do local imports. Not quite enough to create a new helper, I guess.

In the xx-pofile-export pagetest you'll see a stretch of test removed. That's because I added a unit test for the removed privilege; everything else that bit of test did was duplicated under a different login right afterwards.

To test this, you'll want a full Translations test:
{{{
make schema
./bin/test -vvc lp.translations
}}}

There's no lint left. The price is some diff pollution.

Jeroen

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-10-14 15:22:46 +0000
+++ lib/canonical/launchpad/security.py	2010-10-19 12:57:50 +0000
@@ -1226,16 +1226,13 @@
     usedfor = IPOFile
 
 
-class EditPOFileDetails(EditByOwnersOrAdmins):
+class EditPOFile(AuthorizationBase):
+    permission = 'launchpad.Edit'
     usedfor = IPOFile
 
     def checkAuthenticated(self, user):
-        """Allow anyone that can edit translations, owner, experts and admis.
-        """
-
-        return (EditByOwnersOrAdmins.checkAuthenticated(self, user) or
-                self.obj.canEditTranslations(user.person) or
-                user.in_rosetta_experts)
+        """The `POFile` itself keeps track of this permission."""
+        return self.obj.canEditTranslations(user.person)
 
 
 class AdminTranslator(OnlyRosettaExpertsAndAdmins):

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2010-09-30 03:22:18 +0000
+++ lib/lp/translations/browser/potemplate.py	2010-10-19 12:57:50 +0000
@@ -120,9 +120,7 @@
             # XXX CarlosPerelloMarin 2006-04-20 bug=40275: We should
             # check the kind of POST we got.  A logout will also be a
             # POST and we should not create a POFile in that case.
-            pofile = self.context.newPOFile(name)
-            pofile.setOwnerIfPrivileged(user)
-            return pofile
+            return self.context.newPOFile(name, owner=user)
 
 
 class POTemplateFacets(StandardLaunchpadFacets):

=== modified file 'lib/lp/translations/interfaces/pofile.py'
--- lib/lp/translations/interfaces/pofile.py	2010-10-14 08:04:21 +0000
+++ lib/lp/translations/interfaces/pofile.py	2010-10-19 12:57:50 +0000
@@ -237,9 +237,6 @@
     def canEditTranslations(person):
         """Whether the given person is able to add/edit translations."""
 
-    def setOwnerIfPrivileged(person):
-        """Set `owner` to `person`, provided `person` has edit rights."""
-
     def canAddSuggestions(person):
         """Whether the given person is able to add new suggestions."""
 

=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py	2010-10-14 08:24:55 +0000
+++ lib/lp/translations/model/pofile.py	2010-10-19 12:57:50 +0000
@@ -68,7 +68,6 @@
     )
 from canonical.launchpad.webapp.publisher import canonical_url
 from lp.registry.interfaces.person import validate_public_person
-from lp.registry.model.person import Person
 from lp.services.propertycache import cachedproperty
 from lp.translations.interfaces.pofile import (
     IPOFile,
@@ -233,13 +232,10 @@
     if _person_has_not_licensed_translations(person):
         return False
 
-    # Finally, check whether the user is member of the translation team or
-    # owner for the given PO file.
+    # Finally, check whether the user is a member of the translation team.
     translators = [t.translator for t in pofile.translators]
     return _check_translation_perms(
-        pofile.translationpermission,
-        translators,
-        person) or person.inTeam(pofile.owner)
+        pofile.translationpermission, translators, person)
 
 
 def _can_add_suggestions(pofile, person):
@@ -316,11 +312,6 @@
         """See `IPOFile`."""
         return _can_add_suggestions(self, person)
 
-    def setOwnerIfPrivileged(self, person):
-        """See `IPOFile`."""
-        if self.canEditTranslations(person):
-            self.owner = person
-
     def getHeader(self):
         """See `IPOFile`."""
         translation_importer = getUtility(ITranslationImporter)
@@ -599,6 +590,9 @@
     @property
     def contributors(self):
         """See `IPOFile`."""
+        # Avoid circular import.
+        from lp.registry.model.person import Person
+
         # Translation credit messages are "translated" by
         # rosetta_experts.  Shouldn't show up in contributors lists
         # though.
@@ -1358,12 +1352,12 @@
         UTC = pytz.timezone('UTC')
         self.date_changed = None
         self.lastparsed = None
-        self.owner = getUtility(ILaunchpadCelebrities).rosetta_experts
 
-        # The default POFile owner is the Rosetta Experts team unless the
-        # given owner has rights to write into that file.
-        if self.canEditTranslations(owner):
-            self.owner = owner
+        if owner is None:
+            owner = getUtility(ILaunchpadCelebrities).rosetta_experts
+        # The "owner" is really just the creator, without any extra
+        # privileges.
+        self.owner = owner
 
         self.path = u'unknown'
         self.datecreated = datetime.datetime.now(UTC)

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2010-10-18 16:36:46 +0000
+++ lib/lp/translations/model/potemplate.py	2010-10-19 12:57:50 +0000
@@ -103,10 +103,7 @@
 from lp.translations.interfaces.translationimportqueue import (
     RosettaImportStatus,
     )
-from lp.translations.model.pofile import (
-    DummyPOFile,
-    POFile,
-    )
+from lp.translations.model.pofile import POFile
 from lp.translations.model.pomsgid import POMsgID
 from lp.translations.model.potmsgset import POTMsgSet
 from lp.translations.model.translationimportqueue import collect_import_info
@@ -170,7 +167,8 @@
     for entry, pofile in enumerate(result):
         assert pofile == result[entry], "This enumerate confuses me."
         if pofile is None:
-            result[entry] = DummyPOFile(potemplates[entry], language)
+            result[entry] = potemplates[entry].getDummyPOFile(
+                language, check_for_existing=False)
 
     return result
 
@@ -744,7 +742,7 @@
             # Update cache to reflect the change.
             template._cached_pofiles_by_language[language_code] = pofile
 
-    def newPOFile(self, language_code, create_sharing=True):
+    def newPOFile(self, language_code, create_sharing=True, owner=None):
         """See `IPOTemplate`."""
         # Make sure we don't already have a PO file for this language.
         existingpo = self.getPOFileByLang(language_code)
@@ -773,7 +771,8 @@
             data['origin'] = self.sourcepackagename.name
 
         # The default POFile owner is the Rosetta Experts team.
-        owner = getUtility(ILaunchpadCelebrities).rosetta_experts
+        if owner is None:
+            owner = getUtility(ILaunchpadCelebrities).rosetta_experts
 
         path = self._composePOFilePath(language)
 
@@ -806,6 +805,9 @@
     def getDummyPOFile(self, language, requester=None,
                        check_for_existing=True):
         """See `IPOTemplate`."""
+        # Avoid circular import.
+        from lp.translations.model.pofile import DummyPOFile
+
         if check_for_existing:
             # see if a valid one exists.
             existingpo = self.getPOFileByLang(language.code)

=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-export.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-export.txt	2009-12-10 12:46:11 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-export.txt	2010-10-19 12:57:50 +0000
@@ -1,4 +1,5 @@
-= Exporting Single PO Files through the Web =
+Exporting Single PO Files through the Web
+=========================================
 
 Not logged in users can't access the +export page.
 
@@ -57,24 +58,11 @@
     ...     print tag.renderContents()
     Your request has been received. Expect to receive an email shortly.
 
-Since no-privileges person isn't a valid translator for Welsh (cy)
-language, the ownership of the newly created pofile is set to Rosetta
-Admins.
-
-    >>> anon_browser.open(
-    ...     'http://translations.launchpad.dev/evolution'
-    ...     '/trunk/+pots/evolution-2.2/cy/+details')
-    >>> translation_portlet = find_portlet(
-    ...     anon_browser.contents, 'Translation file details')
-    >>> t = extract_text(
-    ...     translation_portlet.firstText('Creator:').findNext('a'))
-    >>> u'Rosetta Administrators' in t
-    True
-
-But if we request an export from a valid translator, he gets the
-ownership. We test using the Swedish (sv) language which doesn't have a
-pofile for evolution yet; it will be created at the moment the export is
-requested.
+If the POFile first has to be created, the requester becomes its owner.
+This implies no special privileges.
+
+We test using the Swedish (sv) language which doesn't have a pofile for
+evolution yet; it will be created at the moment the export is requested.
 
     >>> browser = setupBrowser(auth='Basic [e-mail]:test')
     >>> browser.open(
@@ -115,4 +103,3 @@
     >>> for tag in find_tags_by_class(browser.contents, 'informational'):
     ...     tag.renderContents()
     'Your request has been received. Expect to receive an email shortly.'
-

=== modified file 'lib/lp/translations/tests/test_pofile.py'
--- lib/lp/translations/tests/test_pofile.py	2010-10-18 16:36:46 +0000
+++ lib/lp/translations/tests/test_pofile.py	2010-10-19 12:57:50 +0000
@@ -33,6 +33,7 @@
 from lp.translations.interfaces.translationcommonformat import (
     ITranslationFileData,
     )
+from lp.translations.interfaces.translationgroup import TranslationPermission
 from lp.translations.interfaces.translationmessage import (
     TranslationValidationStatus,
     )
@@ -1888,6 +1889,16 @@
         self.pofile.markChanged(translator=translator)
         self.assertEqual(translator, self.pofile.lasttranslator)
 
+    def test_owner_has_no_privileges(self):
+        # Being a POFile's owner does not imply edit privileges.
+        creator = self.factory.makePerson()
+        removeSecurityProxy(self.pofile).owner = creator
+        naked_product = removeSecurityProxy(
+            self.potemplate.productseries.product)
+        naked_product.translationpermission = TranslationPermission.RESTRICTED
+
+        self.assertFalse(self.pofile.canEditTranslations(creator))
+
     def test_hasPluralFormInformation_bluffs_if_irrelevant(self):
         # If the template has no messages that use plural forms, the
         # POFile has all the relevant plural-form information regardless

=== modified file 'lib/lp/translations/tests/test_potemplate.py'
--- lib/lp/translations/tests/test_potemplate.py	2010-10-18 16:36:46 +0000
+++ lib/lp/translations/tests/test_potemplate.py	2010-10-19 12:57:50 +0000
@@ -12,7 +12,6 @@
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.testing import TestCaseWithFactory
 from lp.translations.interfaces.potemplate import IPOTemplateSet
-from lp.translations.model.pofile import DummyPOFile
 from lp.translations.model.potemplate import (
     get_pofiles_for,
     POTemplateSet,
@@ -29,6 +28,13 @@
         self.potemplate = removeSecurityProxy(self.factory.makePOTemplate(
             translation_domain = "testdomain"))
 
+    def assertIsDummy(self, pofile):
+        """Assert that `pofile` is actually a `DummyPOFile`."""
+        # Avoid circular imports.
+        from lp.translations.model.pofile import DummyPOFile
+
+        self.assertEquals(DummyPOFile, type(pofile))
+
     def test_composePOFilePath(self):
         esperanto = getUtility(ILanguageSet).getLanguageByCode('eo')
         self.potemplate.path = "testdir/messages.pot"
@@ -69,7 +75,7 @@
         # Test basic behaviour of getDummyPOFile.
         language = self.factory.makeLanguage('sr@test')
         dummy = self.potemplate.getDummyPOFile(language)
-        self.assertEquals(DummyPOFile, type(dummy))
+        self.assertIsDummy(dummy)
 
     def test_getDummyPOFile_with_existing_pofile(self):
         # Test that getDummyPOFile fails when trying to get a DummyPOFile
@@ -88,7 +94,22 @@
         # This is just "assertNotRaises".
         dummy = self.potemplate.getDummyPOFile(language,
                                                check_for_existing=False)
-        self.assertEquals(DummyPOFile, type(dummy))
+        self.assertIsDummy(dummy)
+
+    def test_newPOFile_owner(self):
+        # The intended owner of a new POFile can be passed to newPOFile.
+        language = self.factory.makeLanguage('nl@test')
+        person = self.factory.makePerson()
+        pofile = self.potemplate.newPOFile(language.code, owner=person)
+        self.assertEqual(person, pofile.owner)
+
+    def test_getDummyPOFile_owner(self):
+        # The intended owner of a new DummyPOFile can be passed to
+        # getDummyPOFile.
+        language = self.factory.makeLanguage('nl@test')
+        person = self.factory.makePerson()
+        pofile = self.potemplate.getDummyPOFile(language, requester=person)
+        self.assertEqual(person, pofile.owner)
 
     def test_getTranslationCredits(self):
         # getTranslationCredits returns only translation credits.
@@ -478,6 +499,10 @@
     def test_get_pofiles_for_untranslated_template(self):
         # If there is no POFile for a template in a language,
         # get_pofiles_for makes up a DummyPOFile.
+
+        # Avoid circular imports.
+        from lp.translations.model.pofile import DummyPOFile
+
         pofiles = get_pofiles_for([self.potemplate], self.greek)
         pofile = pofiles[0]
-        self.assertTrue(isinstance(pofile, DummyPOFile))
+        self.assertIsInstance(pofile, DummyPOFile)

=== modified file 'lib/lp/translations/tests/test_translatedlanguage.py'
--- lib/lp/translations/tests/test_translatedlanguage.py	2010-10-04 19:50:45 +0000
+++ lib/lp/translations/tests/test_translatedlanguage.py	2010-10-19 12:57:50 +0000
@@ -1,20 +1,25 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import with_statement
+
 __metaclass__ = type
 
 from zope.component import getUtility
 from zope.interface.verify import verifyObject
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.testing.layers import ZopelessDatabaseLayer
 from lp.app.enums import ServiceUsage
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.translations.interfaces.productserieslanguage import (
     IProductSeriesLanguageSet,
     )
 from lp.translations.interfaces.translatedlanguage import ITranslatedLanguage
-from lp.translations.model.pofile import DummyPOFile
 
 
 class TestTranslatedLanguageMixin(TestCaseWithFactory):
@@ -46,6 +51,22 @@
         potemplate.priority = priority
         return potemplate
 
+    def addPOFile(self, potemplate=None):
+        """Add a `POFile` for the given `POTemplate`, in `self.language`.
+
+        If no `potemplate` is given, one will be created.
+        """
+        if potemplate is None:
+            potemplate = self.addPOTemplate()
+        return self.factory.makePOFile(self.language.code, potemplate)
+
+    def assertIsDummy(self, pofile):
+        """Assert that `pofile` is actually a `DummyPOFile`."""
+        # Avoid circular imports.
+        from lp.translations.model.pofile import DummyPOFile
+
+        self.assertIsInstance(pofile, DummyPOFile)
+
     def test_interface(self):
         translated_language = self.getTranslatedLanguage(self.language)
         self.assertTrue(verifyObject(ITranslatedLanguage,
@@ -76,7 +97,7 @@
         # instead.
         dummy_pofile = pofiles[0]
         naked_dummy = removeSecurityProxy(dummy_pofile)
-        self.assertEqual(DummyPOFile, type(naked_dummy))
+        self.assertIsDummy(naked_dummy)
         self.assertEqual(self.language, dummy_pofile.language)
         self.assertEqual(potemplate, dummy_pofile.potemplate)
 
@@ -88,7 +109,7 @@
     def test_pofiles_template_with_pofiles(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate = self.addPOTemplate()
-        pofile = self.factory.makePOFile(self.language.code, potemplate)
+        pofile = self.addPOFile(potemplate)
         self.assertEqual([pofile], list(translated_language.pofiles))
 
         # Two queries get executed when listifying
@@ -101,9 +122,9 @@
         # Two templates with different priorities so they get sorted
         # appropriately.
         potemplate1 = self.addPOTemplate(priority=2)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         potemplate2 = self.addPOTemplate(priority=1)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
+        pofile2 = self.addPOFile(potemplate2)
         self.assertEqual([pofile1, pofile2],
                          list(translated_language.pofiles))
 
@@ -117,12 +138,12 @@
         # Two templates with different priorities so they get sorted
         # appropriately.
         potemplate1 = self.addPOTemplate(priority=2)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         potemplate2 = self.addPOTemplate(priority=1)
         pofiles = translated_language.pofiles
         self.assertEqual(pofile1, pofiles[0])
         dummy_pofile = removeSecurityProxy(pofiles[1])
-        self.assertEqual(DummyPOFile, type(dummy_pofile))
+        self.assertIsDummy(dummy_pofile)
 
         # Two queries get executed when listifying
         # TranslatedLanguageMixin.pofiles: a len() does a count, and
@@ -133,37 +154,39 @@
         # Slicing still works, and always does the same constant number
         # of queries (1).
         translated_language = self.getTranslatedLanguage(self.language)
-        # Three templates with different priorities so they get sorted
-        # appropriately.
-        potemplate1 = self.addPOTemplate(priority=2)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
-        potemplate2 = self.addPOTemplate(priority=1)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
-        potemplate3 = self.addPOTemplate(priority=0)
-
-        pofiles = translated_language.pofiles[0:2]
-        self.assertEqual([pofile1, pofile2], list(pofiles))
-
-        # Slicing executes only a single query.
-        get_slice = lambda of, start, end: list(of[start:end])
-        self.assertStatementCount(1, get_slice,
-                                  translated_language.pofiles, 1, 3)
+        pofile1 = self.addPOFile()
+        pofile2 = self.addPOFile()
+        self.addPOTemplate(priority=-1)
+
+        # This does assume that a few teams with special privileges are
+        # already cached.  For a normal user without those special
+        # privileges, no further queries are needed to authorize access.
+        user = self.factory.makePerson()
+        celebs = getUtility(ILaunchpadCelebrities)
+
+        with person_logged_in(user):
+            self.assertFalse(user.inTeam(celebs.admin))
+            self.assertFalse(user.inTeam(celebs.rosetta_experts))
+            pofiles = translated_language.pofiles[0:2]
+            self.assertContentEqual([pofile1, pofile2], list(pofiles))
+
+            get_slice = lambda of, start, end: list(of[start:end])
+            self.assertStatementCount(
+                1, get_slice, translated_language.pofiles, 1, 3)
 
     def test_pofiles_slicing_dummies(self):
         # Slicing includes DummyPOFiles.
         translated_language = self.getTranslatedLanguage(self.language)
         # Three templates with different priorities so they get sorted
         # appropriately.
-        potemplate1 = self.addPOTemplate(priority=2)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
-        potemplate2 = self.addPOTemplate(priority=1)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
-        potemplate3 = self.addPOTemplate(priority=0)
+        pofile1 = self.addPOFile(self.addPOTemplate(priority=2))
+        pofile2 = self.addPOFile(self.addPOTemplate(priority=1))
+        self.addPOTemplate(priority=0)
 
         pofiles = translated_language.pofiles[1:3]
         self.assertEqual(pofile2, pofiles[0])
         dummy_pofile = removeSecurityProxy(pofiles[1])
-        self.assertEqual(DummyPOFile, type(dummy_pofile))
+        self.assertIsDummy(dummy_pofile)
 
     def test_statistics_empty(self):
         translated_language = self.getTranslatedLanguage(self.language)
@@ -222,7 +245,7 @@
     def test_recalculateCounts_total_one_pofile(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile = self.factory.makePOFile(self.language.code, potemplate)
+        pofile = self.addPOFile(potemplate)
 
         translated_language.recalculateCounts()
         self.assertEqual(
@@ -231,9 +254,9 @@
     def test_recalculateCounts_total_two_pofiles(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate1 = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         potemplate2 = self.addPOTemplate(number_of_potmsgsets=3)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
+        pofile2 = self.addPOFile(potemplate2)
 
         translated_language.recalculateCounts()
         self.assertEqual(
@@ -242,7 +265,7 @@
     def test_recalculateCounts_translated_one_pofile(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile = self.factory.makePOFile(self.language.code, potemplate)
+        pofile = self.addPOFile(potemplate)
         naked_pofile = removeSecurityProxy(pofile)
         # translated count is current + rosetta
         naked_pofile.currentcount = 3
@@ -255,14 +278,14 @@
     def test_recalculateCounts_translated_two_pofiles(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate1 = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         naked_pofile1 = removeSecurityProxy(pofile1)
         # translated count is current + rosetta
         naked_pofile1.currentcount = 3
         naked_pofile1.rosettacount = 1
 
         potemplate2 = self.addPOTemplate(number_of_potmsgsets=3)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
+        pofile2 = self.addPOFile(potemplate2)
         naked_pofile2 = removeSecurityProxy(pofile2)
         # translated count is current + rosetta
         naked_pofile2.currentcount = 1
@@ -275,7 +298,7 @@
     def test_recalculateCounts_changed_one_pofile(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile = self.factory.makePOFile(self.language.code, potemplate)
+        pofile = self.addPOFile(potemplate)
         naked_pofile = removeSecurityProxy(pofile)
         # translated count is current + rosetta
         naked_pofile.updatescount = 3
@@ -287,12 +310,12 @@
     def test_recalculateCounts_changed_two_pofiles(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate1 = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         naked_pofile1 = removeSecurityProxy(pofile1)
         naked_pofile1.updatescount = 3
 
         potemplate2 = self.addPOTemplate(number_of_potmsgsets=3)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
+        pofile2 = self.addPOFile(potemplate2)
         naked_pofile2 = removeSecurityProxy(pofile2)
         naked_pofile2.updatescount = 1
 
@@ -303,7 +326,7 @@
     def test_recalculateCounts_new_one_pofile(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile = self.factory.makePOFile(self.language.code, potemplate)
+        pofile = self.addPOFile(potemplate)
         naked_pofile = removeSecurityProxy(pofile)
         # new count is rosetta - changed
         naked_pofile.rosettacount = 3
@@ -316,14 +339,14 @@
     def test_recalculateCounts_new_two_pofiles(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate1 = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         naked_pofile1 = removeSecurityProxy(pofile1)
         # new count is rosetta - changed
         naked_pofile1.rosettacount = 3
         naked_pofile1.updatescount = 1
 
         potemplate2 = self.addPOTemplate(number_of_potmsgsets=3)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
+        pofile2 = self.addPOFile(potemplate2)
         naked_pofile2 = removeSecurityProxy(pofile2)
         # new count is rosetta - changed
         naked_pofile2.rosettacount = 2
@@ -336,7 +359,7 @@
     def test_recalculateCounts_unreviewed_one_pofile(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile = self.factory.makePOFile(self.language.code, potemplate)
+        pofile = self.addPOFile(potemplate)
         naked_pofile = removeSecurityProxy(pofile)
         # translated count is current + rosetta
         naked_pofile.unreviewed_count = 3
@@ -348,12 +371,12 @@
     def test_recalculateCounts_unreviewed_two_pofiles(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate1 = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         naked_pofile1 = removeSecurityProxy(pofile1)
         naked_pofile1.unreviewed_count = 3
 
         potemplate2 = self.addPOTemplate(number_of_potmsgsets=3)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
+        pofile2 = self.addPOFile(potemplate2)
         naked_pofile2 = removeSecurityProxy(pofile2)
         naked_pofile2.unreviewed_count = 1
 
@@ -364,7 +387,7 @@
     def test_recalculateCounts_one_pofile(self):
         translated_language = self.getTranslatedLanguage(self.language)
         potemplate = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile = self.factory.makePOFile(self.language.code, potemplate)
+        pofile = self.addPOFile(potemplate)
         naked_pofile = removeSecurityProxy(pofile)
         # translated count is current + rosetta
         naked_pofile.currentcount = 3
@@ -394,7 +417,7 @@
 
         # Set up one template with a single PO file.
         potemplate1 = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         naked_pofile1 = removeSecurityProxy(pofile1)
         # translated count is current + rosetta
         naked_pofile1.currentcount = 2
@@ -407,7 +430,7 @@
 
         # Set up second template with a single PO file.
         potemplate2 = self.addPOTemplate(number_of_potmsgsets=3)
-        pofile2 = self.factory.makePOFile(self.language.code, potemplate2)
+        pofile2 = self.addPOFile(potemplate2)
         naked_pofile2 = removeSecurityProxy(pofile2)
         # translated count is current + rosetta
         naked_pofile2.currentcount = 1
@@ -438,7 +461,7 @@
 
         # Set up one template with a single PO file.
         potemplate1 = self.addPOTemplate(number_of_potmsgsets=5)
-        pofile1 = self.factory.makePOFile(self.language.code, potemplate1)
+        pofile1 = self.addPOFile(potemplate1)
         naked_pofile1 = removeSecurityProxy(pofile1)
         # translated count is current + rosetta
         naked_pofile1.currentcount = 2