Merge lp:~abentley/launchpad/unnecessary-upgrade into lp:launchpad

[Aaron Bentley]

Nice error if branch cannot be upgraded.

= Summary =
Fix bug #823850: AssertionError raised upgrading a branch that doesn't need upgrade

== Proposed fix ==
Produce a notification explaining why the branch could not be upgraded.

== Pre-implementation notes ==
None

== Implementation details ==
Extracted the Branch.needs_upgrade logic to Branch.checkUpgrade, so that we can raise exceptions based on the precise checks.

Fixed lint.

== Tests ==
bin/test -t checkUpgrade -t test_upgrade_branch_action_cannot_upgrade

== Demo and Q/A ==
Upgrade a branch, and then immediately request another upgrade. You should get a nice error notification.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/errors.py
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/tests/test_branch.py
  lib/lp/code/interfaces/branch.py
  lib/lp/code/model/branch.py
  lib/lp/code/model/branchjob.py
  lib/lp/code/model/tests/test_branch.py

[Deryck Hodge (deryck)]

Looks good. Thanks for fixing this!

review: Approve (code)

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2011-08-22 18:45:27 +0000
+++ lib/lp/code/browser/branch.py	2011-08-22 18:45:28 +0000
@@ -131,6 +131,7 @@
 from lp.code.errors import (
     BranchCreationForbidden,
     BranchExists,
+    CannotUpgradeBranch,
     CodeImportAlreadyRequested,
     CodeImportAlreadyRunning,
     CodeImportNotInReviewedState,
@@ -993,7 +994,10 @@
 
     @action('Upgrade', name='upgrade_branch')
     def upgrade_branch_action(self, action, data):
-        self.context.requestUpgrade(self.user)
+        try:
+            self.context.requestUpgrade(self.user)
+        except CannotUpgradeBranch as e:
+            self.request.response.addErrorNotification(e)
 
 
 class BranchEditView(BranchEditFormView, BranchNameValidationMixin):

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2011-07-21 02:18:54 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2011-08-22 18:45:28 +0000
@@ -7,12 +7,10 @@
 
 from datetime import (
     datetime,
-    timedelta,
     )
 from textwrap import dedent
 
 import pytz
-import simplejson
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
@@ -41,7 +39,11 @@
     BranchView,
     )
 from lp.code.browser.branchlisting import PersonOwnedBranchesView
