launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2015-08-27 16:06:41 +0000
> +++ lib/lp/snappy/browser/snap.py	2015-09-04 16:25:33 +0000
> @@ -46,6 +77,28 @@
>              text="Snap packages", inside=self.context.owner)
>  
>  
> +class SnapNavigationMenu(NavigationMenu):
> +    """Navigation menu for snap packages."""
> +
> +    usedfor = ISnap
> +
> +    facet = 'overview'
> +
> +    links = ('admin', 'delete', 'edit')

Order should be edit, delete, admin.

> +
> +    @enabled_with_permission('launchpad.Admin')
> +    def admin(self):
> +        return Link('+admin', 'Administer snap package', icon='edit')
> +
> +    @enabled_with_permission('launchpad.Edit')
> +    def edit(self):
> +        return Link('+edit', 'Edit snap package', icon='edit')
> +
> +    @enabled_with_permission('launchpad.Edit')
> +    def delete(self):
> +        return Link('+delete', 'Delete snap package', icon='trash-icon')
> +
> +
>  class SnapView(LaunchpadView):
>      """Default view of a Snap."""
>  
> @@ -54,6 +107,22 @@
>          return builds_for_snap(self.context)
>  
>      @property
> +    def name_widget(self):
> +        name = ISnap['name']
> +        title = "Edit the snap package name"
> +        return TextLineEditorWidget(
> +            self.context, name, title, 'h1', max_width='95%', truncate_lines=1)

At least in Firefox invoking the editor doesn't cause the uneditable text to vanish.

> +
> +    @property
> +    def person_picker(self):
> +        field = copy_field(
> +            ISnap['owner'],
> +            vocabularyName='UserTeamsParticipationPlusSelfSimpleDisplay')
> +        return InlinePersonEditPickerWidget(
> +            self.context, field, format_link(self.context.owner),
> +            header='Change owner', step_title='Select a new owner')
> +
> +    @property
>      def source(self):
>          if self.context.branch is not None:
>              return self.context.branch
> @@ -86,3 +155,161 @@
>          if len(builds) >= 10:
>              break
>      return builds
> +
> +
> +class ISnapEditSchema(Interface):
> +    """Schema for adding or editing a snap package."""
> +
> +    use_template(ISnap, include=[
> +        'owner',
> +        'name',
> +        'require_virtualized',
> +        ])
> +    distro_series = Choice(
> +        vocabulary='BuildableDistroSeries', title=u'Distribution series')
> +    vcs = Choice(vocabulary=VCSType, required=True, title=u'VCS')
> +
> +    # Each of these is only required if vcs has an appropriate value.  Later
> +    # validation takes care of adjusting the required attribute.
> +    branch = copy_field(ISnap['branch'], required=True)
> +    git_repository = copy_field(ISnap['git_repository'], required=True)
> +    git_path = copy_field(ISnap['git_path'], required=True)
> +
> +
> +class BaseSnapAddEditView(LaunchpadEditFormView):
> +
> +    schema = ISnapEditSchema
> +
> +    @property
> +    def cancel_url(self):
> +        return canonical_url(self.context)
> +
> +    def setUpWidgets(self):
> +        """See `LaunchpadFormView`."""
> +        super(BaseSnapAddEditView, self).setUpWidgets()
> +        widget = self.widgets.get('vcs')
> +        if widget is not None:
> +            current_value = widget._getFormValue()
> +            self.vcs_bzr, self.vcs_git = [
> +                render_radio_widget_part(widget, value, current_value)
> +                for value in (VCSType.BZR, VCSType.GIT)]
> +
> +    def validate_widgets(self, data, names=None):
> +        """See `LaunchpadFormView`."""
> +        if 'vcs' in self.widgets:
> +            # Set widgets as required or optional depending on the vcs
> +            # field.
> +            super(BaseSnapAddEditView, self).validate_widgets(data, ['vcs'])
> +            vcs = data.get('vcs')
> +            if vcs == VCSType.BZR:
> +                self.widgets['branch'].context.required = True
> +                self.widgets['git_repository'].context.required = False
> +                self.widgets['git_path'].context.required = False
> +            elif vcs == VCSType.GIT:
> +                self.widgets['branch'].context.required = False
> +                self.widgets['git_repository'].context.required = True
> +                self.widgets['git_path'].context.required = True
> +            else:
> +                raise AssertionError("Unknown branch type %s" % vcs)
> +        super(BaseSnapAddEditView, self).validate_widgets(data, names=names)
> +
> +
> +class BaseSnapEditView(BaseSnapAddEditView):
> +
> +    @action('Update snap package', name='update')
> +    def request_action(self, action, data):
> +        vcs = data.pop('vcs', None)
> +        if vcs == VCSType.BZR:
> +            data['git_repository'] = None
> +            data['git_path'] = None
> +        elif vcs == VCSType.GIT:
> +            data['branch'] = None
> +        self.updateContextFromData(data)
> +        self.next_url = canonical_url(self.context)
> +
> +    @property
> +    def adapters(self):
> +        """See `LaunchpadFormView`."""
> +        return {ISnapEditSchema: self.context}
> +
> +
> +class SnapAdminView(BaseSnapEditView):
> +    """View for administering snap packages."""
> +
> +    @property
> +    def title(self):
> +        return 'Administer %s snap package' % self.context.name

