Code review comment for lp:~rockstar/launchpad/upgrade-job-2.0-changes

Hrm, this diff seems confused. It has this branch's changes as well as the next branch's changes... Here's the real diff:

=== modified file 'lib/lp/code/bzr.py'
--- lib/lp/code/bzr.py 2009-06-30 16:56:07 +0000
+++ lib/lp/code/bzr.py 2010-01-16 07:38:47 +0000
@@ -8,6 +8,8 @@
     'BranchFormat',
     'BRANCH_FORMAT_UPGRADE_PATH',
     'ControlFormat',
+ 'CURRENT_BRANCH_FORMATS',
+ 'CURRENT_REPOSITORY_FORMATS',
     'RepositoryFormat',
     'REPOSITORY_FORMAT_UPGRADE_PATH',
     ]
@@ -222,6 +224,36 @@
     BZR_METADIR_1 = _format_enum(1, BzrDirMetaFormat1)

+# A tuple of branch formats that should not suggest upgrading.
+CURRENT_BRANCH_FORMATS = (
+ BranchFormat.UNRECOGNIZED,
+ BranchFormat.BRANCH_REFERENCE,
+ BranchFormat.BZR_BRANCH_8,
+ BranchFormat.BZR_LOOM_1,
+ BranchFormat.BZR_LOOM_2,
+ BranchFormat.BZR_LOOM_3,
+)
+
+# A tuple of repository formats that should not suggest upgrading.
+CURRENT_REPOSITORY_FORMATS = (
+ RepositoryFormat.UNRECOGNIZED,
+ RepositoryFormat.BZR_KNITPACK_3,
+ RepositoryFormat.BZR_KNITPACK_5,
+ RepositoryFormat.BZR_KNITPACK_5_RR,
+ RepositoryFormat.BZR_KNITPACK_6,
+ RepositoryFormat.BZR_KNITPACK_6_RR,
+ RepositoryFormat.BZR_PACK_DEV_0,
+ RepositoryFormat.BZR_PACK_DEV_0_SUBTREE,
+ RepositoryFormat.BZR_DEV_1,
+ RepositoryFormat.BZR_DEV_1_SUBTREE,
+ RepositoryFormat.BZR_DEV_2,
+ RepositoryFormat.BZR_DEV_2_SUBTREE,
+ RepositoryFormat.BZR_CHK1,
+ RepositoryFormat.BZR_CHK2,
+ RepositoryFormat.BZR_CHK_2A
+
+)
+
 BRANCH_FORMAT_UPGRADE_PATH = {
     BranchFormat.UNRECOGNIZED: None,
     BranchFormat.BRANCH_REFERENCE: None,

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2010-01-06 14:14:35 +0000
+++ lib/lp/code/configure.zcml 2010-01-18 01:19:38 +0000
@@ -487,6 +487,7 @@
                     pending_writes
                     commitsForDays
                     needs_upgrading
+ upgrade_pending
                     requestUpgrade
                     getUpgradeFormat
                     isBranchMergeable

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2010-01-06 12:15:42 +0000
+++ lib/lp/code/interfaces/branch.py 2010-01-18 01:19:39 +0000
@@ -1057,6 +1057,8 @@
         """

     needs_upgrading = Attribute("Whether the branch needs to be upgraded.")
+ upgrade_pending = Attribute(
+ "Whether a branch has had an upgrade requested.")

     def requestUpgrade():
         """Create an IBranchUpgradeJob to upgrade this branch."""

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2009-12-11 00:56:16 +0000
+++ lib/lp/code/model/branch.py 2010-01-19 21:57:57 +0000
@@ -42,7 +42,8 @@
     IStoreSelector, MAIN_STORE, SLAVE_FLAVOR)

 from lp.code.bzr import (
- BranchFormat, BRANCH_FORMAT_UPGRADE_PATH, ControlFormat, RepositoryFormat,
+ BranchFormat, BRANCH_FORMAT_UPGRADE_PATH, ControlFormat,
+ CURRENT_BRANCH_FORMATS, CURRENT_REPOSITORY_FORMATS, RepositoryFormat,
     REPOSITORY_FORMAT_UPGRADE_PATH)
 from lp.code.enums import (
     BranchLifecycleStatus, BranchMergeControlStatus,
@@ -991,10 +992,22 @@
     @property
     def needs_upgrading(self):
         """See `IBranch`."""
- if (REPOSITORY_FORMAT_UPGRADE_PATH.get(self.repository_format, None)
- or BRANCH_FORMAT_UPGRADE_PATH.get(self.branch_format, None)):
- return True
- return False
+ if self.upgrade_pending:
+ return False
+ return not (
+ self.branch_format in CURRENT_BRANCH_FORMATS and
+ self.repository_format in CURRENT_REPOSITORY_FORMATS)
+
+ @property
+ def upgrade_pending(self):
+ """See `IBranch`."""
+ from lp.code.model.branchjob import BranchJob, BranchJobType
+ store = Store.of(self)
+ jobs = store.find(
+ BranchJob,
+ BranchJob.branch == self,
+ BranchJob.job_type == BranchJobType.UPGRADE_BRANCH)
+ return jobs.count() > 0

     def requestUpgrade(self):
         """See `IBranch`."""

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py 2010-01-15 01:20:20 +0000
+++ lib/lp/code/model/branchjob.py 2010-01-16 08:33:16 +0000
@@ -264,6 +264,8 @@
         """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, {})
         return cls(branch_job)

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2010-01-07 06:37:14 +0000
+++ lib/lp/code/model/tests/test_branch.py 2010-01-19 22:00:02 +0000
@@ -315,24 +315,33 @@
             SourcePackage(branch.sourcepackagename, branch.distroseries),
             branch.sourcepackage)

