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

[Jeroen T. Vermeulen]

TranslationMessage.approveAsDiverged

= approveAsDiverged =

For the Recife feature branch. This implements the option to approve a translation suggestion, not as the new shared translation as you'd normally want, but as a diverged translation specific to one particular POTemplate.

Interface-wise, the new method is on TranslationMessage: "approve this one in this POFile." The actual implementation is in POTMsgSet, where all the supporting infrastructure is, and the published one forwards to it.

To test:
{{{
./bin/test -vvc lp.translations.tests.test_translationmessage -t ApproveAsDiverged
}}}

(Although of course I ran the rest of 'em as well).

I also seized this opportunity to replace a factory method, previously a hack to work around the lack of this new method, to do things more properly.

No lint,

Jeroen

[Michael Nelson (michael.nelson)]

Hi Jeroen,

As a person not-so-familiar with the translations infrastructure, I find a lot about this branch hard to understand... but I'm not sure if they are things that are just due to the complexity of the problem, or could be improved by the vocabulary. See what you think. To me it seems to be going the direction that soyuz went a while ago (methods that are hard for anyone without insider knowledge to understand) and we've been trying to make it easier to maintain ever since.

Also, if possible, it would be great if approvedAsDiverged() could be simplified (by creating smaller methods that encapsulate sections of it... some thoughts below).

That said, if you look at what I've written and think it doesn't apply, I'll recant and approve - but I would love to be able to understand more easily what's going on in this branch.

> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py 2010-09-01 13:47:24 +0000
> +++ lib/lp/testing/factory.py 2010-09-02 12:37:46 +0000
> @@ -2300,18 +2298,16 @@
> """Create a diverged, current `TranslationMessage`."""
> if pofile is None:
> pofile = self.makePOFile('lt')
> + if reviewer is None:
> + reviewer = self.makePerson()
>
> # This creates a suggestion, then diverges it, then activates it.
> # Once we have a method for diverging messages, do this in a more
> # proper way.

With your changes below, is it time to update the above comment?

> - message = self.makeSharedTranslationMessage(
> + message = self.makeSuggestion(
> pofile=pofile, potmsgset=potmsgset, translator=translator,
> - reviewer=reviewer, translations=translations, suggestion=True)
> - traits = getUtility(ITranslationSideTraitsSet).getTraits(
> - pofile.potemplate.translation_side)
> - removeSecurityProxy(message).potemplate = pofile.potemplate
> - traits.setFlag(message, True)
> - return message
> + translations=translations)
> + return message.approveAsDiverged(pofile, reviewer)
>
> def makeTranslation(self, pofile, sequence,
> english=None, translated=None,
>
> === modified file 'lib/lp/translations/model/potmsgset.py'
> --- lib/lp/translations/model/potmsgset.py 2010-09-01 15:11:18 +0000
> +++ lib/lp/translations/model/potmsgset.py 2010-09-02 12:37:46 +0000
> @@ -1145,6 +1145,61 @@
> template.awardKarma(translator, 'translationsuggestionapproved')
> template.awardKarma(reviewer, 'translationreview')
>
> + def approveAsDiverged(self, pofile, suggestion, reviewer,
> + lock_timestamp=None):
> + """Approve a suggestion to become a diverged translation."""
> + template = pofile.potemplate
> + traits = getUtility(ITranslationSideTraitsSet).getTraits(
> + template.translation_side)
> + if suggestion.potemplate == template and traits.getFlag(suggestion):
> + # The suggestion is already current and diverged for the
> + # right template.
> + return suggestion

Could this be extracted (so that this method...

review: Needs Information (code)

[Данило Шеган (danilo)]

У чет, 02. 09 2010. у 13:55 +0000, Michael Nelson пише:
>
> Should it be obvious to me what traits.setFlag(message, True) actually
> means, without going and looking it up?

It should at least be "more obvious". We should probably make the
method be named "traits.setCurrent(message, value)", but that would be a
different branch ("make the traits class understandable"), because
setFlags was not introduced with this branch.

Anyway, I agree we should make code more understandable and I am sure
Jeroen and you can make a good job out of it :)

> + def test_makeCounterpartPOFile(self):
> > + # self.factory.makePOFile makes POFiles on the upstream
side by
> > + # default. self._makeCounterpartPOFile makes POFiles on
the
> > + # Ubuntu side.
> > + pofile = self.factory.makePOFile('es')
> > + other_pofile = self._makeCounterpartPOFile(pofile)
> > +
> > + self.assertEqual(pofile.language, other_pofile.language)
> > + self.assertNotEqual(
> > + pofile.potemplate.translation_side,
> > + other_pofile.potemplate.translation_side)
>
> Wow... a test for your factory method.

That should be a norm these days (perhaps worth raising in the reviewers
meeting if not everybody is aware of it :). Though, that's for "real"
factory methods with tests in lib/lp/testing/tests/test_factory.py

[Jeroen T. Vermeulen (jtv)]

Hi Michael,

As I mentioned on IRC when you started on this, it's a good thing to get an outside view from time to time. Like Soyuz, Translations suffers from a lot of "inside knowledge" that one needs before certain things become clear. We'll try to come up with a clearer naming pattern for the TranslationSideTraits methods that wrap the TranslationMessage "is_current" flags.

I greatly simplified approveAsDiverged, leaving a sequence of top-level "if"s and two early return statements as the only signs of structural complexity.

With that, on to your individual notes:

> > === modified file 'lib/lp/testing/factory.py'
> > --- lib/lp/testing/factory.py 2010-09-01 13:47:24 +0000
> > +++ lib/lp/testing/factory.py 2010-09-02 12:37:46 +0000
> > @@ -2300,18 +2298,16 @@
> > """Create a diverged, current `TranslationMessage`."""
> > if pofile is None:
> > pofile = self.makePOFile('lt')
> > + if reviewer is None:
> > + reviewer = self.makePerson()
> >
> > # This creates a suggestion, then diverges it, then activates it.
> > # Once we have a method for diverging messages, do this in a more
> > # proper way.
>
> With your changes below, is it time to update the above comment?

Yes, thank you for spotting that. This is exactly the change that that comment foreshadowed.

> > === modified file 'lib/lp/translations/model/potmsgset.py'
> > --- lib/lp/translations/model/potmsgset.py 2010-09-01 15:11:18 +0000
> > +++ lib/lp/translations/model/potmsgset.py 2010-09-02 12:37:46 +0000
> > @@ -1145,6 +1145,61 @@
> > template.awardKarma(translator,
> 'translationsuggestionapproved')
> > template.awardKarma(reviewer, 'translationreview')
> >
> > + def approveAsDiverged(self, pofile, suggestion, reviewer,
> > + lock_timestamp=None):
> > + """Approve a suggestion to become a diverged translation."""
> > + template = pofile.potemplate
> > + traits = getUtility(ITranslationSideTraitsSet).getTraits(
> > + template.translation_side)
> > + if suggestion.potemplate == template and
> traits.getFlag(suggestion):
> > + # The suggestion is already current and diverged for the
> > + # right template.
> > + return suggestion
>
> Could this be extracted (so that this method doesn't deal with traits and
> flags - which I have no idea about), into
> suggestion.isCurrentAndDiverged(pofile.potemplate) or similar? Eg.

I was unable to extract the traits and the flags, because they are at the very core of what this message does. But you could say I managed to simplify everything else so that the intent of the traits-related code stands out more.

Also, I introduced some explanatory variables. This is a say-it-in-code pattern: give an expression or statement a name, not because it's long or complex or highly reusable, but because it allows the code that uses it to be phrased in terms of intent rather than implementationn.

As a relevant example, we used to say "foo.potemplate is None" instead of "foo.is_diverged" and that worked relatively well, but the introduction of the i...

[Michael Nelson (michael.nelson)]

On Wed, Sep 8, 2010 at 5:16 AM, Jeroen T. Vermeulen <email address hidden> wrote:
> Hi Michael,
>
> As I mentioned on IRC when you started on this, it's a good thing to get an outside view from time to time.  Like Soyuz, Translations suffers from a lot of "inside knowledge" that one needs before certain things become clear.  We'll try to come up with a clearer naming pattern for the TranslationSideTraits methods that wrap the TranslationMessage "is_current" flags.
>
> I greatly simplified approveAsDiverged, leaving a sequence of top-level "if"s and two early return statements as the only signs of structural complexity.

Great! Thanks Jeroen. In summary, it's much easier for me to
understand now. Approved, with a few comments that you can
take-or-leave below.

Cheers,
Michael

>
> With that, on to your individual notes:
>
[snip]
>
>> > === modified file 'lib/lp/translations/model/potmsgset.py'
>> > --- lib/lp/translations/model/potmsgset.py    2010-09-01 15:11:18 +0000
>> > +++ lib/lp/translations/model/potmsgset.py    2010-09-02 12:37:46 +0000
>> > @@ -1145,6 +1145,61 @@
>> >              template.awardKarma(translator,
>> 'translationsuggestionapproved')
>> >              template.awardKarma(reviewer, 'translationreview')
>> >
>> > +    def approveAsDiverged(self, pofile, suggestion, reviewer,
>> > +                          lock_timestamp=None):
>> > +        """Approve a suggestion to become a diverged translation."""
>> > +        template = pofile.potemplate
>> > +        traits = getUtility(ITranslationSideTraitsSet).getTraits(
>> > +            template.translation_side)
>> > +        if suggestion.potemplate == template and
>> traits.getFlag(suggestion):
>> > +            # The suggestion is already current and diverged for the
>> > +            # right template.
>> > +            return suggestion
>>
>> Could this be extracted (so that this method doesn't deal with traits and
>> flags - which I have no idea about), into
>> suggestion.isCurrentAndDiverged(pofile.potemplate) or similar? Eg.
>
> I was unable to extract the traits and the flags, because they are at the very core of what this message does.  But you could say I managed to simplify everything else so that the intent of the traits-related code stands out more.
>
> Also, I introduced some explanatory variables.  This is a say-it-in-code pattern: give an expression or statement a name, not because it's long or complex or highly reusable, but because it allows the code that uses it to be phrased in terms of intent rather than implementationn.

Yes, that makes a huge difference! Small point: you could move the
definition of used_on_other_side down to where it is first checked.

>
> As a relevant example, we used to say "foo.potemplate is None" instead of "foo.is_diverged" and that worked relatively well, but the introduction of the is_diverged attribute made a lot of code much clearer despite hiding the relationship between divergence and potemplate.
>
>
>> > +
>> > +        incumbent = traits.getCurrentMessage(self, template,
>> pofile.language)
>> > +        if incumbent is not None:
>> > +            self._checkForConflict(incumbent, lock_timestamp)
>> > +            if incumben...

[Michael Nelson (michael.nelson)]

review: Approve (code)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-09-06 10:40:54 +0000
+++ lib/lp/testing/factory.py	2010-09-08 02:43:41 +0000
@@ -250,7 +250,6 @@
     time_counter,
     )
 from lp.translations.interfaces.potemplate import IPOTemplateSet
-from lp.translations.interfaces.side import ITranslationSideTraitsSet
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat,
     )
