launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04713
[Merge] lp:~abentley/launchpad/unnecessary-upgrade into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/unnecessary-upgrade into lp:launchpad with lp:~abentley/launchpad/upgrade-not-branch-error as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #823850 in Launchpad itself: "AssertionError raised upgrading a branch that doesn't need upgrade"
https://bugs.launchpad.net/launchpad/+bug/823850
For more details, see:
https://code.launchpad.net/~abentley/launchpad/unnecessary-upgrade/+merge/72480
= 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
--
https://code.launchpad.net/~abentley/launchpad/unnecessary-upgrade/+merge/72480
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/unnecessary-upgrade into lp:launchpad.
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py 2011-08-22 18:19:25 +0000
+++ lib/lp/code/browser/branch.py 2011-08-22 18:19:25 +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:19:25 +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 <eric@xxxxxxxxxxxxxxxxxxxxxxxx>",
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:19:25 +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:19:25 +0000
+++ lib/lp/code/interfaces/branch.py 2011-08-22 18:19:25 +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:19:25 +0000
+++ lib/lp/code/model/branch.py 2011-08-22 18:19:25 +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:19:25 +0000
+++ lib/lp/code/model/branchjob.py 2011-08-22 18:19:25 +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:19:25 +0000
+++ lib/lp/code/model/tests/test_branch.py 2011-08-22 18:19:25 +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
Follow ups
