← Back to team overview

launchpad-reviewers team mailing list archive

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

 

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/recife-approveSuggestion into lp:~launchpad/launchpad/recife.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code


= 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
-- 
https://code.launchpad.net/~jtv/launchpad/recife-approveSuggestion/+merge/32848
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/recife-approveSuggestion into lp:~launchpad/launchpad/recife.
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-08-10 17:28:08 +0000
+++ lib/lp/testing/factory.py	2010-08-17 06:45:56 +0000
@@ -29,7 +29,6 @@
 from itertools import count
 from operator import isSequenceType
 import os
-import os.path
 from random import randint
 from StringIO import StringIO
 from textwrap import dedent
@@ -171,6 +170,8 @@
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat)
 from lp.translations.interfaces.translationgroup import ITranslationGroupSet
+from lp.translations.interfaces.translationmessage import (
+    RosettaTranslationOrigin)
 from lp.translations.interfaces.translationsperson import ITranslationsPerson
 from lp.translations.interfaces.translationtemplatesbuildjob import (
     ITranslationTemplatesBuildJobSource)
@@ -2105,6 +2106,60 @@
             force_shared=True)
         return translation_message
 
+    def makeCurrentTranslationMessage(self, pofile=None, potmsgset=None,
+                                      translator=None, reviewer=None,
+                                      translations=None, diverged=False,
+                                      current_other=False):
+        """Create a `TranslationMessage` and make it current.
+
+        This is similar to `makeTranslationMessage`, except:
+         * It doesn't create suggestions.
+         * It doesn't rely on the obsolescent updateTranslation.
+         * It activates the message on the correct translation side.
+
+        By default the message will only be current on the side (Ubuntu
+        or upstream) that `pofile` is on.
+
+        Be careful: if the message is already translated, calling this
+        method may violate database unique constraints.
+
+        :param pofile: `POFile` to put translation in; if omitted, one
+            will be created.
+        :param potmsgset: `POTMsgSet` to translate; if omitted, one will
+            be created (with sequence number 1).
+        :param translator: `Person` who created the translation.  If
+            omitted, one will be created.
+        :param reviewer: `Person` who reviewed the translation.  If
+            omitted, one will be created.
+        :param translations: List of strings to translate the `POTMsgSet`
+            to.  If omitted, will translate to a single random string.
+        :param diverged: Create a diverged message?
+        :param current_other: Should the message also be current on the
+            other translation side?  (Cannot be combined with `diverged`).
+        """
+        assert not (diverged and current_other), (
+            "A diverged message can't be current on the other side.")
+        if pofile is None:
+            pofile = self.makePOFile('nl')
+        if potmsgset is None:
+            potmsgset = self.makePOTMsgSet(pofile.potemplate, sequence=1)
+        if translator is None:
+            translator = self.makePerson()
+        if reviewer is None:
+            reviewer = self.makePerson()
+        if translations is None:
+            translations = [self.getUniqueString()]
+
+        message = potmsgset.setCurrentTranslation(
+            pofile, translator, dict(enumerate(translations)),
+            RosettaTranslationOrigin.ROSETTAWEB,
+            share_with_other_side=current_other)
+
+        if diverged:
+            removeSecurityProxy(message).potemplate = pofile.potemplate
+
+        return message
+
     def makeTranslation(self, pofile, sequence,
                         english=None, translated=None,
                         is_current_upstream=False):

=== modified file 'lib/lp/testing/tests/test_factory.py'
--- lib/lp/testing/tests/test_factory.py	2010-08-10 17:28:08 +0000
+++ lib/lp/testing/tests/test_factory.py	2010-08-17 06:45:56 +0000
@@ -7,7 +7,6 @@
 
 from datetime import datetime
 import pytz
-import unittest
 
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -210,6 +209,55 @@
         ssp = self.factory.makeSuiteSourcePackage()
         self.assertThat(ssp, ProvidesAndIsProxied(ISuiteSourcePackage))
 
