Merge ~ilasc/launchpad:multiple-tracks-charm-recipe into launchpad:master

Proposed by Ioana Lasc
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)
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

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

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.

review: Needs Information
Revision history for this message
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://chat.canonical.com/canonical/pl/79ec8q9gjideieczn7k8y9j83h).

Revision history for this message
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://chat.canonical.com/canonical/pl/cozosq98f7n89nzfsyjgb5pory

Revision history for this message
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://chat.canonical.com/canonical/pl/h79gm5c4o7n38binpxc5kn637a

Revision history for this message
Colin Watson (cjwatson) wrote :
Download full text (3.5 KiB)

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:

`StoreChannelsWidget`'s current job is to select a list of store channels. Its `setRenderedValue` method currently takes a list of channel strings and sets up a single set of track/risks/branch subwidgets to match (with risks being a multi-checkbox widget), while its `getInputValue` method looks at the HTML form data associated with those widgets and parses it back into a list of channel strings. Unless I've missed something, I believe that this basic contract can stay as it is.

Instead of just having three subwidgets, `StoreChannelsWidget` should now have track/risk/branch/delete subwidgets for each item in the context's `store_channels` list, plus a set of "add" subwidgets; those should be bound to something like `0.{track,risk,branch,delete}`, `1.{track,risk,branch,delete}`, etc. in the HTML form data. This would be much like what you've done in the charm recipe views, but pushed down into code that we can reuse across snap and charm recipes.

The template changes would go in `lib/lp/snappy/browser/widgets/templates/storechannels.pt`.

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.browser.widgets.tests.test_storechannelswidget`.

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...

Read more...

review: Needs Fixing
Revision history for this message
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.

Revision history for this message
Colin Watson (cjwatson) :
review: Approve
Revision history for this message
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.

Revision history for this message
Colin Watson (cjwatson) wrote :

Great. Just a few non-blocking remarks now, as far as I can see. Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/app/templates/launchpad-widget-macros.pt b/lib/lp/app/templates/launchpad-widget-macros.pt
2index 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%"/>
26diff --git a/lib/lp/charms/browser/tests/test_charmrecipe.py b/lib/lp/charms/browser/tests/test_charmrecipe.py
27index 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)
380diff --git a/lib/lp/charms/templates/charmrecipe-edit.pt b/lib/lp/charms/templates/charmrecipe-edit.pt
381index 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>
419diff --git a/lib/lp/charms/templates/charmrecipe-new.pt b/lib/lp/charms/templates/charmrecipe-new.pt
420index 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>
433diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
434index 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
484diff --git a/lib/lp/snappy/browser/widgets/storechannels.py b/lib/lp/snappy/browser/widgets/storechannels.py
485index 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`."""
779diff --git a/lib/lp/snappy/browser/widgets/templates/storechannels.pt b/lib/lp/snappy/browser/widgets/templates/storechannels.pt
780index 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
889diff --git a/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py b/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py
890index 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)
1309diff --git a/lib/lp/snappy/templates/snap-edit.pt b/lib/lp/snappy/templates/snap-edit.pt
1310index 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>
1348diff --git a/lib/lp/snappy/templates/snap-new.pt b/lib/lp/snappy/templates/snap-new.pt
1349index 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>
1362diff --git a/lib/lp/snappy/validators/channels.py b/lib/lp/snappy/validators/channels.py
1363index 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
1382diff --git a/lib/lp/snappy/validators/tests/test_channels.py b/lib/lp/snappy/validators/tests/test_channels.py
1383index 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(

Subscribers

People subscribed via source and target branches

to status/vote changes: