launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

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.