launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

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

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


= Set messages as reviewed =

For the Recife feature branch.

When a TranslationMessage is selected to be a message's current translation, we update its review date (so we can later see which suggestions came in since last review) and keep track of its reviewer.  Here I implement this for the Recife data model which is to replace our current one.

We have agreed not to do this in setCurrentTranslation, but one level higher in the call tree.  We already had it implemented in approveSuggestion, and we haven't implemented the method for divergence yet.  So that left the change to be made in clearCurrentTranslation.  This method didn't even always leave a message current, which means there might not be any place to mark the review date at all.

We do have a standard way of dealing with this: create an empty TranslationMessage.  This will show up as "untranslated," just like the absence of a current TM does, but it also acts as a review marker to compare suggestions' submission dates against.

Adding a message gets complicated: what of divergence, what of tracking between upstream and Ubuntu, what of existing messages, what of clashes between demoted diverged messages and existing shared ones?  Rather than deal with it all again, I just call setCurrentTranslation to enable a blank message.  This simplifies the code a lot.  One white-box test failed, but it actually asserted that there would be no current message left.  That is no longer the case, so I scratched the test.

We also gained some helpful factory methods for TranslationMessages since the original clearCurrentTranslation was written, so I converted the tests to use them.  This exposed another white-box test that we no longer need: what if we deactivate a shared translation message when there's already a suggestion (a non-current shared translation message) with the same contents?  That's not really supposed to be possible and moreover, it is now for setCurrentTranslation to worry about.

To test,
{{{
./bin/test -vvc -m lp.translations.tests.test_clearcurrenttranslation
}}}

(I decapitalized the test name).

No lint.


Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/recife-review/+merge/33183
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/recife-review into lp:~launchpad/launchpad/recife.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-08-19 09:37:13 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-08-20 04:52:42 +0000
@@ -1275,48 +1275,10 @@
     def clearCurrentTranslation(self, pofile, submitter, origin,
                                 share_with_other_side=False):
         """See `IPOTMsgSet`."""
-        template = pofile.potemplate
-        traits = getUtility(ITranslationSideTraitsSet).getTraits(
-            template.translation_side)
-
-        current = traits.getCurrentMessage(self, template, pofile.language)
-        if current is None:
-            # Trivial case: there's nothing to disable.
-            return
-
-        if current.is_diverged:
-            # Disable the current message.
-            if current.getSharedEquivalent() is not None:
-                current.destroySelf()
-            else:
-                traits.setFlag(current, False)
-
-            shared = traits.getCurrentMessage(self, template, pofile.language)
-            assert shared is None or not shared.is_diverged, (
-                "I killed a divergence but the current message is still "
-                "diverged.")
-            if shared is not None and not shared.is_empty:
-                # Mask the shared message with a diverged empty message.
-                existing_empty_message = self._findTranslationMessage(
-                    pofile, self._findPOTranslations([]), prefer_shared=True)
-                can_reuse = not (
-                    existing_empty_message is None or
-                    existing_empty_message.is_current_upstream or
-                    existing_empty_message.is_current_ubuntu)
-                if can_reuse:
-                    existing_empty_message.potemplate = template
-                    mask = existing_empty_message
-                else:
-                    mask = self._makeTranslationMessage(
-                        pofile, submitter, {}, origin, diverged=True)
-
-                Store.of(mask).add_flush_order(current, mask)
-                mask.markReviewed(submitter, UTC_NOW)
-                traits.setFlag(mask, True)
-        else:
-            traits.setFlag(current, False)
-            if share_with_other_side:
-                traits.other_side_traits.setFlag(current, False)
+        message = self.setCurrentTranslation(
+            pofile, submitter, [], origin,
+            share_with_other_side=share_with_other_side)
+        message.markReviewed(submitter)
 
     def applySanityFixes(self, text):
         """See `IPOTMsgSet`."""

=== modified file 'lib/lp/translations/tests/test_clearCurrentTranslation.py'
--- lib/lp/translations/tests/test_clearCurrentTranslation.py	2010-08-10 07:39:01 +0000
+++ lib/lp/translations/tests/test_clearCurrentTranslation.py	2010-08-20 04:52:42 +0000
@@ -5,6 +5,13 @@
 
 __metaclass__ = type
 
+from datetime import (
+    datetime,
+    timedelta,
+    )
+
+from pytz import UTC
+
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -59,16 +66,15 @@
     def _makeTranslationMessage(self, potmsgset, pofile, translations=None,
                                 diverged=False):
         """Create a (non-current) TranslationMessage for potmsgset."""
