launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Information

Looks generally OK, but I'd like to know what we're planning to do about the store name extraction issue, and I'd also like to see a screenshot or two of the new UI.

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'])

This should use `super(SnapFormMixin, self)` and rely on the normal MRO resolution order, rather than taking the class as an argument.

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

"filter out"

> +            # 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:

Hm, this is awkward because it's a noticeable drop in usability.  I guess we could either abandon the idea of having Product:+new-snap (but that makes it conceptually difficult to do things like a web UI for creating snap recipes based on an external Git repository), or we could populate this from JavaScript once we know the source branch?  What do you think?

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

You could use `<tr tal:condition="view/is_project_context">` to reduce the deep indentation a bit.

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