+    def test_makeCurrentTranslationMessage_makes_shared_message(self):
+        tm = self.factory.makeCurrentTranslationMessage()
+        self.assertFalse(tm.is_diverged)
+
+    def test_makeCurrentTranslationMessage_makes_diverged_message(self):
+        tm = self.factory.makeCurrentTranslationMessage(diverged=True)
+        self.assertTrue(tm.is_diverged)
+
+    def test_makeCurrentTranslationMessage_makes_current_upstream(self):
+        pofile = self.factory.makePOFile(
+            'ka', potemplate=self.factory.makePOTemplate(
+                productseries=self.factory.makeProductSeries()))
+
+        tm = self.factory.makeCurrentTranslationMessage(pofile=pofile)
+
+        self.assertTrue(tm.is_current_upstream)
+        self.assertFalse(tm.is_current_ubuntu)
+
+    def test_makeCurrentTranslationMessage_makes_current_ubuntu(self):
+        package = self.factory.makeSourcePackage()
+        pofile = self.factory.makePOFile(
+            'kk', self.factory.makePOTemplate(
+                sourcepackagename=package.sourcepackagename,
+                distroseries=package.distroseries))
+
+        tm = self.factory.makeCurrentTranslationMessage(pofile=pofile)
+
+        self.assertFalse(tm.is_current_upstream)
+        self.assertTrue(tm.is_current_ubuntu)
+
+    def test_makeCurrentTranslationMessage_makes_current_tracking(self):
+        tm = self.factory.makeCurrentTranslationMessage(current_other=True)
+
+        self.assertTrue(tm.is_current_upstream)
+        self.assertTrue(tm.is_current_ubuntu)
+
+    def test_makeCurrentTranslationMessage_uses_given_translation(self):
+        translations = [
+            self.factory.getUniqueString(),
+            self.factory.getUniqueString(),
+            ]
+
+        tm = self.factory.makeCurrentTranslationMessage(
+            translations=translations)
+
+        self.assertEqual(
+            translations, [tm.msgstr0.translation, tm.msgstr1.translation])
+        self.assertIs(None, tm.msgstr2)
+
 
 class TestFactoryWithLibrarian(TestCaseWithFactory):
 
@@ -318,7 +366,3 @@
         unproxied_person = removeSecurityProxy(self.factory.makePerson())
         self.assertFalse(
             is_security_proxied_or_harmless([1, '2', unproxied_person]))
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py	2010-08-10 17:28:08 +0000
+++ lib/lp/translations/interfaces/potemplate.py	2010-08-17 06:45:56 +0000
@@ -494,6 +494,9 @@
     def getTranslationRows():
         """Return the `IVPOTexport` objects for this template."""
 
+    def awardKarma(person, action_name):
+        """Award karma for a translation action on this template."""
+
 
 class IPOTemplateSubset(Interface):
     """A subset of POTemplate."""

=== modified file 'lib/lp/translations/interfaces/translationmessage.py'
--- lib/lp/translations/interfaces/translationmessage.py	2010-08-10 14:39:46 +0000
+++ lib/lp/translations/interfaces/translationmessage.py	2010-08-17 06:45:56 +0000
@@ -240,6 +240,9 @@
         It must not be referenced by any other object.
         """
 
+    def approve(pofile, reviewer, share_with_other_side=False):
+        """Approve this suggestion, making it a current 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.
@@ -252,6 +255,16 @@
             HTML element id.
         """
 
+    def markReviewed(reviewer, timestamp=None):
+        """Mark this message as reviewed.
+
+        Sets the reviewer and review date for this message.
+
+        :param reviewer: the person who reviewed the message.
+        :param timestamp: optional timestamp indicating when the review
+            happened.  Defaults to "now."
+        """
+
     def makeCurrentUbuntu(new_value=True):
         """Set the `is_current_ubuntu` flag.
 

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2010-08-10 17:28:08 +0000
+++ lib/lp/translations/model/potemplate.py	2010-08-17 06:45:56 +0000
@@ -970,6 +970,12 @@
         else:
             return TranslationSide.UBUNTU
 
