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

On Thu, 2011-11-03 at 22:21 +0000, James Tunnicliffe wrote:
> === modified file 'lib/offspring/web/templates/queuemanager/project_create.html'
> --- lib/offspring/web/templates/queuemanager/project_create.html 2011-02-28 22:48:46 +0000
> +++ lib/offspring/web/templates/queuemanager/project_create.html 2011-11-03 22:20:29 +0000
> @@ -5,7 +5,9 @@
> {% 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" src="/media/js/jquery.min.js"></script>
> +<script type="text/javascript" src="/assets/js/jquery.placeholder.js"></script>
> <script type="text/javascript">
> /* Override showAddAnotherPopup to use custom height and width. */
> function showAddAnotherPopup(triggeringLink) {
> @@ -29,29 +31,68 @@
> {% 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 catches lp_user and lp_ssh_key_input so they aren't shown in the top of
> + the page. They are displayed below in their own section after some help text -->
> + {% 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.field.field.help_text %}
> + <p class="help">{{ field.field.field.help_text|safe }}</p>
> + {% endif %}
> + </div>
> + {{ field.errors }}
> </div>
> - {{ field.errors }}
> - </div>
> + {% endif %}
>
> {% endfor %}
> +
> + <br>
> + 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?

> +
> + <br>
> +
> + <div class="form-row {{ form.lp_user.name }}">
> + <div class="field-box">
> + {{ form.lp_user.label_tag }}
> + <br>
> + {{ form.lp_user }}
> + </div>
> + {{ form.lp_user.errors }}
> + </div>
> +
> + <div class="form-row {{ form.lp_ssh_key_input.name }}">
> + <div class="field-box">
> + {{ form.lp_ssh_key_input.label_tag }}
> + <br>
> + <textarea id="id_lp_ssh_key_input" rows="4" cols="70" name="lp_ssh_key_input"
> + placeholder="{% if form.lp_ssh_set %}{{ form.lp_ssh_set_message }}{% else %}{{ form.lp_ssh_clear_meessage }}{% endif %}"></textarea>
> + {% if form.lp_ssh_key_input.help_text %}
> + <p class="help">{{ form.lp_ssh_key_input.help_text|safe }}</p>
> + {% endif %}
> + </div>
> + {{ form.lp_ssh_key_input.errors|linebreaksbr }}
> + </div>
> +
> + <br>
> +
> <div class="submit-row" style="overflow: auto;">
> <input type="submit" value="Save" class="default" name="_save"/>
> <input type="button" value="Cancel" class="default" name="_cancel" OnClick="window.location.href = '/';"/>
>
> === 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.

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

> </div>
> - {{ field.errors }}
> - </div>
> + {% endif %}
>
> {% endfor %}
> +
> + <br>
> + 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 form.lp_fields_set %}
> + To update or delete these details, use
> + {% else %}
> + If you would like to add these details,
> + {% endif %}
> + <a href="+editcredentials">use this form.</a>
> +
> <div class="submit-row" style="overflow: auto;">
> <input type="submit" value="Save" class="default" name="_save"/>
> <input type="button" value="Cancel" class="default" name="_cancel" OnClick="window.location.href = '{% url offspring.web.queuemanager.views.project_details project.name %}';"/>
>