On Mon, 03 Aug 2009 17:54:59 Michael Hudson wrote: > Review: Approve > I think, on reviewing this branch, that I'd rather you'd have done the > setOwner thing and moveBranch thing in separate branches, if that > would have been possible. Oh well! Probably, but it wasn't until I realised what was needed that it got done together. I guess I could have worked harder to separate the bits. Perhaps next time :) > > === modified file > > 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py' --- > > lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2009-07-17 > > 00:26:05 +0000 +++ > > lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2009-08-03 > > 03:36:16 +0000 @@ -75,6 +75,11 @@ > > IBranch['linkSpecification'].queryTaggedValue( > > LAZR_WEBSERVICE_EXPORTED)['params']['spec'].schema= ISpecification > > IBranch['product'].schema = IProduct > > +IBranch['setTarget'].queryTaggedValue( > > + LAZR_WEBSERVICE_EXPORTED)['params']['project'].schema= IProduct > > +IBranch['setTarget'].queryTaggedValue( > > + LAZR_WEBSERVICE_EXPORTED)['params']['source_package'].schema= \ > > + ISourcePackage > > IBranch['spec_links'].value_type.schema = ISpecificationBranch > > IBranch['subscribe'].queryTaggedValue( > > LAZR_WEBSERVICE_EXPORTED)['return_type'].schema = > > IBranchSubscription > > I guess once the lp.foo dependencies are properly sorted out, we won't > need this kind of rubbish so much any more.... Unlikely, as IProduct imports some bits that need IBranch, so we have the classical circular dependency. > > === modified file 'lib/canonical/launchpad/webapp/launchpadform.py' > > --- lib/canonical/launchpad/webapp/launchpadform.py 2009-06-25 05:30:52 > > +0000 +++ lib/canonical/launchpad/webapp/launchpadform.py 2009-08-03 > > 03:36:16 +0000 @@ -372,7 +372,7 @@ > > > > render_context = True > > > > - def updateContextFromData(self, data, context=None): > > + def updateContextFromData(self, data, context=None, > > notify_modified=True): """Update the context object based on form data. > > > > If no context is given, the view's context is used. > > It's micro-optimization, but I don't think there's any reason to take > the snapshot if you're not going to use it. Added. > > === modified file 'lib/lp/code/browser/branch.py' > > --- lib/lp/code/browser/branch.py 2009-07-19 04:41:14 +0000 > > +++ lib/lp/code/browser/branch.py 2009-08-03 03:36:16 +0000 > > @@ -36,12 +36,15 @@ > > from zope.app.form.browser import TextAreaWidget > > from zope.traversing.interfaces import IPathAdapter > > from zope.component import getUtility, queryAdapter > > +from zope.event import notify > > from zope.formlib import form > > -from zope.interface import Interface, implements > > +from zope.interface import Interface, implements, providedBy > > from zope.publisher.interfaces import NotFound > > from zope.schema import Choice, Text > > from lazr.delegates import delegates > > from lazr.enum import EnumeratedType, Item > > +from lazr.lifecycle.event import ObjectModifiedEvent > > +from lazr.lifecycle.snapshot import Snapshot > > from lazr.uri import URI > > > > from canonical.cachedproperty import cachedproperty > > @@ -574,7 +577,6 @@ > > the user to be able to edit it. > > """ > > use_template(IBranch, include=[ > > - 'owner', > > 'name', > > 'url', > > 'description', > > @@ -582,6 +584,7 @@ > > 'whiteboard', > > ]) > > private = copy_field(IBranch['private'], readonly=False) > > + owner = copy_field(IBranch['owner'], readonly=False) > > > > > > class BranchEditFormView(LaunchpadEditFormView): > > @@ -598,9 +601,12 @@ > > @action('Change Branch', name='change') > > def change_action(self, action, data): > > # If the owner or product has changed, add an explicit > > notification. + branch_before_modification = Snapshot( > > + self.context, providing=providedBy(self.context)) > > I think a quick commet here on why we're doing the snapshot ourself > would be nice here. Comment added. > > === modified file > > 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py' --- > > lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2009-07-17 > > 00:26:05 +0000 +++ > > lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2009-08-03 > > 03:36:16 +0000 @@ -8,14 +8,15 @@ > > from unittest import TestLoader > > > > import transaction > > +from zope.security.proxy import removeSecurityProxy > > > > +from canonical.launchpad.webapp.servers import LaunchpadTestRequest > > +from canonical.testing import DatabaseFunctionalLayer > > from lp.code.browser.branchmergeproposallisting import ( > > ActiveReviewsView, BranchMergeProposalListingView, > > PersonActiveReviewsView, ProductActiveReviewsView) > > from lp.code.enums import CodeReviewVote > > from lp.testing import ANONYMOUS, login, login_person, > > TestCaseWithFactory -from canonical.launchpad.webapp.servers import > > LaunchpadTestRequest -from canonical.testing import > > DatabaseFunctionalLayer > > > > _default = object() > > > > @@ -201,8 +202,7 @@ > > self.assertReviewGroupForUser( > > self.bmp.registrant, self._view.OTHER) > > team = self.factory.makeTeam(self.bmp.registrant) > > - login_person(self.bmp.source_branch.owner) > > - self.bmp.source_branch.owner = team > > + removeSecurityProxy(self.bmp.source_branch).owner = team > > Why not use setOwner here? Because setOwner does membership and policy checks, and in this test we don't care, we just want the branch in a particular state. > > self.assertReviewGroupForUser( > > self.bmp.registrant, ActiveReviewsView.MINE) > > > > > > === modified file 'lib/lp/code/interfaces/branch.py' > > --- lib/lp/code/interfaces/branch.py 2009-07-29 03:44:05 +0000 > > +++ lib/lp/code/interfaces/branch.py 2009-08-03 03:36:16 +0000 > > @@ -116,6 +122,10 @@ > > """Raised when the user specifies an unrecognized branch type.""" > > > > > > +class UnknownBranchTargetError(Exception): > > + """Raised when an unexpected type is passed through as a branch > > target.""" > > This class isn't used any more. Removed. > > === modified file 'lib/lp/code/model/branch.py' > > --- lib/lp/code/model/branch.py 2009-07-19 04:41:14 +0000 > > +++ lib/lp/code/model/branch.py 2009-08-03 03:36:16 +0000 > > @@ -57,7 +57,7 @@ > > from lp.code.event.branchmergeproposal import > > NewBranchMergeProposalEvent from lp.code.interfaces.branch import ( > > bazaar_identity, BranchCannotBePrivate, BranchCannotBePublic, > > - BranchTypeError, CannotDeleteBranch, > > + BranchTargetError, BranchTypeError, CannotDeleteBranch, > > DEFAULT_BRANCH_STATUS_IN_LISTING, IBranch, > > IBranchNavigationMenu, IBranchSet) > > from lp.code.interfaces.branchcollection import IAllBranches > > @@ -71,7 +71,7 @@ > > from lp.code.interfaces.seriessourcepackagebranch import ( > > IFindOfficialBranchLinks) > > from lp.registry.interfaces.person import ( > > - validate_person_not_private_membership, validate_public_person) > > + IPerson, validate_person_not_private_membership, > > validate_public_person) > > > > > > class Branch(SQLBase): > > @@ -113,6 +113,12 @@ > > owner = ForeignKey( > > dbName='owner', foreignKey='Person', > > storm_validator=validate_person_not_private_membership, > > notNull=True) + > > + def setOwner(self, new_owner, user): > > + """See `IBranch`.""" > > + new_namespace = self.target.getNamespace(new_owner) > > + new_namespace.moveBranch(self, user, rename_if_necessary=True) > > + > > reviewer = ForeignKey( > > dbName='reviewer', foreignKey='Person', > > storm_validator=validate_public_person, default=None) > > @@ -162,6 +168,28 @@ > > target = self.product > > return IBranchTarget(target) > > > > + def setTarget(self, user, project=None, source_package=None): > > + """See `IBranch`.""" > > + if project is not None: > > + if source_package is not None: > > + raise BranchTargetError( > > + 'Cannot specify both a project and a source > > package.') + else: > > + target = IBranchTarget(project) > > + if target is None: > > + raise BranchTargetError( > > + '%r is not a valid project target' % project) > > + elif source_package is not None: > > + target = IBranchTarget(source_package) > > + if target is None: > > + raise BranchTargetError( > > + '%r is not a valid source package target' % > > source_package) + else: > > + target = IBranchTarget(self.owner) > > + # Person targets are always valid. > > + namespace = target.getNamespace(self.owner) > > + namespace.moveBranch(self, user, rename_if_necessary=True) > > I guess it won't work through the API, but I looks like you could say > branch.setTarget(product=some_source_package) and it would act the > same as branch.setTarget(source_package=some_source_package). > > I also don't understand when you expect BranchTargetError exceptions > to be raised (apart from the both non-None case). The docstring > doesn't say, and it's not tested. Hmm.. added some bits to the docstring and another test. > > @property > > def namespace(self): > > """See `IBranch`.""" > > > > === modified file 'lib/lp/code/model/branchtarget.py' > > --- lib/lp/code/model/branchtarget.py 2009-07-17 00:26:05 +0000 > > +++ lib/lp/code/model/branchtarget.py 2009-08-03 03:36:16 +0000 > > @@ -13,7 +13,8 @@ > > > > from zope.component import getUtility > > from zope.interface import implements > > -from zope.security.proxy import isinstance as zope_isinstance > > +from zope.security.proxy import ( > > + removeSecurityProxy, isinstance as zope_isinstance) > > > > from lp.code.interfaces.branchcollection import IAllBranches > > from lp.code.interfaces.branchtarget import ( > > @@ -128,6 +129,16 @@ > > # those cases. > > return bug.default_bugtask > > > > + def retargetBranch(self, branch): > > + """See `IBranchTarget`.""" > > + # Since product, distroseries and sourcepackagename are not > > writable + # as defined by the interface, we need to rip off the > > security proxy + # here. > > + naked_branch = removeSecurityProxy(branch) > > + naked_branch.product = None > > + naked_branch.distroseries = self.sourcepackage.distroseries > > + naked_branch.sourcepackagename = > > self.sourcepackage.sourcepackagename > > I notice this method is actually only called with already naked > branches. So I think that should be a requirement. Changed. > Also, I think that the confusion of setTarget/moveBranch > /retargetBranch is can be helped a little by very firmly relegating > retargetBranch to the realms of implementation detail: rename it > _retargetBranch and take it out of the interface. What do you think? Yeah, good idea. Done. > > === modified file 'lib/lp/code/model/tests/test_branch.py' > > --- lib/lp/code/model/tests/test_branch.py 2009-07-23 02:06:55 +0000 > > +++ lib/lp/code/model/tests/test_branch.py 2009-08-03 03:36:16 +0000 > > @@ -39,6 +39,7 @@ > > BranchVisibilityRule, CodeReviewNotificationLevel) > > from lp.code.interfaces.branch import ( > > BranchCannotBePrivate, BranchCannotBePublic, > > + BranchCreatorNotMemberOfOwnerTeam, BranchCreatorNotOwner, > > CannotDeleteBranch, DEFAULT_BRANCH_STATUS_IN_LISTING) > > from lp.code.interfaces.branchlookup import IBranchLookup > > from lp.code.interfaces.branchnamespace import IBranchNamespaceSet > > @@ -213,8 +214,7 @@ > > # attribute is updated too. > > branch = self.factory.makeAnyBranch() > > new_owner = self.factory.makePerson() > > - login('