"title" is meaningless; it should be "page_title" instead.

But the title should probably be just "Administer", as it automatically gets the other breadcrumbs appended in reverse order. label is OK as defined here, as no other heading on the page describes the context object.

> +
> +    label = title
> +
> +    field_names = ['require_virtualized']
> +
> +    @property
> +    def initial_values(self):
> +        return {'require_virtualized': self.context.require_virtualized}

Other edit forms don't tend to override this. I suspect setUpWidgets pulls it out of the context object if it's present.

> +
> +
> +class SnapEditView(BaseSnapEditView):
> +    """View for editing snap packages."""
> +
> +    @property
> +    def title(self):
> +        return 'Edit %s snap package' % self.context.name
> +
> +    label = title
> +
> +    field_names = [
> +        'owner', 'name', 'distro_series', 'vcs', 'branch', 'git_repository',
> +        'git_path']
> +    custom_widget('distro_series', LaunchpadRadioWidget)
> +    custom_widget('vcs', LaunchpadRadioWidget)
> +
> +    @property
> +    def initial_values(self):
> +        if self.context.git_repository is not None:
> +            vcs = VCSType.GIT
> +        else:
> +            vcs = VCSType.BZR
> +        return {
> +            'distro_series': self.context.distro_series,

Does this need to be manually provided?

> +            'vcs': vcs,
> +            }
> +
> +    def validate(self, data):
> +        super(SnapEditView, self).validate(data)
> +        owner = data.get('owner', None)
> +        name = data.get('name', None)
> +        if owner and name:
> +            try:
> +                snap = getUtility(ISnapSet).getByName(owner, name)
> +                if snap != self.context:
> +                    self.setFieldError(
> +                        'name',
> +                        'There is already a snap package owned by %s with '
> +                        'this name.' % owner.displayname)
> +            except NoSuchSnap:
> +                pass
> +
> +
> +class SnapDeleteView(BaseSnapEditView):
> +    """View for deleting snap packages."""
> +
> +    @property
> +    def title(self):
> +        return 'Delete %s snap package' % self.context.name
> +
> +    label = title
> +
> +    field_names = []
> +
> +    @property
> +    def has_builds(self):
> +        return not self.context.builds.is_empty()
> +
> +    @action('Delete snap package', name='delete')
> +    def delete_action(self, action, data):
> +        owner = self.context.owner
> +        self.context.destroySelf()
> +        # XXX cjwatson 2015-07-17: This should go to Person:+snaps or
> +        # similar (or something on SnapSet?) once that exists.
> +        self.next_url = canonical_url(owner)
> 
> === added file 'lib/lp/snappy/templates/snap-edit.pt'
> --- lib/lp/snappy/templates/snap-edit.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/templates/snap-edit.pt	2015-09-04 16:25:33 +0000
> @@ -0,0 +1,79 @@
> +<html
> +  xmlns="http://www.w3.org/1999/xhtml";
> +  xmlns:tal="http://xml.zope.org/namespaces/tal";
> +  xmlns:metal="http://xml.zope.org/namespaces/metal";
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> +  metal:use-macro="view/macro:page/main_only"
> +  i18n:domain="launchpad">
> +<body>
> +
> +<metal:block fill-slot="head_epilogue">
> +  <style type="text/css">
> +    .subordinate {
> +      margin: 0.5em 0 0.5em 4em;
> +    }
> +  </style>
> +</metal:block>
> +
> +<div metal:fill-slot="main">
> +  <div metal:use-macro="context/@@launchpad_form/form">
> +    <metal:formbody fill-slot="widgets">
> +      <table class="form">
> +        <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/name">
> +          <metal:block use-macro="context/@@launchpad_form/widget_row" />
> +        </tal:widget>
> +        <tal:widget define="widget nocall:view/widgets/distro_series">
> +          <metal:block use-macro="context/@@launchpad_form/widget_row" />
> +        </tal:widget>
> +
> +        <tr>
> +          <td>
> +            <div>
> +              <label for="field.vcs">Source:</label>
> +              <table>
> +                <tr>
> +                  <td>
> +                    <label tal:replace="structure view/vcs_bzr" />

vcs_bzr and vcs_git aren't labels, so the template doesn't make much sense when read in isolation. Can you make it clearer that they're radio buttons, or at least less clear that they're labels?

> +                    <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" />
> +                    <table class="subordinate">
> +                      <tal:widget define="widget nocall:view/widgets/git_repository">
> +                        <metal:block use-macro="context/@@launchpad_form/widget_row" />
> +                      </tal:widget>
> +                      <tal:widget define="widget nocall:view/widgets/git_path">
> +                        <metal:block use-macro="context/@@launchpad_form/widget_row" />
> +                      </tal:widget>
> +                    </table>
> +                  </td>
> +                </tr>
> +              </table>
> +            </div>
> +          </td>
> +        </tr>
> +      </table>
> +    </metal:formbody>
> +  </div>
> +
> +  <script type="text/javascript">
> +    LPJS.use('lp.snappy.snap.edit', function(Y) {
> +      Y.on('domready', function(e) {
> +        Y.lp.snappy.snap.edit.setup();
> +      }, window);
> +    });
> +  </script>
> +</div>
> +
> +</body>
> +</html>


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-edit-views/+merge/270193
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.