Merge lp:~jtv/launchpad/bug-409686 into lp:launchpad

[Jeroen T. Vermeulen]

Notify owners of unpushed translations branches.

= Bug 409686 =

The Code UI unfortunately allows users to "register" a branch without populating it. From the Launchpad database's point of view, this is a perfectly good branch. From bzr's point of view, it just doesn't exist. With our daily translations exports to bzr branches, this situation has been showing up in logs as NotBranchError exceptions. I've been notifying these people of the problem by hand, but I got tired of it and the LOSA team got sick of looking at the log output on error-reports.

Here's a fix that catches this condition, logs it separately from "real" errors, and notifies the branch owner by email. (The branch owner is normally also the project owner; where they're not the same, it's the branch owner who should perform the necessary bzr steps). The branch owner will be receiving this every day, hopefully pushing towards a rapid resolution.

We won't be able to Q/A this on staging, beyond ensuring that the script doesn't break altogether. On production, we can pick a translatable project and temporarily enable translations exports to a branch we registered ourselves (but did not push to the server). Of course we'll also want to make sure that the regular exports still work as well.

To test:
{{{
./bin/test -vv -t 'translations.*to.branch'
}}}

No lint.

Jeroen

[Abel Deuring (adeuring)]

Hi Jeroen,

a nice branch. Just one remark:

Reading

> + bzr launchpad-login LOGIN
> +
> +Instead of LOGIN, type your Launchpad login name. Then:

raised a mental parser warning for me. This may be due to an undercaffeinated brain, but "...login LOGIN" looks a bit odd. What about "bzr launchpad-login your-launchpad-user-name" instead?

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Thanks. Replaced with USERNAME as per IRC.

[Robert Collins (lifeless)]

Rather than unpushed, I suggest calling them 'neverpushed' - its a bit
clearer IMO.

=== modified file 'lib/lp/translations/doc/translations-export-to-branch.txt'
--- lib/lp/translations/doc/translations-export-to-branch.txt	2010-04-01 04:29:46 +0000
+++ lib/lp/translations/doc/translations-export-to-branch.txt	2010-05-28 07:47:34 +0000
@@ -14,7 +14,7 @@
     >>> print stderr
     INFO Creating lockfile: /var/lock/launchpad-translations-export-to-branch.lock
     INFO Exporting to translations branches.
-    INFO Processed 0 item(s); 0 failure(s).
+    INFO Processed 0 item(s); 0 failure(s), 0 unpushed branch(es).
 
     >>> from lp.translations.scripts.translations_to_branch import (
     ...     ExportTranslationsToBranch)
@@ -157,7 +157,7 @@
     DEBUG ...
     INFO Committed 2 file(s).
     INFO Unlock.
-    INFO Processed 1 item(s); 0 failure(s).
+    INFO Processed 1 item(s); 0 failure(s), 0 unpushed branch(es).
 
 When Gazblachko stops using Launchpad for Translations, the exports stop
 also.
@@ -166,7 +166,7 @@
     >>> transaction.commit()
     >>> script.main()
     INFO Exporting to translations branches.
-    INFO Processed 0 item(s); 0 failure(s).
+    INFO Processed 0 item(s); 0 failure(s), 0 unpushed branch(es).
 
     >>> gazblachko.official_rosetta = True
     >>> transaction.commit()
@@ -192,7 +192,7 @@
     DEBUG ....
     DEBUG Last commit was at ....
     INFO Unlock.
-    INFO Processed 1 item(s); 0 failure(s).
+    INFO Processed 1 item(s); 0 failure(s), 0 unpushed branch(es).
 
 If one of the files is updated, it is exported again.  Unchanged files
 are not.
@@ -207,7 +207,56 @@
     INFO ...
     INFO Committed 1 file(s).
     INFO Unlock.
