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

[Jeroen T. Vermeulen]

[r=bac][bug=613823] Punt translations approval conflicts to the user.

= Summary =

The Translations Import Queue Gardener is reporting a lot of approval conflicts: uploads that can't be approved because there is more than one candidate translation template with the same domain, and so it doesn't know which one to import the translations to.

These conflicts have traditionally been logged, and echoed to the launchpad-error-reports list where the translations admins could do something about them. But since then, project owners and such gained the ability to change templates' domains themselves. And meanwhile, there is no more Translations Team. So rather than spewing warnings onto an internal mailing list, we should report the problem to the user.

== Proposed fix ==

Store a description of the problem in TranslationImportQueueEntry.error_output. This field is normally used for error or warning output from the import itself, but there's no reason why the approver shouldn't do it as well.

== Pre-implementation notes ==

Henning and I went through a few possibilities: is there no risk of an approver error being overwritten by an importer error? Only after manual approval, in which case the problem itself usually goes away. (If it doesn't, the error comes back and unless it is fixed or worked around, there will be no import to overwrite it). The situation is buried pretty deep in the code and multiple execution paths may lead to it. What if approval does succeed after all? We decided that successful auto-approval should clear the error field anyway. You certainly don't want a successful re-import of a file to retain an error from a previous unsuccessful import anyway.

== Implementation details ==

We may want to make this problem more obvious to the user in the future. I'm not sure a queue entry's error output is available without Ajax, for instance—though it's going to be an improvement over completely ignored log output. So I carved out a separate method for the notification, which we can extend later with e.g. email if desired.

I stripped getPOTemplateByTranslationDomain from its knowledge of clashes, renamed it, and made it query all matching templates without judgment. Since I had to add some unit tests for it anyway, I also converted its doctest section into unit tests.

== Tests ==

I deliberately did not change security.cfg before testing the new path under the proper database user, thinking that I was building a proper failing test. But obviously, the gardener already has the necessary database privilege (UPDATE on TranslationImportQueueEntry).

{{{
./bin/test -vvc lp.translations.tests.test_autoapproval
./bin/test -vvc lp.translations.tests.test_potemplate
./bin/test -vvc lp.translations.tests.test_translationimportqueue
}}}

== Demo and Q/A ==

Plenty of conflicts have accumulated, so just run the updated approver on staging and query the database for TranslationImportQueueEntry rows with a nonempty error_output and status = 5 (which means RosettaImportStatus.NEEDS_REVIEW). You should see a few of these notices appear.

= Launchpad lint =

I left the lint in the doctest; I can reformat it after this review if desired, but didn't want to pollute the diff too much.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/tests/test_potemplate.py
  lib/lp/translations/interfaces/potemplate.py
  lib/lp/translations/model/translationimportqueue.py
  lib/lp/translations/tests/test_autoapproval.py
  lib/lp/testing/factory.py
  lib/lp/translations/doc/potemplate.txt
  lib/lp/translations/tests/test_translationimportqueue.py
  lib/lp/translations/model/potemplate.py
  lib/lp/translations/browser/potemplate.py

./lib/lp/translations/doc/potemplate.txt
       1: narrative uses a moin header.
       9: narrative uses a moin header.
      23: narrative uses a moin header.
      49: narrative uses a moin header.
      74: narrative uses a moin header.
      89: narrative uses a moin header.
      99: narrative uses a moin header.
     111: narrative uses a moin header.
     152: narrative uses a moin header.
     168: narrative uses a moin header.
     177: narrative uses a moin header.
     189: narrative uses a moin header.
     215: narrative uses a moin header.
     258: narrative uses a moin header.
     336: want exceeds 78 characters.
     337: want exceeds 78 characters.
     389: want exceeds 78 characters.
     411: want exceeds 78 characters.
     412: want exceeds 78 characters.
     413: want exceeds 78 characters.
     414: want exceeds 78 characters.
     419: want exceeds 78 characters.
     420: want exceeds 78 characters.
     421: want exceeds 78 characters.
     426: want exceeds 78 characters.
     427: want exceeds 78 characters.
     428: want exceeds 78 characters.
     430: want exceeds 78 characters.
     438: want exceeds 78 characters.
     441: want exceeds 78 characters.
     446: want exceeds 78 characters.
     447: want exceeds 78 characters.
     448: want exceeds 78 characters.
     450: want exceeds 78 characters.
     452: want exceeds 78 characters.
     453: want exceeds 78 characters.
     455: want exceeds 78 characters.
     456: want exceeds 78 characters.
     457: want exceeds 78 characters.
     460: want exceeds 78 characters.
     464: want exceeds 78 characters.
     468: want exceeds 78 characters.
     470: want exceeds 78 characters.
     475: want exceeds 78 characters.
     478: want exceeds 78 characters.
     479: want exceeds 78 characters.
     482: want exceeds 78 characters.
     485: want exceeds 78 characters.
     493: want exceeds 78 characters.
     494: want exceeds 78 characters.
     496: want exceeds 78 characters.
     497: want exceeds 78 characters.
     498: want exceeds 78 characters.
     501: want exceeds 78 characters.
     502: want exceeds 78 characters.
     505: want exceeds 78 characters.
     509: want exceeds 78 characters.
     511: want exceeds 78 characters.
     513: want exceeds 78 characters.
     514: want exceeds 78 characters.
     519: want exceeds 78 characters.
     522: want exceeds 78 characters.
     524: want exceeds 78 characters.
     532: narrative uses a moin header.
     568: source exceeds 78 characters.

[Brad Crittenden (bac)]

Hi jtv,

On IRC we agreed to just set iscurrent in the factory as it is more straightforward.

s/comtaining/containing/

Otherwise this branch looks good. Thanks for not fixing the lint as it would've confused things a lot. If you'd like to fix it now that would be cool...but it's up to you.

review: Approve