+    def awardKarma(self, person, action_name):
+        """See `IPOTemplate`."""
+        person.assignKarma(
+            action_name, product=self.product, distribution=self.distribution,
+            sourcepackagename=self.sourcepackagename)
+
 
 class POTemplateSubset:
     implements(IPOTemplateSubset)
@@ -1288,8 +1294,7 @@
             preferred_matches = [
                 match
                 for match in matches
-                if match.from_sourcepackagename == sourcepackagename
-            ]
+                if match.from_sourcepackagename == sourcepackagename]
 
             if len(preferred_matches) == 1:
                 return preferred_matches[0]
@@ -1448,6 +1453,7 @@
         name.
         :return: A ResultSet for the query.
         """
+        # Avoid circular imports.
         from lp.registry.model.productseries import ProductSeries
 
         ProductSeries1 = ClassAlias(ProductSeries)
@@ -1461,16 +1467,14 @@
             And(
                 Packaging.distroseriesID == POTemplate.distroseriesID,
                 Packaging.sourcepackagenameID == (
-                        POTemplate.sourcepackagenameID))
-            )
+                        POTemplate.sourcepackagenameID)))
         return Store.of(self.product).using(origin).find(
             POTemplate,
             And(
                 Or(
                     ProductSeries.productID == self.product.id,
                     ProductSeries1.productID == self.product.id),
-                templatename_clause
-                ))
+                templatename_clause))
 
     def _queryBySourcepackagename(self, templatename_clause):
         """Build the query that finds POTemplates by their names.
@@ -1617,8 +1621,7 @@
                 msgset.flags = set([
                     flag.strip()
                     for flag in row.flags_comment.split(',')
-                    if flag
-                    ])
+                    if flag])
 
             # Store the message.
             messages.append(msgset)

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-08-13 05:02:57 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-08-17 06:45:56 +0000
@@ -632,8 +632,7 @@
         # plural forms.
         order.extend([
             'msgstr%s NULLS FIRST' % quote(form)
-            for form in remaining_plural_forms
-            ])
+            for form in remaining_plural_forms])
         matches = list(
             TranslationMessage.select(' AND '.join(clauses), orderBy=order))
 
@@ -722,8 +721,7 @@
                         distribution=pofile.potemplate.distribution,
                         sourcepackagename=pofile.potemplate.sourcepackagename)
 
-                new_message.reviewer = submitter
-                new_message.date_reviewed = UTC_NOW
+                new_message.markReviewed(submitter, UTC_NOW)
                 pofile.date_changed = UTC_NOW
                 pofile.lasttranslator = submitter
 
@@ -986,18 +984,12 @@
             ('msgstr%d' % form, potranslation)
             for form, potranslation in potranslations.iteritems())
 
-        message = TranslationMessage(
+        pofile.potemplate.awardKarma(submitter, 'translationsuggestionadded')
+
+        return TranslationMessage(
             potmsgset=self, language=pofile.language,
-            origin=RosettaTranslationOrigin.ROSETTAWEB,
-            submitter=submitter, **forms)
-
-        template = pofile.potemplate
-        submitter.assignKarma(
-            'translationsuggestionadded', product=template.product,
-            distribution=template.distribution,
-            sourcepackagename=template.sourcepackagename)
-
-        return message
+            origin=RosettaTranslationOrigin.ROSETTAWEB, submitter=submitter,
+            **forms)
 
     def _maybeRaiseTranslationConflict(self, message, lock_timestamp):
         """Checks if there is a translation conflict for the message.
@@ -1029,8 +1021,7 @@
         else:
             # Check for translation conflicts and update review fields.
             self._maybeRaiseTranslationConflict(current, lock_timestamp)
-            current.reviewer = reviewer
-            current.date_reviewed = lock_timestamp
+            current.markReviewed(reviewer, lock_timestamp)
 
     def _nameMessageStatus(self, message, translation_side_traits):
         """Figure out the decision-matrix status of a message.