-    INFO Processed 1 item(s); 0 failure(s).
+    INFO Processed 1 item(s); 0 failure(s), 0 unpushed branch(es).
+
+
+Unpushed branches
+-----------------
+
+The Launchpad UI allows users to register branches in the Launchpad
+database without populating them in bzr.  Exporting to such a branch
+won't work, so we email a notification to the branch owner.
+
+    >>> from email import message_from_string
+    >>> from lp.services.mail import stub
+    >>> from lp.codehosting.vfs import get_rw_server
+    >>> productseries = factory.makeProductSeries()
+    >>> productseries.translations_branch = factory.makeBranch()
+    >>> template = factory.makePOTemplate(productseries=productseries)
+    >>> potmsgset = factory.makePOTMsgSet(template)
+    >>> pofile = removeSecurityProxy(
+    ...     factory.makePOFile('nl', potemplate=template))
+    >>> tm = factory.makeTranslationMessage(
+    ...     pofile=pofile, potmsgset=potmsgset, translator=template.owner)
+
+    >>> server = get_rw_server(direct_database=True)
+    >>> server.start_server()
+    >>> real_script = ExportTranslationsToBranch(
+    ...     'export-to-branch', test_args=[])
+    >>> real_script.logger = FakeLogger()
+    >>> try:
+    ...     real_script._exportToBranches([productseries])
+    ... finally:
+    ...     server.destroy()
+    INFO Exporting ...
+    INFO Processed 1 item(s); 0 failure(s), 1 unpushed branch(es).
+
+    # Give the email a chance to arrive in the test mailbox.
+    >>> transaction.commit()
+
+    >>> sender, recipients, body = stub.test_emails.pop()
+    >>> message = message_from_string(body)
+    >>> print message['Subject']
+    Launchpad: translations branch has not been set up.
+
+    >>> print message.get_payload()
+    Hello,
+    There was a problem with translations branch synchronization for
+    ...
+    Branch synchronization for this release series has been set up to
+    commit translations snapshots to the bzr branch at lp://...
+
+For the full message text, see emailtemplates/unpushed-branch.txt.
 
 
 Race conditions
@@ -243,7 +292,7 @@
     DEBUG ...
     INFO Unlock.
     ERROR Failure: ConcurrentUpdateError(...Simulated race condition...)
-    INFO Processed 1 item(s); 1 failure(s).
+    INFO Processed 1 item(s); 1 failure(s), 0 unpushed branch(es).
 
 
 === Pending imports from same branch ===
@@ -264,7 +313,7 @@
     ERROR Failure: ConcurrentUpdateError(...Translations branch for
     Gazblachko trunk series has pending translations changes.
     Not committing...)
-    INFO Processed 1 item(s); 1 failure(s).
+    INFO Processed 1 item(s); 1 failure(s), 0 unpushed branch(es).
 
 There is one problem with detecting this race condition.  Jobs are never
 cleaned up.  So if the job failed for whatever reason, an unfinished job
@@ -282,4 +331,4 @@
     INFO Writing file 'po/main/nl.po':
     INFO ...
     INFO Unlock.
-    INFO Processed 1 item(s); 0 failure(s).
+    INFO Processed 1 item(s); 0 failure(s), 0 unpushed branch(es).

=== added file 'lib/lp/translations/emailtemplates/unpushed-branch.txt'
--- lib/lp/translations/emailtemplates/unpushed-branch.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/emailtemplates/unpushed-branch.txt	2010-05-28 07:47:34 +0000
@@ -0,0 +1,38 @@
+Hello,
+
+There was a problem with translations branch synchronization for
+%(productseries)s.
+
+Branch synchronization for this release series has been set up to commit
+translations snapshots to the bzr branch at %(branch_url)s.
+
+That branch does not appear to have been pushed to Launchpad yet.
+Please create a branch on your own system that matches your needs, and
+then push it to Launchpad using "bzr push %(branch_url)s".
+
+You'll need the bzr tool.  If all you want is an empty branch for the
+translations to go into, you can create one using the following command
+lines:
+
+    bzr launchpad-login USERNAME
+
+Instead of USERNAME, type your Launchpad login name.  Then:
+
+    bzr init translations-export
+    cd translations-export
+    bzr commit --unchanged -m "Translations branch."
+    bzr push %(branch_url)s
+
+This will create an empty branch on Launchpad.  It should be updated
+with translations within the day.  You can update the branch on your
+system with the latest changes on Launchpad by going back into the
+translations-export directory and typing this command:
+
+    bzr pull --remember %(branch_url)s
+
+More extensive help can be found at
+
+    https://help.launchpad.net/Code
+
+--
+Automatic message from Launchpad.net.

