Merge lp:~jtv/launchpad/bug-599254 into lp:launchpad/db-devel

Ensure defined flushing order when making TranslationMessages current.

= Bug 599254 =

When translating a given message in a given template to a given language, only one TranslationMessage can have the is_current flag set and only one can have the is_imported flag set. The database ensures this through unique constraints.

In the current code (which we are working to replace entirely) we have validator functions that keep us within the bounds of those constraints. Whenever we set a TM's is_current to True, the corresponding validator first finds the message that previously had the flag set, if any, and clears the flag there. Same for the is_imported flag. They also tell the ORM that the change clearing the old flag needs to be flushed to the database before the change setting the new one.

These validators only look for translations that are for the same template. This is not as simple as it seems: with message sharing, most messages will have their template set to None which means they're shared across templates. Things quickly get confusing when changes to these flags are combined with changes to the TranslationMessage.potemplate field. Bug 599254 involves violations of the unique constraints (i.e, oopses) as the ORM flushes changes in an inconvenient order.

In this branch I'm making a minimal change to define flush order in cases where we were already clearing flags but not running the affected TranslationMessages through the validators. There are no functional changes. It looks probable that re-ordering the changes to TranslationMessages could also have solved the problem, but that would involve a risk of unintended functional changes. Our "recife" project is going to replace the entire cluster of methods that triggers the problem, as well as the validators. Not a good time for finicky unpredictable tweaks.

In the validators I did have to add an extra check against defining flush order between a TranslationMessage and itself. Some of the tests triggered such an error at one point.

I found myself unable to come up with a sensible test for this that doesn't take a lot of work or duplicate lots of other tests. We already have tons of tests covering the methods involved, and those all pass with my change.

To test, run all translations tests but in particular:
{{{
./bin/test -vv -t potmsgset -t translationmessage
}}}

There was one piece of lint in code I haven't touched: an unnecessary assignment. I'm leaving it intact to avoid conflicts with the ongoing Recife work that will touch the same code.

Jeroen