@@ -1075,24 +1066,73 @@
             validation_status=TranslationValidationStatus.OK,
             **translation_args)
 
+    def approveSuggestion(self, pofile, suggestion, reviewer,
+                          share_with_other_side=False):
+        """Approve a suggestion.
+
+        :param pofile: The `POFile` that the suggestion is being approved for.
+        :param suggestion: The `TranslationMessage` being approved.
+        :param reviewer: The `Person` responsible for approving the
+            suggestion.
+        :param share_with_other_side: Policy selector: share this change with
+            the other translation side if possible?
+        """
+        template = pofile.potemplate
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(
+            template.translation_side)
+        if traits.getFlag(suggestion):
+            # Message is already current.
+            return
+
+        translator = suggestion.submitter
+        potranslations = suggestion.all_msgstrs
+        activated_message = self._setTranslation(
+            pofile, translator, suggestion.origin, potranslations,
+            share_with_other_side=share_with_other_side,
+            identical_message=suggestion)
+
+        activated_message.markReviewed(reviewer)
+        if reviewer != translator:
+            template.awardKarma(translator, 'translationsuggestionapproved')
+            template.awardKarma(reviewer, 'translationreview')
+
     def setCurrentTranslation(self, pofile, submitter, translations, origin,
                               share_with_other_side=False):
         """See `IPOTMsgSet`."""
+        potranslations = self._findPOTranslations(translations)
+        identical_message = self._findTranslationMessage(
+            pofile, potranslations, prefer_shared=False)
+        return self._setTranslation(
+            pofile, submitter, origin, potranslations,
+            share_with_other_side=share_with_other_side,
+            identical_message=identical_message)
+
+    def _setTranslation(self, pofile, submitter, origin, potranslations,
+                        identical_message=None, share_with_other_side=False):
+        """Set the current translation.
+
+        :param pofile: The `POFile` to set the translation in.
+        :param submitter: The `Person` who produced this translation.
+        :param origin: The translation's `RosettaTranslationOrigin`.
+        :param potranslations: List of `POTranslation`s, with a None for
+            each untranslated plural-form.
+        :param identical_message: The already existing message, if any,
+            that's either shared or diverged for `pofile.potemplate`,
+            whose translations are identical to the ones we're setting.
+        :param share_with_other_side:
+        :return:
+        """
+        # XXX JeroenVermeulen 2010-08-16: Update pofile.date_changed.
+
+        twin = identical_message
+
         translation_side = pofile.potemplate.translation_side
         helper = make_message_side_helpers(
             translation_side, self, pofile.potemplate, pofile.language)
 
-        translations = self._findPOTranslations(translations)
-
         # The current message on this translation side, if any.
         incumbent_message = helper.incumbent_message
 
-        # An already existing message, if any, that's either shared, or
-        # diverged for the template/pofile we're working on, whose
-        # translations are identical to the ones we're setting.
-        twin = self._findTranslationMessage(
-            pofile, translations, prefer_shared=False)
-
         # Summary of the matrix:
         #  * If the incumbent message is diverged and we're setting a
         #    translation that's already shared: converge.
@@ -1158,11 +1198,11 @@
             elif character == '1':
                 # Create & activate.
                 message = self._makeTranslationMessage(
-                    pofile, submitter, translations, origin)
+                    pofile, submitter, potranslations, origin)
             elif character == '2':
                 # Create, diverge, activate.
                 message = self._makeTranslationMessage(
-                    pofile, submitter, translations, origin, diverged=True)
+                    pofile, submitter, potranslations, origin, diverged=True)
             elif character == '4':
                 # Activate.
                 message = twin
@@ -1185,7 +1225,7 @@
                     # just reuse it for our diverged message.  Create a
                     # new one.
                     message = self._makeTranslationMessage(
-                        pofile, submitter, translations, origin,
+                        pofile, submitter, potranslations, origin,
                         diverged=True)
             elif character == '7':
                 # Converge & activate.
