Merge lp:~abentley/launchpad/upgrade-not-branch-error into lp:launchpad

[Aaron Bentley]

NotBranch while upgrading branch is user error, not oops.

= Summary =
Fix bug #823485: NotBranchError raised by upgrade_branches.py script

== Proposed fix ==
Handle NotBranchError as a user error and email to user.

== Pre-implementation notes ==
None

== Implementation details ==
The error notification should go to the user who requested the upgrade, but
until now, the requester was not recorded. Now, BranchUpgradeJob.create
requires a requester, and various tests are updated to match.

== Tests ==
bin/test -t test_branch -t test_branchjob -t test_upgrade_branches -t test_garbo

== Demo and Q/A ==
Create a branch. Request an upgrade. Immediately delete the contents of the
branch (but don't delete the branch from the database.)

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/branch.py
  lib/lp/code/interfaces/branch.py
  lib/lp/code/model/tests/test_branchjob.py
  database/schema/security.cfg
  lib/lp/testing/__init__.py
  lib/lp/scripts/tests/test_garbo.py
  lib/lp/code/model/branch.py
  lib/lp/code/scripts/tests/test_upgrade_branches.py
  lib/lp/code/model/tests/test_branch.py
  lib/lp/code/model/branchjob.py

[Brad Crittenden (bac)]

review: Approve (code)

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-08-17 19:51:38 +0000
+++ database/schema/security.cfg	2011-08-22 18:13:31 +0000
@@ -1903,7 +1903,9 @@
 groups=script
 public.branch                           = SELECT, UPDATE
 public.branchjob                        = SELECT, INSERT
+public.emailaddress                     = SELECT
 public.job                              = SELECT, INSERT, UPDATE
+public.person                           = SELECT
 type=user
 
 [send-branch-mail]

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2011-07-21 02:18:54 +0000
+++ lib/lp/code/browser/branch.py	2011-08-22 18:13:31 +0000
@@ -47,7 +47,6 @@
     )
 from lazr.uri import URI
 import pytz
