Merge lp:~sinzui/launchpad/poimport-typeerror into lp:launchpad

[Curtis Hovey]

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

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

    * Untestable, 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

[Brad Crittenden (bac)]

Hi Curtis,

Typo: enail

When there is no sequence will the users understand '-1'? Would another string like "Invalid sequence" be better?

Overall it looks good.

review: Approve (code)

[Curtis Hovey (sinzui)]

The emails are often parsed by scripts and we want to keep using the number. This is case though where there is nothing wrong with the sequence in the pofile...Lp is bad, not the pofile.

=== 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-13 13:43:23 +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 email 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-13 13:43:23 +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."""