@@ -1268,8 +1308,7 @@
                         pofile, submitter, {}, origin, diverged=True)
 
                 Store.of(mask).add_flush_order(current, mask)
-                mask.reviewer = submitter
-                mask.date_reviewed = UTC_NOW
+                mask.markReviewed(submitter, UTC_NOW)
                 traits.setFlag(mask, True)
         else:
             traits.setFlag(current, False)

=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py	2010-08-13 05:02:57 +0000
+++ lib/lp/translations/model/translationmessage.py	2010-08-17 06:45:56 +0000
@@ -17,6 +17,7 @@
 from storm.expr import And
 from storm.locals import SQL
 from storm.store import Store
+
 from zope.interface import implements
 
 from canonical.cachedproperty import cachedproperty
@@ -31,6 +32,9 @@
 from lp.registry.interfaces.person import validate_public_person
 
 
+UTC = pytz.timezone('UTC')
+
+
 def make_plurals_fragment(fragment, separator):
     """Repeat text fragment for each plural form, separated by separator.
 
@@ -94,6 +98,14 @@
         """See `ITranslationMessage`."""
         self.browser_pofile = pofile
 
+    def markReviewed(self, reviewer, timestamp=None):
+        """See `ITranslationMessage`."""
+        if timestamp is None:
+            timestamp = datetime.now(UTC)
+
+        self.reviewer = reviewer
+        self.date_reviewed = timestamp
+
 
 class DummyTranslationMessage(TranslationMessageMixIn):
     """Represents an `ITranslationMessage` where we don't yet HAVE it.
@@ -119,7 +131,6 @@
         self.language = pofile.language
         self.variant = None
         self.potmsgset = potmsgset
-        UTC = pytz.timezone('UTC')
         self.date_created = datetime.now(UTC)
         self.submitter = None
         self.date_reviewed = None
@@ -146,6 +157,10 @@
         """See `ITranslationMessage`."""
         return True
 
+    def approve(self, *args, **kwargs):
+        """See `ITranslationMessage`."""
+        raise NotImplementedError()
+
     def getOnePOFile(self):
         """See `ITranslationMessage`."""
         return None
@@ -311,6 +326,12 @@
             date_reviewed = current.date_created
         return date_reviewed > self.date_created
 
+    def approve(self, pofile, reviewer, share_with_other_side=False):
+        """See `ITranslationMessage`."""
+        self.potmsgset.approveSuggestion(
+            pofile, self, reviewer,
+            share_with_other_side=share_with_other_side)
+
     def getOnePOFile(self):
         """See `ITranslationMessage`."""
         from lp.translations.model.pofile import POFile

=== modified file 'lib/lp/translations/tests/test_potemplate.py'
--- lib/lp/translations/tests/test_potemplate.py	2010-08-10 14:39:46 +0000
+++ lib/lp/translations/tests/test_potemplate.py	2010-08-17 06:45:56 +0000
@@ -99,6 +99,21 @@
         self.assertContentEqual([gnome_credits, kde_credits],
                                 self.potemplate.getTranslationCredits())
 
+    def test_awardKarma(self):
+        person = self.factory.makePerson()
+        template = self.factory.makePOTemplate()
+        karma_listener = self.installKarmaRecorder(
+            person=person, product=template.product)
+        action = 'translationsuggestionadded'
+
+        # This is not something that browser code or scripts should do,
+        # so we go behind the proxy.
+        removeSecurityProxy(template).awardKarma(person, action)
+
+        karma_events = karma_listener.karma_events
+        self.assertEqual(1, len(karma_events))
+        self.assertEqual(action, karma_events[0].action.name)
+
 
 class EquivalenceClassTestMixin:
     """Helper for POTemplate equivalence class tests."""

=== modified file 'lib/lp/translations/tests/test_translationmessage.py'
--- lib/lp/translations/tests/test_translationmessage.py	2010-08-10 14:39:46 +0000
+++ lib/lp/translations/tests/test_translationmessage.py	2010-08-17 06:45:56 +0000
@@ -5,6 +5,9 @@
 
 __metaclass__ = type
 
