Merge lp:~abentley/launchpad/fix-undiverging into lp:launchpad

[Aaron Bentley]

[r=jtv][ui=none][bug=716586] Allow converging diverged translation

= Summary =
Fix bug #716586: Diverged translation cannot be converged again.

== Proposed fix ==
Fix faulty check for whether the specified translation is already current for the template.

== Pre-implementation notes ==
None

== Implementation details ==
Instead of checking the is_current_ubuntu/is_current_upstream flags, which might be set for an existing shared translation, use getCurrentTranslation, which is the standard way to check whether a translation is current for a template.

== Tests ==
bin/test -v test_potmsgset -t approveExistingShared

== Demo and Q/A ==
Go to a translatable project, e.g. https://translations.qastaging.launchpad.net/mailclipper/trunk/+pots/testcase/fr/1/+translate. Create a shared translation if none exists. Create a diverged translation. Change the diverged translation back to the shared translation (by selecting "In Ubuntu" or by manually specifyiing the same text).

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/tests/test_potmsgset.py
  lib/lp/translations/model/potmsgset.py

[Jeroen T. Vermeulen (jtv)]

Thanks for fixing that—the mistake was mine!

A note about the test: there's a factory.makeDivergedTranslationMessage for creating diverged messages.

About the fix itself: approveSuggestion could be used very intensively, so for performance it may be worth shortcutting the getCurrentTranslation call (which queries the database) in cases where the message is diverged. In those cases, there's no chance of it being shadowed by another diverged message.

review: Approve (code)

[Aaron Bentley (abentley)]

> A note about the test: there's a factory.makeDivergedTranslationMessage for
> creating diverged messages.

There is, but we want this to be a *current* diverged message. makeCurrentTranslationMessage calls makeDivergedTranslationMessage when diverged=True.

> About the fix itself: approveSuggestion could be used very intensively, so for
> performance it may be worth shortcutting the getCurrentTranslation call (which
> queries the database) in cases where the message is diverged. In those cases,
> there's no chance of it being shadowed by another diverged message.

True. This change was obviously about correctness over performance.

Note that _setTranslation also calls getCurrentTranslation, so if I was optimising, I would first change _setTranslation to handle the case where the incumbent is the suggestion. Doing the check in approveSuggestion smells like Look-Before-You-Leap.

[Jeroen T. Vermeulen (jtv)]

Diverged implies current. Barring low-impact bugs, there are no non-current diverged messages. So "diverged" is more specific than "current."

Optimizing _setTranslation would be a lot more risky. We prefer to be very conservative with it.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-02-15 02:09 PM, Jeroen T. Vermeulen wrote:
> Diverged implies current. Barring low-impact bugs, there are no non-current diverged messages.

Thanks for the info. It's surprising that you wouldn't want to know
which messages were previously been associated with a given template.

> So "diverged" is more specific than "current."

I don't understand why we have makeDivergedTranslation, then.
makeCurrentTranslation is more powerful, and according to you, diverged
translations must be current, so there's no reason to have both.

> Optimizing _setTranslation would be a lot more risky. We prefer to be very conservative with it.

Then you need more unit tests, so that you feel comfortable making
whatever performance improvements are necessary.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1a0osACgkQ0F+nu1YWqI1OOACfWBQee2SdICZHcBfxkC1EQ/WH
AFAAn00x/yHNheNj6psG9UzTS0LX0Orh
=Kupi
-----END PGP SIGNATURE-----

[Jeroen T. Vermeulen (jtv)]

Diverged messages should be very rare except as a remnant of pre-sharing data. So by far most messages should not have been associated with a specific template; they will be shared between templates. It's an interesting thought to keep that information around, but currently we don't. We do keep some information about where a message comes from, of course: the project or source package as well as the translator and reviewer.

It's true that the situation with factory methods isn't ideal. Translation messages can be suggestions; shared; or diverged. Yet we have three methods for making, respectively, suggestions; lots of different kinds of messages; or diverged messages. This is largely an artifact of our transition to the Recife model. We used to have a dedicated factory method for making shared messages. I think the cleaner way would be to leave the option for creating diverged messages out of makeCurrentTranslationMessage (taking away a code path and a parameter) and perhaps renaming it makeSharedTranslationMessage. Swiss-army-knife methods with parameters to select different, possibly conflicting operating modes are something we're trying to get away from.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2011-02-04 17:14:20 +0000
+++ lib/lp/translations/model/potmsgset.py	2011-02-14 22:00:11 +0000
@@ -721,9 +721,9 @@
             that this change is based on.
         """
         template = pofile.potemplate
-        traits = getUtility(ITranslationSideTraitsSet).getTraits(
-            template.translation_side)
-        if traits.getFlag(suggestion):
+        current = self.getCurrentTranslation(
+            template, pofile.language, template.translation_side)
+        if current == suggestion:
             # Message is already current.
             return
 

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2011-01-27 23:13:20 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2011-02-14 22:00:11 +0000
@@ -270,6 +270,20 @@
                 serbian, self.stable_potemplate.translation_side),
             shared_translation)
 
+    def test_approveExistingShared(self):
+        """"Existing shared translation become current when approved."""
+        pofile = self.factory.makePOFile()
+        shared = self.factory.makeCurrentTranslationMessage(pofile=pofile)
+        potmsgset = shared.potmsgset
+        diverged = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset, diverged=True)
+        potemplate = diverged.potemplate
+        removeSecurityProxy(potmsgset).approveSuggestion(
+            pofile, shared, shared.reviewer)
+        current = potmsgset.getCurrentTranslation(
+            potemplate, pofile.language, potemplate.translation_side)
+        self.assertEqual(shared, current)
+
     def test_getLocalTranslationMessages(self):
         """Test retrieval of local suggestions."""
         # Share a POTMsgSet in two templates, and get a Serbian POFile.