Merge lp:~thumper/launchpad/branch-move-methods into lp:launchpad
- branch-move-methods
- Merge into devel
Status: | Merged |
---|---|
Merged at revision: | not available |
Proposed branch: | lp:~thumper/launchpad/branch-move-methods |
Merge into: | lp:launchpad |
Diff against target: | None lines |
To merge this branch: | bzr merge lp:~thumper/launchpad/branch-move-methods |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Hudson-Doyle | Approve | ||
Review via email: mp+9559@code.launchpad.net |
Commit message
Description of the change
Tim Penhey (thumper) wrote : | # |
Michael Hudson-Doyle (mwhudson) wrote : | # |
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/
> --- lib/canonical/
> +++ lib/canonical/
> @@ -75,6 +75,11 @@
> IBranch[
> LAZR_WEBSERVICE
> IBranch[
> +IBranch[
> + LAZR_WEBSERVICE
> +IBranch[
> + LAZR_WEBSERVICE
> + ISourcePackage
> IBranch[
> IBranch[
> LAZR_WEBSERVICE
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/
> --- lib/canonical/
> +++ lib/canonical/
> @@ -372,7 +372,7 @@
>
> render_context = True
>
> - def updateContextFr
> + def updateContextFr
> """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.applyChang
> data, self.adapters)
> - if was_changed:
> + if was_changed and notify_modified:
> field_names = [form_field.
> for form_field in self.form_fields]
> notify(
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -36,12 +36,15 @@
> from zope.app.
> from zope.traversing
> 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.
> from zope.schema import Choice, Text
> from lazr.delegates import delegates
> from lazr.enum import EnumeratedType, Item
> +from lazr.lifecycle.
> +from lazr.lifecycle.
> from lazr.uri import URI
>
> from...
Tim Penhey (thumper) wrote : | # |
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/
> > lib/canonical/
> > 00:26:05 +0000 +++
> > lib/canonical/
> > 03:36:16 +0000 @@ -75,6 +75,11 @@
> > IBranch[
> > LAZR_WEBSERVICE
> > IBranch[
> > +IBranch[
> > + LAZR_WEBSERVICE
> > +IBranch[
> > + LAZR_WEBSERVICE
> > + ISourcePackage
> > IBranch[
> > IBranch[
> > LAZR_WEBSERVICE
> > 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/
> > --- lib/canonical/
> > +0000 +++ lib/canonical/
> > 03:36:16 +0000 @@ -372,7 +372,7 @@
> >
> > render_context = True
> >
> > - def updateContextFr
> > + def updateContextFr
> > notify_
> >
> > 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/
> > --- lib/lp/
> > +++ lib/lp/
> > @@ -36,12 +36,15 @@
> > from zope.app.
> > from zope.traversing
> > 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.
> > from zope.schema import Choice, Text
> > from lazr.delegates import delegates
> > from lazr.enum import EnumeratedType, Item
> > +from lazr.lifecycle.
> > +from lazr.lifecycle.
> > from lazr.uri import URI
> >
> > from canonical.
1 | === modified file 'lib/canonical/launchpad/webapp/launchpadform.py' |
2 | --- lib/canonical/launchpad/webapp/launchpadform.py 2009-07-31 02:32:22 +0000 |
3 | +++ lib/canonical/launchpad/webapp/launchpadform.py 2009-08-04 00:25:53 +0000 |
4 | @@ -386,8 +386,9 @@ |
5 | """ |
6 | if context is None: |
7 | context = self.context |
8 | - context_before_modification = Snapshot( |
9 | - context, providing=providedBy(context)) |
10 | + if notify_modified: |
11 | + context_before_modification = Snapshot( |
12 | + context, providing=providedBy(context)) |
13 | |
14 | was_changed = form.applyChanges(context, self.form_fields, |
15 | data, self.adapters) |
16 | |
17 | === modified file 'lib/lp/code/browser/branch.py' |
18 | --- lib/lp/code/browser/branch.py 2009-07-31 02:32:22 +0000 |
19 | +++ lib/lp/code/browser/branch.py 2009-08-04 00:27:41 +0000 |
20 | @@ -601,6 +601,10 @@ |
21 | @action('Change Branch', name='change') |
22 | def change_action(self, action, data): |
23 | # If the owner or product has changed, add an explicit notification. |
24 | + # We take our own snapshot here to make sure that the snapshot records |
25 | + # changes to the owner and private, and we notify the listeners |
26 | + # explicitly below rather than the notification that would normally be |
27 | + # sent in updateContextFromData. |
28 | branch_before_modification = Snapshot( |
29 | self.context, providing=providedBy(self.context)) |
30 | if 'owner' in data: |
31 | |
32 | === modified file 'lib/lp/code/interfaces/branch.py' |
33 | --- lib/lp/code/interfaces/branch.py 2009-07-31 01:07:04 +0000 |
34 | +++ lib/lp/code/interfaces/branch.py 2009-08-05 01:49:53 +0000 |
35 | @@ -32,7 +32,6 @@ |
36 | 'IBranchNavigationMenu', |
37 | 'IBranchSet', |
38 | 'NoSuchBranch', |
39 | - 'UnknownBranchTargetError', |
40 | 'UnknownBranchTypeError', |
41 | 'user_has_special_branch_access', |
42 | ] |
43 | @@ -122,10 +121,6 @@ |
44 | """Raised when the user specifies an unrecognized branch type.""" |
45 | |
46 | |
47 | -class UnknownBranchTargetError(Exception): |
48 | - """Raised when an unexpected type is passed through as a branch target.""" |
49 | - |
50 | - |
51 | class BranchCreationForbidden(BranchCreationException): |
52 | """A Branch visibility policy forbids branch creation. |
53 | |
54 | @@ -445,6 +440,10 @@ |
55 | Only one of `project` or `source_package` can be set, and if neither |
56 | is set, the branch gets moved into the junk namespace of the branch |
57 | owner. |
58 | + |
59 | + :raises BranchTargetError: if both project and source_package are set, |
60 | + or if either the project or source_package fail to be adapted to an |
61 | + `IBranchTarget`. |
62 | """ |
63 | |
64 | reviewer = exported( |
65 | |
66 | === modified file 'lib/lp/code/interfaces/branchtarget.py' |
67 | --- lib/lp/code/interfaces/branchtarget.py 2009-07-28 05:20:23 +0000 |
68 | +++ lib/lp/code/interfaces/branchtarget.py 2009-08-04 00:13:03 +0000 |
69 | @@ -113,12 +113,3 @@ |
70 | |
71 | def getBugTask(bug): |
72 | """Get the BugTask for a given bug related to the branch target.""" |
73 | - |
74 | - def retargetBranch(branch): |
75 | - """Set the branch target to refer to this target. |
76 | - |
77 | - This only updates the target related attributes of the branch. |
78 | - |
79 | - No name clashes are checked, nor privacy policies. Those are handled |
80 | - by the IBranchNamespace.moveBranch method. |
81 | - """ |
82 | |
83 | === modified file 'lib/lp/code/model/branchnamespace.py' |
84 | --- lib/lp/code/model/branchnamespace.py 2009-07-28 05:20:23 +0000 |
85 | +++ lib/lp/code/model/branchnamespace.py 2009-08-04 00:14:50 +0000 |
86 | @@ -177,7 +177,7 @@ |
87 | # attributes are readonly through the interface. |
88 | naked_branch = removeSecurityProxy(branch) |
89 | naked_branch.owner = self.owner |
90 | - self.target.retargetBranch(naked_branch) |
91 | + self.target._retargetBranch(naked_branch) |
92 | naked_branch.name = new_name |
93 | |
94 | def createBranchWithPrefix(self, branch_type, prefix, registrant, |
95 | |
96 | === modified file 'lib/lp/code/model/branchtarget.py' |
97 | --- lib/lp/code/model/branchtarget.py 2009-07-28 05:20:23 +0000 |
98 | +++ lib/lp/code/model/branchtarget.py 2009-08-04 00:14:25 +0000 |
99 | @@ -129,15 +129,15 @@ |
100 | # those cases. |
101 | return bug.default_bugtask |
102 | |
103 | - def retargetBranch(self, branch): |
104 | - """See `IBranchTarget`.""" |
105 | - # Since product, distroseries and sourcepackagename are not writable |
106 | - # as defined by the interface, we need to rip off the security proxy |
107 | - # here. |
108 | - naked_branch = removeSecurityProxy(branch) |
109 | - naked_branch.product = None |
110 | - naked_branch.distroseries = self.sourcepackage.distroseries |
111 | - naked_branch.sourcepackagename = self.sourcepackage.sourcepackagename |
112 | + def _retargetBranch(self, branch): |
113 | + """Set the branch target to refer to this target. |
114 | + |
115 | + This only updates the target related attributes of the branch, and |
116 | + expects a branch without a security proxy as a parameter. |
117 | + """ |
118 | + branch.product = None |
119 | + branch.distroseries = self.sourcepackage.distroseries |
120 | + branch.sourcepackagename = self.sourcepackage.sourcepackagename |
121 | |
122 | |
123 | class PersonBranchTarget(_BaseBranchTarget): |
124 | @@ -193,15 +193,15 @@ |
125 | """See `IBranchTarget`.""" |
126 | return bug.default_bugtask |
127 | |
128 | - def retargetBranch(self, branch): |
129 | - """See `IBranchTarget`.""" |
130 | - # Since product, distroseries and sourcepackagename are not writable |
131 | - # as defined by the interface, we need to rip off the security proxy |
132 | - # here. |
133 | - naked_branch = removeSecurityProxy(branch) |
134 | - naked_branch.product = None |
135 | - naked_branch.distroseries = None |
136 | - naked_branch.sourcepackagename = None |
137 | + def _retargetBranch(self, branch): |
138 | + """Set the branch target to refer to this target. |
139 | + |
140 | + This only updates the target related attributes of the branch, and |
141 | + expects a branch without a security proxy as a parameter. |
142 | + """ |
143 | + branch.product = None |
144 | + branch.distroseries = None |
145 | + branch.sourcepackagename = None |
146 | |
147 | |
148 | class ProductBranchTarget(_BaseBranchTarget): |
149 | @@ -286,15 +286,15 @@ |
150 | task = bug.bugtasks[0] |
151 | return task |
152 | |
153 | - def retargetBranch(self, branch): |
154 | - """See `IBranchTarget`.""" |
155 | - # Since product, distroseries and sourcepackagename are not writable |
156 | - # as defined by the interface, we need to rip off the security proxy |
157 | - # here. |
158 | - naked_branch = removeSecurityProxy(branch) |
159 | - naked_branch.product = self.product |
160 | - naked_branch.distroseries = None |
161 | - naked_branch.sourcepackagename = None |
162 | + def _retargetBranch(self, branch): |
163 | + """Set the branch target to refer to this target. |
164 | + |
165 | + This only updates the target related attributes of the branch, and |
166 | + expects a branch without a security proxy as a parameter. |
167 | + """ |
168 | + branch.product = self.product |
169 | + branch.distroseries = None |
170 | + branch.sourcepackagename = None |
171 | |
172 | |
173 | def get_canonical_url_data_for_target(branch_target): |
174 | |
175 | === modified file 'lib/lp/code/model/tests/test_branch.py' |
176 | --- lib/lp/code/model/tests/test_branch.py 2009-08-02 23:17:10 +0000 |
177 | +++ lib/lp/code/model/tests/test_branch.py 2009-08-05 02:03:41 +0000 |
178 | @@ -40,7 +40,7 @@ |
179 | from lp.code.interfaces.branch import ( |
180 | BranchCannotBePrivate, BranchCannotBePublic, |
181 | BranchCreatorNotMemberOfOwnerTeam, BranchCreatorNotOwner, |
182 | - CannotDeleteBranch, DEFAULT_BRANCH_STATUS_IN_LISTING) |
183 | + BranchTargetError, CannotDeleteBranch, DEFAULT_BRANCH_STATUS_IN_LISTING) |
184 | from lp.code.interfaces.branchlookup import IBranchLookup |
185 | from lp.code.interfaces.branchnamespace import IBranchNamespaceSet |
186 | from lp.code.interfaces.branchmergeproposal import InvalidBranchMergeProposal |
187 | @@ -58,7 +58,6 @@ |
188 | from lp.code.model.codeimport import CodeImport, CodeImportSet |
189 | from lp.code.model.codereviewcomment import CodeReviewComment |
190 | from lp.registry.interfaces.person import IPersonSet |
191 | -from lp.registry.interfaces.product import IProductSet |
192 | from lp.registry.model.product import ProductSet |
193 | from lp.registry.model.sourcepackage import SourcePackage |
194 | from lp.soyuz.interfaces.publishing import PackagePublishingPocket |
195 | @@ -1722,6 +1721,17 @@ |
196 | |
197 | layer = DatabaseFunctionalLayer |
198 | |
199 | + def test_not_both_project_and_source_package(self): |
200 | + # Only one of project or source_package can be passed in, not both. |
201 | + branch = self.factory.makePersonalBranch() |
202 | + project = self.factory.makeProduct() |
203 | + source_package = self.factory.makeSourcePackage() |
204 | + login_person(branch.owner) |
205 | + self.assertRaises( |
206 | + BranchTargetError, |
207 | + branch.setTarget, |
208 | + user=branch.owner, project=project, source_package=source_package) |
209 | + |
210 | def test_junk_branch_to_project_branch(self): |
211 | # A junk branch can be moved to a project. |
212 | branch = self.factory.makePersonalBranch() |
213 | |
214 | === modified file 'lib/lp/code/model/tests/test_branchtarget.py' |
215 | --- lib/lp/code/model/tests/test_branchtarget.py 2009-07-28 05:20:23 +0000 |
216 | +++ lib/lp/code/model/tests/test_branchtarget.py 2009-08-04 00:23:05 +0000 |
217 | @@ -49,19 +49,19 @@ |
218 | def test_retargetBranch_packageBranch(self): |
219 | # Retarget an existing package branch to this target. |
220 | branch = self.factory.makePackageBranch() |
221 | - self.target.retargetBranch(branch) |
222 | + self.target._retargetBranch(removeSecurityProxy(branch)) |
223 | self.assertEqual(self.target, branch.target) |
224 | |
225 | def test_retargetBranch_productBranch(self): |
226 | # Retarget an existing product branch to this target. |
227 | branch = self.factory.makeProductBranch() |
228 | - self.target.retargetBranch(branch) |
229 | + self.target._retargetBranch(removeSecurityProxy(branch)) |
230 | self.assertEqual(self.target, branch.target) |
231 | |
232 | def test_retargetBranch_personalBranch(self): |
233 | # Retarget an existing personal branch to this target. |
234 | branch = self.factory.makePersonalBranch() |
235 | - self.target.retargetBranch(branch) |
236 | + self.target._retargetBranch(removeSecurityProxy(branch)) |
237 | self.assertEqual(self.target, branch.target) |
238 | |
239 | |
240 | @@ -236,7 +236,7 @@ |
241 | # match the target as the target is the branch owner for a personal |
242 | # branch. |
243 | branch = self.factory.makePackageBranch(owner=self.original) |
244 | - self.target.retargetBranch(branch) |
245 | + self.target._retargetBranch(removeSecurityProxy(branch)) |
246 | self.assertEqual(self.target, branch.target) |
247 | |
248 | def test_retargetBranch_productBranch(self): |
249 | @@ -245,7 +245,7 @@ |
250 | # match the target as the target is the branch owner for a personal |
251 | # branch. |
252 | branch = self.factory.makeProductBranch(owner=self.original) |
253 | - self.target.retargetBranch(branch) |
254 | + self.target._retargetBranch(removeSecurityProxy(branch)) |
255 | self.assertEqual(self.target, branch.target) |
256 | |
257 | def test_retargetBranch_personalBranch(self): |
258 | @@ -254,7 +254,7 @@ |
259 | # match the target as the target is the branch owner for a personal |
260 | # branch. |
261 | branch = self.factory.makePersonalBranch(owner=self.original) |
262 | - self.target.retargetBranch(branch) |
263 | + self.target._retargetBranch(removeSecurityProxy(branch)) |
264 | self.assertEqual(self.target, branch.target) |
265 | |
266 | |
267 | |
268 | === modified file 'lib/lp/code/scripts/tests/test_revisionkarma.py' |
269 | --- lib/lp/code/scripts/tests/test_revisionkarma.py 2009-08-02 23:17:10 +0000 |
270 | +++ lib/lp/code/scripts/tests/test_revisionkarma.py 2009-08-04 00:36:50 +0000 |
271 | @@ -49,7 +49,7 @@ |
272 | branch.createBranchRevision(1, rev) |
273 | # Once the branch is connected to the revision, we now specify |
274 | # a product for the branch. |
275 | - project=self.factory.makeProduct() |
276 | + project = self.factory.makeProduct() |
277 | branch.setTarget(user=branch.owner, project=project) |
278 | # Commit and switch to the script db user. |
279 | transaction.commit() |
280 | |
281 | === modified file 'lib/lp/codehosting/tests/test_acceptance.py' |
282 | --- lib/lp/codehosting/tests/test_acceptance.py 2009-08-03 01:41:07 +0000 |
283 | +++ lib/lp/codehosting/tests/test_acceptance.py 2009-08-04 00:40:11 +0000 |
284 | @@ -14,7 +14,6 @@ |
285 | from bzrlib.tests import TestCaseWithTransport |
286 | from bzrlib.urlutils import local_path_from_url |
287 | from bzrlib.workingtree import WorkingTree |
288 | -from zope.security.proxy import removeSecurityProxy |
289 | |
290 | from lp.codehosting.bzrutils import DenyingServer |
291 | from lp.codehosting.tests.helpers import ( |
292 | @@ -383,7 +382,7 @@ |
293 | # rename as far as bzr is concerned: the URL changes. |
294 | LaunchpadZopelessTestSetup().txn.begin() |
295 | branch = self.getDatabaseBranch('testuser', None, 'test-branch') |
296 | - removeSecurityProxy(branch).product = Product.byName('firefox') |
297 | + branch.setTarget(user=branch.owner, project=Product.byName('firefox')) |
298 | LaunchpadZopelessTestSetup().txn.commit() |
299 | |
300 | self.assertNotBranch( |
Preview Diff
1 | === modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py' |
2 | --- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2009-07-17 00:26:05 +0000 |
3 | +++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2009-07-31 01:07:04 +0000 |
4 | @@ -75,6 +75,11 @@ |
5 | IBranch['linkSpecification'].queryTaggedValue( |
6 | LAZR_WEBSERVICE_EXPORTED)['params']['spec'].schema= ISpecification |
7 | IBranch['product'].schema = IProduct |
8 | +IBranch['setTarget'].queryTaggedValue( |
9 | + LAZR_WEBSERVICE_EXPORTED)['params']['project'].schema= IProduct |
10 | +IBranch['setTarget'].queryTaggedValue( |
11 | + LAZR_WEBSERVICE_EXPORTED)['params']['source_package'].schema= \ |
12 | + ISourcePackage |
13 | IBranch['spec_links'].value_type.schema = ISpecificationBranch |
14 | IBranch['subscribe'].queryTaggedValue( |
15 | LAZR_WEBSERVICE_EXPORTED)['return_type'].schema = IBranchSubscription |
16 | |
17 | === modified file 'lib/canonical/launchpad/webapp/launchpadform.py' |
18 | --- lib/canonical/launchpad/webapp/launchpadform.py 2009-06-25 05:30:52 +0000 |
19 | +++ lib/canonical/launchpad/webapp/launchpadform.py 2009-07-31 02:32:22 +0000 |
20 | @@ -372,7 +372,7 @@ |
21 | |
22 | render_context = True |
23 | |
24 | - def updateContextFromData(self, data, context=None): |
25 | + def updateContextFromData(self, data, context=None, notify_modified=True): |
26 | """Update the context object based on form data. |
27 | |
28 | If no context is given, the view's context is used. |
29 | @@ -391,7 +391,7 @@ |
30 | |
31 | was_changed = form.applyChanges(context, self.form_fields, |
32 | data, self.adapters) |
33 | - if was_changed: |
34 | + if was_changed and notify_modified: |
35 | field_names = [form_field.__name__ |
36 | for form_field in self.form_fields] |
37 | notify(ObjectModifiedEvent( |
38 | |
39 | === modified file 'lib/lp/code/browser/branch.py' |
40 | --- lib/lp/code/browser/branch.py 2009-07-19 04:41:14 +0000 |
41 | +++ lib/lp/code/browser/branch.py 2009-07-31 02:32:22 +0000 |
42 | @@ -36,12 +36,15 @@ |
43 | from zope.app.form.browser import TextAreaWidget |
44 | from zope.traversing.interfaces import IPathAdapter |
45 | from zope.component import getUtility, queryAdapter |
46 | +from zope.event import notify |
47 | from zope.formlib import form |
48 | -from zope.interface import Interface, implements |
49 | +from zope.interface import Interface, implements, providedBy |
50 | from zope.publisher.interfaces import NotFound |
51 | from zope.schema import Choice, Text |
52 | from lazr.delegates import delegates |
53 | from lazr.enum import EnumeratedType, Item |
54 | +from lazr.lifecycle.event import ObjectModifiedEvent |
55 | +from lazr.lifecycle.snapshot import Snapshot |
56 | from lazr.uri import URI |
57 | |
58 | from canonical.cachedproperty import cachedproperty |
59 | @@ -574,7 +577,6 @@ |
60 | the user to be able to edit it. |
61 | """ |
62 | use_template(IBranch, include=[ |
63 | - 'owner', |
64 | 'name', |
65 | 'url', |
66 | 'description', |
67 | @@ -582,6 +584,7 @@ |
68 | 'whiteboard', |
69 | ]) |
70 | private = copy_field(IBranch['private'], readonly=False) |
71 | + owner = copy_field(IBranch['owner'], readonly=False) |
72 | |
73 | |
74 | class BranchEditFormView(LaunchpadEditFormView): |
75 | @@ -598,9 +601,12 @@ |
76 | @action('Change Branch', name='change') |
77 | def change_action(self, action, data): |
78 | # If the owner or product has changed, add an explicit notification. |
79 | + branch_before_modification = Snapshot( |
80 | + self.context, providing=providedBy(self.context)) |
81 | if 'owner' in data: |
82 | - new_owner = data['owner'] |
83 | + new_owner = data.pop('owner') |
84 | if new_owner != self.context.owner: |
85 | + self.context.setOwner(new_owner, self.user) |
86 | self.request.response.addNotification( |
87 | "The branch owner has been changed to %s (%s)" |
88 | % (new_owner.displayname, new_owner.name)) |
89 | @@ -616,7 +622,13 @@ |
90 | else: |
91 | self.request.response.addNotification( |
92 | "The branch is now publicly accessible.") |
93 | - if self.updateContextFromData(data): |
94 | + if self.updateContextFromData(data, notify_modified=False): |
95 | + # Notify the object has changed with the snapshot that was taken |
96 | + # earler. |
97 | + field_names = [ |
98 | + form_field.__name__ for form_field in self.form_fields] |
99 | + notify(ObjectModifiedEvent( |
100 | + self.context, branch_before_modification, field_names)) |
101 | # Only specify that the context was modified if there |
102 | # was in fact a change. |
103 | self.context.date_last_modified = UTC_NOW |
104 | |
105 | === modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py' |
106 | --- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2009-07-17 00:26:05 +0000 |
107 | +++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2009-08-02 23:25:37 +0000 |
108 | @@ -8,14 +8,15 @@ |
109 | from unittest import TestLoader |
110 | |
111 | import transaction |
112 | +from zope.security.proxy import removeSecurityProxy |
113 | |
114 | +from canonical.launchpad.webapp.servers import LaunchpadTestRequest |
115 | +from canonical.testing import DatabaseFunctionalLayer |
116 | from lp.code.browser.branchmergeproposallisting import ( |
117 | ActiveReviewsView, BranchMergeProposalListingView, PersonActiveReviewsView, |
118 | ProductActiveReviewsView) |
119 | from lp.code.enums import CodeReviewVote |
120 | from lp.testing import ANONYMOUS, login, login_person, TestCaseWithFactory |
121 | -from canonical.launchpad.webapp.servers import LaunchpadTestRequest |
122 | -from canonical.testing import DatabaseFunctionalLayer |
123 | |
124 | _default = object() |
125 | |
126 | @@ -201,8 +202,7 @@ |
127 | self.assertReviewGroupForUser( |
128 | self.bmp.registrant, self._view.OTHER) |
129 | team = self.factory.makeTeam(self.bmp.registrant) |
130 | - login_person(self.bmp.source_branch.owner) |
131 | - self.bmp.source_branch.owner = team |
132 | + removeSecurityProxy(self.bmp.source_branch).owner = team |
133 | self.assertReviewGroupForUser( |
134 | self.bmp.registrant, ActiveReviewsView.MINE) |
135 | |
136 | |
137 | === modified file 'lib/lp/code/configure.zcml' |
138 | --- lib/lp/code/configure.zcml 2009-07-23 02:06:55 +0000 |
139 | +++ lib/lp/code/configure.zcml 2009-07-30 00:30:02 +0000 |
140 | @@ -430,9 +430,9 @@ |
141 | "/> |
142 | <require |
143 | permission="launchpad.Edit" |
144 | - attributes="destroySelf setPrivate" |
145 | + attributes="destroySelf setPrivate setOwner setTarget" |
146 | set_attributes="name url mirror_status_message |
147 | - owner author description product lifecycle_status |
148 | + description lifecycle_status |
149 | last_mirrored last_mirrored_id last_mirror_attempt |
150 | mirror_failures pull_disabled next_mirror_time |
151 | last_scanned last_scanned_id revision_count branch_type |
152 | |
153 | === modified file 'lib/lp/code/doc/branch.txt' |
154 | --- lib/lp/code/doc/branch.txt 2009-06-16 03:31:05 +0000 |
155 | +++ lib/lp/code/doc/branch.txt 2009-08-02 23:59:31 +0000 |
156 | @@ -116,9 +116,9 @@ |
157 | >>> print new_branch.owner.name |
158 | registrant |
159 | |
160 | -A user can create a branch where the owner is either themselves, |
161 | -or a team that they are a member of. The registrant is not writable, |
162 | -whereas the owner is. |
163 | +A user can create a branch where the owner is either themselves, or a team |
164 | +that they are a member of. Neither the owner nor the registrant are writable, |
165 | +but the owner can be set using the `setOwner` method. |
166 | |
167 | >>> login('admin@canonical.com') |
168 | >>> new_branch.registrant = factory.makePerson() |
169 | @@ -126,7 +126,8 @@ |
170 | ... |
171 | ForbiddenAttribute: ('registrant', <Branch ...>) |
172 | |
173 | - >>> new_branch.owner = factory.makePerson(name='new-owner') |
174 | + >>> team = factory.makeTeam(name='new-owner', owner=new_branch.owner) |
175 | + >>> new_branch.setOwner(new_owner=team, user=new_branch.owner) |
176 | >>> print new_branch.registrant.name |
177 | registrant |
178 | >>> print new_branch.owner.name |
179 | |
180 | === modified file 'lib/lp/code/interfaces/branch.py' |
181 | --- lib/lp/code/interfaces/branch.py 2009-07-29 03:44:05 +0000 |
182 | +++ lib/lp/code/interfaces/branch.py 2009-07-31 01:07:04 +0000 |
183 | @@ -19,6 +19,7 @@ |
184 | 'BranchCreatorNotMemberOfOwnerTeam', |
185 | 'BranchCreatorNotOwner', |
186 | 'BranchExists', |
187 | + 'BranchTargetError', |
188 | 'BranchTypeError', |
189 | 'CannotDeleteBranch', |
190 | 'DEFAULT_BRANCH_STATUS_IN_LISTING', |
191 | @@ -31,6 +32,7 @@ |
192 | 'IBranchNavigationMenu', |
193 | 'IBranchSet', |
194 | 'NoSuchBranch', |
195 | + 'UnknownBranchTargetError', |
196 | 'UnknownBranchTypeError', |
197 | 'user_has_special_branch_access', |
198 | ] |
199 | @@ -90,7 +92,7 @@ |
200 | """Raised when creating a branch that already exists.""" |
201 | |
202 | def __init__(self, existing_branch): |
203 | - # XXX: JonathanLange 2008-12-04 spec=package-branches: This error |
204 | + # XXX: TimPenhey 2009-07-12 bug=405214: This error |
205 | # message logic is incorrect, but the exact text is being tested |
206 | # in branch-xmlrpc.txt. |
207 | params = {'name': existing_branch.name} |
208 | @@ -108,6 +110,10 @@ |
209 | BranchCreationException.__init__(self, message) |
210 | |
211 | |
212 | +class BranchTargetError(Exception): |
213 | + """Raised when there is an error determining a branch target.""" |
214 | + |
215 | + |
216 | class CannotDeleteBranch(Exception): |
217 | """The branch cannot be deleted at this time.""" |
218 | |
219 | @@ -116,6 +122,10 @@ |
220 | """Raised when the user specifies an unrecognized branch type.""" |
221 | |
222 | |
223 | +class UnknownBranchTargetError(Exception): |
224 | + """Raised when an unexpected type is passed through as a branch target.""" |
225 | + |
226 | + |
227 | class BranchCreationForbidden(BranchCreationException): |
228 | """A Branch visibility policy forbids branch creation. |
229 | |
230 | @@ -402,14 +412,41 @@ |
231 | title=_("The user that registered the branch."), |
232 | required=True, readonly=True, |
233 | vocabulary='ValidPersonOrTeam')) |
234 | + |
235 | owner = exported( |
236 | ParticipatingPersonChoice( |
237 | title=_('Owner'), |
238 | - required=True, |
239 | + required=True, readonly=True, |
240 | vocabulary='UserTeamsParticipationPlusSelf', |
241 | description=_("Either yourself or a team you are a member of. " |
242 | "This controls who can modify the branch."))) |
243 | |
244 | + @call_with(user=REQUEST_USER) |
245 | + @operation_parameters( |
246 | + new_owner=Reference( |
247 | + title=_("The new owner of the branch."), |
248 | + schema=IPerson)) |
249 | + @export_write_operation() |
250 | + def setOwner(new_owner, user): |
251 | + """Set the owner of the branch to be `new_owner`.""" |
252 | + |
253 | + @call_with(user=REQUEST_USER) |
254 | + @operation_parameters( |
255 | + project=Reference( |
256 | + title=_("The project the branch belongs to."), |
257 | + schema=Interface, required=False), # Really IProduct |
258 | + source_package=Reference( |
259 | + title=_("The source package the branch belongs to."), |
260 | + schema=Interface, required=False)) # Really ISourcePackage |
261 | + @export_write_operation() |
262 | + def setTarget(user, project=None, source_package=None): |
263 | + """Set the target of the branch to be `project` or `source_package`. |
264 | + |
265 | + Only one of `project` or `source_package` can be set, and if neither |
266 | + is set, the branch gets moved into the junk namespace of the branch |
267 | + owner. |
268 | + """ |
269 | + |
270 | reviewer = exported( |
271 | PublicPersonChoice( |
272 | title=_('Default Review Team'), |
273 | @@ -456,7 +493,7 @@ |
274 | product = exported( |
275 | ReferenceChoice( |
276 | title=_('Project'), |
277 | - required=False, |
278 | + required=False, readonly=True, |
279 | vocabulary='Product', |
280 | schema=Interface, |
281 | description=_("The project this branch belongs to.")), |
282 | |
283 | === modified file 'lib/lp/code/interfaces/branchnamespace.py' |
284 | --- lib/lp/code/interfaces/branchnamespace.py 2009-06-25 04:06:00 +0000 |
285 | +++ lib/lp/code/interfaces/branchnamespace.py 2009-07-27 10:19:21 +0000 |
286 | @@ -70,6 +70,24 @@ |
287 | def isNameUsed(name): |
288 | """Is 'name' already used in this namespace?""" |
289 | |
290 | + def moveBranch(branch, mover, new_name=None, rename_if_necessary=False): |
291 | + """Move the branch into this namespace. |
292 | + |
293 | + :param branch: The `IBranch` to move. |
294 | + :param mover: The `IPerson` doing the moving. |
295 | + :param new_name: A new name for the branch. |
296 | + :param rename_if_necessary: Rename the branch if the branch name |
297 | + exists already in this namespace. |
298 | + :raises BranchCreatorNotMemberOfOwnerTeam: if the namespace owner is |
299 | + a team, and 'mover' is not in that team. |
300 | + :raises BranchCreatorNotOwner: if the namespace owner is an individual |
301 | + and 'mover' is not the owner. |
302 | + :raises BranchCreationForbidden: if 'mover' is not allowed to create |
303 | + a branch in this namespace due to privacy rules. |
304 | + :raises BranchExists: if a branch with the 'name' exists already in |
305 | + the namespace, and 'rename_if_necessary' is False. |
306 | + """ |
307 | + |
308 | |
309 | class IBranchNamespacePolicy(Interface): |
310 | """Methods relating to branch creation and validation.""" |
311 | @@ -133,11 +151,13 @@ |
312 | validation constraints on IBranch.name. |
313 | """ |
314 | |
315 | - def validateMove(branch, mover): |
316 | + def validateMove(branch, mover, name=None): |
317 | """Check that 'mover' can move 'branch' into this namespace. |
318 | |
319 | :param branch: An `IBranch` that might be moved. |
320 | :param mover: The `IPerson` who would move it. |
321 | + :param name: A new name for the branch. If None, the branch name is |
322 | + used. |
323 | :raises BranchCreatorNotMemberOfOwnerTeam: if the namespace owner is |
324 | a team, and 'mover' is not in that team. |
325 | :raises BranchCreatorNotOwner: if the namespace owner is an individual |
326 | |
327 | === modified file 'lib/lp/code/interfaces/branchtarget.py' |
328 | --- lib/lp/code/interfaces/branchtarget.py 2009-07-17 00:26:05 +0000 |
329 | +++ lib/lp/code/interfaces/branchtarget.py 2009-07-28 05:20:23 +0000 |
330 | @@ -113,3 +113,12 @@ |
331 | |
332 | def getBugTask(bug): |
333 | """Get the BugTask for a given bug related to the branch target.""" |
334 | + |
335 | + def retargetBranch(branch): |
336 | + """Set the branch target to refer to this target. |
337 | + |
338 | + This only updates the target related attributes of the branch. |
339 | + |
340 | + No name clashes are checked, nor privacy policies. Those are handled |
341 | + by the IBranchNamespace.moveBranch method. |
342 | + """ |
343 | |
344 | === modified file 'lib/lp/code/model/branch.py' |
345 | --- lib/lp/code/model/branch.py 2009-07-19 04:41:14 +0000 |
346 | +++ lib/lp/code/model/branch.py 2009-07-31 01:07:04 +0000 |
347 | @@ -57,7 +57,7 @@ |
348 | from lp.code.event.branchmergeproposal import NewBranchMergeProposalEvent |
349 | from lp.code.interfaces.branch import ( |
350 | bazaar_identity, BranchCannotBePrivate, BranchCannotBePublic, |
351 | - BranchTypeError, CannotDeleteBranch, |
352 | + BranchTargetError, BranchTypeError, CannotDeleteBranch, |
353 | DEFAULT_BRANCH_STATUS_IN_LISTING, IBranch, |
354 | IBranchNavigationMenu, IBranchSet) |
355 | from lp.code.interfaces.branchcollection import IAllBranches |
356 | @@ -71,7 +71,7 @@ |
357 | from lp.code.interfaces.seriessourcepackagebranch import ( |
358 | IFindOfficialBranchLinks) |
359 | from lp.registry.interfaces.person import ( |
360 | - validate_person_not_private_membership, validate_public_person) |
361 | + IPerson, validate_person_not_private_membership, validate_public_person) |
362 | |
363 | |
364 | class Branch(SQLBase): |
365 | @@ -113,6 +113,12 @@ |
366 | owner = ForeignKey( |
367 | dbName='owner', foreignKey='Person', |
368 | storm_validator=validate_person_not_private_membership, notNull=True) |
369 | + |
370 | + def setOwner(self, new_owner, user): |
371 | + """See `IBranch`.""" |
372 | + new_namespace = self.target.getNamespace(new_owner) |
373 | + new_namespace.moveBranch(self, user, rename_if_necessary=True) |
374 | + |
375 | reviewer = ForeignKey( |
376 | dbName='reviewer', foreignKey='Person', |
377 | storm_validator=validate_public_person, default=None) |
378 | @@ -162,6 +168,28 @@ |
379 | target = self.product |
380 | return IBranchTarget(target) |
381 | |
382 | + def setTarget(self, user, project=None, source_package=None): |
383 | + """See `IBranch`.""" |
384 | + if project is not None: |
385 | + if source_package is not None: |
386 | + raise BranchTargetError( |
387 | + 'Cannot specify both a project and a source package.') |
388 | + else: |
389 | + target = IBranchTarget(project) |
390 | + if target is None: |
391 | + raise BranchTargetError( |
392 | + '%r is not a valid project target' % project) |
393 | + elif source_package is not None: |
394 | + target = IBranchTarget(source_package) |
395 | + if target is None: |
396 | + raise BranchTargetError( |
397 | + '%r is not a valid source package target' % source_package) |
398 | + else: |
399 | + target = IBranchTarget(self.owner) |
400 | + # Person targets are always valid. |
401 | + namespace = target.getNamespace(self.owner) |
402 | + namespace.moveBranch(self, user, rename_if_necessary=True) |
403 | + |
404 | @property |
405 | def namespace(self): |
406 | """See `IBranch`.""" |
407 | |
408 | === modified file 'lib/lp/code/model/branchnamespace.py' |
409 | --- lib/lp/code/model/branchnamespace.py 2009-06-25 04:06:00 +0000 |
410 | +++ lib/lp/code/model/branchnamespace.py 2009-07-28 05:20:23 +0000 |
411 | @@ -16,6 +16,7 @@ |
412 | from zope.component import getUtility |
413 | from zope.event import notify |
414 | from zope.interface import implements |
415 | +from zope.security.proxy import removeSecurityProxy |
416 | |
417 | from lazr.lifecycle.event import ObjectCreatedEvent |
418 | from storm.locals import And |
419 | @@ -160,6 +161,25 @@ |
420 | self.validateBranchName(name) |
421 | self.validateRegistrant(mover) |
422 | |
423 | + def moveBranch(self, branch, mover, new_name=None, |
424 | + rename_if_necessary=False): |
425 | + """See `IBranchNamespace`.""" |
426 | + # Check to see if the branch is already in this namespace. |
427 | + old_namespace = branch.namespace |
428 | + if self.name == old_namespace.name: |
429 | + return |
430 | + if new_name is None: |
431 | + new_name = branch.name |
432 | + if rename_if_necessary: |
433 | + new_name = self.findUnusedName(new_name) |
434 | + self.validateMove(branch, mover, new_name) |
435 | + # Remove the security proxy of the branch as the owner and target |
436 | + # attributes are readonly through the interface. |
437 | + naked_branch = removeSecurityProxy(branch) |
438 | + naked_branch.owner = self.owner |
439 | + self.target.retargetBranch(naked_branch) |
440 | + naked_branch.name = new_name |
441 | + |
442 | def createBranchWithPrefix(self, branch_type, prefix, registrant, |
443 | url=None): |
444 | """See `IBranchNamespace`.""" |
445 | |
446 | === modified file 'lib/lp/code/model/branchtarget.py' |
447 | --- lib/lp/code/model/branchtarget.py 2009-07-17 00:26:05 +0000 |
448 | +++ lib/lp/code/model/branchtarget.py 2009-07-28 05:20:23 +0000 |
449 | @@ -13,7 +13,8 @@ |
450 | |
451 | from zope.component import getUtility |
452 | from zope.interface import implements |
453 | -from zope.security.proxy import isinstance as zope_isinstance |
454 | +from zope.security.proxy import ( |
455 | + removeSecurityProxy, isinstance as zope_isinstance) |
456 | |
457 | from lp.code.interfaces.branchcollection import IAllBranches |
458 | from lp.code.interfaces.branchtarget import ( |
459 | @@ -128,6 +129,16 @@ |
460 | # those cases. |
461 | return bug.default_bugtask |
462 | |
463 | + def retargetBranch(self, branch): |
464 | + """See `IBranchTarget`.""" |
465 | + # Since product, distroseries and sourcepackagename are not writable |
466 | + # as defined by the interface, we need to rip off the security proxy |
467 | + # here. |
468 | + naked_branch = removeSecurityProxy(branch) |
469 | + naked_branch.product = None |
470 | + naked_branch.distroseries = self.sourcepackage.distroseries |
471 | + naked_branch.sourcepackagename = self.sourcepackage.sourcepackagename |
472 | + |
473 | |
474 | class PersonBranchTarget(_BaseBranchTarget): |
475 | implements(IBranchTarget) |
476 | @@ -182,6 +193,16 @@ |
477 | """See `IBranchTarget`.""" |
478 | return bug.default_bugtask |
479 | |
480 | + def retargetBranch(self, branch): |
481 | + """See `IBranchTarget`.""" |
482 | + # Since product, distroseries and sourcepackagename are not writable |
483 | + # as defined by the interface, we need to rip off the security proxy |
484 | + # here. |
485 | + naked_branch = removeSecurityProxy(branch) |
486 | + naked_branch.product = None |
487 | + naked_branch.distroseries = None |
488 | + naked_branch.sourcepackagename = None |
489 | + |
490 | |
491 | class ProductBranchTarget(_BaseBranchTarget): |
492 | implements(IBranchTarget) |
493 | @@ -265,6 +286,16 @@ |
494 | task = bug.bugtasks[0] |
495 | return task |
496 | |
497 | + def retargetBranch(self, branch): |
498 | + """See `IBranchTarget`.""" |
499 | + # Since product, distroseries and sourcepackagename are not writable |
500 | + # as defined by the interface, we need to rip off the security proxy |
501 | + # here. |
502 | + naked_branch = removeSecurityProxy(branch) |
503 | + naked_branch.product = self.product |
504 | + naked_branch.distroseries = None |
505 | + naked_branch.sourcepackagename = None |
506 | + |
507 | |
508 | def get_canonical_url_data_for_target(branch_target): |
509 | """Return the `ICanonicalUrlData` for an `IBranchTarget`.""" |
510 | |
511 | === modified file 'lib/lp/code/model/tests/test_branch.py' |
512 | --- lib/lp/code/model/tests/test_branch.py 2009-07-23 02:06:55 +0000 |
513 | +++ lib/lp/code/model/tests/test_branch.py 2009-08-02 23:17:10 +0000 |
514 | @@ -39,6 +39,7 @@ |
515 | BranchVisibilityRule, CodeReviewNotificationLevel) |
516 | from lp.code.interfaces.branch import ( |
517 | BranchCannotBePrivate, BranchCannotBePublic, |
518 | + BranchCreatorNotMemberOfOwnerTeam, BranchCreatorNotOwner, |
519 | CannotDeleteBranch, DEFAULT_BRANCH_STATUS_IN_LISTING) |
520 | from lp.code.interfaces.branchlookup import IBranchLookup |
521 | from lp.code.interfaces.branchnamespace import IBranchNamespaceSet |
522 | @@ -213,8 +214,7 @@ |
523 | # attribute is updated too. |
524 | branch = self.factory.makeAnyBranch() |
525 | new_owner = self.factory.makePerson() |
526 | - login('admin@canonical.com') |
527 | - branch.owner = new_owner |
528 | + removeSecurityProxy(branch).owner = new_owner |
529 | # Call the function that is normally called through the event system |
530 | # to auto reload the fields updated by the db triggers. |
531 | update_trigger_modified_fields(branch) |
532 | @@ -1054,9 +1054,9 @@ |
533 | |
534 | def setUp(self): |
535 | TestCaseWithFactory.setUp(self, 'admin@canonical.com') |
536 | - self.product = getUtility(IProductSet).getByName('firefox') |
537 | + self.product = self.factory.makeProduct() |
538 | |
539 | - self.user = getUtility(IPersonSet).getByName('no-priv') |
540 | + self.user = self.factory.makePerson() |
541 | self.source = self.factory.makeProductBranch( |
542 | name='source-branch', owner=self.user, product=self.product) |
543 | self.target = self.factory.makeProductBranch( |
544 | @@ -1069,7 +1069,7 @@ |
545 | |
546 | def test_junkSource(self): |
547 | """Junk branches cannot be used as a source for merge proposals.""" |
548 | - self.source.product = None |
549 | + self.source.setTarget(user=self.source.owner) |
550 | self.assertRaises( |
551 | InvalidBranchMergeProposal, self.source.addLandingTarget, |
552 | self.user, self.target) |
553 | @@ -1078,12 +1078,13 @@ |
554 | """The product of the target branch must match the product of the |
555 | source branch. |
556 | """ |
557 | - self.target.product = None |
558 | + self.target.setTarget(user=self.target.owner) |
559 | self.assertRaises( |
560 | InvalidBranchMergeProposal, self.source.addLandingTarget, |
561 | self.user, self.target) |
562 | |
563 | - self.target.product = getUtility(IProductSet).getByName('bzr') |
564 | + project = self.factory.makeProduct() |
565 | + self.target.setTarget(user=self.target.owner, project=project) |
566 | self.assertRaises( |
567 | InvalidBranchMergeProposal, self.source.addLandingTarget, |
568 | self.user, self.target) |
569 | @@ -1097,12 +1098,13 @@ |
570 | def test_dependentBranchSameProduct(self): |
571 | """The dependent branch, if it is there, must be for the same product. |
572 | """ |
573 | - self.dependent.product = None |
574 | + self.dependent.setTarget(user=self.dependent.owner) |
575 | self.assertRaises( |
576 | InvalidBranchMergeProposal, self.source.addLandingTarget, |
577 | self.user, self.target, self.dependent) |
578 | |
579 | - self.dependent.product = getUtility(IProductSet).getByName('bzr') |
580 | + project = self.factory.makeProduct() |
581 | + self.dependent.setTarget(user=self.dependent.owner, project=project) |
582 | self.assertRaises( |
583 | InvalidBranchMergeProposal, self.source.addLandingTarget, |
584 | self.user, self.target, self.dependent) |
585 | @@ -1650,5 +1652,138 @@ |
586 | self.assertEqual(branch.spec_links.count(), 0) |
587 | |
588 | |
589 | +class TestBranchSetOwner(TestCaseWithFactory): |
590 | + """Tests for IBranch.setOwner.""" |
591 | + |
592 | + layer = DatabaseFunctionalLayer |
593 | + |
594 | + def test_owner_sets_team(self): |
595 | + # The owner of the branch can set the owner of the branch to be a team |
596 | + # they are a member of. |
597 | + branch = self.factory.makeAnyBranch() |
598 | + team = self.factory.makeTeam(owner=branch.owner) |
599 | + login_person(branch.owner) |
600 | + branch.setOwner(team, branch.owner) |
601 | + self.assertEqual(team, branch.owner) |
602 | + |
603 | + def test_owner_cannot_set_nonmember_team(self): |
604 | + # The owner of the branch cannot set the owner to be a team they are |
605 | + # not a member of. |
606 | + branch = self.factory.makeAnyBranch() |
607 | + team = self.factory.makeTeam() |
608 | + login_person(branch.owner) |
609 | + self.assertRaises( |
610 | + BranchCreatorNotMemberOfOwnerTeam, |
611 | + branch.setOwner, |
612 | + team, branch.owner) |
613 | + |
614 | + def test_owner_cannot_set_other_user(self): |
615 | + # The owner of the branch cannot set the new owner to be another |
616 | + # person. |
617 | + branch = self.factory.makeAnyBranch() |
618 | + person = self.factory.makePerson() |
619 | + login_person(branch.owner) |
620 | + self.assertRaises( |
621 | + BranchCreatorNotOwner, |
622 | + branch.setOwner, |
623 | + person, branch.owner) |
624 | + |
625 | + def test_admin_can_set_any_team_or_person(self): |
626 | + # A Launchpad admin can set the branch to be owned by any team or |
627 | + # person. |
628 | + branch = self.factory.makeAnyBranch() |
629 | + team = self.factory.makeTeam() |
630 | + # To get a random administrator, choose the admin team owner. |
631 | + admin = getUtility(ILaunchpadCelebrities).admin.teamowner |
632 | + login_person(admin) |
633 | + branch.setOwner(team, admin) |
634 | + self.assertEqual(team, branch.owner) |
635 | + person = self.factory.makePerson() |
636 | + branch.setOwner(person, admin) |
637 | + self.assertEqual(person, branch.owner) |
638 | + |
639 | + def test_bazaar_experts_can_set_any_team_or_person(self): |
640 | + # A bazaar expert can set the branch to be owned by any team or |
641 | + # person. |
642 | + branch = self.factory.makeAnyBranch() |
643 | + team = self.factory.makeTeam() |
644 | + # To get a random administrator, choose the admin team owner. |
645 | + experts = getUtility(ILaunchpadCelebrities).bazaar_experts.teamowner |
646 | + login_person(experts) |
647 | + branch.setOwner(team, experts) |
648 | + self.assertEqual(team, branch.owner) |
649 | + person = self.factory.makePerson() |
650 | + branch.setOwner(person, experts) |
651 | + self.assertEqual(person, branch.owner) |
652 | + |
653 | + |
654 | +class TestBranchSetTarget(TestCaseWithFactory): |
655 | + """Tests for IBranch.setTarget.""" |
656 | + |
657 | + layer = DatabaseFunctionalLayer |
658 | + |
659 | + def test_junk_branch_to_project_branch(self): |
660 | + # A junk branch can be moved to a project. |
661 | + branch = self.factory.makePersonalBranch() |
662 | + project = self.factory.makeProduct() |
663 | + login_person(branch.owner) |
664 | + branch.setTarget(user=branch.owner, project=project) |
665 | + self.assertEqual(project, branch.target.context) |
666 | + |
667 | + def test_junk_branch_to_package_branch(self): |
668 | + # A junk branch can be moved to a source package. |
669 | + branch = self.factory.makePersonalBranch() |
670 | + source_package = self.factory.makeSourcePackage() |
671 | + login_person(branch.owner) |
672 | + branch.setTarget(user=branch.owner, source_package=source_package) |
673 | + self.assertEqual(source_package, branch.target.context) |
674 | + |
675 | + def test_project_branch_to_other_project_branch(self): |
676 | + # Move a branch from one project to another. |
677 | + branch = self.factory.makeProductBranch() |
678 | + project = self.factory.makeProduct() |
679 | + login_person(branch.owner) |
680 | + branch.setTarget(user=branch.owner, project=project) |
681 | + self.assertEqual(project, branch.target.context) |
682 | + |
683 | + def test_project_branch_to_package_branch(self): |
684 | + # Move a branch from a project to a package. |
685 | + branch = self.factory.makeProductBranch() |
686 | + source_package = self.factory.makeSourcePackage() |
687 | + login_person(branch.owner) |
688 | + branch.setTarget(user=branch.owner, source_package=source_package) |
689 | + self.assertEqual(source_package, branch.target.context) |
690 | + |
691 | + def test_project_branch_to_junk_branch(self): |
692 | + # Move a branch from a project to junk. |
693 | + branch = self.factory.makeProductBranch() |
694 | + login_person(branch.owner) |
695 | + branch.setTarget(user=branch.owner) |
696 | + self.assertEqual(branch.owner, branch.target.context) |
697 | + |
698 | + def test_package_branch_to_other_package_branch(self): |
699 | + # Move a branch from one package to another. |
700 | + branch = self.factory.makePackageBranch() |
701 | + source_package = self.factory.makeSourcePackage() |
702 | + login_person(branch.owner) |
703 | + branch.setTarget(user=branch.owner, source_package=source_package) |
704 | + self.assertEqual(source_package, branch.target.context) |
705 | + |
706 | + def test_package_branch_to_project_branch(self): |
707 | + # Move a branch from a package to a project. |
708 | + branch = self.factory.makePackageBranch() |
709 | + project = self.factory.makeProduct() |
710 | + login_person(branch.owner) |
711 | + branch.setTarget(user=branch.owner, project=project) |
712 | + self.assertEqual(project, branch.target.context) |
713 | + |
714 | + def test_package_branch_to_junk_branch(self): |
715 | + # Move a branch from a package to junk. |
716 | + branch = self.factory.makePackageBranch() |
717 | + login_person(branch.owner) |
718 | + branch.setTarget(user=branch.owner) |
719 | + self.assertEqual(branch.owner, branch.target.context) |
720 | + |
721 | + |
722 | def test_suite(): |
723 | return TestLoader().loadTestsFromName(__name__) |
724 | |
725 | === renamed file 'lib/lp/code/tests/test_branchnamespace.py' => 'lib/lp/code/model/tests/test_branchnamespace.py' |
726 | --- lib/lp/code/tests/test_branchnamespace.py 2009-06-25 04:06:00 +0000 |
727 | +++ lib/lp/code/model/tests/test_branchnamespace.py 2009-07-29 02:16:43 +0000 |
728 | @@ -1871,5 +1871,70 @@ |
729 | BranchCreatorNotOwner, self.albert, self.doug) |
730 | |
731 | |
732 | +class TestBranchNamespaceMoveBranch(TestCaseWithFactory): |
733 | + """Test the IBranchNamespace.moveBranch method. |
734 | + |
735 | + The edge cases of the validateMove are tested in the NamespaceMixin for |
736 | + each of the namespaces. |
737 | + """ |
738 | + |
739 | + layer = DatabaseFunctionalLayer |
740 | + |
741 | + def assertNamespacesEqual(self, expected, result): |
742 | + """Assert that the namespaces refer to the same thing. |
743 | + |
744 | + The name of the namespace contains the user name and the context |
745 | + parts, so is the easiest thing to check. |
746 | + """ |
747 | + self.assertEqual(expected.name, result.name) |
748 | + |
749 | + def test_move_to_same_namespace(self): |
750 | + # Moving to the same namespace is effectively a no-op. No exceptions |
751 | + # about matching branch names should be raised. |
752 | + branch = self.factory.makeAnyBranch() |
753 | + namespace = branch.namespace |
754 | + namespace.moveBranch(branch, branch.owner) |
755 | + self.assertNamespacesEqual(namespace, branch.namespace) |
756 | + |
757 | + def test_name_clash_raises(self): |
758 | + # A name clash will raise an exception. |
759 | + branch = self.factory.makeAnyBranch(name="test") |
760 | + another = self.factory.makeAnyBranch(owner=branch.owner, name="test") |
761 | + namespace = another.namespace |
762 | + self.assertRaises( |
763 | + BranchExists, namespace.moveBranch, branch, branch.owner) |
764 | + |
765 | + def test_move_with_rename(self): |
766 | + # A name clash with 'rename_if_necessary' set to True will cause the |
767 | + # branch to be renamed instead of raising an error. |
768 | + branch = self.factory.makeAnyBranch(name="test") |
769 | + another = self.factory.makeAnyBranch(owner=branch.owner, name="test") |
770 | + namespace = another.namespace |
771 | + namespace.moveBranch(branch, branch.owner, rename_if_necessary=True) |
772 | + self.assertEqual("test-1", branch.name) |
773 | + self.assertNamespacesEqual(namespace, branch.namespace) |
774 | + |
775 | + def test_move_with_new_name(self): |
776 | + # A new name for the branch can be specified as part of the move. |
777 | + branch = self.factory.makeAnyBranch(name="test") |
778 | + another = self.factory.makeAnyBranch(owner=branch.owner, name="test") |
779 | + namespace = another.namespace |
780 | + namespace.moveBranch(branch, branch.owner, new_name="foo") |
781 | + self.assertEqual("foo", branch.name) |
782 | + self.assertNamespacesEqual(namespace, branch.namespace) |
783 | + |
784 | + def test_sets_branch_owner(self): |
785 | + # Moving to a new namespace may change the owner of the branch if the |
786 | + # owner of the namespace is different. |
787 | + branch = self.factory.makeAnyBranch(name="test") |
788 | + team = self.factory.makeTeam(branch.owner) |
789 | + product = self.factory.makeProduct() |
790 | + namespace = ProductNamespace(team, product) |
791 | + namespace.moveBranch(branch, branch.owner) |
792 | + self.assertEqual(team, branch.owner) |
793 | + # And for paranoia. |
794 | + self.assertNamespacesEqual(namespace, branch.namespace) |
795 | + |
796 | + |
797 | def test_suite(): |
798 | return unittest.TestLoader().loadTestsFromName(__name__) |
799 | |
800 | === modified file 'lib/lp/code/model/tests/test_branchtarget.py' |
801 | --- lib/lp/code/model/tests/test_branchtarget.py 2009-06-25 04:06:00 +0000 |
802 | +++ lib/lp/code/model/tests/test_branchtarget.py 2009-07-28 05:20:23 +0000 |
803 | @@ -46,6 +46,24 @@ |
804 | branches = self.target.collection.getBranches() |
805 | self.assertEqual([branch], list(branches)) |
806 | |
807 | + def test_retargetBranch_packageBranch(self): |
808 | + # Retarget an existing package branch to this target. |
809 | + branch = self.factory.makePackageBranch() |
810 | + self.target.retargetBranch(branch) |
811 | + self.assertEqual(self.target, branch.target) |
812 | + |
813 | + def test_retargetBranch_productBranch(self): |
814 | + # Retarget an existing product branch to this target. |
815 | + branch = self.factory.makeProductBranch() |
816 | + self.target.retargetBranch(branch) |
817 | + self.assertEqual(self.target, branch.target) |
818 | + |
819 | + def test_retargetBranch_personalBranch(self): |
820 | + # Retarget an existing personal branch to this target. |
821 | + branch = self.factory.makePersonalBranch() |
822 | + self.target.retargetBranch(branch) |
823 | + self.assertEqual(self.target, branch.target) |
824 | + |
825 | |
826 | class TestPackageBranchTarget(TestCaseWithFactory, BaseBranchTargetTests): |
827 | |
828 | @@ -212,6 +230,33 @@ |
829 | # The default merge target is always None. |
830 | self.assertIs(None, self.target.default_merge_target) |
831 | |
832 | + def test_retargetBranch_packageBranch(self): |
833 | + # Retarget an existing package branch to this target. Override the |
834 | + # mixin tests, and specify the owner of the branch. This is needed to |
835 | + # match the target as the target is the branch owner for a personal |
836 | + # branch. |
837 | + branch = self.factory.makePackageBranch(owner=self.original) |
838 | + self.target.retargetBranch(branch) |
839 | + self.assertEqual(self.target, branch.target) |
840 | + |
841 | + def test_retargetBranch_productBranch(self): |
842 | + # Retarget an existing product branch to this target. Override the |
843 | + # mixin tests, and specify the owner of the branch. This is needed to |
844 | + # match the target as the target is the branch owner for a personal |
845 | + # branch. |
846 | + branch = self.factory.makeProductBranch(owner=self.original) |
847 | + self.target.retargetBranch(branch) |
848 | + self.assertEqual(self.target, branch.target) |
849 | + |
850 | + def test_retargetBranch_personalBranch(self): |
851 | + # Retarget an existing personal branch to this target. Override the |
852 | + # mixin tests, and specify the owner of the branch. This is needed to |
853 | + # match the target as the target is the branch owner for a personal |
854 | + # branch. |
855 | + branch = self.factory.makePersonalBranch(owner=self.original) |
856 | + self.target.retargetBranch(branch) |
857 | + self.assertEqual(self.target, branch.target) |
858 | + |
859 | |
860 | class TestProductBranchTarget(TestCaseWithFactory, BaseBranchTargetTests): |
861 | |
862 | |
863 | === modified file 'lib/lp/code/model/tests/test_revision.py' |
864 | --- lib/lp/code/model/tests/test_revision.py 2009-06-25 04:06:00 +0000 |
865 | +++ lib/lp/code/model/tests/test_revision.py 2009-08-02 23:17:10 +0000 |
866 | @@ -146,7 +146,8 @@ |
867 | branch.createBranchRevision(1, rev) |
868 | # Once the branch is connected to the revision, we now specify |
869 | # a product for the branch. |
870 | - branch.product = self.factory.makeProduct() |
871 | + project = self.factory.makeProduct() |
872 | + branch.setTarget(user=branch.owner, project=project) |
873 | # The revision is now identified as needing karma allocated. |
874 | self.assertEqual( |
875 | [rev], list(RevisionSet.getRevisionsNeedingKarmaAllocated())) |
876 | |
877 | === modified file 'lib/lp/code/scripts/tests/test_revisionkarma.py' |
878 | --- lib/lp/code/scripts/tests/test_revisionkarma.py 2009-06-25 04:06:00 +0000 |
879 | +++ lib/lp/code/scripts/tests/test_revisionkarma.py 2009-08-02 23:17:10 +0000 |
880 | @@ -49,7 +49,8 @@ |
881 | branch.createBranchRevision(1, rev) |
882 | # Once the branch is connected to the revision, we now specify |
883 | # a product for the branch. |
884 | - branch.product = self.factory.makeProduct() |
885 | + project=self.factory.makeProduct() |
886 | + branch.setTarget(user=branch.owner, project=project) |
887 | # Commit and switch to the script db user. |
888 | transaction.commit() |
889 | LaunchpadZopelessLayer.switchDbUser(config.revisionkarma.dbuser) |
890 | |
891 | === modified file 'lib/lp/codehosting/tests/test_acceptance.py' |
892 | --- lib/lp/codehosting/tests/test_acceptance.py 2009-07-17 00:26:05 +0000 |
893 | +++ lib/lp/codehosting/tests/test_acceptance.py 2009-08-02 23:38:51 +0000 |
894 | @@ -8,13 +8,13 @@ |
895 | import atexit |
896 | import os |
897 | import unittest |
898 | -from xml.dom.minidom import parseString |
899 | import xmlrpclib |
900 | |
901 | import bzrlib.branch |
902 | from bzrlib.tests import TestCaseWithTransport |
903 | from bzrlib.urlutils import local_path_from_url |
904 | from bzrlib.workingtree import WorkingTree |
905 | +from zope.security.proxy import removeSecurityProxy |
906 | |
907 | from lp.codehosting.bzrutils import DenyingServer |
908 | from lp.codehosting.tests.helpers import ( |
909 | @@ -382,7 +382,8 @@ |
910 | # rename as far as bzr is concerned: the URL changes. |
911 | LaunchpadZopelessTestSetup().txn.begin() |
912 | branch = self.getDatabaseBranch('testuser', None, 'test-branch') |
913 | - branch.product = database.Product.byName('firefox') |
914 | + removeSecurityProxy(branch).product = database.Product.byName( |
915 | + 'firefox') |
916 | LaunchpadZopelessTestSetup().txn.commit() |
917 | |
918 | self.assertNotBranch( |
919 | |
920 | === modified file 'lib/lp/registry/browser/tests/private-team-creation-views.txt' |
921 | --- lib/lp/registry/browser/tests/private-team-creation-views.txt 2009-06-16 17:12:30 +0000 |
922 | +++ lib/lp/registry/browser/tests/private-team-creation-views.txt 2009-08-03 01:02:44 +0000 |
923 | @@ -400,8 +400,7 @@ |
924 | ... bugtask = bug.default_bugtask |
925 | ... bugtask.transitionToAssignee(team) |
926 | ... # A branch. |
927 | - ... branch = factory.makeBranch() |
928 | - ... branch.owner = team |
929 | + ... branch = factory.makeBranch(owner=team) |
930 | ... # A branch subscription. |
931 | ... from lp.code.enums import ( |
932 | ... BranchSubscriptionDiffSize, |
933 | |
934 | === modified file 'lib/lp/registry/doc/private-team-roles.txt' |
935 | --- lib/lp/registry/doc/private-team-roles.txt 2009-06-19 01:27:32 +0000 |
936 | +++ lib/lp/registry/doc/private-team-roles.txt 2009-08-03 00:41:34 +0000 |
937 | @@ -23,9 +23,10 @@ |
938 | ----------------- |
939 | |
940 | >>> # Create the necessary teams. |
941 | - >>> from lp.registry.interfaces.person import PersonVisibility |
942 | >>> team_owner = factory.makePerson(name='team-owner') |
943 | - >>> login('foo.bar@canonical.com') |
944 | + >>> from lp.registry.interfaces.person import IPersonSet, PersonVisibility |
945 | + >>> admin_user = getUtility(IPersonSet).getByEmail('admin@canonical.com') |
946 | + >>> login_person(admin_user) |
947 | >>> priv_team = factory.makeTeam(name='private-team', |
948 | ... owner=team_owner, |
949 | ... visibility=PersonVisibility.PRIVATE) |
950 | @@ -102,12 +103,12 @@ |
951 | Private teams can be assigned as the owner of a branch |
952 | |
953 | >>> branch = factory.makeBranch() |
954 | - >>> branch.owner = priv_team |
955 | + >>> branch.setOwner(priv_team, user=admin_user) |
956 | |
957 | But private membership teams cannot own a branch. |
958 | |
959 | >>> branch = factory.makeBranch() |
960 | - >>> branch.owner = pm_team |
961 | + >>> branch.setOwner(pm_team, user=admin_user) |
962 | Traceback (most recent call last): |
963 | ... |
964 | PrivatePersonLinkageError: Cannot link person |
= Summary =
This branch adds setOwner and setTarget to IBranch.
IBranch.owner and IBranch.product are changed to be read only in the interface
and API.
== Implementation details ==
Both setOwner and setTarget defer to the IBranchNamespac e.moveBranch method.
This method does the validation checks for both owner setting, name clashes,
and visibility policy checks.
The LaunchpadEditForm view updateContextFr omData was modified to take a boolean omData will fail if it is there, but we
to determine whether or not the ObjectModifiedEvent listeners should be
notified. This is because the setOwner call needs to remove the 'owner' key
out of the dict as the updateContextFr
still want to show that the change has occurred in the email.
A number of changes are just noise in the move to make owner and product read
only through the interface.
== Tests ==
TestBranchSetOwner paceMoveBranch
TestBranchSetTarget
TestBranchNames
test_retargetBranch
== Demo and Q/A ==
Not much has chanced in the UI. If you can still change the owner of the
branch, then we have succeeded.
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files: /launchpad/ webapp/ launchpadform. py code/interfaces /branchtarget. py code/configure. zcml code/browser/ branch. py code/interfaces /branch. py registry/ doc/private- team-roles. txt code/interfaces /branchnamespac e.py registry/ browser/ tests/private- team-creation- views.txt code/model/ branchtarget. py codehosting/ tests/test_ acceptance. py code/model/ branch. py code/browser/ tests/test_ branchmergeprop osallisting. py code/model/ tests/test_ branchtarget. py code/model/ tests/test_ branchnamespace .py code/model/ branchnamespace .py code/doc/ branch. txt code/model/ tests/test_ revision. py code/scripts/ tests/test_ revisionkarma. py /launchpad/ interfaces/ _schema_ circular_ imports. py code/model/ tests/test_ branch. py
lib/canonical
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/canonical
lib/lp/
== Pyflakes notices ==
lib/canonical/ launchpad/ webapp/ launchpadform. py
22: 'action' imported but unused
94: redefinition of unused 'action' from line 22
195: redefinition of unused 'action' from line 22
lib/lp/ code/model/ branch. py
73: 'IPerson' imported but unused
lib/lp/ code/model/ tests/test_ branch. py
61: 'IProductSet' imported but unused
== Pylint notices ==
lib/canonical/ launchpad/ webapp/ launchpadform. py ew.validate_ none] Static method with 'self' as
245: [W0211, LaunchpadFormVi
first argument
lib/lp/ code/browser/ branch. py
48: [F0401] Unable to import 'lazr.uri' (No module named uri)
lib/lp/ code/model/ branch. py
186: [C0301] Line too long (79/78)
73: [W0611] Unused import IPerson
lib/lp/ code/browser/ tests/test_ branchmergeprop osallisting. py
16: [C0301] Line too long (79/78)
lib/lp/ code/scripts/ tests/test_ revisionkarma. py ma.test_ junkBranchMoved ] Operator not preceded self.factory. makeProduct( )
52: [C0322, TestRevisionKar
by a space
project=
^
lib/lp/ code/model/ tests/test_ branch. py...