+from datetime import datetime, timedelta
+from pytz import UTC
+
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -13,6 +16,7 @@
 from lp.testing import TestCaseWithFactory
 from lp.translations.model.potranslation import POTranslation
 from lp.translations.model.translationmessage import DummyTranslationMessage
+from lp.translations.interfaces.side import ITranslationSideTraitsSet
 from lp.translations.interfaces.translationmessage import (
     ITranslationMessage)
 from lp.translations.interfaces.translations import TranslationConstants
@@ -49,6 +53,181 @@
         message = self.factory.makeTranslationMessage(force_diverged=True)
         self.assertTrue(message.is_diverged)
 
+    def test_markReviewed(self):
+        message = self.factory.makeTranslationMessage()
+        reviewer = self.factory.makePerson()
+        tomorrow = datetime.now(UTC) + timedelta(days=1)
+
+        message.markReviewed(reviewer, tomorrow)
+
+        self.assertEqual(reviewer, message.reviewer)
+        self.assertEqual(tomorrow, message.date_reviewed)
+
+
+class TestApprove(TestCaseWithFactory):
+    layer = ZopelessDatabaseLayer
+
+    def test_approve_activates_message(self):
+        pofile = self.factory.makePOFile('br')
+        suggestion = self.factory.makeSharedTranslationMessage(
+            pofile=pofile, suggestion=True)
+        reviewer = self.factory.makePerson()
+        self.assertFalse(suggestion.is_current_upstream)
+        self.assertFalse(suggestion.is_current_ubuntu)
+
+        suggestion.approve(pofile, reviewer)
+
+        self.assertTrue(suggestion.is_current_upstream)
+        self.assertFalse(suggestion.is_current_ubuntu)
+
+    def test_approve_disables_incumbent_message(self):
+        pofile, potmsgset = self.factory.makePOFileAndPOTMsgSet('te')
+        suggestion = self.factory.makeSharedTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset, suggestion=True)
+        incumbent_message = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset)
+
+        self.assertTrue(incumbent_message.is_current_upstream)
+        self.assertFalse(suggestion.is_current_upstream)
+
+        suggestion.approve(pofile, self.factory.makePerson())
+
+        self.assertFalse(incumbent_message.is_current_upstream)
+        self.assertTrue(suggestion.is_current_upstream)
+
+    def test_approve_ignores_current_message(self):
+        pofile = self.factory.makePOFile('eu')
+        translation = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile)
+        submitter = translation.submitter
+        original_reviewer = translation.reviewer
+        new_reviewer = self.factory.makePerson()
+        self.assertTrue(translation.is_current_upstream)
+        self.assertFalse(translation.is_current_ubuntu)
+
+        translation.approve(pofile, new_reviewer)
+
+        # The message was already approved, so nothing changes.
+        self.assertTrue(translation.is_current_upstream)
+        self.assertFalse(translation.is_current_ubuntu)
+        self.assertEqual(submitter, translation.submitter)
+        self.assertEqual(original_reviewer, translation.reviewer)
+
+    def test_approve_can_converge(self):
+        pofile = self.factory.makePOFile('he')
+        translations = [self.factory.getUniqueString()]
+        reviewer = self.factory.makePerson()
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(
+            pofile.potemplate.translation_side)
+
+        diverged = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, diverged=True)
+        potmsgset = diverged.potmsgset
+        shared = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset, translations=translations)
+
+        shared.approve(pofile, reviewer)
+
+        self.assertFalse(diverged.is_current_upstream)
+        self.assertEqual(shared, traits.getCurrentMessage(
+            potmsgset, pofile.potemplate, pofile.language))
+
+    def test_approve_can_track_other_side(self):
+        upstream_pofile = self.factory.makePOFile('ar')
+        package = self.factory.makeSourcePackage()
+        ubuntu_template = self.factory.makePOTemplate(
+            sourcepackagename=package.sourcepackagename,
+            distroseries=package.distroseries)
+        ubuntu_pofile = self.factory.makePOFile(
+            'ar', potemplate=ubuntu_template)
+        other_side_message = self.factory.makeCurrentTranslationMessage(
+            pofile=upstream_pofile)
+        self.assertTrue(other_side_message.is_current_upstream)
+        self.assertFalse(other_side_message.is_current_ubuntu)
+
+        other_side_message.approve(ubuntu_pofile, self.factory.makePerson())
+
+        self.assertTrue(other_side_message.is_current_upstream)
+        self.assertTrue(other_side_message.is_current_ubuntu)
+
+    def test_approve_can_make_other_side_track(self):
+        pofile = self.factory.makePOFile('ki')
+        suggestion = self.factory.makeSharedTranslationMessage(
+            pofile=pofile, suggestion=True)
+        reviewer = self.factory.makePerson()
+
+        self.assertFalse(suggestion.is_current_upstream)
+        self.assertFalse(suggestion.is_current_ubuntu)
+
+        suggestion.approve(pofile, reviewer, share_with_other_side=True)
+
+        self.assertTrue(suggestion.is_current_upstream)
+        self.assertTrue(suggestion.is_current_ubuntu)
+
+    def test_approve_marks_reviewed(self):
+        pofile = self.factory.makePOFile('ko')
+        suggestion = self.factory.makeSharedTranslationMessage(
+            pofile=pofile, suggestion=True)
+        reviewer = self.factory.makePerson()
+
+        self.assertIs(None, suggestion.reviewer)
+        self.assertIs(None, suggestion.date_reviewed)
+
+        suggestion.approve(pofile, reviewer)
+
+        self.assertEqual(reviewer, suggestion.reviewer)
+        self.assertNotEqual(None, suggestion.date_reviewed)
+
+    def test_approve_awards_no_karma_if_no_change(self):
+        translator = self.factory.makePerson()
+        reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('ku')
+        existing_translation = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, translator=translator)
+
+        karmarecorder = self.installKarmaRecorder()
+        existing_translation.approve(pofile, reviewer)
+
+        self.assertEqual([], karmarecorder.karma_events)
+
+    def test_approve_awards_review_karma(self):
+        reviewer = self.factory.makePerson()
+        pofile = self.factory.makePOFile('be')
+        suggestion = self.factory.makeSharedTranslationMessage(
+            pofile=pofile, suggestion=True)
+
+        karmarecorder = self.installKarmaRecorder(person=reviewer)
+        suggestion.approve(pofile, reviewer)
+
+        self.assertEqual(1, len(karmarecorder.karma_events))
+        self.assertEqual(
+            'translationreview', karmarecorder.karma_events[0].action.name)
+
+    def test_approve_awards_suggestion_karma(self):
+        translator = self.factory.makePerson()
+        pofile = self.factory.makePOFile('ba')
+        suggestion = self.factory.makeSharedTranslationMessage(
+            pofile=pofile, translator=translator, suggestion=True)
+
+        karmarecorder = self.installKarmaRecorder(person=translator)
+        suggestion.approve(pofile, self.factory.makePerson())
+
+        self.assertEqual(1, len(karmarecorder.karma_events))
+        self.assertEqual(
+            'translationsuggestionapproved',
+            karmarecorder.karma_events[0].action.name)
+
+    def test_approve_awards_no_karma_for_self_approval(self):
+        translator = self.factory.makePerson()
+        pofile = self.factory.makePOFile('bi')
+        suggestion = self.factory.makeSharedTranslationMessage(
+            pofile=pofile, translator=translator, suggestion=True)
+
+        karmarecorder = self.installKarmaRecorder(person=translator)
+        suggestion.approve(pofile, translator)
+
+        self.assertEqual([], karmarecorder.karma_events)
+
 
 class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
     """Tests for `TranslationMessage.findIdenticalMessage`."""