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

Proposed by Jeroen T. Vermeulen on 2011-09-16
Status: Merged
Approved by: Jeroen T. Vermeulen on 2011-09-16
Approved revision: 13960
Merged at revision: 13973
Proposed branch: lp:~jtv/launchpad/bug-613823
Merge into: lp:launchpad
Diff against target: 0 lines
To merge this branch: bzr merge lp:~jtv/launchpad/bug-613823
Reviewer Review Type Date Requested Status
Brad Crittenden 2011-09-16 Approve on 2011-09-16
Review via email: mp+75773@code.launchpad.net

Commit Message

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

Description of the Change

= 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.

To post a comment you must log in.
lp:~jtv/launchpad/bug-613823 updated on 2011-09-16
13960. By Jeroen T. Vermeulen on 2011-09-16

Review change for clarity.

Brad Crittenden (bac) wrote :

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
lp:~jtv/launchpad/bug-613823 updated on 2011-09-16
13961. By Jeroen T. Vermeulen on 2011-09-16

Review change: typo.

13962. By Jeroen T. Vermeulen on 2011-09-16

Automatically reformatted doctest. Did not fix most of the lint. :(

Preview Diff

Empty