Code review comment for lp:~henninge/launchpad/bug-128324

Revision history for this message
Henning Eggers (henninge) wrote :

= Details =
See bug 128324.
This branch only contains half the fix as proposed in the bug. It adds the creation of dummy translations to the creation of POTMsgSets. The question of where the creation of POTMsgSets actually happens lead to some more related changes so that it is best to land this part separately now.

A POTMsgSet is created by POTemplate.createPOTMsgSetFromMsgIDs() whose only call site is in POTemplate.createMessageSetFromText(). This used to create an "unbound" POTMsgSet that is not linked to a POTemplate by a TranslationTemplateItem instance. This had to be done by a subsequent call to setSequence.

Having identified createPOTMsgSetFromMsgIDs as the place where POTMsgSets are born, I thought that might be a good place to put the "create dummy translations for all pofiles that belong to this potemplate". But wait, the POTMsgSet had not yet officially been linked to the POTemplate, so setting translations in its POFile seems premature.

The first obvious solution was to do it in setSequence but that seemed to be too much because that methods gets called a lot when re-importing templates where the order has changed.

So really the question is: Should unbound POTMsgSets exist at all? My answer is a clear: No, there is no point in having them. The logical solution is to set the sequence to 0 in createPOTMsgSetFromMsgIDs which the only production call site for createMessageSetFromText had done anyway.

So it was only the makePOTMsgSet factory method that needed to be adjusted too and any test that depended on this missfeature. Luckily they were few.

== Implementation details ==
I decided to move some tests from the doctest to the unit tests for better coverage. Also, testing POTMsgSet.setTranslationCreditsToTranslated separately does not make sense anymore because the factory method will already call it. So it is now tested as part of the creation test.

One of the call sites for createMessageSetFromText was in the old test_vpoexport.py whose content I could move to test_pofile.py completely, thereby removing this relict.

== Test ==

Run the new tests and those affected by the change. I ran all of lp.translations tests to find out which were affected by the test. (A full ec2 test run has taken place since then, too.)

bin/test -vvt potmsgset.txt -t TestTranslationCredits -t getTranslationRows_sequence -t shared_potemplate

== QA/Demo ==

1. Download an existing potemplate that does not have translator-credits.
2. Add translator-credits to the template and import it again.
3. Filter for untranslated messages in any of the languages that this template has translations in.
4. The translator-credits message should not appear among the untranslated messages.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/testing/factory.py
  lib/lp/translations/doc/potmsgset.txt
  lib/lp/translations/interfaces/potemplate.py
  lib/lp/translations/model/potemplate.py
  lib/lp/translations/tests/test_pofile.py
  lib/lp/translations/tests/test_potmsgset.py
  lib/lp/translations/tests/test_shared_potemplate.py

== Pylint notices ==

lib/lp/translations/interfaces/potemplate.py
    9: [F0401] Unable to import 'lazr.enum' (No module named enum)
[

« Back to merge proposal