=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
--- lib/lp/translations/scripts/tests/test_translations_to_branch.py	2010-04-23 08:27:40 +0000
+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py	2010-05-28 07:47:34 +0000
@@ -11,6 +11,10 @@
 
 from zope.security.proxy import removeSecurityProxy
 
+from bzrlib.errors import NotBranchError
+
+from canonical.config import config
+
 from canonical.launchpad.scripts.logger import QuietFakeLogger
 from canonical.launchpad.scripts.tests import run_script
 from canonical.testing import ZopelessAppServerLayer
@@ -23,7 +27,7 @@
 
 
 class GruesomeException(Exception):
-    """CPU on fire.  Or some other kind of failure, like."""
+    """CPU on fire.  Or some other kind of failure."""
 
 
 class TestExportTranslationsToBranch(TestCaseWithFactory):
@@ -76,10 +80,13 @@
 
         self.assertEqual('', stdout)
         self.assertEqual(
-            'INFO    Creating lockfile: /var/lock/launchpad-translations-export-to-branch.lock\n'
+            'INFO    '
+            'Creating lockfile: '
+            '/var/lock/launchpad-translations-export-to-branch.lock\n'
             'INFO    Exporting to translations branches.\n'
             'INFO    Exporting Committobranch trunk series.\n'
-            'INFO    Processed 1 item(s); 0 failure(s).',
+            'INFO    '
+            'Processed 1 item(s); 0 failure(s), 0 unpushed branch(es).',
             self._filterOutput(stderr))
         self.assertIn('No previous translations commit found.', stderr)
         self.assertEqual(0, retcode)
@@ -123,7 +130,9 @@
             ['-vvv', '--no-fudge'])
         self.assertEqual(0, retcode)
         self.assertIn('Last commit was at', stderr)
-        self.assertIn("Processed 1 item(s); 0 failure(s).", stderr)
+        self.assertIn(
+            "Processed 1 item(s); 0 failure(s), 0 unpushed branch(es).",
+            stderr)
         self.assertEqual(
             None, re.search("INFO\s+Committed [0-9]+ file", stderr))
 
@@ -144,6 +153,61 @@
         self.assertTrue(message.startswith("ERROR"))
         self.assertTrue("GruesomeException" in message)
 
+    def test_exportToBranches_handles_unpushed_branches(self):
+        # bzrlib raises NotBranchError when accessing a nonexistent
+        # branch.  The exporter deals with that by calling
+        # _handleUnpushedBranch.
+        exporter = ExportTranslationsToBranch(test_args=[])
+        exporter.logger = QuietFakeLogger()
+        productseries = self.factory.makeProductSeries()
+        productseries.translations_branch = self.factory.makeBranch()
+
+        # _handleUnpushedBranch is called if _exportToBranch raises
+        # NotBranchError.
+        exporter._handleUnpushedBranch = FakeMethod()
+        exporter._exportToBranch = FakeMethod(failure=NotBranchError("No!"))
+        exporter._exportToBranches([productseries])
+        self.assertEqual(1, exporter._handleUnpushedBranch.call_count)
+
+        # This does not happen if the export succeeds.
+        exporter._handleUnpushedBranch = FakeMethod()
+        exporter._exportToBranch = FakeMethod()
+        exporter._exportToBranches([productseries])
+        self.assertEqual(0, exporter._handleUnpushedBranch.call_count)
+
+        # Nor does it happen if the export fails in some other way.
+        exporter._handleUnpushedBranch = FakeMethod()
+        exporter._exportToBranch = FakeMethod(failure=IndexError("Ayyeee!"))
+        exporter._exportToBranches([productseries])
+        self.assertEqual(0, exporter._handleUnpushedBranch.call_count)
+
+    def test_handleUnpushedBranch_mails_branch_owner(self):
+        exporter = ExportTranslationsToBranch(test_args=[])
+        exporter.logger = QuietFakeLogger()
+        productseries = self.factory.makeProductSeries()
+        email = self.factory.getUniqueEmailAddress()
+        branch_owner = self.factory.makePerson(email=email)
+        productseries.translations_branch = self.factory.makeBranch(
+            owner=branch_owner)
+        exporter._exportToBranch = FakeMethod(failure=NotBranchError("Ow"))
+        exporter._sendMail = FakeMethod()
+
+        exporter._exportToBranches([productseries])
+
+        self.assertEqual(1, exporter._sendMail.call_count)
+        (sender, recipients, subject, text), kwargs = (
+            exporter._sendMail.calls[-1])
+        self.assertIn(config.canonical.noreply_from_address, sender)
+        self.assertIn(email, recipients)
+        self.assertEqual(
+            "Launchpad: translations branch has not been set up.", subject)
+
+        self.assertIn(
+            "problem with translations branch synchronization", text)
+        self.assertIn(productseries.title, text)
+        self.assertIn(productseries.translations_branch.bzr_identity, text)
+        self.assertIn('bzr push lp://', text)
+
 
 class TestExportToStackedBranch(TestCaseWithFactory):
     """Test workaround for bzr bug 375013."""