+ def test_needsUpgrading_already_requested(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
+ # unrecognized, we don't try to upgrade it.
+ branch = self.factory.makePersonalBranch(
+ branch_format=BranchFormat.BZR_BRANCH_6,
+ repository_format=RepositoryFormat.BZR_CHK_2A)
+ branch.requestUpgrade()
+
+ self.assertFalse(branch.needs_upgrading)
+
     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
         # unrecognized, we don't try to upgrade it.
         branch = self.factory.makePersonalBranch(
- branch_format=BranchFormat.UNRECOGNIZED)
+ branch_format=BranchFormat.UNRECOGNIZED,
+ repository_format=RepositoryFormat.BZR_CHK_2A)
         self.assertFalse(branch.needs_upgrading)

     def test_needsUpgrading_branch_format_upgrade_not_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 is up to date, it
         # doesn't need to be upgraded.
- #
- # XXX: JonathanLange 2009-06-06: This test needs to be changed every
- # time Bazaar adds a new branch format. Surely we can think of a
- # better way of testing this?
         branch = self.factory.makePersonalBranch(
- branch_format=BranchFormat.BZR_BRANCH_8)
+ branch_format=BranchFormat.BZR_BRANCH_8,
+ repository_format=RepositoryFormat.BZR_CHK_2A)
         self.assertFalse(branch.needs_upgrading)

     def test_needsUpgrading_branch_format_upgrade_needed(self):
@@ -340,7 +349,8 @@
         # a branch needs to be upgraded or not. If a branch doesn't support
         # stacking, it needs to be upgraded.
         branch = self.factory.makePersonalBranch(
- branch_format=BranchFormat.BZR_BRANCH_6)
+ branch_format=BranchFormat.BZR_BRANCH_6,
+ repository_format=RepositoryFormat.BZR_CHK_2A)
         self.assertTrue(branch.needs_upgrading)

     def test_needsUpgrading_repository_format_unrecognized(self):
@@ -348,6 +358,7 @@
         # a branch needs to be upgraded or not. In the repo format is
         # unrecognized, we don't try to upgrade it.
         branch = self.factory.makePersonalBranch(
+ branch_format=BranchFormat.BZR_BRANCH_8,
             repository_format=RepositoryFormat.UNRECOGNIZED)
         self.assertFalse(branch.needs_upgrading)

@@ -356,6 +367,7 @@
         # branch needs to be upgraded or not. If the repo format is up to
         # date, there's no need to upgrade it.
         branch = self.factory.makePersonalBranch(
+ branch_format=BranchFormat.BZR_BRANCH_8,
             repository_format=RepositoryFormat.BZR_KNITPACK_6)
         self.assertFalse(branch.needs_upgrading)

@@ -364,6 +376,7 @@
         # branch needs to be upgraded or not. If the format doesn't support
         # stacking, it needs to be upgraded.
         branch = self.factory.makePersonalBranch(
+ branch_format=BranchFormat.BZR_BRANCH_8,
             repository_format=RepositoryFormat.BZR_REPOSITORY_4)
         self.assertTrue(branch.needs_upgrading)

@@ -378,6 +391,37 @@
             jobs,
             [job,])

+ def test_requestUpgrade_no_upgrade_needed(self):
+ # If a branch doesn't need to be upgraded, requestUpgrade raises an
+ # AssertionError.
+ branch = self.factory.makeAnyBranch(
+ branch_format=BranchFormat.BZR_BRANCH_8,
+ repository_format=RepositoryFormat.BZR_CHK_2A)
+ self.assertRaises(AssertionError, branch.requestUpgrade)
+
+ def test_requestUpgrade_upgrade_pending(self):
+ # If there is a pending upgrade already requested, requestUpgrade
+ # raises an AssertionError.
+ branch = self.factory.makeAnyBranch(
+ branch_format=BranchFormat.BZR_BRANCH_6)
+ branch.requestUpgrade()
+
+ self.assertRaises(AssertionError, branch.requestUpgrade)
+
+ def test_upgradePending(self):
+ # If there is a BranchUpgradeJob pending for the branch, return True.
+ branch = self.factory.makeAnyBranch(
+ branch_format=BranchFormat.BZR_BRANCH_6)
+ branch.requestUpgrade()
+
+ self.assertTrue(branch.upgrade_pending)
+
+ def test_upgradePending_no_upgrade_requested(self):
+ # If the branch never had an upgrade requested, return False.
+ branch = self.factory.makeAnyBranch()
+
+ self.assertFalse(branch.upgrade_pending)
+

 class TestBzrIdentity(TestCaseWithFactory):
     """Test IBranch.bzr_identity."""