launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2016-05-28 00:21:40 +0000
> +++ lib/lp/snappy/browser/snap.py	2016-06-29 21:46:52 +0000
> @@ -369,6 +370,16 @@
>                  self.context.information_type in PRIVATE_INFORMATION_TYPES):
>                  raise SnapPrivateFeatureDisabled
>  
> +    def setUpFields(self):
> +        """See `LaunchpadFormView`."""
> +        super(SnapAddView, self).setUpFields()
> +        processors = getUtility(ISnapSet).availableProcessors()
> +        self.form_fields += self.createEnabledProcessors(
> +            processors,
> +            u"The architectures that this snap package builds for. Some "
> +            u"architectures are restricted and may only be enabled or "
> +            u"disabled by administrators.")
> +

I'm not sure that anything arranges for this form field to have sensible initial values; I haven't actually run it, but it looks to me as though all the processors will come up unchecked, and that seems to be what your tests require.  All build_by_default processors should be checked by default, matching the behaviour of SnapSet.new when processors is None.

>      @property
>      def cancel_url(self):
>          return canonical_url(self.context)
> @@ -636,7 +648,7 @@
>                          data['processors'].append(processor)
>                      elif processor.name in widget.disabled_items:
>                          # This processor is restricted and currently
> -                        # enabled.  Leave it untouched.
> +                        # enabled. Leave it untouched.

Grumble :-)

>                          data['processors'].append(processor)
>  
>  
> 
> === modified file 'lib/lp/snappy/interfaces/snap.py'
> --- lib/lp/snappy/interfaces/snap.py	2016-05-28 00:21:40 +0000
> +++ lib/lp/snappy/interfaces/snap.py	2016-06-29 21:46:52 +0000
> @@ -543,3 +543,8 @@
>  
>          This only exists to keep lazr.restful happy.
>          """
> +
> +    def availableProcessors():

Method names should be verbs in most cases, so I'd spell this "getAvailableProcessors".

> +        """Return all architectures that are available to be enabled or
> +        disabled for new snap packages.")

Superfluous '")' at the end there.

> +        """
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2016-05-28 00:21:40 +0000
> +++ lib/lp/snappy/model/snap.py	2016-06-29 21:46:52 +0000
> @@ -676,3 +676,12 @@
>      def empty_list(self):
>          """See `ISnapSet`."""
>          return []
> +
> +    @classmethod
> +    def availableProcessors(self):
> +        """See `ISnapSet`."""
> +        store = IMasterStore(Snap)

This is a read-only query, so there's no need to force it onto the master; also a little odd to ask for the store for a different table, although that part won't matter in practice.  Use IStore(Processor) instead.

> +        processors = store.find(
> +            Processor,
> +            Processor.id == DistroArchSeries.processor_id)
> +        return processors.config(distinct=True)
> 
> === modified file 'lib/lp/snappy/templates/snap-new.pt'
> --- lib/lp/snappy/templates/snap-new.pt	2016-05-24 05:15:50 +0000
> +++ lib/lp/snappy/templates/snap-new.pt	2016-06-29 21:46:52 +0000
> @@ -49,6 +49,13 @@
>              </p>
>            </td>
>          </tr>
> +
> +        <br/>

As noted on IRC, this is a bit odd - I'm not sure you're allowed a br element as the immediate child of a table.  If it's for visual adjustment, CSS on the following row would be better.  Continuing to use the widget_row macro would be strongly preferable, so you'll have to work out how to push that down into the macro; I'd probably look around for some suitable existing class in lib/canonical/launchpad/icing/ if there is one and add one if there isn't, and then set widget_class on the widget in the corresponding browser code.

> +
> +        <tal:widget define="widget nocall:view/widgets/processors">
> +          <metal:block use-macro="context/@@launchpad_form/widget_row" />
> +        </tal:widget>
> +
>        </table>
>      </metal:formbody>
>    </div>


-- 
https://code.launchpad.net/~maxiberta/launchpad/new-snap-select-processors/+merge/298242
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.