-import simplejson
 from zope.app.form.browser import TextAreaWidget
 from zope.component import (
     getUtility,
@@ -838,7 +837,7 @@
     def mirror_of_ssh(self):
         """True if this a mirror branch with an sftp or bzr+ssh URL."""
         if not self.context.url:
-            return False # not a mirror branch
+            return False  # not a mirror branch
         uri = URI(self.context.url)
         return uri.scheme in ('sftp', 'bzr+ssh')
 
@@ -994,7 +993,7 @@
 
     @action('Upgrade', name='upgrade_branch')
     def upgrade_branch_action(self, action, data):
-        self.context.requestUpgrade()
+        self.context.requestUpgrade(self.user)
 
 
 class BranchEditView(BranchEditFormView, BranchNameValidationMixin):

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2011-08-17 10:01:10 +0000
+++ lib/lp/code/interfaces/branch.py	2011-08-22 18:13:31 +0000
@@ -1063,7 +1063,7 @@
           adapted to an IBranchTarget.
         """
 
-    def requestUpgrade():
+    def requestUpgrade(requester):
         """Create an IBranchUpgradeJob to upgrade this branch."""
 
     def branchChanged(stacked_on_url, last_revision_id, control_format,

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2011-08-17 20:21:01 +0000
+++ lib/lp/code/model/branch.py	2011-08-22 18:13:31 +0000
@@ -1152,10 +1152,10 @@
             BranchJob.job_type == BranchJobType.UPGRADE_BRANCH)
         return jobs.count() > 0
 
-    def requestUpgrade(self):
+    def requestUpgrade(self, requester):
         """See `IBranch`."""
         from lp.code.interfaces.branchjob import IBranchUpgradeJobSource
-        return getUtility(IBranchUpgradeJobSource).create(self)
+        return getUtility(IBranchUpgradeJobSource).create(self, requester)
 
     def _checkBranchVisibleByUser(self, user):
         """Is *this* branch visible by the user.

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2011-07-18 20:46:27 +0000
+++ lib/lp/code/model/branchjob.py	2011-08-22 18:13:31 +0000
@@ -21,7 +21,10 @@
 
 from bzrlib.branch import Branch as BzrBranch
 from bzrlib.diff import show_diff_trees
-from bzrlib.errors import NoSuchFile
+from bzrlib.errors import (
+    NoSuchFile,
+    NotBranchError,
+    )
 from bzrlib.log import (
     log_formatter,
     show_log,
@@ -107,6 +110,7 @@
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
+from lp.services.mail.sendmail import format_address_for_person
 from lp.translations.interfaces.translationimportqueue import (
     ITranslationImportQueue,
     )
@@ -275,6 +279,11 @@
             vars.append(('branch_name', self.context.branch.unique_name))
         return vars
 
+    def getErrorRecipients(self):
+        if self.requester is None:
+            return []
+        return [format_address_for_person(self.requester)]
+
 
 class BranchDiffJob(BranchJobDerived):
     """A Job that calculates the a diff related to a Branch."""
@@ -361,14 +370,20 @@
     classProvides(IBranchUpgradeJobSource)
     class_job_type = BranchJobType.UPGRADE_BRANCH
 
+    user_error_types = (NotBranchError,)
+
+    def getOperationDescription(self):
+        return 'upgrading a branch'
+
     @classmethod
-    def create(cls, branch):
+    def create(cls, branch, requester):
         """See `IBranchUpgradeJobSource`."""
         if not branch.needs_upgrading:
             raise AssertionError('Branch does not need upgrading.')
         if branch.upgrade_pending:
             raise AssertionError('Branch already has upgrade pending.')
-        branch_job = BranchJob(branch, BranchJobType.UPGRADE_BRANCH, {})
+        branch_job = BranchJob(
+            branch, BranchJobType.UPGRADE_BRANCH, {}, requester=requester)
         return cls(branch_job)
 
     @staticmethod

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2011-08-09 14:53:07 +0000
+++ lib/lp/code/model/tests/test_branch.py	2011-08-22 18:13:31 +0000
@@ -617,7 +617,7 @@
         owner = removeSecurityProxy(branch).owner
         login_person(owner)
         self.addCleanup(logout)
-        branch.requestUpgrade()
+        branch.requestUpgrade(branch.owner)
 
         self.assertFalse(branch.needs_upgrading)
 
@@ -682,7 +682,7 @@
         owner = removeSecurityProxy(branch).owner
         login_person(owner)
         self.addCleanup(logout)
-        job = removeSecurityProxy(branch.requestUpgrade())
+        job = removeSecurityProxy(branch.requestUpgrade(branch.owner))
 
         jobs = list(getUtility(IBranchUpgradeJobSource).iterReady())
         self.assertEqual(
@@ -698,7 +698,7 @@
         owner = removeSecurityProxy(branch).owner
         login_person(owner)
         self.addCleanup(logout)
-        self.assertRaises(AssertionError, branch.requestUpgrade)
+        self.assertRaises(AssertionError, branch.requestUpgrade, branch.owner)
 
     def test_requestUpgrade_upgrade_pending(self):
         # If there is a pending upgrade already requested, requestUpgrade
@@ -708,9 +708,9 @@
         owner = removeSecurityProxy(branch).owner
         login_person(owner)
         self.addCleanup(logout)
-        branch.requestUpgrade()
+        branch.requestUpgrade(branch.owner)
 
-        self.assertRaises(AssertionError, branch.requestUpgrade)
+        self.assertRaises(AssertionError, branch.requestUpgrade, branch.owner)
 
     def test_upgradePending(self):
         # If there is a BranchUpgradeJob pending for the branch, return True.
@@ -719,7 +719,7 @@
         owner = removeSecurityProxy(branch).owner
         login_person(owner)
         self.addCleanup(logout)
-        branch.requestUpgrade()
+        branch.requestUpgrade(branch.owner)
 
         self.assertTrue(branch.upgrade_pending)
 
@@ -737,7 +737,7 @@
         owner = removeSecurityProxy(branch).owner
         login_person(owner)
         self.addCleanup(logout)
-        branch_job = removeSecurityProxy(branch.requestUpgrade())
+        branch_job = removeSecurityProxy(branch.requestUpgrade(branch.owner))
         branch_job.job.start()
         branch_job.job.complete()
 
@@ -1430,7 +1430,6 @@
             package.distribution.owner,
             package.development_version.setBranch,
             pocket, branch, package.distribution.owner)
-        series_set = getUtility(IFindOfficialBranchLinks)
         self.assertEqual(
             {package: ('alter',
                     _('Branch is officially linked to a source package.'))},

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2011-08-12 11:37:08 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2011-08-22 18:13:31 +0000
@@ -15,7 +15,10 @@
     Branch,
     BzrBranchFormat7,
     )
-from bzrlib.bzrdir import BzrDirMetaFormat1
+from bzrlib.bzrdir import (
+    BzrDir,
+    BzrDirMetaFormat1,
+    )
 from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack6
 from bzrlib.revision import NULL_REVISION
 from bzrlib.transport import get_transport
@@ -77,6 +80,7 @@
 from lp.scripts.helpers import TransactionFreeOperation
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
+from lp.services.job.runner import JobRunner
 from lp.services.osutils import override_environ
 from lp.testing import TestCaseWithFactory
 from lp.testing.mail_helpers import pop_notifications
@@ -291,20 +295,18 @@
         branch = self.factory.makeAnyBranch(
             branch_format=BranchFormat.BZR_BRANCH_5,
             repository_format=RepositoryFormat.BZR_REPOSITORY_4)
-        job = BranchUpgradeJob.create(branch)
+        job = BranchUpgradeJob.create(branch, self.factory.makePerson())
         verifyObject(IBranchUpgradeJob, job)
 
     def test_upgrades_branch(self):
         """Ensure that a branch with an outdated format is upgraded."""
         self.useBzrBranches(direct_database=True)
-        db_branch, tree = self.create_branch_and_tree(format='knit')
-        db_branch.branch_format = BranchFormat.BZR_BRANCH_5
-        db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
+        db_branch, tree = self.create_knit()
         self.assertEqual(
             tree.branch.repository._format.get_format_string(),
             'Bazaar-NG Knit Repository Format 1')
 
-        job = BranchUpgradeJob.create(db_branch)
+        job = BranchUpgradeJob.create(db_branch, self.factory.makePerson())
         self.becomeDbUser(config.upgrade_branches.dbuser)
         with TransactionFreeOperation.require():
             job.run()
@@ -321,15 +323,21 @@
         branch = self.factory.makeAnyBranch(
             branch_format=BranchFormat.BZR_BRANCH_7,
             repository_format=RepositoryFormat.BZR_CHK_2A)
-        self.assertRaises(AssertionError, BranchUpgradeJob.create, branch)
+        self.assertRaises(
+            AssertionError, BranchUpgradeJob.create, branch,
+            self.factory.makePerson())
+
+    def create_knit(self):
+        db_branch, tree = self.create_branch_and_tree(format='knit')
+        db_branch.branch_format = BranchFormat.BZR_BRANCH_5
+        db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
+        return db_branch, tree
 
     def test_existing_bzr_backup(self):
         # If the target branch already has a backup.bzr dir, the upgrade copy
         # should remove it.
         self.useBzrBranches(direct_database=True)
-        db_branch, tree = self.create_branch_and_tree(format='knit')
-        db_branch.branch_format = BranchFormat.BZR_BRANCH_5
-        db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
+        db_branch, tree = self.create_knit()
 
         # Add a fake backup.bzr dir
         source_branch_transport = get_transport(db_branch.getInternalBzrUrl())
@@ -337,7 +345,7 @@
         source_branch_transport.clone('.bzr').copy_tree_to_transport(
             source_branch_transport.clone('backup.bzr'))
 
-        job = BranchUpgradeJob.create(db_branch)
+        job = BranchUpgradeJob.create(db_branch, self.factory.makePerson())
         self.becomeDbUser(config.upgrade_branches.dbuser)
         job.run()
 
@@ -355,6 +363,27 @@
         branch.branchChanged('', 'new-id', None, None, None)
         Store.of(branch).flush()
 
+    def test_not_branch_error(self):
+        self.useBzrBranches(direct_database=True)
+        db_branch, tree = self.create_branch_and_tree()
+        branch2 = BzrDir.create_branch_convenience('.')
+        tree.branch.set_stacked_on_url(branch2.base)
+        branch2.bzrdir.destroy_branch()
+        # Create BranchUpgradeJob manually, because we're trying to upgrade a
+        # branch that doesn't need upgrading.
+        requester = self.factory.makePerson()
+        branch_job = BranchJob(
+            db_branch, BranchJobType.UPGRADE_BRANCH, {}, requester=requester)
+        job = BranchUpgradeJob(branch_job)
+        self.becomeDbUser(config.upgrade_branches.dbuser)
+        runner = JobRunner([job])
+        with self.noOops():
+            runner.runJobHandleError(job)
+        (mail,) = pop_notifications()
+        self.assertEqual(
+            'Launchpad error while upgrading a branch', mail['subject'])
+        self.assertIn('Not a branch', mail.get_payload(decode=True))
+
 
 class TestRevisionMailJob(TestCaseWithFactory):
     """Tests for BranchDiffJob."""
@@ -898,7 +927,7 @@
         self.layer.switchDbUser('branchscanner')
         self.updateDBRevisions(db_branch, tree.branch)
         expected = (
-            u"-"*60 + '\n'
+            u"-" * 60 + '\n'
             "revno: 1" '\n'
             "committer: Joe Bloggs <[e-mail]>" '\n'
             "branch nick: %s" '\n'
@@ -912,7 +941,7 @@
             job.getRevisionMessage(first_revision, 1), expected)
 
         expected_message = (
-            u"-"*60 + '\n'
+            u"-" * 60 + '\n'
             "revno: 2" '\n'
             "committer: Joe Bloggs <[e-mail]>" '\n'
             "branch nick: %s" '\n'
@@ -988,7 +1017,7 @@
         super(TestRosettaUploadJob, self).setUp()
         self.series = None
 
-    def _makeBranchWithTreeAndFile(self, file_name, file_content = None):
+    def _makeBranchWithTreeAndFile(self, file_name, file_content=None):
         return self._makeBranchWithTreeAndFiles(((file_name, file_content), ))
 
     def _makeBranchWithTreeAndFiles(self, files):
@@ -1035,7 +1064,7 @@
             try:
                 file_content = file_pair[1]
                 if file_content is None:
-                    raise IndexError # Same as if missing.
+                    raise IndexError  # Same as if missing.
             except IndexError:
                 file_content = self.factory.getUniqueString()
             dname = os.path.dirname(file_name)
@@ -1060,7 +1089,7 @@
             self.series.branch = self.branch
             self.series.translations_autoimport_mode = mode
 
-    def _runJobWithFile(self, import_mode, file_name, file_content = None):
+    def _runJobWithFile(self, import_mode, file_name, file_content=None):
         return self._runJobWithFiles(
             import_mode, ((file_name, file_content), ))
 
@@ -1293,7 +1322,7 @@
         # iterReady does not return jobs for branches where last_scanned_id
         # and last_mirror_id are different.
         self._makeBranchWithTreeAndFiles([])
-        self.branch.last_scanned_id = NULL_REVISION # Was not scanned yet.
+        self.branch.last_scanned_id = NULL_REVISION  # Was not scanned yet.
         self._makeProductSeries(
             TranslationsBranchImportMode.IMPORT_TEMPLATES)
         # Put the job in ready state.

=== modified file 'lib/lp/code/scripts/tests/test_upgrade_branches.py'
--- lib/lp/code/scripts/tests/test_upgrade_branches.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/scripts/tests/test_upgrade_branches.py	2011-08-22 18:13:31 +0000
@@ -34,7 +34,7 @@
             target_tree.branch.repository._format.get_format_string(),
             'Bazaar-NG Knit Repository Format 1')
 
-        BranchUpgradeJob.create(target)
+        BranchUpgradeJob.create(target, self.factory.makePerson())
         transaction.commit()
 
         retcode, stdout, stderr = run_script(
@@ -62,7 +62,7 @@
             target_tree.branch.repository._format.get_format_string(),
             'Bazaar-NG Knit Repository Format 1')
 
-        BranchUpgradeJob.create(target)
+        BranchUpgradeJob.create(target, self.factory.makePerson())
         transaction.commit()
 
         retcode, stdout, stderr = run_script(

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2011-08-17 10:43:26 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2011-08-22 18:13:31 +0000
@@ -848,7 +848,8 @@
         db_branch.branch_format = BranchFormat.BZR_BRANCH_5
         db_branch.repository_format = RepositoryFormat.BZR_KNIT_1
         Store.of(db_branch).flush()
-        branch_job = BranchUpgradeJob.create(db_branch)
+        branch_job = BranchUpgradeJob.create(
+            db_branch, self.factory.makePerson())
         branch_job.job.date_finished = THIRTY_DAYS_AGO
 
         self.assertEqual(
@@ -876,13 +877,14 @@
             branch_format=BranchFormat.BZR_BRANCH_5,
             repository_format=RepositoryFormat.BZR_KNIT_1)
 
-        branch_job = BranchUpgradeJob.create(db_branch)
+        branch_job = BranchUpgradeJob.create(
+            db_branch, self.factory.makePerson())
         branch_job.job.date_finished = THIRTY_DAYS_AGO
 
         db_branch2 = self.factory.makeAnyBranch(
             branch_format=BranchFormat.BZR_BRANCH_5,
             repository_format=RepositoryFormat.BZR_KNIT_1)
-        BranchUpgradeJob.create(db_branch2)
+        BranchUpgradeJob.create(db_branch2, self.factory.makePerson())
 
         self.runDaily()
 

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2011-08-15 09:21:48 +0000
+++ lib/lp/testing/__init__.py	2011-08-22 18:13:31 +0000
@@ -437,13 +437,22 @@
                 'Events were generated: %s.' % event_list)
         return result
 
+    @contextmanager
+    def noOops(self):
+        oops = errorlog.globalErrorUtility.getLastOopsReport()
+        try:
+            yield
+        finally:
+            self.assertNoNewOops(oops)
+
     def assertNoNewOops(self, old_oops):
         """Assert that no oops has been recorded since old_oops."""
         oops = errorlog.globalErrorUtility.getLastOopsReport()
         if old_oops is None:
             self.assertIs(None, oops)
         else:
-            self.assertEqual(oops.id, old_oops.id)
+            self.assertTrue(
+                oops.id == old_oops.id, 'Oops recorded: %s' % oops.id)
 
     def assertSqlAttributeEqualsDate(self, sql_object, attribute_name, date):
         """Fail unless the value of the attribute is equal to the date.