@@ -2393,18 +2392,13 @@
         """Create a diverged, current `TranslationMessage`."""
         if pofile is None:
             pofile = self.makePOFile('lt')
+        if reviewer is None:
+            reviewer = self.makePerson()
 
-        # This creates a suggestion, then diverges it, then activates it.
-        # Once we have a method for diverging messages, do this in a more
-        # proper way.
-        message = self.makeSharedTranslationMessage(
+        message = self.makeSuggestion(
             pofile=pofile, potmsgset=potmsgset, translator=translator,
-            reviewer=reviewer, translations=translations, suggestion=True)
-        traits = getUtility(ITranslationSideTraitsSet).getTraits(
-            pofile.potemplate.translation_side)
-        removeSecurityProxy(message).potemplate = pofile.potemplate
-        traits.setFlag(message, True)
-        return message
+            translations=translations)
+        return message.approveAsDiverged(pofile, reviewer)
 
     def makeTranslation(self, pofile, sequence,
                         english=None, translated=None,

=== modified file 'lib/lp/testing/tests/test_factory.py'
--- lib/lp/testing/tests/test_factory.py	2010-09-06 10:40:54 +0000
+++ lib/lp/testing/tests/test_factory.py	2010-09-08 02:43:41 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 
 from datetime import datetime
-import unittest
 
 import pytz
 from zope.component import getUtility

=== modified file 'lib/lp/translations/interfaces/translationmessage.py'
--- lib/lp/translations/interfaces/translationmessage.py	2010-09-06 10:40:54 +0000
+++ lib/lp/translations/interfaces/translationmessage.py	2010-09-08 02:43:41 +0000
@@ -265,6 +265,9 @@
                 lock_timestamp=None):
         """Approve this suggestion, making it a current translation."""
 
