Code review comment for lp:~wallyworld/launchpad/recipe-find-related-branches

On 06/12/10 14:22, Tim Penhey wrote:
> Review: Needs Fixing
> I have two issues with this branch.
>
> 1) What do we show when we can't find any related branches?
>

Nothing. The page appears as it does now before the branch. Do you want
some text to say something like "No related branches"? I didn't think
that was needed but can easily add it.

> 2) Creating a widget is overkill for what is needed, which is some text on the page.

The page template uses this construct to render the form:

<div metal:use-macro="context/@@launchpad_form/form" />

The first implementation this branch involved replacing the above with a
macro:

<div metal:use-macro="context/@@+sourcepackagerecipe-macros/recipe-form"/>

which:

- expanded the default metal form definition to explicitly list the
required form fields to display:
- adds the related branches markup after all the form fields are specified

eg

<div metal:use-macro="context/@@launchpad_form/form">
  <tal:product-name define="widget nocall:widgets/name">
    <metal:block use-macro="context/@@launchpad_form/widget_row" />
  </tal:product-name>
  <tal:product-name define="widget nocall:widgets/description">
    <metal:block use-macro="context/@@launchpad_form/widget_row" />
  </tal:product-name>
  <another form field...>
  <another form field...>

  <!-- markup for related branches goes here -->
  <table>
  xxxx
  </table>

</div>

The macro was used for the add and edit page templates. Because the edit
form used the generic edit template, a new sourcepackagerecipe-edit page
template was created.

I thought that expanding the default form element and explicitly listing
the form fields in the page template was perhaps not the best approach
since a change to the form schema would require editing several
different files. The current implementation programatically adds the
custom field used to display the related branches to the form schema and
is more robust to changes in the underlying form.

I can revert to this approach is that's considered better.