Merge ~ilasc/launchpad:multiple-tracks-charm-recipe into launchpad:master
- Git
- lp:~ilasc/launchpad
- multiple-tracks-charm-recipe
- Merge into master
Status: | Merged |
---|---|
Approved by: | Ioana Lasc |
Approved revision: | b3ed298edb4ec0800929c50256ce3245d6bde105 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~ilasc/launchpad:multiple-tracks-charm-recipe |
Merge into: | launchpad:master |
Diff against target: |
1406 lines (+783/-284) 12 files modified
lib/lp/app/templates/launchpad-widget-macros.pt (+8/-6) lib/lp/charms/browser/tests/test_charmrecipe.py (+319/-3) lib/lp/charms/templates/charmrecipe-edit.pt (+19/-2) lib/lp/charms/templates/charmrecipe-new.pt (+1/-2) lib/lp/snappy/browser/tests/test_snap.py (+19/-7) lib/lp/snappy/browser/widgets/storechannels.py (+152/-80) lib/lp/snappy/browser/widgets/templates/storechannels.pt (+67/-29) lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py (+176/-130) lib/lp/snappy/templates/snap-edit.pt (+19/-2) lib/lp/snappy/templates/snap-new.pt (+1/-2) lib/lp/snappy/validators/channels.py (+0/-11) lib/lp/snappy/validators/tests/test_channels.py (+2/-10) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Review via email: mp+423161@code.launchpad.net |
Commit message
Enable multiple tracks for a single charm recipe
Description of the change
Ioana Lasc (ilasc) wrote (last edit ): | # |
Committed a stripped down version that shows only the new buttons to add rows with store channel data in a table above the StoreChannelWidget. Took out all the data parsing and the table and only left in the buttons as the current design suffers from a problem when attempting to submit the new store channel row: it submits the entire form and results in an "required input is missing" on the charm recipe name (unit test added to reflect this).
What we're trying to do here is different from the Git permissions view or the OCI Credentials view where the new row table is added by submitting the entire form. Here we would need a form inside a form to be able to add multiple rows before submitting the entire recipe creation form (https:/
Ioana Lasc (ilasc) wrote : | # |
Still some layout things to work out for the Edit Charm Recipe screen but otherwise ready for a first review:
1: removing labels from the Risks widgets in the edit existing channels table - current method (setting the 'display_label' to false) fails to completely remove the label - semicolon still visible.
2: deciding on whether to make the existing channels table a subordinate or align it under "Automatically upload to store" ? Current layout suffers from the fact that it is now emphasizing the different spacing / alignment between the cells of the edit table and the subwidgets of the StoreChannelsWidget used to add a new widget.
3: Are we also allowing different branches as input to the same recipe ? (if not I need to add validation as it is currently possible to edit that way after which the screen crashes with "Channels belong to different branches")
Screenshot of current layout state:
https:/
Ioana Lasc (ilasc) wrote : | # |
Managed to remove the labels (issue 1 in comments above) and align everything in the table (issue 2) that leaves only the third issue as an open question for this.
Current layout of the screen:
https:/
Colin Watson (cjwatson) wrote : | # |
Make sure to rebase this onto master and run something like `pre-commit run --files $(git diff --name-only master)` to force all the `pre-commit` hooks to run on all the files you've changed, since you started this before we reformatted everything using `black`.
This is definitely looking better than the last time I reviewed it; sorry for taking so very long to get back to you! I have a few general comments which don't really fit into any one place.
Widget usage
============
The idea of widgets is to encapsulate common bits of web UI and form machinery that are used by multiple views - they essentially work like sub-views and are responsible for their own subtree of the HTML form data, rooted at whatever `self.name` says. One of the things you've done here is to pull much of the handling of store channels out of the widget and put it directly in the views and templates for charm recipes. I think that's the wrong direction. I realize that the particular bug you're working on here only talks about charm recipes, but where possible we should keep the two in sync.
Here's the rough design I'd like to see implemented here:
`StoreChannelsW
Instead of just having three subwidgets, `StoreChannelsW
The template changes would go in `lib/lp/
If done correctly, I think there's a good chance that the snap and charm recipe views and templates could remain entirely unchanged, although the view tests would need some minor adjustments for the new HTML form data structure. Most of the test changes, however, should be specific to the widget, and would go in `lp.snappy.
UI layout
=========
The risk checkboxes are reasonably compact now, which is a definite improvement. However, I think we could do better.
Firstly, now that we have a structure that explicitly allows for selecting multiple channels, there isn't really a good reason to allow selecting multiple risks for each channel. As such, each "risks" field should be simply "risk", and it should be a radio button widget rather than a multi-checkbox widget.
Secondly, it would be well worth our time for the web UI to progressively enhance each "risk" radio button field into a pop-up choice selection widget if JavaScript is av...
Ioana Lasc (ilasc) wrote : | # |
Thanks Colin!
Changes are now contained within the StoreChannelsWidget that is usable for Create & Edit Snap and Charm recipes and ready to be reviewed.
The layout issue (around widget labels) noted during pulse review is still on my To Do list for this but they might be easier to make once we get this to a functionally stable state.
Colin Watson (cjwatson) : | # |
Ioana Lasc (ilasc) wrote : | # |
Thanks Colin!
By enforcing a default value on the Risk dropdown widget we caused the Edit Recipe (Snap & Charm) to add a new channel with the default risk (Edge) when the Update Recipe button is clicked. We need to give the user the ability to just edit existing channels on these edit forms, for this reason we want to allow the drop down to contain the "Nothing selected" option. This resulted in 2 types of validation necessary for the Add widgets row:
1: on the Add screens the Risk widget is required
2: on the Edit screens the Risk is required only if any of Track or Branch are filled in.
MP ready for another review.
Colin Watson (cjwatson) wrote : | # |
Great. Just a few non-blocking remarks now, as far as I can see. Thanks!
Preview Diff
1 | diff --git a/lib/lp/app/templates/launchpad-widget-macros.pt b/lib/lp/app/templates/launchpad-widget-macros.pt |
2 | index f50f4fd..c25d6fc 100644 |
3 | --- a/lib/lp/app/templates/launchpad-widget-macros.pt |
4 | +++ b/lib/lp/app/templates/launchpad-widget-macros.pt |
5 | @@ -28,12 +28,14 @@ |
6 | <div tal:attributes="class python:error and 'error' or None" |
7 | tal:define="error widget/error"> |
8 | |
9 | - <label tal:attributes="for widget/name" |
10 | - tal:content="string:${widget/label}:">Label</label> |
11 | - |
12 | - <span tal:condition="widget/required" |
13 | - class="fieldRequired" |
14 | - title="Required">(Required)</span> |
15 | + <tal:label condition="display_label|widget/display_label|python:True"> |
16 | + <label tal:attributes="for widget/name" |
17 | + tal:content="string:${widget/label}:">Label</label> |
18 | + |
19 | + <span tal:condition="widget/required" |
20 | + class="fieldRequired" |
21 | + title="Required">(Required)</span> |
22 | + </tal:label> |
23 | |
24 | <div tal:content="structure widget"> |
25 | <input type="text" style="width:100%"/> |
26 | diff --git a/lib/lp/charms/browser/tests/test_charmrecipe.py b/lib/lp/charms/browser/tests/test_charmrecipe.py |
27 | index 0a0a377..7db9c4c 100644 |
28 | --- a/lib/lp/charms/browser/tests/test_charmrecipe.py |
29 | +++ b/lib/lp/charms/browser/tests/test_charmrecipe.py |
30 | @@ -325,8 +325,12 @@ class TestCharmRecipeAddView(BaseTestCharmRecipeView): |
31 | browser.getControl("Automatically upload to store").selected = True |
32 | browser.getControl("Registered store name").value = "charmhub-name" |
33 | self.assertFalse(browser.getControl("Stable").selected) |
34 | - browser.getControl(name="field.store_channels.track").value = "track" |
35 | - browser.getControl("Edge").selected = True |
36 | + browser.getControl( |
37 | + name="field.store_channels.add_track" |
38 | + ).value = "track" |
39 | + browser.getControl(name="field.store_channels.add_risk").value = [ |
40 | + "edge" |
41 | + ] |
42 | root_macaroon = Macaroon(version=2) |
43 | root_macaroon.add_third_party_caveat( |
44 | "https://candid.test/", "", "identity" |
45 | @@ -468,6 +472,80 @@ class TestCharmRecipeAddView(BaseTestCharmRecipeView): |
46 | ), |
47 | ) |
48 | |
49 | + def test_create_new_recipe_multiple_tracks_missing_recipe_name(self): |
50 | + # Missing charm recipe will result in error in browser |
51 | + self.factory.makeProduct( |
52 | + name="test-project", displayname="Test Project" |
53 | + ) |
54 | + [git_ref] = self.factory.makeGitRefs() |
55 | + view_url = canonical_url(git_ref, view_name="+new-charm-recipe") |
56 | + browser = self.getNonRedirectingBrowser(url=view_url, user=self.person) |
57 | + browser.getControl(name="field.project").value = "test-project" |
58 | + browser.getControl("Automatically upload to store").selected = True |
59 | + browser.getControl("Registered store name").value = "charmhub-name" |
60 | + self.assertFalse(browser.getControl("Stable").selected) |
61 | + browser.getControl( |
62 | + name="field.store_channels.add_track" |
63 | + ).value = "track" |
64 | + browser.getControl("Edge").selected = True |
65 | + browser.getControl( |
66 | + name="field.store_channels.add_branch" |
67 | + ).value = "branch" |
68 | + |
69 | + browser.getControl("Create charm recipe").click() |
70 | + |
71 | + self.assertIn("Required input is missing.", browser.contents) |
72 | + |
73 | + def test_create_new_recipe_missing_channel_risk(self): |
74 | + # No track or branch selected |
75 | + self.factory.makeProduct( |
76 | + name="test-project", displayname="Test Project" |
77 | + ) |
78 | + [git_ref] = self.factory.makeGitRefs() |
79 | + view_url = canonical_url(git_ref, view_name="+new-charm-recipe") |
80 | + browser = self.getNonRedirectingBrowser(url=view_url, user=self.person) |
81 | + browser.getControl(name="field.project").value = "test-project" |
82 | + browser.getControl("Automatically upload to store").selected = True |
83 | + browser.getControl(name="field.name").value = "test-recipe-name" |
84 | + browser.getControl("Registered store name").value = "charmhub-name" |
85 | + |
86 | + browser.getControl("Create charm recipe").click() |
87 | + |
88 | + self.assertIn("You must select a risk.", browser.contents) |
89 | + |
90 | + # Entering only the track is not enough |
91 | + view_url = canonical_url(git_ref, view_name="+new-charm-recipe") |
92 | + browser = self.getNonRedirectingBrowser(url=view_url, user=self.person) |
93 | + browser.getControl(name="field.project").value = "test-project" |
94 | + browser.getControl("Automatically upload to store").selected = True |
95 | + browser.getControl(name="field.name").value = "test-recipe-name" |
96 | + browser.getControl("Registered store name").value = "charmhub-name" |
97 | + browser.getControl( |
98 | + name="field.store_channels.add_track" |
99 | + ).value = "new-track" |
100 | + |
101 | + browser.getControl("Create charm recipe").click() |
102 | + |
103 | + self.assertIn("You must select a risk.", browser.contents) |
104 | + |
105 | + # Entering only the track and branch will error |
106 | + view_url = canonical_url(git_ref, view_name="+new-charm-recipe") |
107 | + browser = self.getNonRedirectingBrowser(url=view_url, user=self.person) |
108 | + browser.getControl(name="field.project").value = "test-project" |
109 | + browser.getControl("Automatically upload to store").selected = True |
110 | + browser.getControl(name="field.name").value = "test-recipe-name" |
111 | + browser.getControl("Registered store name").value = "charmhub-name" |
112 | + browser.getControl( |
113 | + name="field.store_channels.add_track" |
114 | + ).value = "new-track" |
115 | + browser.getControl( |
116 | + name="field.store_channels.add_branch" |
117 | + ).value = "new-branch" |
118 | + |
119 | + browser.getControl("Create charm recipe").click() |
120 | + |
121 | + self.assertIn("You must select a risk.", browser.contents) |
122 | + |
123 | |
124 | class TestCharmRecipeAdminView(BaseTestCharmRecipeView): |
125 | def test_unauthorized(self): |
126 | @@ -528,7 +606,10 @@ class TestCharmRecipeEditView(BaseTestCharmRecipeView): |
127 | def test_edit_recipe(self): |
128 | [old_git_ref] = self.factory.makeGitRefs() |
129 | recipe = self.factory.makeCharmRecipe( |
130 | - registrant=self.person, owner=self.person, git_ref=old_git_ref |
131 | + registrant=self.person, |
132 | + owner=self.person, |
133 | + git_ref=old_git_ref, |
134 | + store_channels=["track1/stable/branch1", "track2/edge/branch1"], |
135 | ) |
136 | self.factory.makeTeam( |
137 | name="new-team", displayname="New Team", members=[self.person] |
138 | @@ -573,6 +654,241 @@ class TestCharmRecipeEditView(BaseTestCharmRecipeView): |
139 | MatchesTagText(content, "store_upload"), |
140 | ) |
141 | |
142 | + @responses.activate |
143 | + def test_edit_recipe_add_store_channel(self): |
144 | + self.pushConfig("charms", charmhub_url="http://charmhub.example/") |
145 | + self.pushConfig( |
146 | + "launchpad", |
147 | + candid_service_root="https://candid.test/", |
148 | + csrf_secret="test secret", |
149 | + ) |
150 | + [git_ref] = self.factory.makeGitRefs() |
151 | + recipe = self.factory.makeCharmRecipe( |
152 | + registrant=self.person, |
153 | + owner=self.person, |
154 | + git_ref=git_ref, |
155 | + store_channels=["track1/stable/branch1", "track2/edge/branch1"], |
156 | + store_name="Store name", |
157 | + store_upload=True, |
158 | + ) |
159 | + self.factory.makeTeam( |
160 | + name="new-team", displayname="New Team", members=[self.person] |
161 | + ) |
162 | + view_url = canonical_url(recipe, view_name="+edit") |
163 | + browser = self.getNonRedirectingBrowser(url=view_url, user=self.person) |
164 | + browser.getControl( |
165 | + name="field.store_channels.add_track" |
166 | + ).value = "new-track" |
167 | + browser.getControl( |
168 | + name="field.store_channels.add_branch" |
169 | + ).value = "new-branch" |
170 | + browser.getControl(name="field.store_channels.add_risk").value = [ |
171 | + "edge" |
172 | + ] |
173 | + root_macaroon = Macaroon(version=2) |
174 | + root_macaroon.add_third_party_caveat( |
175 | + "https://candid.test/", "", "identity" |
176 | + ) |
177 | + root_macaroon_raw = root_macaroon.serialize(JsonSerializer()) |
178 | + responses.add( |
179 | + "POST", |
180 | + "http://charmhub.example/v1/tokens", |
181 | + json={"macaroon": root_macaroon_raw}, |
182 | + ) |
183 | + responses.add( |
184 | + "POST", |
185 | + "https://candid.test/discharge", |
186 | + status=401, |
187 | + json={ |
188 | + "Code": "interaction required", |
189 | + "Message": ( |
190 | + "macaroon discharge required: authentication required" |
191 | + ), |
192 | + "Info": { |
193 | + "InteractionMethods": { |
194 | + "browser-redirect": { |
195 | + "LoginURL": "https://candid.test/login-redirect", |
196 | + }, |
197 | + }, |
198 | + }, |
199 | + }, |
200 | + ) |
201 | + |
202 | + browser.getControl("Update charm recipe").click() |
203 | + |
204 | + self.assertEqual(303, int(browser.headers["Status"].split(" ", 1)[0])) |
205 | + |
206 | + browser = self.getViewBrowser(recipe, user=self.person) |
207 | + content = find_main_content(browser.contents) |
208 | + self.assertThat( |
209 | + "Store channels:\n" |
210 | + "track1/stable/branch1, " |
211 | + "track2/edge/branch1, new-track/edge/new-branch" |
212 | + "\nEdit charm recipe", |
213 | + MatchesTagText(content, "store_channels"), |
214 | + ) |
215 | + |
216 | + @responses.activate |
217 | + def test_edit_recipe_edit_store_channel_list(self): |
218 | + # Verify we can edit the first store channel defined for this recipe |
219 | + # from "track1/stable/branch1" to "new-track/candidate/new-branch" |
220 | + self.pushConfig("charms", charmhub_url="http://charmhub.example/") |
221 | + self.pushConfig( |
222 | + "launchpad", |
223 | + candid_service_root="https://candid.test/", |
224 | + csrf_secret="test secret", |
225 | + ) |
226 | + [git_ref] = self.factory.makeGitRefs() |
227 | + recipe = self.factory.makeCharmRecipe( |
228 | + registrant=self.person, |
229 | + owner=self.person, |
230 | + git_ref=git_ref, |
231 | + store_channels=["track1/stable/branch1", "track2/edge/branch1"], |
232 | + store_name="Store name", |
233 | + store_upload=True, |
234 | + ) |
235 | + self.factory.makeTeam( |
236 | + name="new-team", displayname="New Team", members=[self.person] |
237 | + ) |
238 | + view_url = canonical_url(recipe, view_name="+edit") |
239 | + browser = self.getNonRedirectingBrowser(url=view_url, user=self.person) |
240 | + browser.getControl( |
241 | + name="field.store_channels.track_0" |
242 | + ).value = "new-track" |
243 | + browser.getControl( |
244 | + name="field.store_channels.branch_0" |
245 | + ).value = "new-branch" |
246 | + browser.getControl(name="field.store_channels.risk_0").value = [ |
247 | + "candidate" |
248 | + ] |
249 | + root_macaroon = Macaroon(version=2) |
250 | + root_macaroon.add_third_party_caveat( |
251 | + "https://candid.test/", "", "identity" |
252 | + ) |
253 | + root_macaroon_raw = root_macaroon.serialize(JsonSerializer()) |
254 | + responses.add( |
255 | + "POST", |
256 | + "http://charmhub.example/v1/tokens", |
257 | + json={"macaroon": root_macaroon_raw}, |
258 | + ) |
259 | + responses.add( |
260 | + "POST", |
261 | + "https://candid.test/discharge", |
262 | + status=401, |
263 | + json={ |
264 | + "Code": "interaction required", |
265 | + "Message": ( |
266 | + "macaroon discharge required: authentication required" |
267 | + ), |
268 | + "Info": { |
269 | + "InteractionMethods": { |
270 | + "browser-redirect": { |
271 | + "LoginURL": "https://candid.test/login-redirect", |
272 | + }, |
273 | + }, |
274 | + }, |
275 | + }, |
276 | + ) |
277 | + |
278 | + browser.getControl("Update charm recipe").click() |
279 | + |
280 | + self.assertEqual(303, int(browser.headers["Status"].split(" ", 1)[0])) |
281 | + |
282 | + browser = self.getViewBrowser(recipe, user=self.person) |
283 | + content = find_main_content(browser.contents) |
284 | + self.assertThat( |
285 | + "Store channels:\n" |
286 | + "new-track/candidate/new-branch, track2/edge/branch1" |
287 | + "\nEdit charm recipe", |
288 | + MatchesTagText(content, "store_channels"), |
289 | + ) |
290 | + |
291 | + def test_edit_recipe_delete_store_channel_list(self): |
292 | + # Verify we can edit the first store channel defined for this recipe |
293 | + # from "track1/stable/branch1" to "new-track/candidate/new-branch" |
294 | + [git_ref] = self.factory.makeGitRefs() |
295 | + recipe = self.factory.makeCharmRecipe( |
296 | + registrant=self.person, |
297 | + owner=self.person, |
298 | + git_ref=git_ref, |
299 | + store_channels=["track1/stable/branch1", "track2/edge/branch1"], |
300 | + store_name="Store name", |
301 | + store_upload=True, |
302 | + ) |
303 | + self.factory.makeTeam( |
304 | + name="new-team", displayname="New Team", members=[self.person] |
305 | + ) |
306 | + browser = self.getViewBrowser(recipe, user=self.person) |
307 | + browser.getLink("Edit charm recipe").click() |
308 | + browser.getControl(name="field.store_channels.delete_0").value = 1 |
309 | + |
310 | + browser.getControl("Update charm recipe").click() |
311 | + |
312 | + content = find_main_content(browser.contents) |
313 | + self.assertThat( |
314 | + "Store channels:\n" "track2/edge/branch1" "\nEdit charm recipe", |
315 | + MatchesTagText(content, "store_channels"), |
316 | + ) |
317 | + |
318 | + def test_edit_recipe_missing_channel_risk(self): |
319 | + # On the edit form we can also add a new channel. |
320 | + # The only valid combination for the |
321 | + # new channel entry on this form are: |
322 | + # 1: nothing selected - allows the user to edit existent channels, |
323 | + # without adding a new empty channel row when pressing Update Recipe. |
324 | + # 2: Risk selected and any combination of track / branch entered |
325 | + [git_ref] = self.factory.makeGitRefs() |
326 | + recipe = self.factory.makeCharmRecipe( |
327 | + registrant=self.person, |
328 | + owner=self.person, |
329 | + git_ref=git_ref, |
330 | + store_channels=["track1/stable/branch1", "track2/edge/branch1"], |
331 | + store_name="Store name", |
332 | + store_upload=True, |
333 | + ) |
334 | + self.factory.makeTeam( |
335 | + name="new-team", displayname="New Team", members=[self.person] |
336 | + ) |
337 | + browser = self.getViewBrowser(recipe, user=self.person) |
338 | + browser.getLink("Edit charm recipe").click() |
339 | + |
340 | + # If the user entered only Track for the new channel entry, |
341 | + # ensure we error with missing Risk message. |
342 | + browser.getControl( |
343 | + name="field.store_channels.add_track" |
344 | + ).value = "new-track" |
345 | + |
346 | + browser.getControl("Update charm recipe").click() |
347 | + |
348 | + self.assertIn("You must select a risk.", browser.contents) |
349 | + |
350 | + # If the user entered only Branch for the new channel entry, |
351 | + # ensure we error with missing Risk message. |
352 | + browser = self.getViewBrowser(recipe, user=self.person) |
353 | + browser.getLink("Edit charm recipe").click() |
354 | + browser.getControl( |
355 | + name="field.store_channels.add_branch" |
356 | + ).value = "new-branch" |
357 | + |
358 | + browser.getControl("Update charm recipe").click() |
359 | + |
360 | + self.assertIn("You must select a risk.", browser.contents) |
361 | + |
362 | + # For Track and Branch entered but no Risk selected, |
363 | + # ensure we error with missing Risk message. |
364 | + browser = self.getViewBrowser(recipe, user=self.person) |
365 | + browser.getLink("Edit charm recipe").click() |
366 | + browser.getControl( |
367 | + name="field.store_channels.add_track" |
368 | + ).value = "new-track" |
369 | + browser.getControl( |
370 | + name="field.store_channels.add_branch" |
371 | + ).value = "new-branch" |
372 | + |
373 | + browser.getControl("Update charm recipe").click() |
374 | + |
375 | + self.assertIn("You must select a risk.", browser.contents) |
376 | + |
377 | def test_edit_recipe_sets_date_last_modified(self): |
378 | # Editing a charm recipe sets the date_last_modified property. |
379 | date_created = datetime(2000, 1, 1, tzinfo=pytz.UTC) |
380 | diff --git a/lib/lp/charms/templates/charmrecipe-edit.pt b/lib/lp/charms/templates/charmrecipe-edit.pt |
381 | index 81172b8..a68bd40 100644 |
382 | --- a/lib/lp/charms/templates/charmrecipe-edit.pt |
383 | +++ b/lib/lp/charms/templates/charmrecipe-edit.pt |
384 | @@ -11,6 +11,24 @@ |
385 | <style type="text/css"> |
386 | .subordinate { |
387 | margin: 0.5em 0 0.5em 4em; |
388 | + max-width: 80%; |
389 | + margin-bottom: 1em; |
390 | + } |
391 | + table.subordinate tr.even { |
392 | + background-color: #eee; |
393 | + } |
394 | + /* These add up to 100%. */ |
395 | + tr .channel-row-track { |
396 | + width: 35%; |
397 | + } |
398 | + tr .channel-row-risk { |
399 | + width: 25%; |
400 | + } |
401 | + tr .channel-row-branch { |
402 | + width: 35%; |
403 | + } |
404 | + tr .channel-row-delete { |
405 | + width: 5%; |
406 | } |
407 | </style> |
408 | </metal:block> |
409 | @@ -55,8 +73,7 @@ |
410 | <tal:widget define="widget nocall:view/widgets/store_name"> |
411 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
412 | </tal:widget> |
413 | - <tal:widget define="widget nocall:view/widgets/store_channels" |
414 | - condition="widget/has_risks_vocabulary"> |
415 | + <tal:widget define="widget nocall:view/widgets/store_channels"> |
416 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
417 | </tal:widget> |
418 | </table> |
419 | diff --git a/lib/lp/charms/templates/charmrecipe-new.pt b/lib/lp/charms/templates/charmrecipe-new.pt |
420 | index f5e9a0a..6946d59 100644 |
421 | --- a/lib/lp/charms/templates/charmrecipe-new.pt |
422 | +++ b/lib/lp/charms/templates/charmrecipe-new.pt |
423 | @@ -64,8 +64,7 @@ |
424 | <tal:widget define="widget nocall:view/widgets/store_name"> |
425 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
426 | </tal:widget> |
427 | - <tal:widget define="widget nocall:view/widgets/store_channels" |
428 | - condition="widget/has_risks_vocabulary"> |
429 | + <tal:widget define="widget nocall:view/widgets/store_channels"> |
430 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
431 | </tal:widget> |
432 | </table> |
433 | diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py |
434 | index 10af3a1..4089efc 100644 |
435 | --- a/lib/lp/snappy/browser/tests/test_snap.py |
436 | +++ b/lib/lp/snappy/browser/tests/test_snap.py |
437 | @@ -629,8 +629,12 @@ class TestSnapAddView(BaseTestSnapView): |
438 | "Registered store package name" |
439 | ).value = "store-name" |
440 | self.assertFalse(browser.getControl("Stable").selected) |
441 | - browser.getControl(name="field.store_channels.track").value = "track" |
442 | - browser.getControl("Edge").selected = True |
443 | + browser.getControl( |
444 | + name="field.store_channels.add_track" |
445 | + ).value = "track" |
446 | + browser.getControl(name="field.store_channels.add_risk").value = [ |
447 | + "edge" |
448 | + ] |
449 | root_macaroon = Macaroon() |
450 | root_macaroon.add_third_party_caveat( |
451 | urlsplit(config.launchpad.openid_provider_root).netloc, "", "dummy" |
452 | @@ -1678,10 +1682,18 @@ class TestSnapEditView(BaseTestSnapView): |
453 | view_url = canonical_url(snap, view_name="+edit") |
454 | browser = self.getNonRedirectingBrowser(url=view_url, user=self.person) |
455 | browser.getControl("Registered store package name").value = "two" |
456 | - self.assertEqual("track", browser.getControl("Track").value) |
457 | - self.assertTrue(browser.getControl("Edge").selected) |
458 | - browser.getControl("Track").value = "" |
459 | - browser.getControl("Stable").selected = True |
460 | + self.assertEqual( |
461 | + "track", |
462 | + browser.getControl(name="field.store_channels.track_0").value, |
463 | + ) |
464 | + self.assertEqual( |
465 | + ["edge"], |
466 | + browser.getControl(name="field.store_channels.risk_0").value, |
467 | + ) |
468 | + browser.getControl(name="field.store_channels.track_0").value = "" |
469 | + browser.getControl(name="field.store_channels.risk_0").value = [ |
470 | + "stable" |
471 | + ] |
472 | root_macaroon = Macaroon() |
473 | root_macaroon.add_third_party_caveat( |
474 | urlsplit(config.launchpad.openid_provider_root).netloc, "", "dummy" |
475 | @@ -1700,7 +1712,7 @@ class TestSnapEditView(BaseTestSnapView): |
476 | MatchesStructure.byEquality( |
477 | store_name="two", |
478 | store_secrets={"root": root_macaroon_raw}, |
479 | - store_channels=["stable", "edge"], |
480 | + store_channels=["stable"], |
481 | ), |
482 | ) |
483 | [call] = responses.calls |
484 | diff --git a/lib/lp/snappy/browser/widgets/storechannels.py b/lib/lp/snappy/browser/widgets/storechannels.py |
485 | index 271eac0..af23ff5 100644 |
486 | --- a/lib/lp/snappy/browser/widgets/storechannels.py |
487 | +++ b/lib/lp/snappy/browser/widgets/storechannels.py |
488 | @@ -8,19 +8,14 @@ __all__ = [ |
489 | from zope.browserpage import ViewPageTemplateFile |
490 | from zope.formlib.interfaces import IInputWidget, WidgetInputError |
491 | from zope.formlib.utility import setUpWidget |
492 | -from zope.formlib.widget import ( |
493 | - BrowserWidget, |
494 | - CustomWidgetFactory, |
495 | - InputErrors, |
496 | - InputWidget, |
497 | -) |
498 | +from zope.formlib.widget import BrowserWidget, InputErrors, InputWidget |
499 | from zope.interface import implementer |
500 | -from zope.schema import Choice, List, TextLine |
501 | +from zope.schema import Bool, Choice, TextLine |
502 | |
503 | -from lp import _ |
504 | from lp.app.errors import UnexpectedFormData |
505 | from lp.app.validators import LaunchpadValidationError |
506 | -from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget |
507 | +from lp.charms.interfaces.charmrecipe import ICharmRecipe |
508 | +from lp.registry.enums import StoreRisk |
509 | from lp.services.channels import ( |
510 | CHANNEL_COMPONENTS_DELIMITER, |
511 | channel_list_to_string, |
512 | @@ -30,6 +25,7 @@ from lp.services.webapp.interfaces import ( |
513 | IAlwaysSubmittedWidget, |
514 | ISingleLineWidgetLayout, |
515 | ) |
516 | +from lp.snappy.interfaces.snap import ISnap |
517 | |
518 | |
519 | @implementer(ISingleLineWidgetLayout, IAlwaysSubmittedWidget, IInputWidget) |
520 | @@ -45,87 +41,125 @@ class StoreChannelsWidget(BrowserWidget, InputWidget): |
521 | # disable help_text for the global widget |
522 | self.hint = None |
523 | |
524 | + def _getFieldName(self, name, channel_index): |
525 | + return "%s_%d" % (name, channel_index) |
526 | + |
527 | + @property |
528 | + def show_edit(self): |
529 | + channels = getattr(self.context.context, "store_channels", None) |
530 | + if channels and len(channels) >= 1: |
531 | + return True |
532 | + return False |
533 | + |
534 | + @property |
535 | + def is_edit(self): |
536 | + # XXX ilasc 2022-10-10: Both this and is_add are layering violations |
537 | + # that could make it difficult to reuse this widget in other contexts. |
538 | + # These two checks will disappear when we move to the Javascript |
539 | + # approach for adding a new row. |
540 | + return bool(self.request.get("field.actions.update")) |
541 | + |
542 | + @property |
543 | + def is_add(self): |
544 | + add_operation = self.request.get("field.actions.create") |
545 | + with_upload = self.request.get("field.store_upload") |
546 | + if add_operation and with_upload: |
547 | + if with_upload == "on": |
548 | + return True |
549 | + return False |
550 | + |
551 | + @property |
552 | + def number_of_channels(self): |
553 | + if ICharmRecipe.providedBy(self.context.context) or ISnap.providedBy( |
554 | + self.context.context |
555 | + ): |
556 | + return len(self.context.context.store_channels) |
557 | + return 0 |
558 | + |
559 | def setUpSubWidgets(self): |
560 | if self._widgets_set_up: |
561 | return |
562 | - fields = [ |
563 | - TextLine( |
564 | - __name__="track", |
565 | - title="Track", |
566 | - required=False, |
567 | - description=_( |
568 | - "Track defines a series for your software. " |
569 | - "If not specified, the default track ('latest') is " |
570 | - "assumed." |
571 | + fields = [] |
572 | + channels = getattr(self.context.context, "store_channels", None) |
573 | + if channels: |
574 | + for index in range(len(channels)): |
575 | + fields.extend( |
576 | + [ |
577 | + TextLine( |
578 | + __name__=self._getFieldName("track", index), |
579 | + required=False, |
580 | + ), |
581 | + Choice( |
582 | + __name__=self._getFieldName("risk", index), |
583 | + required=False, |
584 | + default=StoreRisk.EDGE, |
585 | + vocabulary="SnapStoreChannel", |
586 | + ), |
587 | + TextLine( |
588 | + __name__=self._getFieldName("branch", index), |
589 | + required=False, |
590 | + ), |
591 | + Bool( |
592 | + __name__=self._getFieldName("delete", index), |
593 | + required=False, |
594 | + default=False, |
595 | + ), |
596 | + ] |
597 | + ) |
598 | + fields.extend( |
599 | + [ |
600 | + TextLine( |
601 | + __name__="add_track", |
602 | + title="Track", |
603 | + required=False, |
604 | + ), |
605 | + Choice( |
606 | + __name__="add_risk", |
607 | + title="Risk", |
608 | + required=False, |
609 | + default=StoreRisk.EDGE, |
610 | + vocabulary="SnapStoreChannel", |
611 | ), |
612 | - ), |
613 | - List( |
614 | - __name__="risks", |
615 | - title="Risk", |
616 | - required=False, |
617 | - value_type=Choice(vocabulary="SnapStoreChannel"), |
618 | - description=_("Risks denote the stability of your software."), |
619 | - ), |
620 | - TextLine( |
621 | - __name__="branch", |
622 | - title="Branch", |
623 | - required=False, |
624 | - description=_( |
625 | - "Branches provide users with an easy way to test bug " |
626 | - "fixes. They are temporary and created on demand. If " |
627 | - "not specified, no branch is used." |
628 | + TextLine( |
629 | + __name__="add_branch", |
630 | + title="Branch", |
631 | + required=False, |
632 | ), |
633 | - ), |
634 | - ] |
635 | + ] |
636 | + ) |
637 | |
638 | - self.risks_widget = CustomWidgetFactory(LabeledMultiCheckBoxWidget) |
639 | for field in fields: |
640 | setUpWidget( |
641 | self, field.__name__, field, IInputWidget, prefix=self.name |
642 | ) |
643 | - self.risks_widget.orientation = "horizontal" |
644 | - self._widgets_set_up = True |
645 | + widget = getattr(self, "%s_widget" % field.__name__) |
646 | + widget.display_label = False |
647 | + widget.hint = None |
648 | |
649 | - @property |
650 | - def has_risks_vocabulary(self): |
651 | - risks_widget = getattr(self, "risks_widget", None) |
652 | - return risks_widget and bool(risks_widget.vocabulary) |
653 | + self._widgets_set_up = True |
654 | |
655 | def setRenderedValue(self, value): |
656 | """See `IWidget`.""" |
657 | self.setUpSubWidgets() |
658 | if value: |
659 | - # NOTE: atm target channels must belong to the same track and |
660 | - # branch |
661 | - tracks = set() |
662 | - branches = set() |
663 | - risks = [] |
664 | - for channel in value: |
665 | - track, risk, branch = channel_string_to_list(channel) |
666 | - tracks.add(track) |
667 | - risks.append(risk) |
668 | - branches.add(branch) |
669 | - if len(tracks) != 1: |
670 | - raise ValueError( |
671 | - "Channels belong to different tracks: %r" % value |
672 | - ) |
673 | - if len(branches) != 1: |
674 | - raise ValueError( |
675 | - "Channels belong to different branches: %r" % value |
676 | - ) |
677 | - track = tracks.pop() |
678 | - self.track_widget.setRenderedValue(track) |
679 | - self.risks_widget.setRenderedValue(risks) |
680 | - branch = branches.pop() |
681 | - self.branch_widget.setRenderedValue(branch) |
682 | + for i in range(0, len(value)): |
683 | + track, risk, branch = channel_string_to_list(value[i]) |
684 | + track_widget = getattr(self, "track_%s_widget" % i) |
685 | + track_widget.setRenderedValue(track) |
686 | + risk_widget = getattr(self, "risk_%s_widget" % i) |
687 | + risk_widget.setRenderedValue(risk) |
688 | + branch_widget = getattr(self, "branch_%s_widget" % i) |
689 | + branch_widget.setRenderedValue(branch) |
690 | + delete_widget = getattr(self, "delete_%s_widget" % i) |
691 | + delete_widget.setRenderedValue(False) |
692 | else: |
693 | - self.track_widget.setRenderedValue(None) |
694 | - self.risks_widget.setRenderedValue(None) |
695 | - self.branch_widget.setRenderedValue(None) |
696 | + self.add_track_widget.setRenderedValue(None) |
697 | + self.add_risk_widget.setRenderedValue(None) |
698 | + self.add_branch_widget.setRenderedValue(None) |
699 | |
700 | def hasInput(self): |
701 | """See `IInputWidget`.""" |
702 | - return ("%s.risks" % self.name) in self.request.form |
703 | + return ("%s.add_risk" % self.name) in self.request.form |
704 | |
705 | def hasValidInput(self): |
706 | """See `IInputWidget`.""" |
707 | @@ -137,12 +171,7 @@ class StoreChannelsWidget(BrowserWidget, InputWidget): |
708 | except UnexpectedFormData: |
709 | return False |
710 | |
711 | - def getInputValue(self): |
712 | - """See `IInputWidget`.""" |
713 | - self.setUpSubWidgets() |
714 | - track = self.track_widget.getInputValue() |
715 | - risks = self.risks_widget.getInputValue() |
716 | - branch = self.branch_widget.getInputValue() |
717 | + def get_list(self, track, risk, branch, operation): |
718 | if track and CHANNEL_COMPONENTS_DELIMITER in track: |
719 | error_msg = "Track name cannot include '%s'." % ( |
720 | CHANNEL_COMPONENTS_DELIMITER |
721 | @@ -157,10 +186,53 @@ class StoreChannelsWidget(BrowserWidget, InputWidget): |
722 | raise WidgetInputError( |
723 | self.name, self.label, LaunchpadValidationError(error_msg) |
724 | ) |
725 | - channels = [ |
726 | - channel_list_to_string(track, risk, branch) for risk in risks |
727 | - ] |
728 | - return channels |
729 | + if not risk: |
730 | + error_msg = "You must select a risk." |
731 | + if operation == "add": |
732 | + raise WidgetInputError( |
733 | + "add_risk_widget", |
734 | + self.label, |
735 | + LaunchpadValidationError(error_msg), |
736 | + ) |
737 | + if operation == "edit": |
738 | + if track or branch: |
739 | + raise WidgetInputError( |
740 | + "add_risk_widget", |
741 | + self.label, |
742 | + LaunchpadValidationError(error_msg), |
743 | + ) |
744 | + return channel_list_to_string(track, risk, branch) |
745 | + |
746 | + def getInputValue(self): |
747 | + """See `IInputWidget`.""" |
748 | + self.setUpSubWidgets() |
749 | + store_channels = [] |
750 | + track = self.add_track_widget.getInputValue() |
751 | + risk = self.add_risk_widget.getInputValue() |
752 | + branch = self.add_branch_widget.getInputValue() |
753 | + add_row = None |
754 | + if self.is_edit: |
755 | + add_row = self.get_list(track, risk, branch, "edit") |
756 | + for index in range(len(self.context.context.store_channels)): |
757 | + track = getattr( |
758 | + self, "track_%s_widget" % index |
759 | + ).getInputValue() |
760 | + risk = getattr(self, "risk_%s_widget" % index).getInputValue() |
761 | + branch = getattr( |
762 | + self, "branch_%s_widget" % index |
763 | + ).getInputValue() |
764 | + delete = getattr( |
765 | + self, "delete_%s_widget" % index |
766 | + ).getInputValue() |
767 | + if not delete: |
768 | + store_channels.append( |
769 | + self.get_list(track, risk, branch, "edit") |
770 | + ) |
771 | + if self.is_add: |
772 | + add_row = self.get_list(track, risk, branch, "add") |
773 | + if add_row: |
774 | + store_channels.append(add_row) |
775 | + return store_channels |
776 | |
777 | def error(self): |
778 | """See `IBrowserWidget`.""" |
779 | diff --git a/lib/lp/snappy/browser/widgets/templates/storechannels.pt b/lib/lp/snappy/browser/widgets/templates/storechannels.pt |
780 | index ade718a..248f33c 100644 |
781 | --- a/lib/lp/snappy/browser/widgets/templates/storechannels.pt |
782 | +++ b/lib/lp/snappy/browser/widgets/templates/storechannels.pt |
783 | @@ -18,38 +18,76 @@ |
784 | </td> |
785 | </tr> |
786 | <tr> |
787 | - <td> |
788 | - <table class="subordinate"> |
789 | + <table class="listing subordinate"> |
790 | + <thead> |
791 | <tr> |
792 | - <td> |
793 | - <tal:widget define="widget nocall:view/risks_widget" |
794 | - condition="widget/context/value_type/vocabulary"> |
795 | - <metal:block |
796 | - use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
797 | - </tal:widget> |
798 | - </td> |
799 | + <th class="channel-row-track">Track</th> |
800 | + <th class="channel-row-risk">Risk</th> |
801 | + <th class="channel-row-branch">Branch</th> |
802 | + <th class="channel-row-delete">Delete?</th> |
803 | </tr> |
804 | - <tr> |
805 | - <td> |
806 | - <tal:widget define="widget nocall:view/track_widget"> |
807 | - <metal:block |
808 | - use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
809 | - </tal:widget> |
810 | - <p class="formHelp"> |
811 | + </thead> |
812 | + <tbody> |
813 | + <tal:channel tal:condition="view/show_edit" repeat="channel view/context/context/store_channels"> |
814 | + <tal:channel_widgets |
815 | + define="parity python:'even' if repeat['channel'].even() else 'odd'; |
816 | + edit_track string:track_${repeat/channel/index}_widget; |
817 | + edit_branch string:branch_${repeat/channel/index}_widget; |
818 | + edit_risk string:risk_${repeat/channel/index}_widget; |
819 | + delete string:delete_${repeat/channel/index}_widget"> |
820 | + <tr tal:attributes="class string:channel-row ${parity}"> |
821 | + <td class="channel-row-track" |
822 | + tal:define="widget nocall:view/?edit_track"> |
823 | + <metal:block |
824 | + use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
825 | + </td> |
826 | + <td class="channel-row-risk" |
827 | + tal:define="widget nocall:view/?edit_risk"> |
828 | + <metal:block |
829 | + use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
830 | + </td> |
831 | + <td class="channel-row-branch" |
832 | + tal:define="widget nocall:view/?edit_branch"> |
833 | + <metal:block |
834 | + use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
835 | + </td> |
836 | + <td class="channel-row-delete" |
837 | + tal:define="widget nocall:view/?delete"> |
838 | + <metal:block |
839 | + use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
840 | + </td> |
841 | + </tr> |
842 | + </tal:channel_widgets> |
843 | + </tal:channel> |
844 | + <tal:new-channel |
845 | + define="parity python:'odd' if view.number_of_channels % 2 else 'even'"> |
846 | + <tr tal:attributes="class string:channel-row ${parity}"> |
847 | + <td class="channel-row-track"> |
848 | + <tal:widget define="widget nocall:view/add_track_widget"> |
849 | + <metal:block |
850 | + use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
851 | + </tal:widget> |
852 | + <p class="formHelp"> |
853 | To open a new track, <a href="https://snapcraft.io/community">ask the store admins</a>. |
854 | - </p> |
855 | - </td> |
856 | - </tr> |
857 | - <tr> |
858 | - <td> |
859 | - <tal:widget define="widget nocall:view/branch_widget"> |
860 | - <metal:block |
861 | - use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
862 | - </tal:widget> |
863 | - </td> |
864 | - </tr> |
865 | - </table> |
866 | - </td> |
867 | + </p> |
868 | + </td> |
869 | + <td class="channel-row-risk"> |
870 | + <tal:widget define="widget nocall:view/add_risk_widget"> |
871 | + <metal:block |
872 | + use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
873 | + </tal:widget> |
874 | + </td> |
875 | + <td class="channel-row-branch"> |
876 | + <tal:widget define="widget nocall:view/add_branch_widget"> |
877 | + <metal:block |
878 | + use-macro="context/@@launchpad_widget_macros/launchpad_widget_row" /> |
879 | + </tal:widget> |
880 | + </td> |
881 | + <td class="channel-row-delete" /> |
882 | + </tr> |
883 | + </tal:new-channel> |
884 | + </tbody> |
885 | + </table> |
886 | </tr> |
887 | </table> |
888 | |
889 | diff --git a/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py b/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py |
890 | index 83d42be..0acdca7 100644 |
891 | --- a/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py |
892 | +++ b/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py |
893 | @@ -16,239 +16,285 @@ from lp.services.beautifulsoup import BeautifulSoup |
894 | from lp.services.webapp.escaping import html_escape |
895 | from lp.services.webapp.servers import LaunchpadTestRequest |
896 | from lp.snappy.browser.widgets.storechannels import StoreChannelsWidget |
897 | -from lp.snappy.vocabularies import SnapStoreChannelVocabulary |
898 | from lp.testing import TestCaseWithFactory, verifyObject |
899 | from lp.testing.layers import DatabaseFunctionalLayer |
900 | |
901 | |
902 | class TestStoreChannelsWidget(TestCaseWithFactory): |
903 | - |
904 | layer = DatabaseFunctionalLayer |
905 | |
906 | def setUp(self): |
907 | super().setUp() |
908 | field = List(__name__="channels", title="Store channels") |
909 | - self.context = self.factory.makeSnap() |
910 | - field = field.bind(self.context) |
911 | + self.new_snap = self.factory.makeSnap(store_channels=[]) |
912 | + field = field.bind(self.new_snap) |
913 | request = LaunchpadTestRequest() |
914 | - self.widget = StoreChannelsWidget(field, None, request) |
915 | + self.new_widget = StoreChannelsWidget(field, None, request) |
916 | + |
917 | + self.edit_snap = self.factory.makeSnap( |
918 | + store_channels=["track1/stable/branch1", "track2/edge/branch1"] |
919 | + ) |
920 | + field = field.bind(self.edit_snap) |
921 | + self.edit_widget = StoreChannelsWidget(field, None, request) |
922 | |
923 | def test_implements(self): |
924 | - self.assertTrue(verifyObject(IBrowserWidget, self.widget)) |
925 | - self.assertTrue(verifyObject(IInputWidget, self.widget)) |
926 | + self.assertTrue(verifyObject(IBrowserWidget, self.new_widget)) |
927 | + self.assertTrue(verifyObject(IInputWidget, self.new_widget)) |
928 | |
929 | def test_template(self): |
930 | self.assertTrue( |
931 | - self.widget.template.filename.endswith("storechannels.pt"), |
932 | + self.new_widget.template.filename.endswith("storechannels.pt"), |
933 | "Template was not set up.", |
934 | ) |
935 | |
936 | def test_setUpSubWidgets_first_call(self): |
937 | # The subwidgets are set up and a flag is set. |
938 | - self.widget.setUpSubWidgets() |
939 | - self.assertTrue(self.widget._widgets_set_up) |
940 | - self.assertIsNotNone(getattr(self.widget, "track_widget", None)) |
941 | - self.assertIsInstance( |
942 | - self.widget.risks_widget.vocabulary, SnapStoreChannelVocabulary |
943 | + self.new_widget.setUpSubWidgets() |
944 | + self.assertTrue(self.new_widget._widgets_set_up) |
945 | + self.assertIsNotNone( |
946 | + getattr(self.new_widget, "add_track_widget", None) |
947 | + ) |
948 | + self.assertIsNotNone( |
949 | + getattr(self.new_widget, "add_branch_widget", None) |
950 | ) |
951 | - self.assertTrue(self.widget.has_risks_vocabulary) |
952 | - self.assertIsNotNone(getattr(self.widget, "branch_widget", None)) |
953 | |
954 | def test_setUpSubWidgets_second_call(self): |
955 | # The setUpSubWidgets method exits early if a flag is set to |
956 | # indicate that the widgets were set up. |
957 | - self.widget._widgets_set_up = True |
958 | - self.widget.setUpSubWidgets() |
959 | - self.assertIsNone(getattr(self.widget, "track_widget", None)) |
960 | - self.assertIsNone(getattr(self.widget, "risks_widget", None)) |
961 | - self.assertIsNone(getattr(self.widget, "branch_widget", None)) |
962 | - self.assertIsNone(self.widget.has_risks_vocabulary) |
963 | + self.new_widget._widgets_set_up = True |
964 | + self.new_widget.setUpSubWidgets() |
965 | + self.assertIsNone(getattr(self.new_widget, "add_track_widget", None)) |
966 | + self.assertIsNone(getattr(self.new_widget, "add_risk_widget", None)) |
967 | + self.assertIsNone(getattr(self.new_widget, "add_branch_widget", None)) |
968 | |
969 | def test_setRenderedValue_empty(self): |
970 | - self.widget.setRenderedValue([]) |
971 | - self.assertIsNone(self.widget.track_widget._getCurrentValue()) |
972 | - self.assertIsNone(self.widget.risks_widget._getCurrentValue()) |
973 | + self.new_widget.setRenderedValue([]) |
974 | + self.assertIsNone(self.new_widget.add_track_widget._getCurrentValue()) |
975 | + self.assertIsNone(self.new_widget.add_risk_widget._getCurrentValue()) |
976 | |
977 | def test_setRenderedValue_no_track_or_branch(self): |
978 | - # Channels do not include a track or branch |
979 | - risks = ["candidate", "edge"] |
980 | - self.widget.setRenderedValue(risks) |
981 | - self.assertIsNone(self.widget.track_widget._getCurrentValue()) |
982 | - self.assertEqual(risks, self.widget.risks_widget._getCurrentValue()) |
983 | - self.assertIsNone(self.widget.branch_widget._getCurrentValue()) |
984 | + # Channel does not include a track or branch |
985 | + channels = ["/edge/"] |
986 | + self.edit_widget.setRenderedValue(channels) |
987 | + self.assertIsNone(self.edit_widget.add_track_widget._getCurrentValue()) |
988 | + self.assertEqual( |
989 | + StoreRisk.EDGE, self.edit_widget.add_risk_widget._getCurrentValue() |
990 | + ) |
991 | + self.assertIsNone( |
992 | + self.edit_widget.add_branch_widget._getCurrentValue() |
993 | + ) |
994 | |
995 | def test_setRenderedValue_with_track(self): |
996 | # Channels including a track |
997 | channels = ["2.2/candidate", "2.2/edge"] |
998 | - self.widget.setRenderedValue(channels) |
999 | - self.assertEqual("2.2", self.widget.track_widget._getCurrentValue()) |
1000 | + self.edit_widget.setRenderedValue(channels) |
1001 | self.assertEqual( |
1002 | - ["candidate", "edge"], self.widget.risks_widget._getCurrentValue() |
1003 | + "2.2", self.edit_widget.track_0_widget._getCurrentValue() |
1004 | ) |
1005 | - self.assertIsNone(self.widget.branch_widget._getCurrentValue()) |
1006 | + self.assertEqual( |
1007 | + "candidate", self.edit_widget.risk_0_widget._getCurrentValue() |
1008 | + ) |
1009 | + self.assertIsNone(self.edit_widget.branch_0_widget._getCurrentValue()) |
1010 | + |
1011 | + self.assertEqual( |
1012 | + "2.2", self.edit_widget.track_1_widget._getCurrentValue() |
1013 | + ) |
1014 | + self.assertEqual( |
1015 | + "edge", self.edit_widget.risk_1_widget._getCurrentValue() |
1016 | + ) |
1017 | + self.assertIsNone(self.edit_widget.branch_1_widget._getCurrentValue()) |
1018 | |
1019 | def test_setRenderedValue_with_branch(self): |
1020 | # Channels including a branch |
1021 | channels = ["candidate/fix-123", "edge/fix-123"] |
1022 | - self.widget.setRenderedValue(channels) |
1023 | - self.assertIsNone(self.widget.track_widget._getCurrentValue()) |
1024 | + self.edit_widget.setRenderedValue(channels) |
1025 | + self.assertIsNone(self.edit_widget.track_0_widget._getCurrentValue()) |
1026 | + self.assertEqual( |
1027 | + "candidate", self.edit_widget.risk_0_widget._getCurrentValue() |
1028 | + ) |
1029 | + self.assertEqual( |
1030 | + "fix-123", self.edit_widget.branch_0_widget._getCurrentValue() |
1031 | + ) |
1032 | + |
1033 | + self.assertIsNone(self.edit_widget.track_1_widget._getCurrentValue()) |
1034 | self.assertEqual( |
1035 | - ["candidate", "edge"], self.widget.risks_widget._getCurrentValue() |
1036 | + "edge", self.edit_widget.risk_1_widget._getCurrentValue() |
1037 | ) |
1038 | self.assertEqual( |
1039 | - "fix-123", self.widget.branch_widget._getCurrentValue() |
1040 | + "fix-123", self.edit_widget.branch_1_widget._getCurrentValue() |
1041 | ) |
1042 | |
1043 | def test_setRenderedValue_with_track_and_branch(self): |
1044 | # Channels including a track and branch |
1045 | channels = ["2.2/candidate/fix-123", "2.2/edge/fix-123"] |
1046 | - self.widget.setRenderedValue(channels) |
1047 | - self.assertEqual("2.2", self.widget.track_widget._getCurrentValue()) |
1048 | + self.edit_widget.setRenderedValue(channels) |
1049 | + self.assertEqual( |
1050 | + "2.2", self.edit_widget.track_0_widget._getCurrentValue() |
1051 | + ) |
1052 | + self.assertEqual( |
1053 | + "candidate", self.edit_widget.risk_0_widget._getCurrentValue() |
1054 | + ) |
1055 | + self.assertEqual( |
1056 | + "fix-123", self.edit_widget.branch_0_widget._getCurrentValue() |
1057 | + ) |
1058 | self.assertEqual( |
1059 | - ["candidate", "edge"], self.widget.risks_widget._getCurrentValue() |
1060 | + "2.2", self.edit_widget.track_1_widget._getCurrentValue() |
1061 | ) |
1062 | self.assertEqual( |
1063 | - "fix-123", self.widget.branch_widget._getCurrentValue() |
1064 | + "edge", self.edit_widget.risk_1_widget._getCurrentValue() |
1065 | + ) |
1066 | + self.assertEqual( |
1067 | + "fix-123", self.edit_widget.branch_1_widget._getCurrentValue() |
1068 | ) |
1069 | |
1070 | def test_setRenderedValue_invalid_value(self): |
1071 | - # Multiple channels, different tracks or branches, unsupported |
1072 | - self.assertRaisesWithContent( |
1073 | - ValueError, |
1074 | - "Channels belong to different tracks: " |
1075 | - "['2.2/candidate', '2.1/edge']", |
1076 | - self.widget.setRenderedValue, |
1077 | - ["2.2/candidate", "2.1/edge"], |
1078 | - ) |
1079 | - self.assertRaisesWithContent( |
1080 | - ValueError, |
1081 | - "Channels belong to different branches: " |
1082 | - "['candidate/fix-123', 'edge/fix-124']", |
1083 | - self.widget.setRenderedValue, |
1084 | - ["candidate/fix-123", "edge/fix-124"], |
1085 | - ) |
1086 | - self.assertRaisesWithContent( |
1087 | - ValueError, |
1088 | - "Channels belong to different tracks: " |
1089 | - "['2.2/candidate', 'edge/fix-123']", |
1090 | - self.widget.setRenderedValue, |
1091 | - ["2.2/candidate", "edge/fix-123"], |
1092 | + # We allow multiple channels, different tracks or branches. |
1093 | + # We don't raise ValueError exceptions on these. |
1094 | + self.edit_widget.setRenderedValue(["2.2/candidate", "2.1/edge"]) |
1095 | + self.edit_widget.setRenderedValue( |
1096 | + ["candidate/fix-123", "edge/fix-124"] |
1097 | ) |
1098 | + self.edit_widget.setRenderedValue(["2.2/candidate", "edge/fix-123"]) |
1099 | |
1100 | def test_hasInput_false(self): |
1101 | # hasInput is false when there is no risk set in the form data. |
1102 | - self.widget.request = LaunchpadTestRequest( |
1103 | + self.new_widget.request = LaunchpadTestRequest( |
1104 | form={"field.channels.track": "track"} |
1105 | ) |
1106 | - self.assertFalse(self.widget.hasInput()) |
1107 | + self.assertFalse(self.new_widget.hasInput()) |
1108 | |
1109 | def test_hasInput_true(self): |
1110 | # hasInput is true if there are risks set in the form data. |
1111 | - self.widget.request = LaunchpadTestRequest( |
1112 | - form={"field.channels.risks": ["beta"]} |
1113 | + self.new_widget.request = LaunchpadTestRequest( |
1114 | + form={"field.channels.add_risk": ["beta"]} |
1115 | ) |
1116 | - self.assertTrue(self.widget.hasInput()) |
1117 | + self.assertTrue(self.new_widget.hasInput()) |
1118 | |
1119 | def test_hasValidInput_false(self): |
1120 | # The field input is invalid if any of the submitted parts are |
1121 | # invalid. |
1122 | form = { |
1123 | - "field.channels.track": "", |
1124 | - "field.channels.risks": ["invalid"], |
1125 | - "field.channels.branch": "", |
1126 | + "field.channels.add_track": "", |
1127 | + "field.channels.add_risk": ["invalid"], |
1128 | + "field.channels.add_branch": "", |
1129 | } |
1130 | - self.widget.request = LaunchpadTestRequest(form=form) |
1131 | - self.assertFalse(self.widget.hasValidInput()) |
1132 | + self.new_widget.request = LaunchpadTestRequest(form=form) |
1133 | + self.assertFalse(self.new_widget.hasValidInput()) |
1134 | |
1135 | def test_hasValidInput_true(self): |
1136 | + field = List(__name__="channels", title="Store channels") |
1137 | + self.context = self.new_snap |
1138 | + field.bind(self.context) |
1139 | + |
1140 | # The field input is valid when all submitted parts are valid. |
1141 | form = { |
1142 | - "field.channels.track": "track", |
1143 | - "field.channels.risks": ["stable", "beta"], |
1144 | - "field.channels.branch": "branch", |
1145 | + "field.channels.add_track": "track", |
1146 | + "field.channels.add_risk": "stable", |
1147 | + "field.channels.add_branch": "branch", |
1148 | } |
1149 | - self.widget.request = LaunchpadTestRequest(form=form) |
1150 | - self.assertTrue(self.widget.hasValidInput()) |
1151 | + |
1152 | + request = LaunchpadTestRequest(form=form) |
1153 | + self.new_widget = StoreChannelsWidget(field, None, request) |
1154 | + self.assertTrue(self.new_widget.hasValidInput()) |
1155 | |
1156 | def assertGetInputValueError(self, form, message): |
1157 | - self.widget.request = LaunchpadTestRequest(form=form) |
1158 | - e = self.assertRaises(WidgetInputError, self.widget.getInputValue) |
1159 | + extra = { |
1160 | + "field.actions.create": "Create snap package", |
1161 | + "field.store_upload": "on", |
1162 | + } |
1163 | + self.new_widget.request = LaunchpadTestRequest(form=form, **extra) |
1164 | + e = self.assertRaises(WidgetInputError, self.new_widget.getInputValue) |
1165 | self.assertEqual(LaunchpadValidationError(message), e.errors) |
1166 | - self.assertEqual(html_escape(message), self.widget.error()) |
1167 | + self.assertEqual(html_escape(message), self.new_widget.error()) |
1168 | |
1169 | def test_getInputValue_invalid_track(self): |
1170 | # An error is raised when the track includes a '/'. |
1171 | form = { |
1172 | - "field.channels.track": "tra/ck", |
1173 | - "field.channels.risks": ["beta"], |
1174 | - "field.channels.branch": "", |
1175 | + "field.channels.add_track": "tra/ck", |
1176 | + "field.channels.add_risk": "beta", |
1177 | + "field.channels.add_branch": "", |
1178 | } |
1179 | self.assertGetInputValueError(form, "Track name cannot include '/'.") |
1180 | |
1181 | def test_getInputValue_invalid_branch(self): |
1182 | # An error is raised when the branch includes a '/'. |
1183 | form = { |
1184 | - "field.channels.track": "", |
1185 | - "field.channels.risks": ["beta"], |
1186 | - "field.channels.branch": "bra/nch", |
1187 | + "field.channels.add_track": "", |
1188 | + "field.channels.add_risk": "beta", |
1189 | + "field.channels.add_branch": "bra/nch", |
1190 | } |
1191 | self.assertGetInputValueError(form, "Branch name cannot include '/'.") |
1192 | |
1193 | def test_getInputValue_no_track_or_branch(self): |
1194 | - self.widget.request = LaunchpadTestRequest( |
1195 | - form={ |
1196 | - "field.channels.track": "", |
1197 | - "field.channels.risks": ["beta", "edge"], |
1198 | - "field.channels.branch": "", |
1199 | - } |
1200 | - ) |
1201 | - expected = ["beta", "edge"] |
1202 | - self.assertEqual(expected, self.widget.getInputValue()) |
1203 | + form = { |
1204 | + "field.channels.add_track": "", |
1205 | + "field.channels.add_risk": "edge", |
1206 | + "field.channels.add_branch": "", |
1207 | + } |
1208 | + extra = { |
1209 | + "field.actions.create": "Create snap package", |
1210 | + "field.store_upload": "on", |
1211 | + } |
1212 | + self.new_widget.request = LaunchpadTestRequest(form=form, **extra) |
1213 | + expected = ["edge"] |
1214 | + self.assertEqual(expected, self.new_widget.getInputValue()) |
1215 | |
1216 | def test_getInputValue_with_track(self): |
1217 | - self.widget.request = LaunchpadTestRequest( |
1218 | - form={ |
1219 | - "field.channels.track": "track", |
1220 | - "field.channels.risks": ["beta", "edge"], |
1221 | - "field.channels.branch": "", |
1222 | - } |
1223 | - ) |
1224 | - expected = ["track/beta", "track/edge"] |
1225 | - self.assertEqual(expected, self.widget.getInputValue()) |
1226 | + form = { |
1227 | + "field.channels.add_track": "track", |
1228 | + "field.channels.add_risk": "beta", |
1229 | + "field.channels.add_branch": "", |
1230 | + } |
1231 | + extra = { |
1232 | + "field.actions.create": "Create snap package", |
1233 | + "field.store_upload": "on", |
1234 | + } |
1235 | + self.new_widget.request = LaunchpadTestRequest(form=form, **extra) |
1236 | + expected = ["track/beta"] |
1237 | + self.assertEqual(expected, self.new_widget.getInputValue()) |
1238 | |
1239 | def test_getInputValue_with_branch(self): |
1240 | - self.widget.request = LaunchpadTestRequest( |
1241 | - form={ |
1242 | - "field.channels.track": "", |
1243 | - "field.channels.risks": ["beta", "edge"], |
1244 | - "field.channels.branch": "fix-123", |
1245 | - } |
1246 | - ) |
1247 | - expected = ["beta/fix-123", "edge/fix-123"] |
1248 | - self.assertEqual(expected, self.widget.getInputValue()) |
1249 | + form = { |
1250 | + "field.channels.add_track": "", |
1251 | + "field.channels.add_risk": "beta", |
1252 | + "field.channels.add_branch": "fix-123", |
1253 | + } |
1254 | + extra = { |
1255 | + "field.actions.create": "Create snap package", |
1256 | + "field.store_upload": "on", |
1257 | + } |
1258 | + self.new_widget.request = LaunchpadTestRequest(form=form, **extra) |
1259 | + expected = ["beta/fix-123"] |
1260 | + self.assertEqual(expected, self.new_widget.getInputValue()) |
1261 | |
1262 | def test_getInputValue_with_track_and_branch(self): |
1263 | - self.widget.request = LaunchpadTestRequest( |
1264 | - form={ |
1265 | - "field.channels.track": "track", |
1266 | - "field.channels.risks": ["beta", "edge"], |
1267 | - "field.channels.branch": "fix-123", |
1268 | - } |
1269 | - ) |
1270 | - expected = ["track/beta/fix-123", "track/edge/fix-123"] |
1271 | - self.assertEqual(expected, self.widget.getInputValue()) |
1272 | + form = { |
1273 | + "field.channels.add_track": "track", |
1274 | + "field.channels.add_risk": "beta", |
1275 | + "field.channels.add_branch": "fix-123", |
1276 | + } |
1277 | + extra = { |
1278 | + "field.actions.create": "Create snap package", |
1279 | + "field.store_upload": "on", |
1280 | + } |
1281 | + self.new_widget.request = LaunchpadTestRequest(form=form, **extra) |
1282 | + expected = ["track/beta/fix-123"] |
1283 | + self.assertEqual(expected, self.new_widget.getInputValue()) |
1284 | |
1285 | def test_call(self): |
1286 | # The __call__ method sets up the widgets. |
1287 | - markup = self.widget() |
1288 | - self.assertIsNotNone(self.widget.track_widget) |
1289 | - self.assertIsNotNone(self.widget.risks_widget) |
1290 | + markup = self.new_widget() |
1291 | + self.assertIsNotNone(self.new_widget.add_track_widget) |
1292 | + self.assertIsNotNone(self.new_widget.add_risk_widget) |
1293 | + self.assertIsNotNone(self.new_widget.add_branch_widget) |
1294 | + |
1295 | soup = BeautifulSoup(markup) |
1296 | - fields = soup.find_all(["input"], {"id": re.compile(".*")}) |
1297 | + fields = soup.find_all(["input", "select"], {"id": re.compile(".*")}) |
1298 | expected_ids = [ |
1299 | - "field.channels.risks.%d" % i for i in range(len(StoreRisk)) |
1300 | + "field.channels.add_risk", |
1301 | + "field.channels.add_track", |
1302 | + "field.channels.add_branch", |
1303 | ] |
1304 | - expected_ids.append("field.channels.track") |
1305 | - expected_ids.append("field.channels.branch") |
1306 | + |
1307 | ids = [field["id"] for field in fields] |
1308 | self.assertContentEqual(expected_ids, ids) |
1309 | diff --git a/lib/lp/snappy/templates/snap-edit.pt b/lib/lp/snappy/templates/snap-edit.pt |
1310 | index eb9bc9e..ad92312 100644 |
1311 | --- a/lib/lp/snappy/templates/snap-edit.pt |
1312 | +++ b/lib/lp/snappy/templates/snap-edit.pt |
1313 | @@ -11,6 +11,24 @@ |
1314 | <style type="text/css"> |
1315 | .subordinate { |
1316 | margin: 0.5em 0 0.5em 4em; |
1317 | + max-width: 80%; |
1318 | + margin-bottom: 1em; |
1319 | + } |
1320 | + table.subordinate tr.even { |
1321 | + background-color: #eee; |
1322 | + } |
1323 | + /* These add up to 100%. */ |
1324 | + tr .channel-row-track { |
1325 | + width: 35%; |
1326 | + } |
1327 | + tr .channel-row-risk { |
1328 | + width: 25%; |
1329 | + } |
1330 | + tr .channel-row-branch { |
1331 | + width: 35%; |
1332 | + } |
1333 | + tr .channel-row-delete { |
1334 | + width: 5%; |
1335 | } |
1336 | </style> |
1337 | </metal:block> |
1338 | @@ -99,8 +117,7 @@ |
1339 | <tal:widget define="widget nocall:view/widgets/store_name"> |
1340 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
1341 | </tal:widget> |
1342 | - <tal:widget define="widget nocall:view/widgets/store_channels" |
1343 | - condition="widget/has_risks_vocabulary"> |
1344 | + <tal:widget define="widget nocall:view/widgets/store_channels"> |
1345 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
1346 | </tal:widget> |
1347 | </table> |
1348 | diff --git a/lib/lp/snappy/templates/snap-new.pt b/lib/lp/snappy/templates/snap-new.pt |
1349 | index 480d51c..29e8f64 100644 |
1350 | --- a/lib/lp/snappy/templates/snap-new.pt |
1351 | +++ b/lib/lp/snappy/templates/snap-new.pt |
1352 | @@ -112,8 +112,7 @@ |
1353 | <tal:widget define="widget nocall:view/widgets/store_name"> |
1354 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
1355 | </tal:widget> |
1356 | - <tal:widget define="widget nocall:view/widgets/store_channels" |
1357 | - condition="widget/has_risks_vocabulary"> |
1358 | + <tal:widget define="widget nocall:view/widgets/store_channels"> |
1359 | <metal:block use-macro="context/@@launchpad_form/widget_row" /> |
1360 | </tal:widget> |
1361 | </table> |
1362 | diff --git a/lib/lp/snappy/validators/channels.py b/lib/lp/snappy/validators/channels.py |
1363 | index 07757d9..811667c 100644 |
1364 | --- a/lib/lp/snappy/validators/channels.py |
1365 | +++ b/lib/lp/snappy/validators/channels.py |
1366 | @@ -29,15 +29,4 @@ def channels_validator(channels): |
1367 | tracks.add(track) |
1368 | branches.add(branch) |
1369 | |
1370 | - # XXX cjwatson 2018-05-08: These are slightly arbitrary restrictions, |
1371 | - # but they make the UI much simpler. |
1372 | - |
1373 | - if len(tracks) != 1: |
1374 | - message = _("Channels must belong to the same track.") |
1375 | - raise LaunchpadValidationError(structured(message)) |
1376 | - |
1377 | - if len(branches) != 1: |
1378 | - message = _("Channels must belong to the same branch.") |
1379 | - raise LaunchpadValidationError(structured(message)) |
1380 | - |
1381 | return True |
1382 | diff --git a/lib/lp/snappy/validators/tests/test_channels.py b/lib/lp/snappy/validators/tests/test_channels.py |
1383 | index 61e160f..a9d9e5e 100644 |
1384 | --- a/lib/lp/snappy/validators/tests/test_channels.py |
1385 | +++ b/lib/lp/snappy/validators/tests/test_channels.py |
1386 | @@ -20,18 +20,10 @@ class TestChannelsValidator(TestCaseWithFactory): |
1387 | self.assertTrue(channels_validator(["beta", "edge"])) |
1388 | |
1389 | def test_channels_validator_multiple_tracks(self): |
1390 | - self.assertRaises( |
1391 | - LaunchpadValidationError, |
1392 | - channels_validator, |
1393 | - ["1.1/stable", "2.1/edge"], |
1394 | - ) |
1395 | + self.assertTrue(channels_validator(["1.1/stable", "2.1/edge"])) |
1396 | |
1397 | def test_channels_validator_multiple_branches(self): |
1398 | - self.assertRaises( |
1399 | - LaunchpadValidationError, |
1400 | - channels_validator, |
1401 | - ["stable/fix-123", "edge/fix-124"], |
1402 | - ) |
1403 | + self.assertTrue(channels_validator(["stable/fix-123", "edge/fix-124"])) |
1404 | |
1405 | def test_channels_validator_invalid_channel(self): |
1406 | self.assertRaises( |
This would probably deal with the API side, but another significant piece here is the web UI. In `lib/lp/ snappy/ browser/ widgets/ storechannels. py`, there's a field structure with single text lines for track and branch and a list of choices for risk, and `setRenderedValue` has a similar constraint on single tracks/branches. What's your plan for dealing with this? The UI will likely need to be rearranged in order to cope with more disparate channels, and I don't think we should land just the validation separately because any charm recipes that took advantage of it via the API would probably end up with a crashing or unusable web UI.