launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/poimport-typeerror into lp:launchpad.

Commit message:
Ensure the error data is sane enough to send an error email.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #872335 in Launchpad itself: "TypeError raised by rosetta-poimport.py script"
  https://bugs.launchpad.net/launchpad/+bug/872335

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/poimport-typeerror/+merge/133713

The TypeError happens when the script is trying to log an error. All the
data is sanitised except for the call to
potmsgset.getSequence(self.potemplate) to get the sequence. This can be
None if the TTI is incomplete. We just want to ensure the data is sane
enough so that we can log the error.

--------------------------------------------------------------------

RULES

    Pre-implementation: stevenk, jcsackett
    * Extract the logic that makes the error email to a separate testable
      method.
    * Add a test for the current behaviour.
    * Update the method to fallback to -1 if the sequence is None
    * Revise the loop to avoid needless string concatenation of errordetails


QA

    * Untestible, the case happens when the object is in an invalid state.


LINT

    lib/lp/translations/model/pofile.py
    lib/lp/translations/tests/test_pofile.py


LoC

    I have a credit of 4000 lines.


TEST

    ./bin/test -vvc -t prepare_pomessage lp.translations.tests.test_pofile


IMPLEMENTATION

Extracted the logic to a method and added a test. Added a rule to use -1
of the sequence is None.
    lib/lp/translations/model/pofile.py
    lib/lp/translations/tests/test_pofile.py
-- 
https://code.launchpad.net/~sinzui/launchpad/poimport-typeerror/+merge/133713
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/poimport-typeerror into lp:launchpad.

=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py	2012-09-28 06:25:44 +0000
+++ lib/lp/translations/model/pofile.py	2012-11-09 16:55:34 +0000
@@ -1098,29 +1098,10 @@
             subject = 'Import problem - %s - %s' % (
                 self.language.displayname, self.potemplate.displayname)
         elif len(errors) > 0:
-            # There were some errors with translations.
-            errorsdetails = ''
-            for error in errors:
-                potmsgset = error['potmsgset']
-                pomessage = error['pomessage']
-                error_message = error['error-message']
-                errorsdetails = '%s%d. "%s":\n\n%s\n\n' % (
-                    errorsdetails,
-                    potmsgset.getSequence(self.potemplate),
-                    error_message,
-                    pomessage)
-
+            data = self._prepare_pomessage_error_message(errors, replacements)
+            subject, template_mail, errorsdetails = data
             entry_to_import.setErrorOutput(
                 "Imported, but with errors:\n" + errorsdetails)
-
-            replacements['numberoferrors'] = len(errors)
-            replacements['errorsdetails'] = errorsdetails
-            replacements['numberofcorrectmessages'] = (
-                msgsets_imported - len(errors))
-
-            template_mail = 'poimport-with-errors.txt'
-            subject = 'Translation problems - %s - %s' % (
-                self.language.displayname, self.potemplate.displayname)
         else:
             # The import was successful.
             template_mail = 'poimport-confirmation.txt'
@@ -1165,6 +1146,28 @@
         message = template % replacements
         return (subject, message)
 
+    def _prepare_pomessage_error_message(self, errors, replacements):
+        # Return subject, template_mail, and errorsdetails to make
+        # an error enail message.
+        error_count = len(errors)
+        error_text = []
+        for error in errors:
+            potmsgset = error['potmsgset']
+            pomessage = error['pomessage']
+            sequence = potmsgset.getSequence(self.potemplate) or -1
+            error_message = error['error-message']
+            error_text.append('%d. "%s":\n\n%s\n\n' % (
+                sequence, error_message, pomessage))
+        errorsdetails = ''.join(error_text)
+        replacements['numberoferrors'] = error_count
+        replacements['errorsdetails'] = errorsdetails
+        replacements['numberofcorrectmessages'] = (
+            replacements['numberofmessages'] - error_count)
+        template_mail = 'poimport-with-errors.txt'
+        subject = 'Translation problems - %s - %s' % (
+            self.language.displayname, self.potemplate.displayname)
+        return subject, template_mail, errorsdetails
+
     def export(self, ignore_obsolete=False, force_utf8=False):
         """See `IPOFile`."""
         translation_exporter = getUtility(ITranslationExporter)

=== modified file 'lib/lp/translations/tests/test_pofile.py'
--- lib/lp/translations/tests/test_pofile.py	2012-02-15 21:14:05 +0000
+++ lib/lp/translations/tests/test_pofile.py	2012-11-09 16:55:34 +0000
@@ -20,7 +20,10 @@
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.services.database.constants import UTC_NOW
 from lp.services.webapp.publisher import canonical_url
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    monkey_patch,
+    TestCaseWithFactory,
+    )
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import ZopelessDatabaseLayer
 from lp.translations.interfaces.pofile import IPOFileSet
@@ -2149,6 +2152,60 @@
             language.code, with_plural=True)
         self.assertTrue(pofile.hasPluralFormInformation())
 
+    def test_prepare_pomessage_error_message(self):
+        # The method returns subject, template_mail, and errorsdetails
+        # to make an email message about errors.
+        errors = []
+        errors.append({
+            'potmsgset': self.factory.makePOTMsgSet(
+                potemplate=self.pofile.potemplate, sequence=1),
+            'pomessage': 'purrs',
+            'error-message': 'claws error',
+            })
+        errors.append({
+            'potmsgset': self.factory.makePOTMsgSet(
+                potemplate=self.pofile.potemplate, sequence=2),
+            'pomessage': 'plays',
+            'error-message': 'string error',
+            })
+        replacements = {'numberofmessages': 5}
+        pofile = removeSecurityProxy(self.pofile)
+        data = pofile._prepare_pomessage_error_message(
+            errors, replacements)
+        subject, template_mail, errorsdetails = data
+        pot_displayname = self.pofile.potemplate.displayname
+        self.assertEqual(
+            'Translation problems - Esperanto (eo) - %s' % pot_displayname,
+            subject)
+        self.assertEqual('poimport-with-errors.txt', template_mail)
+        self.assertEqual(2, replacements['numberoferrors'])
+        self.assertEqual(3, replacements['numberofcorrectmessages'])
+        self.assertEqual(errorsdetails, replacements['errorsdetails'])
+        self.assertEqual(
+            '1. "claws error":\n\npurrs\n\n2. "string error":\n\nplays\n\n',
+            errorsdetails)
+
+    def test_prepare_pomessage_error_message_sequence_is_invalid(self):
+        # The errordetails can be contructed when the sequnce is invalid.
+        errors = [{
+            'potmsgset': self.factory.makePOTMsgSet(
+                potemplate=self.pofile.potemplate, sequence=None),
+            'pomessage': 'purrs',
+            'error-message': 'claws error',
+            }]
+        replacements = {'numberofmessages': 5}
+        pofile = removeSecurityProxy(self.pofile)
+        potmsgset = removeSecurityProxy(errors[0]['potmsgset'])
+
+        def get_sequence(pot):
+            return None
+
+        with monkey_patch(potmsgset, getSequence=get_sequence):
+            data = pofile._prepare_pomessage_error_message(
+                errors, replacements)
+        subject, template_mail, errorsdetails = data
+        self.assertEqual('-1. "claws error":\n\npurrs\n\n', errorsdetails)
+
 
 class TestPOFileUbuntuUpstreamSharingMixin:
     """Test sharing between Ubuntu und upstream POFiles."""