On Mon, 2011-11-07 at 17:10 +0000, James Tunnicliffe wrote:
> >> === modified file 'lib/offspring/web/templates/queuemanager/project_edit.html'
> >> --- lib/offspring/web/templates/queuemanager/project_edit.html        2011-03-03 01:50:40 +0000
> >> +++ lib/offspring/web/templates/queuemanager/project_edit.html        2011-11-03 22:20:29 +0000
> >> @@ -5,7 +5,7 @@
> >>  {% endblock %}
> >>
> >>  {% block header_js %}
> >> -<script type="text/javascript" src="/media/js/admin/RelatedObjectLookups.js"></script>
> >> +<script type="text/javascript" src="/media/js/admin/RelatedObjectLookups.js"></script>
> >>  <script type="text/javascript">
> >>  /* Override showAddAnotherPopup to use custom height and width. */
> >>  function showAddAnotherPopup(triggeringLink) {
> >> @@ -29,29 +29,46 @@
> >>  {% endblock %}
> >>
> >>  {% block content %}
> >> -    <form method="POST" action="">{% csrf_token %}
> >> +    <form method="POST" action="" name="projsettings">{% csrf_token %}
> >>          <div class="module aligned ">
> >>               {% for field in form %}
> >> -                <div class="form-row {% if line.errors %} errors{% endif %} {{ field.name }}">
> >> -                    <div{% if not line.fields|length_is:"1" %} class="field-box"{% endif %}>
> >> -                        {% if field.is_checkbox %}
> >> -                            {{ field }}{{ field.label_tag }}
> >> -                        {% else %}
> >> -                            {{ field.label_tag }}
> >> -                            {% if field.is_readonly %}
> >> -                                <p>{{ field.contents }}</p>
> >> +                {% if field.html_name == "lp_user" or field.html_name == "lp_ssh_key_input"%}
> >
> > This is no longer needed, right?  We've agreed on that in the previous
> > review already, IIRC.
> 
> It is still required because the new fields are part of the project
> form, so they need to be filtered out where they aren't needed.

But the template is the worst place to do that, because the templating
language is not as powerfull as python and testing templates is harder
(and usually slower) than testing forms/views. And the above looks like
an ugly hack to me because of the empty if block, without even a
comment.

Having said that, I've just noticed that you already exclude the
unwanted fields in the form used by this template, so unless I'm missing
something this isn't needed.

        class EditProjectForm(ProjectBaseForm):
            launchpad_project = ModelChoiceField(
                LaunchpadProject.objects, widget=SelectWithAddNew, required=False)
            class Meta(ProjectBaseForm.Meta):
                exclude = ('name', 'priority', 'is_active', 'access_groups',
                           'lp_user', 'lp_ssh_key_input')

I thought the above would be enough to get rid of the lp_* fields on the
edit page, but I've just reverted the template changes locally and the
ssh key text area is still present on the form. That's probably why you
ended up doing the template changes, but instead of doing that we need
to find out why the ssh key field is still shown and fix it so that we
don't need the template hack to omit the field. I'll keep digging to see
if I figure this out 

> Actually I just noticed that since I have switched lp_user to a
> TextField to match the internal representation of Launchpad, it can
> now contain newlines and the form doesn't stop this. I see my options
> as:
> 
> 01234567890123456789012345678901234567890123456789012345678901234567890123456789
> 1. Leave it as it is and add a custom validate to exclude newlines
> 2. Switch it back to a CharField in the DB and set max_length to None, or if
>    that isn't allowed, something large, like 256.
> 3. Do something similar to the ssh input field - leave the internal
>    representation as is, and have a CharField input box with a max_length
>    set to something massive.
> 
> I imagine option 3 is the most like Launchpad, since it would keep the
> same data type, but not impose a name length limit, or at least one
> that wouldn't be hit by any reasonable person! That said, option 2 is
> the easiest to implement and I am having trouble foreseeing a time
> when we need > 256 char user names. I guess some databases may store a
> fixed length char array for this, which may be wasteful if we set a
> large maximum length. Do you have an opinion?

I think a CharField is the correct django representation for this. What
I didn't want was to have a length constraint on the database schema,
because changing that is not as easy (here, because we don't use django
south for migrations) as changing one line of python code. I think it's
fine to use a CharField with max_length=256 as long as the migration
patch doesn't add that constraint to the DB.

> 
> >> +                {% else %}
> >> +                    <div class="form-row {% if line.errors %} errors{% endif %} {{ field.name }}">
> >> +                        <div{% if not line.fields|length_is:"1" %} class="field-box"{% endif %}>
> >> +                            {% if field.is_checkbox %}
> >> +                                {{ field }}{{ field.label_tag }}
> >>                              {% else %}
> >> -                                {{ field }}
> >> -                            {% endif %}
> >> -                        {% endif %}
> >> -                        {% if field.field.field.help_text %}
> >> -                            <p class="help">{{ field.field.field.help_text|safe }}</p>
> >> -                        {% endif %}
> >> +                                {{ field.label_tag }}
> >> +                                {% if field.is_readonly %}
> >> +                                    <p>{{ field.contents }}</p>
> >> +                                {% else %}
> >> +                                    {{ field }}
> >> +                                {% endif %}
> >> +                            {% endif %}
> >> +                            {% if field.help_text %}
> >> +                                <p class="help">{{ field.help_text|safe }}</p>
> >> +                            {% endif %}
> >> +                        </div>
> >> +                        {{ field.errors }}
> >
> > What are these changes for?  Can't we get rid of them?
> 
> They fix the help text (field.field.field.help_text becomes
> field.help_text) and makes the template match the one in

Did you change anything to make field.help_text work or is that just an
existing shortcut to field.field.field.help_text?

> project_create, so it displays read only text in the same way. I think
> they should probably stay just to be consistent, but we could cut it
> down to just the help text fix.

What read only fields can we have here?