launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/612171-diff-generation-spam into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #612171 in Launchpad itself: "Error generating merge proposal diff when "source branch has pending writes""
  https://bugs.launchpad.net/launchpad/+bug/612171

For more details, see:
https://code.launchpad.net/~mbp/launchpad/612171-diff-generation-spam/+merge/79351

This was proposed into the wrong branch, and anyhow had a confused message, here's the right thing:

* log all job failures, even those caused by user errors
* don't send mail about failure to generate diffs, which can be caused by
 - source or origin branch is empty
 - branch is in use

The mail that is currently sent has no useful information and the user can do nothing with it.

We can do other things to make diff generation automatically retry or more robust, but there's no benefit from telling the user about it.  See the many complaints in the linked bugs and duplicates about this.  I'm happy to split out a separate bug saying we ought to retry.  

In practice the diffs do generally seem to get updated eventually, even when this mail is sent, perhaps because a new job is kicked off when the later update completes.
-- 
https://code.launchpad.net/~mbp/launchpad/612171-diff-generation-spam/+merge/79351
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/612171-diff-generation-spam into lp:launchpad.

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2011-09-26 20:00:50 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2011-10-14 00:15:23 +0000
@@ -386,8 +386,11 @@
 
     def getErrorRecipients(self):
         """Return a list of email-ids to notify about user errors."""
-        registrant = self.branch_merge_proposal.registrant
-        return format_address_for_person(registrant)
+        # All these failures are either obvious user errors in the web ui (eg
+        # nothing in the branch), or Launchpad internal errors (eg the branch
+        # is being written to).  See bug 612171.  Either way, the user can't
+        # be anything but annoyed by mail about it. -- mbp 2011-10-13
+        return []
 
 
 class CreateMergeProposalJob(BaseRunnableJob):

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2011-09-26 20:00:50 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2011-10-14 00:15:23 +0000
@@ -234,24 +234,17 @@
     def test_run_branches_not_ready(self):
         # If the job has been waiting for a significant period of time (15
         # minutes for now), we run the job anyway.  The checkReady method
-        # then raises and this is caught as a user error by the job system,
-        # and as such sends an email to the error recipients, which for this
-        # job is the merge proposal registrant.
+        # then raises and this is caught as an error by the job system.
+        # But, it does not send mail to the user, because there is likely
+        # nothing they can do about it: see bug 640882.
         eric = self.factory.makePerson(name='eric', email='eric@xxxxxxxxxxx')
         bmp = self.factory.makeBranchMergeProposal(registrant=eric)
         job = UpdatePreviewDiffJob.create(bmp)
         pop_notifications()
         JobRunner([job]).runAll()
-        [email] = pop_notifications()
-        self.assertEqual('Eric <eric@xxxxxxxxxxx>', email['to'])
-        self.assertEqual(
-            'Launchpad error while generating the diff for a merge proposal',
-            email['subject'])
-        self.assertEqual(
-            'Launchpad encountered an error during the following operation: '
-            'generating the diff for a merge proposal.  '
-            'The source branch has no revisions.',
-            email.get_payload(decode=True))
+        self.assertEqual(
+            [],
+            pop_notifications())
 
     def test_10_minute_lease(self):
         self.useBzrBranches(direct_database=True)

=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py	2011-09-26 07:21:53 +0000
+++ lib/lp/services/job/runner.py	2011-10-14 00:15:23 +0000
@@ -259,6 +259,8 @@
                     self.logger.debug('Running %r', job)
                     self.runJob(job)
                 except job.user_error_types, e:
+                    self.logger.info('Job %r failed with user error %r' %
+                        (job, e))
                     job.notifyUserError(e)
                 except Exception:
                     info = sys.exc_info()