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

On 4 November 2011 20:46, Guilherme Salgado
<email address hidden> wrote:
> On Thu, 2011-11-03 at 22:21 +0000, James Tunnicliffe wrote:
>> === modified file 'lib/offspring/web/templates/queuemanager/project_create.html'
<snip>
>> +            If the config URL above points to a Bazaar repository that is only
>> +            available to authenticated users, you should add a user and private SSH
>> +            key to give the builder access. The SSH key will never be revealed. It
>> +            can be changed or deleted later. It should not be passphrase protected.
>> +            If you would like to add these details, tick here:
>
> Hmm. There's nothing to tick here anymore, right?

Good catch!

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

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

--
James Tunnicliffe