+    def approveAsDiverged(pofile, reviewer, lock_timestamp=None):
+        """Approve this suggestion, as a diverged translation."""
+
     # XXX CarlosPerelloMarin 20071022: We should move this into browser code.
     def makeHTMLID(description):
         """Unique identifier for self, suitable for use in HTML element ids.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-09-06 10:40:54 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-09-08 02:43:41 +0000
@@ -1144,6 +1144,70 @@
             template.awardKarma(translator, 'translationsuggestionapproved')
             template.awardKarma(reviewer, 'translationreview')
 
+    def _cloneAndDiverge(self, original_message, pofile):
+        """Create a diverged clone of a `TranslationMessage`.
+
+        The message is not made current; the caller must do so in order
+        to keep the message in a consistent state.
+        """
+        potranslations = self._findPOTranslations(
+            dict(enumerate(original_message.translations)))
+        message = self._makeTranslationMessage(
+            pofile, original_message.submitter, potranslations,
+            original_message.origin, diverged=True)
+        return message
+
+    def approveAsDiverged(self, pofile, suggestion, reviewer,
+                          lock_timestamp=None):
+        """Approve a suggestion to become a diverged translation."""
+        template = pofile.potemplate
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(
+            template.translation_side)
+
+        diverged = suggestion.is_diverged
+        used_here = traits.getFlag(suggestion)
+        used_on_other_side = traits.other_side_traits.getFlag(suggestion)
+
+        if used_here and suggestion.potemplate == template:
+            # The suggestion is already current and diverged for the
+            # right template.
+            return suggestion
+
+        incumbent = traits.getCurrentMessage(self, template, pofile.language)
+        if incumbent is not None:
+            # Ensure that the current message hasn't changed from the
+            # state the reviewer inspected before making this change.
+            self._checkForConflict(incumbent, lock_timestamp)
+
+        if incumbent is not None and incumbent.is_diverged:
+            # The incumbent is also diverged, so it's in the way of the
+            # suggestion we're trying to diverge.  Disable it.
+            traits.setFlag(incumbent, False)
+            incumbent.markReviewed(reviewer)
+            incumbent.shareIfPossible()
+            pofile.markChanged()
+
+        if used_here and not diverged and not used_on_other_side:
+            # This message is already the shared current message.  If it
+            # was previously masked by a diverged message, it no longer
+            # is.  This is probably the behaviour the user would expect.
+            return suggestion
+
+        if used_here or used_on_other_side:
+            # The suggestion is already current somewhere else.  Can't
+            # reuse it as a diverged message in this template, so clone
+            # it.
+            message = self._cloneAndDiverge(suggestion, pofile)
+        else:
+            # No obstacles.  Diverge.
+            message = suggestion
+            message.potemplate = template
+
+        traits.setFlag(message, True)
+        message.markReviewed(reviewer)
+        pofile.markChanged()
+        return message
+
     def setCurrentTranslation(self, pofile, submitter, translations, origin,
                               share_with_other_side=False,
                               lock_timestamp=None):

=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py	2010-09-06 10:40:54 +0000
+++ lib/lp/translations/model/translationmessage.py	2010-09-08 02:43:41 +0000
@@ -175,6 +175,10 @@
         """See `ITranslationMessage`."""
         raise NotImplementedError()
 
+    def approveAsDiverged(self, *args, **kwargs):
+        """See `ITranslationMessage`."""
+        raise NotImplementedError()
+
     def getOnePOFile(self):
         """See `ITranslationMessage`."""
         return None
@@ -352,6 +356,11 @@
             share_with_other_side=share_with_other_side,
             lock_timestamp=lock_timestamp)
 
+    def approveAsDiverged(self, pofile, reviewer, lock_timestamp=None):
+        """See `ITranslationMessage`."""
+        return self.potmsgset.approveAsDiverged(
+            pofile, self, reviewer, lock_timestamp=lock_timestamp)
+
     def getOnePOFile(self):
         """See `ITranslationMessage`."""
         from lp.translations.model.pofile import POFile

=== modified file 'lib/lp/translations/tests/test_translationmessage.py'
--- lib/lp/translations/tests/test_translationmessage.py	2010-08-24 11:39:06 +0000
+++ lib/lp/translations/tests/test_translationmessage.py	2010-09-08 02:43:41 +0000
@@ -269,6 +269,213 @@
             pofile, self.factory.makePerson(), lock_timestamp=old)
 
 
+class TestApproveAsDiverged(TestCaseWithFactory):
+    """Tests for `TranslationMessage.approveAsDiverged`."""
+
+    layer = ZopelessDatabaseLayer
+
+    def _makeCounterpartPOFile(self, pofile):
+        """Make a `POFile` on the opposite translation side.
+
+        :param pofile: A `POFile` to match.  Assumed to be on the
+            "upstream" translation side.
+        """
+        assert pofile.potemplate.productseries is not None, (
+            "This test needs a product template; got a package template.""")
+        other_template = self.factory.makePOTemplate(
+            distroseries=self.factory.makeDistroSeries(),
+            sourcepackagename=self.factory.makeSourcePackageName())
+
+        return self.factory.makePOFile(
+            pofile.language.code, potemplate=other_template)
+
+    def test_makeCounterpartPOFile(self):
+        # self.factory.makePOFile makes POFiles on the upstream side by
+        # default.  self._makeCounterpartPOFile makes POFiles on the
+        # Ubuntu side.
+        pofile = self.factory.makePOFile('es')
+        other_pofile = self._makeCounterpartPOFile(pofile)
+
+        self.assertEqual(pofile.language, other_pofile.language)
+        self.assertNotEqual(
+            pofile.potemplate.translation_side,
+            other_pofile.potemplate.translation_side)
+
+    def test_no_change(self):
+        # Approving and diverging a message that's already active and
+        # diverged for this POFile does nothing.  Even the reviewer
+        # stays unchanged.
+        translator = self.factory.makePerson()
+        original_reviewer = self.factory.makePerson()
+        later_reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('es_AR')
+        message = self.factory.makeDivergedTranslationMessage(
+            pofile=pofile, translator=translator, reviewer=original_reviewer)
+
+        resulting_message = message.approveAsDiverged(pofile, later_reviewer)
+
+        self.assertEqual(message, resulting_message)
+        self.assertEqual(translator, message.submitter)
+        self.assertEqual(original_reviewer, message.reviewer)
+        self.assertEqual(pofile.potemplate, message.potemplate)
+
+    def test_activates_and_diverges(self):
+        # Approving a simple suggestion activates and diverges it.
+        pofile = self.factory.makePOFile('es_BO')
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+
+        resulting_message = suggestion.approveAsDiverged(
+            pofile, self.factory.makePerson())
+
+        self.assertEqual(suggestion, resulting_message)
+        self.assertTrue(suggestion.is_current_upstream)
+        self.assertTrue(suggestion.is_diverged)
+        self.assertEqual(pofile.potemplate, suggestion.potemplate)
+
+    def test_activating_reviews(self):
+        # Activating a message marks it as reviewed.
+        pofile = self.factory.makePOFile('es_BO')
+        reviewer = self.factory.makePerson()
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+
+        resulting_message = suggestion.approveAsDiverged(pofile, reviewer)
+
+        self.assertEqual(reviewer, resulting_message.reviewer)
+
+    def test_diverge_current_shared_leaves_message_intact(self):
+        # Calling approveAsDiverged on the current shared translation
+        # leaves it untouched.
+        original_reviewer = self.factory.makePerson()
+        later_reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('es_CL')
+        message = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, reviewer=original_reviewer)
+
+        resulting_message = message.approveAsDiverged(pofile, later_reviewer)
+
+        self.assertEqual(message, resulting_message)
+        self.assertEqual(original_reviewer, message.reviewer)
+        self.assertFalse(message.is_diverged)
+
+    def test_diverge_current_shared_message_unmasks_it(self):
+        # Calling approveAsDiverged on the current shared translation
+        # deactivates any diverged message that may be masking it.
+        pofile = self.factory.makePOFile('es_CO')
+        reviewer = self.factory.makePerson()
+        shared = self.factory.makeCurrentTranslationMessage(pofile=pofile)
+        diverged = self.factory.makeDivergedTranslationMessage(
+            pofile=pofile, potmsgset=shared.potmsgset)
+
+        self.assertTrue(shared.is_current_upstream)
+        self.assertTrue(diverged.is_current_upstream)
+        self.assertFalse(shared.is_diverged)
+        self.assertTrue(diverged.is_diverged)
+
+        shared.approveAsDiverged(pofile, reviewer)
+
+        self.assertTrue(shared.is_current_upstream)
+        self.assertFalse(diverged.is_current_upstream)
+        self.assertFalse(shared.is_diverged)
+
+    def test_does_not_affect_other_side(self):
+        # Approving a message that's current on the other side clones
+        # it, so that the other side remains unaffected by this local
+        # change.
+        reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('es_CR')
+        other_pofile = self._makeCounterpartPOFile(pofile)
+        suggestion = self.factory.makeCurrentTranslationMessage(
+            pofile=other_pofile)
+        self.assertEqual(
+            (False, True),
+            (suggestion.is_current_upstream, suggestion.is_current_ubuntu))
+
+        resulting_message = suggestion.approveAsDiverged(pofile, reviewer)
+
+        self.assertNotEqual(suggestion, resulting_message)
+        self.assertEqual(pofile.potemplate, resulting_message.potemplate)
+        self.assertFalse(suggestion.is_diverged)
+        self.assertTrue(resulting_message.is_current_upstream)
+        self.assertEqual(
+            (False, True),
+            (suggestion.is_current_upstream, suggestion.is_current_ubuntu))
+
+    def test_does_not_affect_diverged_elsewhere(self):
+        # Approving a message that's current and diverged to another
+        # template clones it, so that the other template remains
+        # unaffected by this local change.
+        reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('es_DO')
+        elsewhere = self.factory.makePOFile('es_DO')
+
+        suggestion = self.factory.makeDivergedTranslationMessage(
+            pofile=elsewhere)
+
+        resulting_message = suggestion.approveAsDiverged(pofile, reviewer)
+
+        self.assertNotEqual(suggestion, resulting_message)
+        self.assertEqual(pofile.potemplate, resulting_message.potemplate)
+        self.assertEqual(elsewhere.potemplate, suggestion.potemplate)
+        self.assertTrue(resulting_message.is_current_upstream)
+        self.assertTrue(suggestion.is_current_upstream)
+
+    def test_detects_conflict(self):
+        # Trying to approve and diverge a message based on outdated
+        # information raises TranslationConflict.
+        reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('es_EC')
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+        current = self.factory.makeDivergedTranslationMessage(
+            pofile=pofile, potmsgset=suggestion.potmsgset)
+        earlier = current.date_reviewed - timedelta(1)
+
+        self.assertRaises(
+            TranslationConflict,
+            suggestion.approveAsDiverged,
+            pofile, reviewer, lock_timestamp=earlier)
+
+    def test_passes_conflict_check_if_no_conflict(self):
+        # Trying to approve and diverge a message based on up-to-date
+        # works without raising a conflict.
+        reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('es_GT')
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+        current = self.factory.makeDivergedTranslationMessage(
+            pofile=pofile, potmsgset=suggestion.potmsgset)
+        later = current.date_reviewed + timedelta(1)
+
+        suggestion.approveAsDiverged(pofile, reviewer, lock_timestamp=later)
+
+        self.assertTrue(suggestion.is_current_upstream)
+
+    def test_passes_conflict_check_if_same_translations(self):
+        # Trying to approve and diverge a message based on outdated
+        # information works just fine if in the meantime the suggestion
+        # has become the diverged current translation.
+        reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('es_HN')
+        current = self.factory.makeDivergedTranslationMessage(pofile=pofile)
+        earlier = current.date_reviewed - timedelta(1)
+
+        current.approveAsDiverged(pofile, reviewer, lock_timestamp=earlier)
+
+        self.assertTrue(current.is_current_upstream)
+
+    def test_updates_pofile(self):
+        # Approving a message as a diverged translation marks the POFile
+        # as updated.
+        reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('es_MX')
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+        earlier = pofile.date_changed - timedelta(1)
+        pofile.markChanged(timestamp=earlier)
+
+        self.assertEqual(earlier, pofile.date_changed)
+        suggestion.approveAsDiverged(pofile, reviewer)
+        self.assertNotEqual(earlier, pofile.date_changed)
+        self.assertEqual(suggestion.date_reviewed, pofile.date_changed)
+
+
 class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
     """Tests for `TranslationMessage.findIdenticalMessage`."""