Merge lp:~jtv/launchpad/recife-approveSuggestion into lp:~launchpad/launchpad/recife

[Jeroen T. Vermeulen]

TranslationMessage.approve

= Approving Suggestions =

For the Recife feature branch. I learned one big advantage of TDD while working on this branch: writing the tests brought the diff close to the review limit, and implementation was almost an afterthought. (I even had a working filthy-hack shortcut implementation for a while that passed tests). Watching the tests grow before you consider implementation is a great way to keep yourself under the limit.

Here I implement a new method: TranslationMessage.approve, which approves a suggestion (making it its message's current translation, at least in its POFile but more likely also in other POFiles that share the same POTMsgSet). The method tunnels straight through to one on POTMsgSet, but I felt that calling "approve" on a suggestion was a nicer, simpler API.

The method on POTMsgSet re-uses the bulk of setCurrentTranslation, but provides some values that the old setCurrentTranslation would have to query the database for. So I spliced these values out, and moved their existing definitions into a new setCurrentTranslation. The bulk then became a private method, for shared use together with approveSuggestion.

The first thing you'll notice in the diff, however, is a new factory method: makeCurrentTranslationMessage. The old makeTranslationMessage was highly complex, based as it was on the obsolescent Swiss-army-knife-cum-sliderule updateTranslation method. In the current transient state of the Recife branch, the latter produces nonsensical results that we can't easily change because of test breakage. We're getting rid of updateTranslation, replacing it with more specialized methods such as "submitSuggestion" and "approve"—the latter being what you're reviewing here. The new factory method is both more explicit about what it does, and entirely correct under the new Recife data model.

The diff gets awkward in lib/lp/translations/model/potmsgset.py, under "@@ -986,18 +984,12 @@." To help interpret it: the old code created a TranslationMessage, assigned karma to the submitter, and returned the message. The new code delegates the karma work to a helper in POTemplate, then creates and returns the message.

In what is now _setTranslation (previously setCurrentTranslation), I renamed the "translations" variable to a parameter "potranslations." This is to clarify the distinction between the translations argument that setCurrentTranslation receives (a list of strings) and the one that _setTranslation takes (a list of POTranslation references supplemented with Nones).

To test:
{{{
./bin/test -vvc -m lp.testing.tests.test_factory -t makeCurrent
./bin/test -vvc -m lp.translations.tests.test_potemplate -t Karma
./bin/test -vvc -m lp.translations.tests.test_translationmessage -t markReviewed -t TestApprove
}}}

I eliminated all lint, save two identical cases in IPOTemplate where the linter reads comments as blank lines and so claims that there are 2 blank lines between consecutive methods. I say it's wrong.

Jeroen

[Graham Binns (gmb)]

Hi Jeroen,

I'm happy with this branch, but you need to add some comments or docstrings to the new tests in test_translationmessage.py; I shouldn't have to read a unit test to find out what it tests ;).

Other than that this is fine, so I'll mark it approved pending the comments.

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Comments added.