Code review comment for lp:~dooferlad/offspring/ssh_ui_mods

On 8 November 2011 15:46, Guilherme Salgado
<email address hidden> wrote:
> 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

The problem is this (queuemanager/forms.py):

class ProjectSSHCredForm(ModelForm):
    ssh_help = ("Enter a private SSH ASCII key block, complete with begin "+
                "and end markers.")

    lp_ssh_key_input = SSHPrivateKeyField(label="Launchpad User's SSH key",
                                       required=False,
                                       widget=forms.Textarea(
                                               attrs={'cols': 70, 'rows' : 4}),
                                       help_text=ssh_help)

Since lp_ssh_key_input is added like this it is always created without
any reference to self.Meta.exclude. It looks like django won't let me
add it any later - some validation fails if I create lp_ssh_key_input
in __init__. I hope there is a workaround, but I haven't found it.

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

Thanks, will change it.

>>
>> >> +                {% 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?

I remember field.field.field.help_text not working, which is why I
changed it to field.help_text, but I will try reverting it and seeing
if it is still the case. It could be that I broke something during
development.

>> 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?

We don't at the moment. It is safe to remove it for now. I just put it
there so both the create and edit pages would display the same input
in the same way.

--
James Tunnicliffe