-        if translations is None:
-            translations = {0: self.factory.getUniqueString()}
-        message = potmsgset.submitSuggestion(
-            pofile, pofile.potemplate.owner, translations)
-
+        message = self.factory.makeSuggestion(
+            pofile=pofile, potmsgset=potmsgset, translations=translations)
         if diverged:
             removeSecurityProxy(message).potemplate = pofile.potemplate
         return message
 
-    def test_does_nothing_if_not_translated(self):
+    def test_creates_empty_message(self):
+        # Even if there is no current message, clearCurrentTranslation
+        # will create an empty message so as to mark the review time.
         pofile = self._makePOFile()
         template = pofile.potemplate
         potmsgset = self.factory.makePOTMsgSet(template, sequence=1)
@@ -77,7 +83,9 @@
 
         current = get_traits(template).getCurrentMessage(
             potmsgset, template, pofile.language)
-        self.assertIs(None, current)
+        self.assertEqual(
+            [],
+            [msgstr for msgstr in current.translations if msgstr is not None])
 
     def test_deactivates_shared_message(self):
         pofile = self._makePOFile()
@@ -176,22 +184,6 @@
         self.assertFalse(traits.getFlag(tm))
         self.assertFalse(traits.other_side_traits.getFlag(tm))
 
-    def test_discards_redundant_suggestion(self):
-        translations = [self.factory.getUniqueString()]
-        pofile = self._makePOFile()
-        template = pofile.potemplate
-        potmsgset = self.factory.makePOTMsgSet(template, sequence=1)
-        tm = self._makeTranslationMessage(potmsgset, pofile, translations)
-        get_traits(template).setFlag(tm, True)
-        suggestion = self._makeTranslationMessage(
-            potmsgset, pofile, translations)
-
-        potmsgset.clearCurrentTranslation(pofile, template.owner, ORIGIN)
-
-        remaining_tms = list(potmsgset.getAllTranslationMessages())
-        self.assertEqual(1, len(remaining_tms))
-        self.assertIn(remaining_tms[0], [tm, suggestion])
-
     def test_converges_with_empty_shared_message(self):
         pofile = self._makePOFile()
         template = pofile.potemplate
@@ -210,6 +202,54 @@
             potmsgset, template, pofile.language)
         self.assertEqual(blank_shared_tm, current)
 
+    def test_reviews_new_blank(self):
+        # When clearCurrentTranslation creates a blank message in order
+        # to mark the review, the blank message does indeed have its
+        # review fields set.
+        pofile = self._makePOFile()
+        template = pofile.potemplate
+        potmsgset = self.factory.makePOTMsgSet(template, sequence=1)
+        reviewer = self.factory.makePerson()
+
+        potmsgset.clearCurrentTranslation(pofile, reviewer, ORIGIN)
+
+        blank = get_traits(template).getCurrentMessage(
+            potmsgset, template, pofile.language)
+
+        self.assertNotEqual(None, blank.date_reviewed)
+        self.assertEqual(reviewer, blank.reviewer)
+
+    def test_reviews_existing_blank(self):
+        # When clearCurrentTranslation reuses an existing blank message
+        # in order to mark the review, the blank message's review
+        # information is updated.
+        pofile = self._makePOFile()
+        template = pofile.potemplate
+        traits = get_traits(template)
+        potmsgset = self.factory.makePOTMsgSet(template, sequence=1)
+        blank = self.factory.makeSuggestion(
+            potmsgset=potmsgset, pofile=pofile, translations=[])
+
+        old_review_date = datetime.now(UTC) - timedelta(days=7, seconds=1)
+        old_reviewer = self.factory.makePerson()
+        blank.markReviewed(old_reviewer, timestamp=old_review_date)
+
+        current = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset)
+
+        new_reviewer = self.factory.makePerson()
+
+        potmsgset.clearCurrentTranslation(pofile, new_reviewer, ORIGIN)
+
+        current = traits.getCurrentMessage(
+            potmsgset, template, pofile.language)
+
+        self.assertEqual(new_reviewer, current.reviewer)
+        self.assertBetween(
+            old_review_date + timedelta(days=7),
+            current.date_reviewed,
+            datetime.now(UTC) + timedelta(seconds=1))
+
 
 class TestClearCurrentTranslationUpstream(TestCaseWithFactory,
                                           ScenarioMixin):