Merge lp:~julian-edwards/launchpad/sync-sponsored-widget into lp:launchpad

[Julian Edwards]

[r=allenap][bug=827555] Add a person picker on the package sync pages so that the user can specify a user to sponsor.

Add a person picker on the package sync pages so that the user can specify a user to sponsor.

[Gavin Panella (allenap)]

Looks good, but I have a few questions.

[1]

+ sponsored = data.get("sponsored_person")
+ if sponsored is None:
+ self.setFieldError("sponsored_person", "Invalid person")

This appears to enforce a sponsored user? I thought it was optional?

[2]

+ <span
+ tal:define="sponsored_person nocall:view/widgets/sponsored_person;
+ field_name sponsored_person/context/__name__;
+ error python:view.getFieldError(field_name);">
+ <label>Person being sponsored (optional):</label>
+ <tal:sponsored replace="structure sponsored_person"/>
+ </span>

Where is the error displayed?

What might work better here is to refactor the widget_row macro from
launchpad-form.pt. It's a rather limited macro at the moment, because
it only ever produces a single table row containing a single table
cell. If you're fed up of UI stuff, I'd be interested in doing that
myself.

[3]

+ assert(
+ force_async or not sponsored,
+ "sponsored must be None for sync copies")

This will never fail. It could instead be written as:

        assert force_async or not sponsored, (
            "sponsored must be None for sync copies")

The original statement was asserting that a tuple (<bool>, <str>) was
truthy. Lint really ought to alert about these things.

[4]

The name "sponsored" is unfortunate because it's ambiguous. Is it
sponsored (boolean), person sponsored by, person being sponsored? It
might be better to use something like "person_being_sponsored".

review: Needs Information

[Julian Edwards (julian-edwards)]

On Thursday 15 December 2011 10:41:39 you wrote:
> Review: Needs Information
>
> Looks good, but I have a few questions.
>
>
> [1]
>
> + sponsored = data.get("sponsored_person")
> + if sponsored is None:
> + self.setFieldError("sponsored_person", "Invalid person")
>
> This appears to enforce a sponsored user? I thought it was optional?

ARGH. I suck. This is as a result of some crappy refactoring, it used to
work, honest :/

>
>
> [2]
>
> + <span
> + tal:define="sponsored_person
> nocall:view/widgets/sponsored_person; + field_name
> sponsored_person/context/__name__; + error
> python:view.getFieldError(field_name);"> + <label>Person being
> sponsored (optional):</label>
> + <tal:sponsored replace="structure sponsored_person"/>
> + </span>
>
> Where is the error displayed?

We discussed this on IRC and you are ....

> What might work better here is to refactor the widget_row macro from
> launchpad-form.pt. It's a rather limited macro at the moment, because
> it only ever produces a single table row containing a single table
> cell. If you're fed up of UI stuff, I'd be interested in doing that
> myself.

... fixing this for me :)

>
>
> [3]
>
> + assert(
> + force_async or not sponsored,
> + "sponsored must be None for sync copies")
>
> This will never fail. It could instead be written as:
>
> assert force_async or not sponsored, (
> "sponsored must be None for sync copies")
>
> The original statement was asserting that a tuple (<bool>, <str>) was
> truthy. Lint really ought to alert about these things.

Oh FFS! Yes, this is such a crappy mistake... :/

> [4]
>
> The name "sponsored" is unfortunate because it's ambiguous. Is it
> sponsored (boolean), person sponsored by, person being sponsored? It
> might be better to use something like "person_being_sponsored".

Right, I've fixed it in a few places.

[Julian Edwards (julian-edwards)]

I just resubmitted with your branch as a pre-req.

[Gavin Panella (allenap)]

review: Approve

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2011-12-19 15:39:20 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2011-12-19 15:39:20 +0000
@@ -104,8 +104,9 @@
 
       <metal:bottom fill-slot="extra_bottom">
         <tal:sponsored-person
-            define="widget nocall:view/widgets/sponsored_person"
-            condition="view/actions/field.actions.sync/available">
+            define="widget nocall:view/widgets/sponsored_person;
+                    sync view/actions/byname/field.actions.sync|nothing"
+            condition="python:sync and sync.available()">
           <metal:widget-div use-macro="widget/@@launchpad_form/widget_div" />
         </tal:sponsored-person>
       </metal:bottom>