Merge lp:~jtv/launchpad/recife-kill-updatetranslation-browser into lp:~launchpad/launchpad/recife

[Jeroen T. Vermeulen]

Get rid of POTMsgSet.updateTranslation in browser code.

= Getting Rid of updateTranslation, Part II: Browser Code =

This eliminates updateTranslation from the browser code responsible for accepting and saving translations entered in the web UI. It's for the Recife feature branch. It moves the entire family of views over to the new data model, and partly the last remaining unmigrated view as well. updateTranslation is a behemoth of such complexity and intelligence that we had to replace it rather than try to maintain it.

I didn't try to get it all done in this branch, or it would get too big. Unfortunately that means that I had to disable one test and hobble another: having one leg in the old data model and one in the new is bound to break some tests. I created a kanban card for re-activating the disabled test—the other one we'll find back automatically as it will break once it becomes ready to move over.

A very pedestrian-looking part of the migration is where we replace getCurrentTranslationMessage calls with getCurrentTranslation ones. The latter is suited for the new model, whereas the former is a holdout from the old world. I also use a traits class here and there that abstracts the difference between the two "translation sides": Ubuntu translations and upstream translations. In the Recife model, a single TranslationMessage can be shared between Ubuntu and a project so it may be current on either side, or both, or neither. The traits allow code to ask things like "does this TranslationMessage have the flag set that designates it as the current translation for this side?"—without having to care which of the flags for which of the two sides that is.

The browser method with The Big Change is _storeTranslations. I extracted some methods to reduce cyclomatic complexity and increase testability. That makes the replacement for updateTranslation look simpler than perhaps it is. Still, simplicity is a good thing!

You'll notice one case where the POTemplate passed to getCurrentTranslation is None. That's because a TM's potemplate is not there to tell us exactly which potemplate it belongs to (normally it can be shared between any number of templates) but rather whether it is diverged to a specific potemplate (and if so of course, which one). So passing None means "I'm not interested in diverged messages; give me only the shared ones i.e. the ones with potemplate set to None."

I also made the getCurrentTranslationMessageOrDummy side-sensitive. It always used to look for the current translation message on the Ubuntu side, which is not very helpful in the new model. Now it looks for the current translation message on whatever side the given POFile is on.

The only good way to test this is to run the full Translations test suite. We'll Q/A it with the rest of the feature branch. There's a bit of pre-existing lint left, but only of a kind where I'm not confident that the linter is right: complaints about the number of blank lines between methods when there is a free-floating comment that needs a blank line on top and another below.

Jeroen

[Henning Eggers (henninge)]

<jtv> hi henninge!
<jtv> Q about my MP?
<henninge> yes
<jtv> It doesn't for me.
<jtv> But give my http connection a bit more time.
<jtv> Ahhh there it is.
<henninge> makeSourcePackage makes an *Ubuntu* source package?
<jtv> What's the surprise?
* StevenK hat das Thema geändert zu: On call: - || Reviewing: || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Heh, I hadn't thought of that. But we now basically assume that all source packages are Ubuntu, don't we?
<StevenK> We shouldn't
<jtv> Because I think you're right: it creates a new distro AFAIK.
<jtv> StevenK: this is Translations, it's special. :)
<henninge> jtv: we do???
<henninge> oh, right, we do .. we are Translations ....
<henninge> ;-)
<jtv> We are special.
<henninge> So it's our checking if productseries is None -> it's ubuntu
<jtv> Our new data model accommodates only one family of distributions. Once we have the sharing working, we can worry about shoehorning in more—if required.
<henninge> but it wasn't in focus ;)
<jtv> 'zacly
<henninge> jtv: but still, I always use the "ubuntu" celebrity, I think.
<henninge> to be explicit
<jtv> That's perfectly correct, just very verbose. My personal feeling is we'll want a makeUbuntuSourcePackage at some point.
<jtv> Or maybe even a makePOFile/makePOTemplate option for_ubuntu.
<henninge> yes, our dear Ubuntu. :-)
<jtv> Or in this case, our ubiquitous ubuntu.
<henninge> jtv: so can you please be verbose in this test and use ubuntu?
<jtv> Our user-friendly but undeniably un-upstream ubiquitous ubuntu.
<henninge> "un-upstream" as in "not is_current_upstream!
<henninge> "
<henninge> ?
<jtv> Why? We're assuming that it'll always be either Ubuntu or a derivative. There's no easy way for the code to discover the difference. If this ever becomes not what we want, we'll have more places to change and that would be a good time to come up with something more convenient.
<jtv> Exactly.
<henninge> ok, ok ...
<jtv> It's really über-un-upstream.
<henninge> jtv: template.getTranslationPolicy
<jtv> uh-huh
<henninge> I guess you added that in a recent branch?
<jtv> Yes. It gets the ITranslationPolicy implementation that applies to the template.
<jtv> (Which is either a Project or a Distribution, though a Project may also inherit part of its policy from a ProjectGroup)
<jtv> I considered calling it getPillar, but we (including yours truly) have been overusing that a bit, and it's not quite right here.
<jtv> This also gives us the option to break out the repetitive policy data into its own ORM-backed class in the future.
<henninge> cool
<henninge> jtv: two more things
<jtv> (I'm sitting here facing a lady reading a house-and-garden magazine. The rear cover shows either a palace or a stately mansion with an elephant either grazing in the massive land around it, or carrying the house on its back. The perspective makes it unclear. The front cover says "Humble Living.")
<jtv> Shoot.
<henninge> It feels to me like there should be more test_revert_unselected_translations where plural_indices_to_store has a value other than []
<henninge> there is only one so far
<henninge> on...

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Thanks. I've updated the tests as suggested.