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! Approved, subject to hiding retargetBranch a little harder. Comments below. Cheers, mwh > === 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.... > === 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. > @@ -391,7 +391,7 @@ > > was_changed = form.applyChanges(context, self.form_fields, > data, self.adapters) > - if was_changed: > + if was_changed and notify_modified: > field_names = [form_field.__name__ > for form_field in self.form_fields] > notify(ObjectModifiedEvent( > === 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. > if 'owner' in data: > - new_owner = data['owner'] > + new_owner = data.pop('owner') > if new_owner != self.context.owner: > + self.context.setOwner(new_owner, self.user) > self.request.response.addNotification( > "The branch owner has been changed to %s (%s)" > % (new_owner.displayname, new_owner.name)) > @@ -616,7 +622,13 @@ > else: > self.request.response.addNotification( > "The branch is now publicly accessible.") > - if self.updateContextFromData(data): > + if self.updateContextFromData(data, notify_modified=False): > + # Notify the object has changed with the snapshot that was taken > + # earler. > + field_names = [ > + form_field.__name__ for form_field in self.form_fields] > + notify(ObjectModifiedEvent( > + self.context, branch_before_modification, field_names)) > # Only specify that the context was modified if there > # was in fact a change. > self.context.date_last_modified = UTC_NOW > === 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? > 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 > @@ -19,6 +19,7 @@ > 'BranchCreatorNotMemberOfOwnerTeam', > 'BranchCreatorNotOwner', > 'BranchExists', > + 'BranchTargetError', > 'BranchTypeError', > 'CannotDeleteBranch', > 'DEFAULT_BRANCH_STATUS_IN_LISTING', > @@ -31,6 +32,7 @@ > 'IBranchNavigationMenu', > 'IBranchSet', > 'NoSuchBranch', > + 'UnknownBranchTargetError', > 'UnknownBranchTypeError', > 'user_has_special_branch_access', > ] > @@ -90,7 +92,7 @@ > """Raised when creating a branch that already exists.""" > > def __init__(self, existing_branch): > - # XXX: JonathanLange 2008-12-04 spec=package-branches: This error > + # XXX: TimPenhey 2009-07-12 bug=405214: This error > # message logic is incorrect, but the exact text is being tested > # in branch-xmlrpc.txt. I don't really buy this comment -- I don't see why you can't preserve the existing error but do something different for the source package case, it's not like branch-xmlrpc.txt is testing the source package case (or can't be changed). But still, not today, thanks for the more accurate bug reference :) > params = {'name': existing_branch.name} > @@ -108,6 +110,10 @@ > BranchCreationException.__init__(self, message) > > > +class BranchTargetError(Exception): > + """Raised when there is an error determining a branch target.""" > + > + > class CannotDeleteBranch(Exception): > """The branch cannot be deleted at this time.""" > > @@ -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. > class BranchCreationForbidden(BranchCreationException): > """A Branch visibility policy forbids branch creation. > > @@ -402,14 +412,41 @@ > title=_("The user that registered the branch."), > required=True, readonly=True, > vocabulary='ValidPersonOrTeam')) > + > owner = exported( > ParticipatingPersonChoice( > title=_('Owner'), > - required=True, > + required=True, readonly=True, > vocabulary='UserTeamsParticipationPlusSelf', > description=_("Either yourself or a team you are a member of. " > "This controls who can modify the branch."))) > > + @call_with(user=REQUEST_USER) > + @operation_parameters( > + new_owner=Reference( > + title=_("The new owner of the branch."), > + schema=IPerson)) > + @export_write_operation() > + def setOwner(new_owner, user): > + """Set the owner of the branch to be `new_owner`.""" > + > + @call_with(user=REQUEST_USER) > + @operation_parameters( > + project=Reference( > + title=_("The project the branch belongs to."), > + schema=Interface, required=False), # Really IProduct > + source_package=Reference( > + title=_("The source package the branch belongs to."), > + schema=Interface, required=False)) # Really ISourcePackage > + @export_write_operation() > + def setTarget(user, project=None, source_package=None): > + """Set the target of the branch to be `project` or `source_package`. > + > + Only one of `project` or `source_package` can be set, and if neither > + is set, the branch gets moved into the junk namespace of the branch > + owner. > + """ > + > reviewer = exported( > PublicPersonChoice( > title=_('Default Review Team'), > @@ -456,7 +493,7 @@ > product = exported( > ReferenceChoice( > title=_('Project'), > - required=False, > + required=False, readonly=True, > vocabulary='Product', > schema=Interface, > description=_("The project this branch belongs to.")), > === modified file 'lib/lp/code/interfaces/branchnamespace.py' > --- lib/lp/code/interfaces/branchnamespace.py 2009-06-25 04:06:00 +0000 > +++ lib/lp/code/interfaces/branchnamespace.py 2009-08-03 03:36:16 +0000 > @@ -70,6 +70,24 @@ > def isNameUsed(name): > """Is 'name' already used in this namespace?""" > > + def moveBranch(branch, mover, new_name=None, rename_if_necessary=False): > + """Move the branch into this namespace. > + > + :param branch: The `IBranch` to move. > + :param mover: The `IPerson` doing the moving. > + :param new_name: A new name for the branch. > + :param rename_if_necessary: Rename the branch if the branch name > + exists already in this namespace. > + :raises BranchCreatorNotMemberOfOwnerTeam: if the namespace owner is > + a team, and 'mover' is not in that team. > + :raises BranchCreatorNotOwner: if the namespace owner is an individual > + and 'mover' is not the owner. > + :raises BranchCreationForbidden: if 'mover' is not allowed to create > + a branch in this namespace due to privacy rules. > + :raises BranchExists: if a branch with the 'name' exists already in > + the namespace, and 'rename_if_necessary' is False. > + """ > + > > class IBranchNamespacePolicy(Interface): > """Methods relating to branch creation and validation.""" > @@ -133,11 +151,13 @@ > validation constraints on IBranch.name. > """ > > - def validateMove(branch, mover): > + def validateMove(branch, mover, name=None): > """Check that 'mover' can move 'branch' into this namespace. > > :param branch: An `IBranch` that might be moved. > :param mover: The `IPerson` who would move it. > + :param name: A new name for the branch. If None, the branch name is > + used. > :raises BranchCreatorNotMemberOfOwnerTeam: if the namespace owner is > a team, and 'mover' is not in that team. > :raises BranchCreatorNotOwner: if the namespace owner is an individual > === modified file 'lib/lp/code/interfaces/branchtarget.py' > --- lib/lp/code/interfaces/branchtarget.py 2009-07-17 00:26:05 +0000 > +++ lib/lp/code/interfaces/branchtarget.py 2009-08-03 03:36:16 +0000 > @@ -113,3 +113,12 @@ > > def getBugTask(bug): > """Get the BugTask for a given bug related to the branch target.""" > + > + def retargetBranch(branch): > + """Set the branch target to refer to this target. > + > + This only updates the target related attributes of the branch. > + > + No name clashes are checked, nor privacy policies. Those are handled > + by the IBranchNamespace.moveBranch method. > + """ I'm not really sure I like having this method in the interface. More on this topic below. > === 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. > @property > def namespace(self): > """See `IBranch`.""" > === modified file 'lib/lp/code/model/branchnamespace.py' > --- lib/lp/code/model/branchnamespace.py 2009-06-25 04:06:00 +0000 > +++ lib/lp/code/model/branchnamespace.py 2009-08-03 03:36:16 +0000 > @@ -16,6 +16,7 @@ > from zope.component import getUtility > from zope.event import notify > from zope.interface import implements > +from zope.security.proxy import removeSecurityProxy > > from lazr.lifecycle.event import ObjectCreatedEvent > from storm.locals import And > @@ -160,6 +161,25 @@ > self.validateBranchName(name) > self.validateRegistrant(mover) > > + def moveBranch(self, branch, mover, new_name=None, > + rename_if_necessary=False): > + """See `IBranchNamespace`.""" > + # Check to see if the branch is already in this namespace. > + old_namespace = branch.namespace > + if self.name == old_namespace.name: > + return > + if new_name is None: > + new_name = branch.name > + if rename_if_necessary: > + new_name = self.findUnusedName(new_name) > + self.validateMove(branch, mover, new_name) > + # Remove the security proxy of the branch as the owner and target > + # attributes are readonly through the interface. > + naked_branch = removeSecurityProxy(branch) > + naked_branch.owner = self.owner > + self.target.retargetBranch(naked_branch) > + naked_branch.name = new_name > + > def createBranchWithPrefix(self, branch_type, prefix, registrant, > url=None): > """See `IBranchNamespace`.""" > === 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. 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? > > class PersonBranchTarget(_BaseBranchTarget): > implements(IBranchTarget) > @@ -182,6 +193,16 @@ > """See `IBranchTarget`.""" > 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 = None > + naked_branch.sourcepackagename = None > + > > class ProductBranchTarget(_BaseBranchTarget): > implements(IBranchTarget) > @@ -265,6 +286,16 @@ > task = bug.bugtasks[0] > return task > > + 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 = self.product > + naked_branch.distroseries = None > + naked_branch.sourcepackagename = None > + > > def get_canonical_url_data_for_target(branch_target): > """Return the `ICanonicalUrlData` for an `IBranchTarget`.""" > === 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('