Pushed the requested changes and replied about the store name issue. Screenshots: - Create snap from gitref: https://private-fileshare.canonical.com/~pappacena/screenshots/private-oci-recipe/new-snap-gitref.png
- Create snap from project: https://private-fileshare.canonical.com/~pappacena/screenshots/private-oci-recipe/new-snap-project.png Diff comments: > diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py > index d4ba8bd..3a6d0c8 100644 > --- a/lib/lp/snappy/browser/snap.py > +++ b/lib/lp/snappy/browser/snap.py > @@ -138,6 +140,32 @@ class SnapNavigation(WebhookTargetNavigationMixin, > Navigation): > return self.context.getSubscription(person) > > > +class SnapFormMixin: > + def validateVCSWidgets(self, cls, data): > + """Validates if VCS sub-widgets.""" > + if self.widgets.get('vcs') is not None: > + # Set widgets as required or optional depending on the vcs > + # field. > + super(cls, self).validate_widgets(data, ['vcs']) It would fail because `super(SnapFormMixin, self)` doesn't have a `validate_widgets` (SnapFormMixin doesn't have a super class). But I agree that the code is not clear this way. I'll move this super() to be called outside of this mixin. > + vcs = data.get('vcs') > + if vcs == VCSType.BZR: > + self.widgets['branch'].context.required = True > + self.widgets['git_ref'].context.required = False > + elif vcs == VCSType.GIT: > + self.widgets['branch'].context.required = False > + self.widgets['git_ref'].context.required = True > + else: > + raise AssertionError("Unknown branch type %s" % vcs) > + > + def setUpVCSWidgets(self): > + widget = self.widgets.get('vcs') > + if widget is not None: > + current_value = widget._getFormValue() > + self.vcs_bzr_radio, self.vcs_git_radio = [ > + render_radio_widget_part(widget, value, current_value) > + for value in (VCSType.BZR, VCSType.GIT)] > + > + > class SnapInformationTypeMixin: > def getPossibleInformationTypes(self, snap, user): > """Get the information types to display on the edit form. > @@ -529,6 +573,15 @@ class SnapAddView(LaunchpadFormView, SnapAuthorizeMixin, > EnableProcessorsMixin, > """See `LaunchpadFormView`.""" > super(SnapAddView, self).setUpWidgets() > self.widgets['processors'].widget_class = 'processors' > + if self.is_project_context: > + # If we are on Project:+new-snap page, we know which information > + # types the project supports. Let's filter our the ones that are Ok! > + # not supported. > + types = getUtility(ISnapSet).getPossibleSnapInformationTypes( > + self.context) > + info_type_widget = self.widgets['information_type'] > + info_type_widget.vocabulary = InformationTypeVocabulary(types) > + self.setUpVCSWidgets() > > @property > def cancel_url(self): > @@ -537,7 +590,7 @@ class SnapAddView(LaunchpadFormView, SnapAuthorizeMixin, > EnableProcessorsMixin, > @property > def initial_values(self): > store_name = None > - if self.has_snappy_distro_series: > + if self.has_snappy_distro_series and not self.is_project_context: I like the idea of pre-filling this field on the UI once the user selects the branch/gitref (although I would defer this improvement for a future MP). I think removing the Product:+new-snap is a bit too much. Not being able to fill the store_name field for the user (in exchange, we are able to fill the allowed information types right away) is a bit annoying, but I think it makes sense for the end user to have a "create snap" flow that starts from the project page. > # Try to extract Snap store name from snapcraft.yaml file. > try: > snapcraft_data = getUtility(ISnapSet).getSnapcraftYaml( > diff --git a/lib/lp/snappy/templates/snap-new.pt > b/lib/lp/snappy/templates/snap-new.pt > index 5334bee..146d2cb 100644 > --- a/lib/lp/snappy/templates/snap-new.pt > +++ b/lib/lp/snappy/templates/snap-new.pt > @@ -30,12 +30,52 @@ > <tal:widget define="widget nocall:view/widgets/owner"> > <metal:block use-macro="context/@@launchpad_form/widget_row" /> > </tal:widget> > - <tal:widget define="widget nocall:view/widgets/project"> > - <metal:block use-macro="context/@@launchpad_form/widget_row" /> > - </tal:widget> > + > + <tal:guard condition="not: view/is_project_context"> > + <tal:widget define="widget nocall:view/widgets/project"> > + <metal:block use-macro="context/@@launchpad_form/widget_row" /> > + </tal:widget> > + </tal:guard> > <tal:widget define="widget nocall:view/widgets/information_type"> > <metal:block use-macro="context/@@launchpad_form/widget_row" /> > </tal:widget> > + > + <tal:guard condition="view/is_project_context"> > + <tr> Ok! > + <td> > + <div> > + <label for="field.vcs">Source:</label> > + <table> > + <tr> > + <td> > + <label tal:replace="structure view/vcs_bzr_radio" /> > + <table class="subordinate"> > + <tal:widget define="widget > nocall:view/widgets/branch"> > + <metal:block > + use-macro="context/@@launchpad_form/widget_row" /> > + </tal:widget> > + </table> > + </td> > + </tr> > + > + <tr> > + <td> > + <label tal:replace="structure view/vcs_git_radio" /> > + <table class="subordinate"> > + <tal:widget define="widget > + nocall:view/widgets/git_ref"> > + <metal:block > + use-macro="context/@@launchpad_form/widget_row" /> > + </tal:widget> > + </table> > + </td> > + </tr> > + </table> > + </div> > + </td> > + </tr> > + </tal:guard> > + > <tal:widget define="widget nocall:view/widgets/store_distro_series"> > <metal:block use-macro="context/@@launchpad_form/widget_row" /> > </tal:widget> -- https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399184 Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:snap-pillar-edit. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
