Merge lp:~wallyworld/launchpad/inline-recipe-distro-series-edit into lp:launchpad
- inline-recipe-distro-series-edit
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Aaron Bentley | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 12646 | ||||
Proposed branch: | lp:~wallyworld/launchpad/inline-recipe-distro-series-edit | ||||
Merge into: | lp:launchpad | ||||
Prerequisite: | lp:~wallyworld/launchpad/inline-multicheckbox-widget | ||||
Diff against target: |
625 lines (+207/-76) 11 files modified
lib/lp/app/doc/lazr-js-widgets.txt (+5/-8) lib/lp/code/browser/sourcepackagerecipe.py (+70/-23) lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+8/-8) lib/lp/code/configure.zcml (+4/-0) lib/lp/code/interfaces/sourcepackagerecipe.py (+37/-24) lib/lp/code/javascript/requestbuild_overlay.js (+2/-2) lib/lp/code/model/sourcepackagerecipe.py (+6/-0) lib/lp/code/templates/sourcepackagerecipe-index.pt (+2/-9) lib/lp/code/templates/sourcepackagerecipe-new.pt (+1/-1) lib/lp/code/templates/sourcepackagerecipe-request-builds.pt (+1/-1) lib/lp/code/windmill/tests/test_recipe_inline_distroseries_edit.py (+71/-0) |
||||
To merge this branch: | bzr merge lp:~wallyworld/launchpad/inline-recipe-distro-series-edit | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Aaron Bentley (community) | Approve | ||
Review via email: mp+52940@code.launchpad.net |
Commit message
[r=abentley][bug=735899] Use the new inline multicheckbox selection widget to edit the source package recipe distroseries attribute.
Description of the change
Use the new inline multicheckbox selection widget to edit the source package recipe distroseries attribute.
== Implementation ==
Not much to tell - just wire up the new widget. Also incorporate Tim's recent changes to the lazr widget infrastructure.
To make everything glue together, the "distros" attribute of the SourcePackageRe
== Demo and QA ==
A screenshot of the widget in action:
http://
== Tests ==
New windmill test added.
/lp/code/
bin/test -vvt test_inline_
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
./lib/lp/
110: Line exceeds 78 characters.
148: Line exceeds 78 characters.
180: Line exceeds 78 characters.
Ian Booth (wallyworld) wrote : | # |
> This looks pretty good.
>
> I wonder whether label_tag="dt" and items_tag="dd" could be defaults.
>
Yeah, thought about that but with a sample size of 1 - this is the only place it's used so far - wasn't sure about whether to encode the assumption the widget would be rendered in a definition list or not. In the end, I thought it safer to go with span. We can always revisit when we get more places using the widget.
> Sorry for using distros rather than distroseries. I was new to the problem
> domain.
>
No probs.
> I don't think you should have a setUp in RecipeEdit until have more than one
> test, because until you have more tests, you won't know what needs to vary.
>
Ok. Will fix.
> I also see you setting up far more constants than you actually use, e.g.
> recipe name. Why not just use makeSourcePacka
> person_
>
Will take alook.
> The implementation of updateSeries is a bit sad. Is there no straightforward
> way to update that?
Ah, the way that is there is absolutely required if the distroseries field is exported as a CollectionField, which it was initially. However, now that the distroseries field is a List, that approach may not be required anymore. I'll have a look.
Aaron Bentley (abentley) : | # |
Ian Booth (wallyworld) wrote : | # |
>
> > The implementation of updateSeries is a bit sad. Is there no
> straightforward
> > way to update that?
>
> Ah, the way that is there is absolutely required if the distroseries field is
> exported as a CollectionField, which it was initially. However, now that the
> distroseries field is a List, that approach may not be required anymore. I'll
> have a look.
The current implementation is still required even when distroseries is exported as a List. The property has a type which subclasses ResultSet and so one needs to use clear(), add(), remove() methods.
Preview Diff
1 | === modified file 'lib/lp/app/doc/lazr-js-widgets.txt' | |||
2 | --- lib/lp/app/doc/lazr-js-widgets.txt 2011-03-22 02:47:21 +0000 | |||
3 | +++ lib/lp/app/doc/lazr-js-widgets.txt 2011-03-22 02:47:23 +0000 | |||
4 | @@ -219,7 +219,8 @@ | |||
5 | 219 | descriptions. since the barrier to create a PPA is relatively low, we | 219 | descriptions. since the barrier to create a PPA is relatively low, we |
6 | 220 | restrict the linkability of some fields. The constructor provides a | 220 | restrict the linkability of some fields. The constructor provides a |
7 | 221 | "linkify_text" parameter that defaults to True. Set this to False to avoid | 221 | "linkify_text" parameter that defaults to True. Set this to False to avoid |
9 | 222 | the linkification of text. See the IArchive['description'] editor for an example. | 222 | the linkification of text. See the IArchive['description'] editor for an |
10 | 223 | example. | ||
11 | 223 | 224 | ||
12 | 224 | 225 | ||
13 | 225 | InlineEditPickerWidget | 226 | InlineEditPickerWidget |
14 | @@ -350,9 +351,9 @@ | |||
15 | 350 | 351 | ||
16 | 351 | >>> from lp.app.browser.lazrjs import EnumChoiceWidget | 352 | >>> from lp.app.browser.lazrjs import EnumChoiceWidget |
17 | 352 | 353 | ||
21 | 353 | As with the other widgets, this one requires a context object and a Choice type | 354 | As with the other widgets, this one requires a context object and a Choice |
22 | 354 | field. The rendering of the widget hooks up to the lazr ChoiceSource with the | 355 | type field. The rendering of the widget hooks up to the lazr ChoiceSource |
23 | 355 | standard patch plugin. | 356 | with the standard patch plugin. |
24 | 356 | 357 | ||
25 | 357 | One of the different things about this widget is the styles that are added. | 358 | One of the different things about this widget is the styles that are added. |
26 | 358 | Many enums have specific colour styles. Generally these are the names of | 359 | Many enums have specific colour styles. Generally these are the names of |
27 | @@ -459,22 +460,18 @@ | |||
28 | 459 | >>> login_person(eric) | 460 | >>> login_person(eric) |
29 | 460 | >>> print widget() | 461 | >>> print widget() |
30 | 461 | <span id="edit-distroseries"> | 462 | <span id="edit-distroseries"> |
31 | 462 | <BLANKLINE> | ||
32 | 463 | <dt> | 463 | <dt> |
33 | 464 | Recipe distro series | 464 | Recipe distro series |
34 | 465 | <BLANKLINE> | ||
35 | 466 | <button class="lazr-btn yui3-activator-act yui3-activator-hidden" | 465 | <button class="lazr-btn yui3-activator-act yui3-activator-hidden" |
36 | 467 | id="edit-distroseries-btn"> | 466 | id="edit-distroseries-btn"> |
37 | 468 | Edit | 467 | Edit |
38 | 469 | </button> | 468 | </button> |
39 | 470 | <BLANKLINE> | ||
40 | 471 | <noscript> | 469 | <noscript> |
41 | 472 | <a class="sprite edit" | 470 | <a class="sprite edit" |
42 | 473 | href="http://code.launchpad.dev/~eric/+recipe/cake_recipe/+edit" | 471 | href="http://code.launchpad.dev/~eric/+recipe/cake_recipe/+edit" |
43 | 474 | title=""></a> | 472 | title=""></a> |
44 | 475 | </noscript> | 473 | </noscript> |
45 | 476 | </dt> | 474 | </dt> |
46 | 477 | <BLANKLINE> | ||
47 | 478 | <span class="yui3-activator-data-box"> | 475 | <span class="yui3-activator-data-box"> |
48 | 479 | <dl id='edit-distroseries-items'> | 476 | <dl id='edit-distroseries-items'> |
49 | 480 | ... | 477 | ... |
50 | 481 | 478 | ||
51 | === modified file 'lib/lp/code/browser/sourcepackagerecipe.py' | |||
52 | --- lib/lp/code/browser/sourcepackagerecipe.py 2011-03-18 10:31:56 +0000 | |||
53 | +++ lib/lp/code/browser/sourcepackagerecipe.py 2011-03-22 02:47:23 +0000 | |||
54 | @@ -24,15 +24,21 @@ | |||
55 | 24 | from lazr.lifecycle.event import ObjectModifiedEvent | 24 | from lazr.lifecycle.event import ObjectModifiedEvent |
56 | 25 | from lazr.lifecycle.snapshot import Snapshot | 25 | from lazr.lifecycle.snapshot import Snapshot |
57 | 26 | from lazr.restful.interface import use_template | 26 | from lazr.restful.interface import use_template |
58 | 27 | from lazr.restful.interfaces import ( | ||
59 | 28 | IFieldHTMLRenderer, | ||
60 | 29 | IWebServiceClientRequest, | ||
61 | 30 | ) | ||
62 | 27 | import simplejson | 31 | import simplejson |
63 | 28 | from storm.locals import Store | 32 | from storm.locals import Store |
64 | 29 | from z3c.ptcompat import ViewPageTemplateFile | 33 | from z3c.ptcompat import ViewPageTemplateFile |
65 | 34 | from zope import component | ||
66 | 30 | from zope.app.form.browser.widget import Widget | 35 | from zope.app.form.browser.widget import Widget |
67 | 31 | from zope.app.form.interfaces import IView | 36 | from zope.app.form.interfaces import IView |
68 | 32 | from zope.component import getUtility | 37 | from zope.component import getUtility |
69 | 33 | from zope.event import notify | 38 | from zope.event import notify |
70 | 34 | from zope.formlib import form | 39 | from zope.formlib import form |
71 | 35 | from zope.interface import ( | 40 | from zope.interface import ( |
72 | 41 | implementer, | ||
73 | 36 | implements, | 42 | implements, |
74 | 37 | Interface, | 43 | Interface, |
75 | 38 | providedBy, | 44 | providedBy, |
76 | @@ -44,6 +50,7 @@ | |||
77 | 44 | Text, | 50 | Text, |
78 | 45 | TextLine, | 51 | TextLine, |
79 | 46 | ) | 52 | ) |
80 | 53 | from zope.schema.interfaces import ICollection | ||
81 | 47 | from zope.schema.vocabulary import ( | 54 | from zope.schema.vocabulary import ( |
82 | 48 | SimpleTerm, | 55 | SimpleTerm, |
83 | 49 | SimpleVocabulary, | 56 | SimpleVocabulary, |
84 | @@ -296,6 +303,43 @@ | |||
85 | 296 | title = "Edit the recipe name" | 303 | title = "Edit the recipe name" |
86 | 297 | return TextLineEditorWidget(self.context, name, title, 'h1') | 304 | return TextLineEditorWidget(self.context, name, title, 'h1') |
87 | 298 | 305 | ||
88 | 306 | @property | ||
89 | 307 | def distroseries_widget(self): | ||
90 | 308 | from lp.app.browser.lazrjs import InlineMultiCheckboxWidget | ||
91 | 309 | field = ISourcePackageEditSchema['distroseries'] | ||
92 | 310 | return InlineMultiCheckboxWidget( | ||
93 | 311 | self.context, | ||
94 | 312 | field, | ||
95 | 313 | attribute_type="reference", | ||
96 | 314 | vocabulary='BuildableDistroSeries', | ||
97 | 315 | label="Distribution series:", | ||
98 | 316 | label_tag="dt", | ||
99 | 317 | header="Change default distribution series:", | ||
100 | 318 | empty_display_value="None", | ||
101 | 319 | selected_items=sorted( | ||
102 | 320 | self.context.distroseries, key=lambda ds: ds.displayname), | ||
103 | 321 | items_tag="dd", | ||
104 | 322 | ) | ||
105 | 323 | |||
106 | 324 | |||
107 | 325 | @component.adapter(ISourcePackageRecipe, ICollection, | ||
108 | 326 | IWebServiceClientRequest) | ||
109 | 327 | @implementer(IFieldHTMLRenderer) | ||
110 | 328 | def distroseries_renderer(context, field, request): | ||
111 | 329 | """Render a distroseries collection as a set of links.""" | ||
112 | 330 | |||
113 | 331 | def render(value): | ||
114 | 332 | distroseries = sorted( | ||
115 | 333 | context.distroseries, key=lambda ds: ds.displayname) | ||
116 | 334 | if not distroseries: | ||
117 | 335 | return 'None' | ||
118 | 336 | html = "<ul>" | ||
119 | 337 | html += ''.join( | ||
120 | 338 | ["<li>%s</li>" % format_link(series) for series in distroseries]) | ||
121 | 339 | html += "</ul>" | ||
122 | 340 | return html | ||
123 | 341 | return render | ||
124 | 342 | |||
125 | 299 | 343 | ||
126 | 300 | def builds_for_recipe(recipe): | 344 | def builds_for_recipe(recipe): |
127 | 301 | """A list of interesting builds. | 345 | """A list of interesting builds. |
128 | @@ -337,7 +381,7 @@ | |||
129 | 337 | 381 | ||
130 | 338 | The distroseries function as defaults for requesting a build. | 382 | The distroseries function as defaults for requesting a build. |
131 | 339 | """ | 383 | """ |
133 | 340 | initial_values = {'distros': self.context.distroseries} | 384 | initial_values = {'distroseries': self.context.distroseries} |
134 | 341 | build = self.context.last_build | 385 | build = self.context.last_build |
135 | 342 | if build is not None: | 386 | if build is not None: |
136 | 343 | initial_values['archive'] = build.archive | 387 | initial_values['archive'] = build.archive |
137 | @@ -346,26 +390,26 @@ | |||
138 | 346 | class schema(Interface): | 390 | class schema(Interface): |
139 | 347 | """Schema for requesting a build.""" | 391 | """Schema for requesting a build.""" |
140 | 348 | archive = Choice(vocabulary='TargetPPAs', title=u'Archive') | 392 | archive = Choice(vocabulary='TargetPPAs', title=u'Archive') |
142 | 349 | distros = List( | 393 | distroseries = List( |
143 | 350 | Choice(vocabulary='BuildableDistroSeries'), | 394 | Choice(vocabulary='BuildableDistroSeries'), |
144 | 351 | title=u'Distribution series') | 395 | title=u'Distribution series') |
145 | 352 | 396 | ||
147 | 353 | custom_widget('distros', LabeledMultiCheckBoxWidget) | 397 | custom_widget('distroseries', LabeledMultiCheckBoxWidget) |
148 | 354 | 398 | ||
149 | 355 | def validate(self, data): | 399 | def validate(self, data): |
151 | 356 | distros = data.get('distros', []) | 400 | distros = data.get('distroseries', []) |
152 | 357 | if not len(distros): | 401 | if not len(distros): |
154 | 358 | self.setFieldError('distros', | 402 | self.setFieldError('distroseries', |
155 | 359 | "You need to specify at least one distro series for which " | 403 | "You need to specify at least one distro series for which " |
156 | 360 | "to build.") | 404 | "to build.") |
157 | 361 | return | 405 | return |
158 | 362 | over_quota_distroseries = [] | 406 | over_quota_distroseries = [] |
160 | 363 | for distroseries in data['distros']: | 407 | for distroseries in data['distroseries']: |
161 | 364 | if self.context.isOverQuota(self.user, distroseries): | 408 | if self.context.isOverQuota(self.user, distroseries): |
162 | 365 | over_quota_distroseries.append(str(distroseries)) | 409 | over_quota_distroseries.append(str(distroseries)) |
163 | 366 | if len(over_quota_distroseries) > 0: | 410 | if len(over_quota_distroseries) > 0: |
164 | 367 | self.setFieldError( | 411 | self.setFieldError( |
166 | 368 | 'distros', | 412 | 'distroseries', |
167 | 369 | "You have exceeded today's quota for %s." % | 413 | "You have exceeded today's quota for %s." % |
168 | 370 | ', '.join(over_quota_distroseries)) | 414 | ', '.join(over_quota_distroseries)) |
169 | 371 | 415 | ||
170 | @@ -378,7 +422,7 @@ | |||
171 | 378 | """ | 422 | """ |
172 | 379 | informational = {} | 423 | informational = {} |
173 | 380 | builds = [] | 424 | builds = [] |
175 | 381 | for distroseries in data['distros']: | 425 | for distroseries in data['distroseries']: |
176 | 382 | try: | 426 | try: |
177 | 383 | build = self.context.requestBuild( | 427 | build = self.context.requestBuild( |
178 | 384 | data['archive'], self.user, distroseries, manual=True) | 428 | data['archive'], self.user, distroseries, manual=True) |
179 | @@ -512,18 +556,13 @@ | |||
180 | 512 | 'description', | 556 | 'description', |
181 | 513 | 'owner', | 557 | 'owner', |
182 | 514 | 'build_daily', | 558 | 'build_daily', |
183 | 559 | 'distroseries', | ||
184 | 515 | ]) | 560 | ]) |
185 | 516 | daily_build_archive = Choice(vocabulary='TargetPPAs', | 561 | daily_build_archive = Choice(vocabulary='TargetPPAs', |
186 | 517 | title=u'Daily build archive', | 562 | title=u'Daily build archive', |
187 | 518 | description=( | 563 | description=( |
188 | 519 | u'If built daily, this is the archive where the package ' | 564 | u'If built daily, this is the archive where the package ' |
189 | 520 | u'will be uploaded.')) | 565 | u'will be uploaded.')) |
190 | 521 | distros = List( | ||
191 | 522 | Choice(vocabulary='BuildableDistroSeries'), | ||
192 | 523 | title=u'Default distribution series', | ||
193 | 524 | description=( | ||
194 | 525 | u'If built daily, these are the distribution versions that ' | ||
195 | 526 | u'the recipe will be built for.')) | ||
196 | 527 | recipe_text = has_structured_doc( | 566 | recipe_text = has_structured_doc( |
197 | 528 | Text( | 567 | Text( |
198 | 529 | title=u'Recipe text', required=True, | 568 | title=u'Recipe text', required=True, |
199 | @@ -577,9 +616,9 @@ | |||
200 | 577 | 616 | ||
201 | 578 | def validate(self, data): | 617 | def validate(self, data): |
202 | 579 | if data['build_daily']: | 618 | if data['build_daily']: |
204 | 580 | if len(data['distros']) == 0: | 619 | if len(data['distroseries']) == 0: |
205 | 581 | self.setFieldError( | 620 | self.setFieldError( |
207 | 582 | 'distros', | 621 | 'distroseries', |
208 | 583 | 'You must specify at least one series for daily builds.') | 622 | 'You must specify at least one series for daily builds.') |
209 | 584 | try: | 623 | try: |
210 | 585 | parser = RecipeParser(data['recipe_text']) | 624 | parser = RecipeParser(data['recipe_text']) |
211 | @@ -673,7 +712,7 @@ | |||
212 | 673 | title = label = 'Create a new source package recipe' | 712 | title = label = 'Create a new source package recipe' |
213 | 674 | 713 | ||
214 | 675 | schema = ISourcePackageAddSchema | 714 | schema = ISourcePackageAddSchema |
216 | 676 | custom_widget('distros', LabeledMultiCheckBoxWidget) | 715 | custom_widget('distroseries', LabeledMultiCheckBoxWidget) |
217 | 677 | custom_widget('owner', RecipeOwnerWidget) | 716 | custom_widget('owner', RecipeOwnerWidget) |
218 | 678 | custom_widget('use_ppa', LaunchpadRadioWidget) | 717 | custom_widget('use_ppa', LaunchpadRadioWidget) |
219 | 679 | 718 | ||
220 | @@ -696,6 +735,11 @@ | |||
221 | 696 | # all confused when we want to create a new PPA. | 735 | # all confused when we want to create a new PPA. |
222 | 697 | archive_widget._displayItemForMissingValue = False | 736 | archive_widget._displayItemForMissingValue = False |
223 | 698 | 737 | ||
224 | 738 | def setUpFields(self): | ||
225 | 739 | super(SourcePackageRecipeAddView, self).setUpFields() | ||
226 | 740 | # Ensure distro series widget allows input | ||
227 | 741 | self.form_fields['distroseries'].for_input = True | ||
228 | 742 | |||
229 | 699 | def getBranch(self): | 743 | def getBranch(self): |
230 | 700 | """The branch on which the recipe is built.""" | 744 | """The branch on which the recipe is built.""" |
231 | 701 | return self.context | 745 | return self.context |
232 | @@ -729,7 +773,7 @@ | |||
233 | 729 | 'name': self._find_unused_name(self.user), | 773 | 'name': self._find_unused_name(self.user), |
234 | 730 | 'recipe_text': MINIMAL_RECIPE_TEXT % self.context.bzr_identity, | 774 | 'recipe_text': MINIMAL_RECIPE_TEXT % self.context.bzr_identity, |
235 | 731 | 'owner': self.user, | 775 | 'owner': self.user, |
237 | 732 | 'distros': series, | 776 | 'distroseries': series, |
238 | 733 | 'build_daily': True, | 777 | 'build_daily': True, |
239 | 734 | 'use_ppa': EXISTING_PPA, | 778 | 'use_ppa': EXISTING_PPA, |
240 | 735 | } | 779 | } |
241 | @@ -750,8 +794,8 @@ | |||
242 | 750 | source_package_recipe = self.error_handler( | 794 | source_package_recipe = self.error_handler( |
243 | 751 | getUtility(ISourcePackageRecipeSource).new, | 795 | getUtility(ISourcePackageRecipeSource).new, |
244 | 752 | self.user, owner, data['name'], | 796 | self.user, owner, data['name'], |
247 | 753 | data['recipe_text'], data['description'], data['distros'], | 797 | data['recipe_text'], data['description'], |
248 | 754 | ppa, data['build_daily']) | 798 | data['distroseries'], ppa, data['build_daily']) |
249 | 755 | Store.of(source_package_recipe).flush() | 799 | Store.of(source_package_recipe).flush() |
250 | 756 | except ErrorHandled: | 800 | except ErrorHandled: |
251 | 757 | return | 801 | return |
252 | @@ -795,11 +839,14 @@ | |||
253 | 795 | label = title | 839 | label = title |
254 | 796 | 840 | ||
255 | 797 | schema = ISourcePackageEditSchema | 841 | schema = ISourcePackageEditSchema |
257 | 798 | custom_widget('distros', LabeledMultiCheckBoxWidget) | 842 | custom_widget('distroseries', LabeledMultiCheckBoxWidget) |
258 | 799 | 843 | ||
259 | 800 | def setUpFields(self): | 844 | def setUpFields(self): |
260 | 801 | super(SourcePackageRecipeEditView, self).setUpFields() | 845 | super(SourcePackageRecipeEditView, self).setUpFields() |
261 | 802 | 846 | ||
262 | 847 | # Ensure distro series widget allows input | ||
263 | 848 | self.form_fields['distroseries'].for_input = True | ||
264 | 849 | |||
265 | 803 | if check_permission('launchpad.Admin', self.context): | 850 | if check_permission('launchpad.Admin', self.context): |
266 | 804 | # Exclude the PPA archive dropdown. | 851 | # Exclude the PPA archive dropdown. |
267 | 805 | self.form_fields = self.form_fields.omit('daily_build_archive') | 852 | self.form_fields = self.form_fields.omit('daily_build_archive') |
268 | @@ -819,7 +866,7 @@ | |||
269 | 819 | @property | 866 | @property |
270 | 820 | def initial_values(self): | 867 | def initial_values(self): |
271 | 821 | return { | 868 | return { |
273 | 822 | 'distros': self.context.distroseries, | 869 | 'distroseries': self.context.distroseries, |
274 | 823 | 'recipe_text': self.context.recipe_text, | 870 | 'recipe_text': self.context.recipe_text, |
275 | 824 | } | 871 | } |
276 | 825 | 872 | ||
277 | @@ -843,7 +890,7 @@ | |||
278 | 843 | except ErrorHandled: | 890 | except ErrorHandled: |
279 | 844 | return | 891 | return |
280 | 845 | 892 | ||
282 | 846 | distros = data.pop('distros') | 893 | distros = data.pop('distroseries') |
283 | 847 | if distros != self.context.distroseries: | 894 | if distros != self.context.distroseries: |
284 | 848 | self.context.distroseries.clear() | 895 | self.context.distroseries.clear() |
285 | 849 | for distroseries_item in distros: | 896 | for distroseries_item in distros: |
286 | 850 | 897 | ||
287 | === modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py' | |||
288 | --- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2011-03-18 10:31:56 +0000 | |||
289 | +++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2011-03-22 02:47:23 +0000 | |||
290 | @@ -272,7 +272,7 @@ | |||
291 | 272 | branch = self.factory.makeAnyBranch() | 272 | branch = self.factory.makeAnyBranch() |
292 | 273 | with person_logged_in(archive.owner): | 273 | with person_logged_in(archive.owner): |
293 | 274 | view = create_initialized_view(branch, '+new-recipe') | 274 | view = create_initialized_view(branch, '+new-recipe') |
295 | 275 | series = set(view.initial_values['distros']) | 275 | series = set(view.initial_values['distroseries']) |
296 | 276 | initial_series = set([development, current]) | 276 | initial_series = set([development, current]) |
297 | 277 | self.assertEqual(initial_series, series.intersection(initial_series)) | 277 | self.assertEqual(initial_series, series.intersection(initial_series)) |
298 | 278 | other_series = set( | 278 | other_series = set( |
299 | @@ -452,7 +452,7 @@ | |||
300 | 452 | browser = self.getViewBrowser(self.makeBranch(), '+new-recipe') | 452 | browser = self.getViewBrowser(self.makeBranch(), '+new-recipe') |
301 | 453 | browser.getControl(name='field.name').value = 'daily' | 453 | browser.getControl(name='field.name').value = 'daily' |
302 | 454 | browser.getControl('Description').value = 'Make some food!' | 454 | browser.getControl('Description').value = 'Make some food!' |
304 | 455 | browser.getControl(name='field.distros').value = [] | 455 | browser.getControl(name='field.distroseries').value = [] |
305 | 456 | browser.getControl('Create Recipe').click() | 456 | browser.getControl('Create Recipe').click() |
306 | 457 | self.assertEqual( | 457 | self.assertEqual( |
307 | 458 | 'You must specify at least one series for daily builds.', | 458 | 'You must specify at least one series for daily builds.', |
308 | @@ -780,8 +780,8 @@ | |||
309 | 780 | 'lp://dev/~chef/ratatouille/meat', | 780 | 'lp://dev/~chef/ratatouille/meat', |
310 | 781 | MatchesTagText(content, 'edit-recipe_text')) | 781 | MatchesTagText(content, 'edit-recipe_text')) |
311 | 782 | self.assertThat( | 782 | self.assertThat( |
314 | 783 | 'Distribution series: Mumbly Midget', | 783 | 'Distribution series: Edit Mumbly Midget', |
315 | 784 | MatchesTagText(content, 'distros')) | 784 | MatchesTagText(content, 'distroseries')) |
316 | 785 | self.assertThat( | 785 | self.assertThat( |
317 | 786 | 'PPA 2', MatchesPickerText(content, 'edit-daily_build_archive')) | 786 | 'PPA 2', MatchesPickerText(content, 'edit-daily_build_archive')) |
318 | 787 | 787 | ||
319 | @@ -797,7 +797,7 @@ | |||
320 | 797 | view.request_action.success({ | 797 | view.request_action.success({ |
321 | 798 | 'name': u'fings', | 798 | 'name': u'fings', |
322 | 799 | 'recipe_text': recipe.recipe_text, | 799 | 'recipe_text': recipe.recipe_text, |
324 | 800 | 'distros': recipe.distroseries}) | 800 | 'distroseries': recipe.distroseries}) |
325 | 801 | self.assertSqlAttributeEqualsDate( | 801 | self.assertSqlAttributeEqualsDate( |
326 | 802 | recipe, 'date_last_modified', UTC_NOW) | 802 | recipe, 'date_last_modified', UTC_NOW) |
327 | 803 | 803 | ||
328 | @@ -846,8 +846,8 @@ | |||
329 | 846 | 'lp://dev/~chef/ratatouille/meat', | 846 | 'lp://dev/~chef/ratatouille/meat', |
330 | 847 | MatchesTagText(content, 'edit-recipe_text')) | 847 | MatchesTagText(content, 'edit-recipe_text')) |
331 | 848 | self.assertThat( | 848 | self.assertThat( |
334 | 849 | 'Distribution series: Mumbly Midget', | 849 | 'Distribution series: Edit Mumbly Midget', |
335 | 850 | MatchesTagText(content, 'distros')) | 850 | MatchesTagText(content, 'distroseries')) |
336 | 851 | 851 | ||
337 | 852 | def test_edit_recipe_forbidden_instruction(self): | 852 | def test_edit_recipe_forbidden_instruction(self): |
338 | 853 | self.factory.makeDistroSeries( | 853 | self.factory.makeDistroSeries( |
339 | @@ -1082,7 +1082,7 @@ | |||
340 | 1082 | Base branch: lp://dev/~chef/chocolate/cake | 1082 | Base branch: lp://dev/~chef/chocolate/cake |
341 | 1083 | Debian version: {debupstream}-0~{revno} | 1083 | Debian version: {debupstream}-0~{revno} |
342 | 1084 | Daily build archive: Secret PPA Edit | 1084 | Daily build archive: Secret PPA Edit |
344 | 1085 | Distribution series: Secret Squirrel | 1085 | Distribution series: Edit Secret Squirrel |
345 | 1086 | 1086 | ||
346 | 1087 | Latest builds | 1087 | Latest builds |
347 | 1088 | Status When complete Distribution series Archive | 1088 | Status When complete Distribution series Archive |
348 | 1089 | 1089 | ||
349 | === modified file 'lib/lp/code/configure.zcml' | |||
350 | --- lib/lp/code/configure.zcml 2011-03-16 06:03:44 +0000 | |||
351 | +++ lib/lp/code/configure.zcml 2011-03-22 02:47:23 +0000 | |||
352 | @@ -1001,6 +1001,10 @@ | |||
353 | 1001 | name="RECIPEBRANCHBUILD" | 1001 | name="RECIPEBRANCHBUILD" |
354 | 1002 | permission="zope.Public"/> | 1002 | permission="zope.Public"/> |
355 | 1003 | 1003 | ||
356 | 1004 | <adapter | ||
357 | 1005 | factory="lp.code.browser.sourcepackagerecipe.distroseries_renderer" | ||
358 | 1006 | name="distroseries"/> | ||
359 | 1007 | |||
360 | 1004 | <!-- RecipeBuildRecordSet and related classes--> | 1008 | <!-- RecipeBuildRecordSet and related classes--> |
361 | 1005 | 1009 | ||
362 | 1006 | <securedutility | 1010 | <securedutility |
363 | 1007 | 1011 | ||
364 | === modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py' | |||
365 | --- lib/lp/code/interfaces/sourcepackagerecipe.py 2011-03-16 06:03:44 +0000 | |||
366 | +++ lib/lp/code/interfaces/sourcepackagerecipe.py 2011-03-22 02:47:23 +0000 | |||
367 | @@ -36,6 +36,7 @@ | |||
368 | 36 | from lazr.restful.fields import ( | 36 | from lazr.restful.fields import ( |
369 | 37 | CollectionField, | 37 | CollectionField, |
370 | 38 | Reference, | 38 | Reference, |
371 | 39 | ReferenceChoice, | ||
372 | 39 | ) | 40 | ) |
373 | 40 | from lazr.restful.interface import copy_field | 41 | from lazr.restful.interface import copy_field |
374 | 41 | from zope.interface import ( | 42 | from zope.interface import ( |
375 | @@ -47,6 +48,7 @@ | |||
376 | 47 | Choice, | 48 | Choice, |
377 | 48 | Datetime, | 49 | Datetime, |
378 | 49 | Int, | 50 | Int, |
379 | 51 | List, | ||
380 | 50 | Text, | 52 | Text, |
381 | 51 | TextLine, | 53 | TextLine, |
382 | 52 | ) | 54 | ) |
383 | @@ -181,26 +183,6 @@ | |||
384 | 181 | """ | 183 | """ |
385 | 182 | 184 | ||
386 | 183 | 185 | ||
387 | 184 | class ISourcePackageRecipeEdit(Interface): | ||
388 | 185 | """ISourcePackageRecipe methods that require launchpad.Edit permission.""" | ||
389 | 186 | |||
390 | 187 | @mutator_for(ISourcePackageRecipeView['recipe_text']) | ||
391 | 188 | @operation_for_version("devel") | ||
392 | 189 | @operation_parameters( | ||
393 | 190 | recipe_text=copy_field( | ||
394 | 191 | ISourcePackageRecipeView['recipe_text'])) | ||
395 | 192 | @export_write_operation() | ||
396 | 193 | def setRecipeText(recipe_text): | ||
397 | 194 | """Set the text of the recipe.""" | ||
398 | 195 | |||
399 | 196 | def destroySelf(): | ||
400 | 197 | """Remove this SourcePackageRecipe from the database. | ||
401 | 198 | |||
402 | 199 | This requires deleting any rows with non-nullable foreign key | ||
403 | 200 | references to this object. | ||
404 | 201 | """ | ||
405 | 202 | |||
406 | 203 | |||
407 | 204 | class ISourcePackageRecipeEditableAttributes(IHasOwner): | 186 | class ISourcePackageRecipeEditableAttributes(IHasOwner): |
408 | 205 | """ISourcePackageRecipe attributes that can be edited. | 187 | """ISourcePackageRecipe attributes that can be edited. |
409 | 206 | 188 | ||
410 | @@ -219,10 +201,13 @@ | |||
411 | 219 | vocabulary='UserTeamsParticipationPlusSelf', | 201 | vocabulary='UserTeamsParticipationPlusSelf', |
412 | 220 | description=_("The person or team who can edit this recipe."))) | 202 | description=_("The person or team who can edit this recipe."))) |
413 | 221 | 203 | ||
418 | 222 | distroseries = CollectionField( | 204 | distroseries = exported(List( |
419 | 223 | Reference(IDistroSeries), title=_("The distroseries this recipe will" | 205 | ReferenceChoice(schema=IDistroSeries, |
420 | 224 | " build a source package for"), | 206 | vocabulary='BuildableDistroSeries'), |
421 | 225 | readonly=False) | 207 | title=_("Default distribution series"), |
422 | 208 | description=_("If built daily, these are the distribution " | ||
423 | 209 | "versions that the recipe will be built for."), | ||
424 | 210 | readonly=True)) | ||
425 | 226 | build_daily = exported(Bool( | 211 | build_daily = exported(Bool( |
426 | 227 | title=_("Built daily"), | 212 | title=_("Built daily"), |
427 | 228 | description=_( | 213 | description=_( |
428 | @@ -245,6 +230,34 @@ | |||
429 | 245 | is_stale = Bool(title=_('Recipe is stale.')) | 230 | is_stale = Bool(title=_('Recipe is stale.')) |
430 | 246 | 231 | ||
431 | 247 | 232 | ||
432 | 233 | class ISourcePackageRecipeEdit(Interface): | ||
433 | 234 | """ISourcePackageRecipe methods that require launchpad.Edit permission.""" | ||
434 | 235 | |||
435 | 236 | @mutator_for(ISourcePackageRecipeView['recipe_text']) | ||
436 | 237 | @operation_for_version("devel") | ||
437 | 238 | @operation_parameters( | ||
438 | 239 | recipe_text=copy_field( | ||
439 | 240 | ISourcePackageRecipeView['recipe_text'])) | ||
440 | 241 | @export_write_operation() | ||
441 | 242 | def setRecipeText(recipe_text): | ||
442 | 243 | """Set the text of the recipe.""" | ||
443 | 244 | |||
444 | 245 | @mutator_for(ISourcePackageRecipeEditableAttributes['distroseries']) | ||
445 | 246 | @operation_parameters(distroseries=copy_field( | ||
446 | 247 | ISourcePackageRecipeEditableAttributes['distroseries'])) | ||
447 | 248 | @export_write_operation() | ||
448 | 249 | @operation_for_version("devel") | ||
449 | 250 | def updateSeries(distroseries): | ||
450 | 251 | """Replace this recipe's distro series.""" | ||
451 | 252 | |||
452 | 253 | def destroySelf(): | ||
453 | 254 | """Remove this SourcePackageRecipe from the database. | ||
454 | 255 | |||
455 | 256 | This requires deleting any rows with non-nullable foreign key | ||
456 | 257 | references to this object. | ||
457 | 258 | """ | ||
458 | 259 | |||
459 | 260 | |||
460 | 248 | class ISourcePackageRecipe(ISourcePackageRecipeData, | 261 | class ISourcePackageRecipe(ISourcePackageRecipeData, |
461 | 249 | ISourcePackageRecipeEdit, ISourcePackageRecipeEditableAttributes, | 262 | ISourcePackageRecipeEdit, ISourcePackageRecipeEditableAttributes, |
462 | 250 | ISourcePackageRecipeView): | 263 | ISourcePackageRecipeView): |
463 | 251 | 264 | ||
464 | === modified file 'lib/lp/code/javascript/requestbuild_overlay.js' | |||
465 | --- lib/lp/code/javascript/requestbuild_overlay.js 2011-03-11 23:54:32 +0000 | |||
466 | +++ lib/lp/code/javascript/requestbuild_overlay.js 2011-03-22 02:47:23 +0000 | |||
467 | @@ -250,7 +250,7 @@ | |||
468 | 250 | * Return: true if data is valid | 250 | * Return: true if data is valid |
469 | 251 | */ | 251 | */ |
470 | 252 | function validate(data) { | 252 | function validate(data) { |
472 | 253 | var distros = data['field.distros'] | 253 | var distros = data['field.distroseries'] |
473 | 254 | if (Y.Object.size(distros) == 0) { | 254 | if (Y.Object.size(distros) == 0) { |
474 | 255 | request_build_response_handler.showError( | 255 | request_build_response_handler.showError( |
475 | 256 | "You need to specify at least one distro series for " + | 256 | "You need to specify at least one distro series for " + |
476 | @@ -391,7 +391,7 @@ | |||
477 | 391 | 391 | ||
478 | 392 | function get_distroseries_nodes() { | 392 | function get_distroseries_nodes() { |
479 | 393 | return request_build_overlay.form_node.all( | 393 | return request_build_overlay.form_node.all( |
481 | 394 | "label[for^='field.distros.']"); | 394 | "label[for^='field.distroseries.']"); |
482 | 395 | } | 395 | } |
483 | 396 | 396 | ||
484 | 397 | var DISABLED_DISTROSERIES_CHECKBOX_HTML = | 397 | var DISABLED_DISTROSERIES_CHECKBOX_HTML = |
485 | 398 | 398 | ||
486 | === modified file 'lib/lp/code/model/sourcepackagerecipe.py' | |||
487 | --- lib/lp/code/model/sourcepackagerecipe.py 2011-03-18 06:47:15 +0000 | |||
488 | +++ lib/lp/code/model/sourcepackagerecipe.py 2011-03-22 02:47:23 +0000 | |||
489 | @@ -187,6 +187,12 @@ | |||
490 | 187 | def recipe_text(self): | 187 | def recipe_text(self): |
491 | 188 | return self.builder_recipe.get_recipe_text() | 188 | return self.builder_recipe.get_recipe_text() |
492 | 189 | 189 | ||
493 | 190 | def updateSeries(self, distroseries): | ||
494 | 191 | if distroseries != self.distroseries: | ||
495 | 192 | self.distroseries.clear() | ||
496 | 193 | for distroseries_item in distroseries: | ||
497 | 194 | self.distroseries.add(distroseries_item) | ||
498 | 195 | |||
499 | 190 | @staticmethod | 196 | @staticmethod |
500 | 191 | def new(registrant, owner, name, recipe, description, | 197 | def new(registrant, owner, name, recipe, description, |
501 | 192 | distroseries=None, daily_build_archive=None, build_daily=False, | 198 | distroseries=None, daily_build_archive=None, build_daily=False, |
502 | 193 | 199 | ||
503 | === modified file 'lib/lp/code/templates/sourcepackagerecipe-index.pt' | |||
504 | --- lib/lp/code/templates/sourcepackagerecipe-index.pt 2011-03-02 01:47:19 +0000 | |||
505 | +++ lib/lp/code/templates/sourcepackagerecipe-index.pt 2011-03-22 02:47:23 +0000 | |||
506 | @@ -118,15 +118,8 @@ | |||
507 | 118 | <dt>Daily build archive:</dt> | 118 | <dt>Daily build archive:</dt> |
508 | 119 | <dd tal:content="structure view/archive_picker"/> | 119 | <dd tal:content="structure view/archive_picker"/> |
509 | 120 | </dl> | 120 | </dl> |
519 | 121 | 121 | <dl id="distroseries"> | |
520 | 122 | <dl id="distros"> | 122 | <tal:distroseries tal:replace="structure view/distroseries_widget"/> |
512 | 123 | <dt>Distribution series:</dt> | ||
513 | 124 | <dd> | ||
514 | 125 | <ul> | ||
515 | 126 | <li tal:repeat="curseries context/distroseries" | ||
516 | 127 | tal:content="structure curseries/fmt:link" /> | ||
517 | 128 | </ul> | ||
518 | 129 | </dd> | ||
521 | 130 | </dl> | 123 | </dl> |
522 | 131 | </div> | 124 | </div> |
523 | 132 | </div> | 125 | </div> |
524 | 133 | 126 | ||
525 | === modified file 'lib/lp/code/templates/sourcepackagerecipe-new.pt' | |||
526 | --- lib/lp/code/templates/sourcepackagerecipe-new.pt 2011-02-14 01:48:57 +0000 | |||
527 | +++ lib/lp/code/templates/sourcepackagerecipe-new.pt 2011-03-22 02:47:23 +0000 | |||
528 | @@ -99,7 +99,7 @@ | |||
529 | 99 | </tal:widget> | 99 | </tal:widget> |
530 | 100 | </tal:create-ppa> | 100 | </tal:create-ppa> |
531 | 101 | 101 | ||
533 | 102 | <tal:widget define="widget nocall:view/widgets/distros"> | 102 | <tal:widget define="widget nocall:view/widgets/distroseries"> |
534 | 103 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> | 103 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
535 | 104 | </tal:widget> | 104 | </tal:widget> |
536 | 105 | <tal:widget define="widget nocall:view/widgets/recipe_text"> | 105 | <tal:widget define="widget nocall:view/widgets/recipe_text"> |
537 | 106 | 106 | ||
538 | === modified file 'lib/lp/code/templates/sourcepackagerecipe-request-builds.pt' | |||
539 | --- lib/lp/code/templates/sourcepackagerecipe-request-builds.pt 2010-03-31 19:27:07 +0000 | |||
540 | +++ lib/lp/code/templates/sourcepackagerecipe-request-builds.pt 2011-03-22 02:47:23 +0000 | |||
541 | @@ -16,7 +16,7 @@ | |||
542 | 16 | <tal:widget define="widget nocall:view/widgets/archive"> | 16 | <tal:widget define="widget nocall:view/widgets/archive"> |
543 | 17 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> | 17 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
544 | 18 | </tal:widget> | 18 | </tal:widget> |
546 | 19 | <tal:widget define="widget nocall:view/widgets/distros"> | 19 | <tal:widget define="widget nocall:view/widgets/distroseries"> |
547 | 20 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> | 20 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
548 | 21 | </tal:widget> | 21 | </tal:widget> |
549 | 22 | </table> | 22 | </table> |
550 | 23 | 23 | ||
551 | === added file 'lib/lp/code/windmill/tests/test_recipe_inline_distroseries_edit.py' | |||
552 | --- lib/lp/code/windmill/tests/test_recipe_inline_distroseries_edit.py 1970-01-01 00:00:00 +0000 | |||
553 | +++ lib/lp/code/windmill/tests/test_recipe_inline_distroseries_edit.py 2011-03-22 02:47:23 +0000 | |||
554 | @@ -0,0 +1,71 @@ | |||
555 | 1 | # Copyright 2011 Canonical Ltd. This software is licensed under the | ||
556 | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). | ||
557 | 3 | |||
558 | 4 | """Tests for requesting recipe builds.""" | ||
559 | 5 | |||
560 | 6 | __metaclass__ = type | ||
561 | 7 | __all__ = [] | ||
562 | 8 | |||
563 | 9 | import transaction | ||
564 | 10 | |||
565 | 11 | from zope.component import getUtility | ||
566 | 12 | from storm.store import Store | ||
567 | 13 | |||
568 | 14 | from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities | ||
569 | 15 | from lp.testing import WindmillTestCase | ||
570 | 16 | from lp.testing.windmill.constants import ( | ||
571 | 17 | FOR_ELEMENT, | ||
572 | 18 | PAGE_LOAD, | ||
573 | 19 | ) | ||
574 | 20 | from lp.code.model.sourcepackagerecipe import SourcePackageRecipe | ||
575 | 21 | from lp.code.windmill.testing import CodeWindmillLayer | ||
576 | 22 | |||
577 | 23 | |||
578 | 24 | class TestRecipeEdit(WindmillTestCase): | ||
579 | 25 | """Test recipe editing with inline widgets.""" | ||
580 | 26 | |||
581 | 27 | layer = CodeWindmillLayer | ||
582 | 28 | suite_name = "Request recipe build" | ||
583 | 29 | |||
584 | 30 | def test_inline_distroseries_edit(self): | ||
585 | 31 | """Test that inline editing of distroseries works.""" | ||
586 | 32 | |||
587 | 33 | chef = self.factory.makePerson( | ||
588 | 34 | displayname='Master Chef', name='chef', password='test', | ||
589 | 35 | email="chef@example.com") | ||
590 | 36 | recipe = self.factory.makeSourcePackageRecipe(owner=chef) | ||
591 | 37 | transaction.commit() | ||
592 | 38 | |||
593 | 39 | client, start_url = self.getClientFor(recipe, user=chef) | ||
594 | 40 | client.waits.forElement( | ||
595 | 41 | id=u'edit-distroseries-items', timeout=PAGE_LOAD) | ||
596 | 42 | |||
597 | 43 | # Edit the distro series. | ||
598 | 44 | client.click(jquery=u'("#edit-distroseries-btn")[0]') | ||
599 | 45 | client.waits.forElement( | ||
600 | 46 | jquery=u'("#edit-distroseries-save")', | ||
601 | 47 | timeout=FOR_ELEMENT) | ||
602 | 48 | # Click the checkbox to select the first distro series | ||
603 | 49 | client.click(name=u'field.distroseries.0') | ||
604 | 50 | client.waits.forElement( | ||
605 | 51 | jquery=u"('[name=\"field.distroseries.0\"][checked=\"checked\"]')", | ||
606 | 52 | timeout=FOR_ELEMENT) | ||
607 | 53 | # Save it | ||
608 | 54 | client.click(jquery=u'("#edit-distroseries-save")[0]') | ||
609 | 55 | |||
610 | 56 | # Wait for the the new one that is added. | ||
611 | 57 | client.waits.forElement( | ||
612 | 58 | jquery=u"('#edit-distroseries-items ul li a')[0]", | ||
613 | 59 | timeout=FOR_ELEMENT) | ||
614 | 60 | |||
615 | 61 | # Check that the new data was saved. | ||
616 | 62 | transaction.commit() | ||
617 | 63 | hoary = getUtility(ILaunchpadCelebrities).ubuntu['hoary'] | ||
618 | 64 | store = Store.of(recipe) | ||
619 | 65 | saved_recipe = store.find( | ||
620 | 66 | SourcePackageRecipe, | ||
621 | 67 | SourcePackageRecipe.name==recipe.name).one() | ||
622 | 68 | self.assertEqual(len(list(saved_recipe.distroseries)), 2) | ||
623 | 69 | distroseries=sorted( | ||
624 | 70 | saved_recipe.distroseries, key=lambda ds: ds.displayname) | ||
625 | 71 | self.assertEqual(distroseries[0], hoary) |
This looks pretty good.
I wonder whether label_tag="dt" and items_tag="dd" could be defaults.
Sorry for using distros rather than distroseries. I was new to the problem domain.
I don't think you should have a setUp in RecipeEdit until have more than one test, because until you have more tests, you won't know what needs to vary.
I also see you setting up far more constants than you actually use, e.g. recipe name. Why not just use makeSourcePacka geRecipe? (you could use "with person_ logged_ in(recipe. owner)" .
The implementation of updateSeries is a bit sad. Is there no straightforward way to update that?