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

On Tue, 2011-11-08 at 16:43 +0000, James Tunnicliffe wrote:
> 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.

In the future, whenever you run into something like this and have to
work it around somewhere else, please do leave a comment detailing your
findings. When reviewing code most people will usually notice those
hacks to workaround stuff, and unless there's a comment explaining them,
there will be a lot of time wasted trying to figure out why it's needed
and in email exchanges with the developer, as just happened. I'd seen
this in the first review I did and was hoping you'd get rid of it, but
if I knew the real reason why the hack could not be removed, we'd have
figured out alternatives by now.

Given that your custom field defines just one validate() method, we can
get rid of the custom field by moving the validate() method to model
code:

    lp_ssh_key = TextField(..., validators=[validate_ssh_key])

    def validate_ssh_key(value):
        ...
        if not is_key:
            raise django.core.exceptions.ValidationError

That actually makes more sense than doing the validation in a form field
because it makes it harder for future views (or non-web stuff, that
don't use the custom form field) to accidentally bypass the validation.

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

Oh, ok, was just curious about that. It's quite likely that it was just
broken before.

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

I call YAGNI, then, but those changes also make it for an even longer
diff for no good reason.