Merge lp:~jtv/launchpad/bug-613823 into lp:launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Jeroen T. Vermeulen | ||||
Approved revision: | no longer in the source branch. | ||||
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Brad Crittenden (community) | Approve | ||
Review via email:
|
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-
== Proposed fix ==
Store a description of the problem in TranslationImpo
== 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 getPOTemplateBy
== 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 TranslationImpo
{{{
./bin/test -vvc lp.translations
./bin/test -vvc lp.translations
./bin/test -vvc lp.translations
}}}
== Demo and Q/A ==
Plenty of conflicts have accumulated, so just run the updated approver on staging and query the database for TranslationImpo
= 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/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
./lib/lp/
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.
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.