-from lp.code.bzr import ControlFormat
+from lp.code.bzr import (
+    BranchFormat,
+    ControlFormat,
+    RepositoryFormat,
+    )
 from lp.code.enums import (
     BranchLifecycleStatus,
     BranchType,
@@ -400,7 +402,7 @@
 
     def _add_revisions(self, branch, nr_revisions=1):
         revisions = []
-        for seq in range(1, nr_revisions+1):
+        for seq in range(1, nr_revisions + 1):
             revision = self.factory.makeRevision(
                 author="Eric the Viking <[e-mail]>",
                 log_body=(
@@ -806,3 +808,23 @@
                 'Some Product.'))
         with person_logged_in(person):
             self.assertEquals(person, branch.owner)
+
+
+class TestBranchUpgradeView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_upgrade_branch_action_cannot_upgrade(self):
+        # A nice error is displayed if a branch cannot be upgraded.
+        branch = self.factory.makePersonalBranch(
+        branch_format=BranchFormat.BZR_BRANCH_6,
+        repository_format=RepositoryFormat.BZR_CHK_2A)
+        login_person(branch.owner)
+        self.addCleanup(logout)
+        branch.requestUpgrade(branch.owner)
+        view = create_initialized_view(branch, '+upgrade')
+        view.upgrade_branch_action.success({})
+        self.assertEqual(1, len(view.request.notifications))
+        self.assertEqual(
+            'An upgrade is already in progress for branch %s.' %
+            branch.bzr_identity, view.request.notifications[0].message)

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2011-06-23 21:09:20 +0000
+++ lib/lp/code/errors.py	2011-08-22 18:45:28 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'AlreadyLatestFormat',
     'BadBranchMergeProposalSearchContext',
     'BadStateTransition',
     'BranchCannotBePrivate',
@@ -20,6 +21,8 @@
     'BuildNotAllowedForDistro',
     'BranchMergeProposalExists',
     'CannotDeleteBranch',
+    'CannotUpgradeBranch',
+    'CannotUpgradeNonHosted',
     'CannotHaveLinkedBranch',
     'CodeImportAlreadyRequested',
     'CodeImportAlreadyRunning',
@@ -36,6 +39,7 @@
     'TooManyBuilds',
     'TooNewRecipeFormat',
     'UnknownBranchTypeError',
+    'UpgradePending',
     'UserHasExistingReview',
     'UserNotBranchReviewer',
     'WrongBranchMergeProposal',
@@ -167,6 +171,36 @@
     _msg_template = "%s cannot have linked branches."
 
 
+class CannotUpgradeBranch(Exception):
+    """"Made for subclassing."""
+
+    def __init__(self, branch):
+        super(CannotUpgradeBranch, self).__init__(
+            self._msg_template % branch.bzr_identity)
+        self.branch = branch
+
+
+class AlreadyLatestFormat(CannotUpgradeBranch):
+    """Raised on attempt to upgrade a branch already in the latest format."""
+
+    _msg_template = (
+        'Branch %s is in the latest format, so it cannot be upgraded.')
+
+
+class CannotUpgradeNonHosted(CannotUpgradeBranch):
+
+    """Raised on attempt to upgrade a non-Hosted branch."""
+
+    _msg_template = 'Cannot upgrade non-hosted branch %s'
+
+
+class UpgradePending(CannotUpgradeBranch):
+
+    """Raised on attempt to upgrade a branch already in the latest format."""
+
+    _msg_template = 'An upgrade is already in progress for branch %s.'
+
+
 class ClaimReviewFailed(Exception):
     """The user cannot claim the pending review."""
 

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2011-08-22 18:45:27 +0000
+++ lib/lp/code/interfaces/branch.py	2011-08-22 18:45:28 +0000
@@ -944,6 +944,12 @@
         :return: A list of tuples like (date, count).
         """
 
+    def checkUpgrade():
+        """Check whether an upgrade should be performed, and raise if not.
+
+        :raises: a `CannotUpgradeBranch`, or a subclass.
+        """
+
     needs_upgrading = Attribute("Whether the branch needs to be upgraded.")
     upgrade_pending = Attribute(
         "Whether a branch has had an upgrade requested.")

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2011-08-22 18:45:27 +0000
+++ lib/lp/code/model/branch.py	2011-08-22 18:45:28 +0000
@@ -87,14 +87,18 @@
     BranchType,
     )
 from lp.code.errors import (
+    AlreadyLatestFormat,
     BranchCannotBePrivate,
     BranchCannotBePublic,
     BranchMergeProposalExists,
     BranchTargetError,
     BranchTypeError,
     CannotDeleteBranch,
+    CannotUpgradeBranch,
+    CannotUpgradeNonHosted,
     InvalidBranchMergeProposal,
     InvalidMergeQueueConfig,
+    UpgradePending,
     )
 from lp.code.event.branchmergeproposal import (
     BranchMergeProposalNeedsReviewEvent,
@@ -1127,16 +1131,25 @@
             DateTrunc(u'day', Revision.revision_date))
         return sorted(results)
 
-    @property
-    def needs_upgrading(self):
-        """See `IBranch`."""
+    def checkUpgrade(self):
         if self.branch_type is not BranchType.HOSTED:
-            return False
+            raise CannotUpgradeNonHosted(self)
         if self.upgrade_pending:
-            return False
-        return not (
+            raise UpgradePending(self)
+        if (
             self.branch_format in CURRENT_BRANCH_FORMATS and
-            self.repository_format in CURRENT_REPOSITORY_FORMATS)
+            self.repository_format in CURRENT_REPOSITORY_FORMATS):
+            raise AlreadyLatestFormat(self)
+
+    @property
+    def needs_upgrading(self):
+        """See `IBranch`."""
+        try:
+            self.checkUpgrade()
+        except CannotUpgradeBranch:
+            return False
+        else:
+            return True
 
     @property
     def upgrade_pending(self):

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2011-08-22 18:45:27 +0000
+++ lib/lp/code/model/branchjob.py	2011-08-22 18:45:28 +0000
@@ -378,10 +378,7 @@
     @classmethod
     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.checkUpgrade()
         branch_job = BranchJob(
             branch, BranchJobType.UPGRADE_BRANCH, {}, requester=requester)
         return cls(branch_job)

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2011-08-22 18:45:27 +0000
+++ lib/lp/code/model/tests/test_branch.py	2011-08-22 18:45:28 +0000
@@ -19,6 +19,7 @@
 import simplejson
 from sqlobject import SQLObjectNotFound
 from storm.locals import Store
+from testtools import ExpectedException
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -56,14 +57,17 @@
     CodeReviewNotificationLevel,
     )
 from lp.code.errors import (
+    AlreadyLatestFormat,
     BranchCannotBePrivate,
     BranchCannotBePublic,
     BranchCreatorNotMemberOfOwnerTeam,
     BranchCreatorNotOwner,
     BranchTargetError,
     CannotDeleteBranch,
+    CannotUpgradeNonHosted,
     InvalidBranchMergeProposal,
     InvalidMergeQueueConfig,
+    UpgradePending,
     )
 from lp.code.interfaces.branch import (
     DEFAULT_BRANCH_STATUS_IN_LISTING,
@@ -586,6 +590,14 @@
         branch = self.factory.makePersonalBranch()
         self.assertFalse(branch.needs_upgrading)
 
+    def test_checkUpgrade_empty_formats(self):
+        branch = self.factory.makePersonalBranch()
+        with ExpectedException(
+            AlreadyLatestFormat,
+            'Branch lp://dev/~person-name.*junk/branch.* is in the latest'
+            ' format, so it cannot be upgraded.'):
+            branch.checkUpgrade()
+
     def test_needsUpgrade_mirrored_branch(self):
         branch = self.factory.makeBranch(
             branch_type=BranchType.MIRRORED,
@@ -593,6 +605,16 @@
             repository_format=RepositoryFormat.BZR_REPOSITORY_4)
         self.assertFalse(branch.needs_upgrading)
 
+    def test_checkUpgrade_mirrored_branch(self):
+        branch = self.factory.makeBranch(
+            branch_type=BranchType.MIRRORED,
+            branch_format=BranchFormat.BZR_BRANCH_6,
+            repository_format=RepositoryFormat.BZR_REPOSITORY_4)
+        with ExpectedException(
+            CannotUpgradeNonHosted,
+            'Cannot upgrade non-hosted branch %s' % branch.bzr_identity):
+            branch.checkUpgrade()
+
     def test_needsUpgrade_remote_branch(self):
         branch = self.factory.makeBranch(
             branch_type=BranchType.REMOTE,
@@ -621,6 +643,20 @@
 
         self.assertFalse(branch.needs_upgrading)
 
+    def test_checkUpgrade_already_requested(self):
+        branch = self.factory.makePersonalBranch(
+            branch_format=BranchFormat.BZR_BRANCH_6,
+            repository_format=RepositoryFormat.BZR_CHK_2A)
+        owner = removeSecurityProxy(branch).owner
+        login_person(owner)
+        self.addCleanup(logout)
+        branch.requestUpgrade(branch.owner)
+        with ExpectedException(
+            UpgradePending,
+            'An upgrade is already in progress for branch'
+            ' lp://dev/~person-name.*junk/branch.*.'):
+            branch.checkUpgrade()
+
     def test_needsUpgrading_branch_format_unrecognized(self):
         # A branch has a needs_upgrading attribute that returns whether or not
         # a branch needs to be upgraded or not.  If the format is
@@ -639,6 +675,17 @@
             repository_format=RepositoryFormat.BZR_CHK_2A)
         self.assertFalse(branch.needs_upgrading)
 
+    def test_checkUpgrade_branch_format_upgrade_not_needed(self):
+        # If a branch is up-to-date, checkUpgrade raises AlreadyLatestFormat
+        branch = self.factory.makePersonalBranch(
+            branch_format=BranchFormat.BZR_BRANCH_8,
+            repository_format=RepositoryFormat.BZR_CHK_2A)
+        with ExpectedException(
+            AlreadyLatestFormat,
+            'Branch lp://dev/~person-name.*junk/branch.* is in the latest'
+            ' format, so it cannot be upgraded.'):
+            branch.checkUpgrade()
+
     def test_needsUpgrading_branch_format_upgrade_needed(self):
         # A branch has a needs_upgrading attribute that returns whether or not
         # a branch needs to be upgraded or not.  If a branch doesn't support
@@ -691,18 +738,19 @@
 
     def test_requestUpgrade_no_upgrade_needed(self):
         # If a branch doesn't need to be upgraded, requestUpgrade raises an
-        # AssertionError.
+        # AlreadyLatestFormat.
         branch = self.factory.makeAnyBranch(
             branch_format=BranchFormat.BZR_BRANCH_8,
             repository_format=RepositoryFormat.BZR_CHK_2A)
         owner = removeSecurityProxy(branch).owner
         login_person(owner)
         self.addCleanup(logout)
-        self.assertRaises(AssertionError, branch.requestUpgrade, branch.owner)
+        self.assertRaises(
+            AlreadyLatestFormat, branch.requestUpgrade, branch.owner)
 
     def test_requestUpgrade_upgrade_pending(self):
         # If there is a pending upgrade already requested, requestUpgrade
-        # raises an AssertionError.
+        # raises an UpgradePending.
         branch = self.factory.makeAnyBranch(
             branch_format=BranchFormat.BZR_BRANCH_6)
         owner = removeSecurityProxy(branch).owner
@@ -710,7 +758,7 @@
         self.addCleanup(logout)
         branch.requestUpgrade(branch.owner)
 
-        self.assertRaises(AssertionError, branch.requestUpgrade, branch.owner)
+        self.assertRaises(UpgradePending, branch.requestUpgrade, branch.owner)
 
     def test_upgradePending(self):
         # If there is a BranchUpgradeJob pending for the branch, return True.

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2011-08-22 18:45:27 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2011-08-22 18:45:28 +0000
@@ -52,6 +52,7 @@
     BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel,
     )
+from lp.code.errors import AlreadyLatestFormat
 from lp.code.interfaces.branchjob import (
     IBranchDiffJob,
     IBranchJob,
@@ -307,6 +308,8 @@
             'Bazaar-NG Knit Repository Format 1')
 
         job = BranchUpgradeJob.create(db_branch, self.factory.makePerson())
+
+        dbuser = config.launchpad.dbuser
         self.becomeDbUser(config.upgrade_branches.dbuser)
         with TransactionFreeOperation.require():
             job.run()
@@ -315,16 +318,17 @@
             new_branch.repository._format.get_format_string(),
             'Bazaar repository format 2a (needs bzr 1.16 or later)\n')
 
+        self.becomeDbUser(dbuser)
         self.assertFalse(db_branch.needs_upgrading)
 
     def test_needs_no_upgrading(self):
-        # Branch upgrade job creation should raise an AssertionError if the
-        # branch does not need to be upgraded.
+        # Branch upgrade job creation should raise an AlreadyLatestFormat if
+        # the branch does not need to be upgraded.
         branch = self.factory.makeAnyBranch(
             branch_format=BranchFormat.BZR_BRANCH_7,
             repository_format=RepositoryFormat.BZR_CHK_2A)
         self.assertRaises(
-            AssertionError, BranchUpgradeJob.create, branch,
+            AlreadyLatestFormat, BranchUpgradeJob.create, branch,
             self.factory.makePerson())
 
     def create_knit(self):