=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py	2010-04-23 03:09:57 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py	2010-05-28 07:47:34 +0000
@@ -14,9 +14,14 @@
 
 from zope.component import getUtility
 
+from bzrlib.errors import NotBranchError
+
 from storm.expr import Join, SQL
 
-from canonical.launchpad.helpers import shortlist
+from canonical.config import config
+from canonical.launchpad.helpers import (
+    get_contact_email_addresses, get_email_template, shortlist)
+from lp.services.mail.sendmail import format_address, simple_sendmail
 from lp.codehosting.vfs import get_rw_server
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from canonical.launchpad.webapp.interfaces import (
@@ -225,6 +230,7 @@
         """Loop over `productseries_iter` and export their translations."""
         items_done = 0
         items_failed = 0
+        unpushed_branches = 0
 
         productseries = shortlist(productseries_iter, longest_expected=2000)
 
@@ -236,6 +242,13 @@
                     self.txn.commit()
             except (KeyboardInterrupt, SystemExit):
                 raise
+            except NotBranchError:
+                unpushed_branches += 1
+                if self.txn:
+                    self.txn.abort()
+                self._handleUnpushedBranch(source)
+                if self.txn:
+                    self.txn.commit()
             except Exception, e:
                 items_failed += 1
                 self.logger.error("Failure: %s" % repr(e))
@@ -244,8 +257,34 @@
 
             items_done += 1
 
-        self.logger.info("Processed %d item(s); %d failure(s)." % (
-            items_done, items_failed))
+        self.logger.info(
+            "Processed %d item(s); %d failure(s), %d unpushed branch(es)." % (
+                items_done, items_failed, unpushed_branches))
+
+    def _sendMail(self, sender, recipients, subject, text):
+        """Wrapper for `simple_sendmail`.  Fakeable for easy testing."""
+        simple_sendmail(sender, recipients, subject, text)
+
+    def _handleUnpushedBranch(self, productseries):
+        """Branch has never been scanned.  Notify owner.
+
+        This means that as far as the Launchpad database knows, there is
+        no actual bzr branch behind this `IBranch` yet.
+        """
+        branch = productseries.translations_branch
+        self.logger.info("Notifying %s of unpushed branch %s." % (
+            branch.owner.name, branch.bzr_identity))
+
+        template = get_email_template('unpushed-branch.txt', 'translations')
+        text = template % {
+            'productseries': productseries.title,
+            'branch_url': branch.bzr_identity,
+        }
+        recipients = get_contact_email_addresses(branch.owner)
+        sender = format_address(
+            "Launchpad Translations", config.canonical.noreply_from_address)
+        subject = "Launchpad: translations branch has not been set up."
+        self._sendMail(sender, recipients, subject, text)
 
     def main(self):
         """See `LaunchpadScript`."""