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

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

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.