Merge into private-builds : ssh_ui_mods : Code : Offspring

Status:	Merged
Merged at revision:	75
Proposed branch:	lp://staging/~dooferlad/offspring/ssh_ui_mods
Merge into:	lp://staging/~linaro-automation/offspring/private-builds
Diff against target:	767 lines (+556/-44) 10 files modified lib/offspring/web/media/js/jquery.placeholder.js (+106/-0) lib/offspring/web/queuemanager/admin.py (+3/-1) lib/offspring/web/queuemanager/forms.py (+133/-5) lib/offspring/web/queuemanager/models.py (+1/-1) lib/offspring/web/queuemanager/tests/test_views.py (+107/-1) lib/offspring/web/queuemanager/views.py (+47/-0) lib/offspring/web/templates/queuemanager/project_create.html (+59/-18) lib/offspring/web/templates/queuemanager/project_edit.html (+35/-18) lib/offspring/web/templates/queuemanager/project_edit_credentials.html (+64/-0) lib/offspring/web/urls.py (+1/-0)
To merge this branch:	bzr merge lp://staging/~dooferlad/offspring/ssh_ui_mods
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Guilherme Salgado	2011-11-03	Pending
James Tunnicliffe		Pending
Review via email: mp+81217@code.staging.launchpad.net

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-25: Posted in a previous version of this proposal

#

Hi James, this branch has a conflict; care to resolve it before I review?

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-26: Posted in a previous version of this proposal

#

Have rebased. Should be fine now.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-26: Posted in a previous version of this proposal

#

Hi James,

In my previous review I suggested creating a new page to edit the SSH keys and lp username because it would:

1. be simpler to implement
2. as a consequence, less fragile and easier to test
3. allow us to provide a simple UI: we could use one form button to save an SSHkey/lp_username change and another to remove the SSHkey/username. Remember that these two things need to be removed together as having just one of them will cause the slave to crash

I see you've added a way to remove an ssh key, but using a checkbox for that is poor UI at best, and it doesn't seem to remove the LP user, as we should do, at the same time (if it did, it'd be even more confusing).

I also suggested placing the "An SSH key is stored for this project...." helper text outside of the textarea instead of using JS to place it inside because the latter doesn't add (IMO) much value and can't be unit tested (unless we spend a significant amount of time writing/configuring some way of testing the JS in our views). This also depends on jquery and an extra jquery plugin, which should not be included without some discussion because Offspring already includes a javascript library (http://www.smartclient.com/product/smartclient.jsp).

I'm not trying to make things perfect here, but I think we should try hard to avoid unnecessary complexity, and I do believe my suggestions would make things just as nice (nicer, in some cases) to the user and much simpler for us.

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-28: Posted in a previous version of this proposal

#

Right, have moved the SSH credentials editing to a new page, removed the extra jquery, added some more unit tests and made sure that help text is always complete outside the placeholder text inside the SSH key entry box.

review: Needs Resubmitting

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-28: Posted in a previous version of this proposal

#

Hi James,
This one still has stuff that ought to have been removed, like the two js files and changes to project_create.html. Please go through the diff when you submit a MP so that the person who's reviewing the code can focus on the interesting things rather than trying to guess what's left over from previous commits and what's actually used in the current code.

review: Needs Resubmitting

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-28: Posted in a previous version of this proposal

#

Sorry, I was under the impression that I was keeping in the updates to
project_create.html, which its why it and the js files are still there. I
will get the changes reverted and remove the js. If we want something more
fancy in the future I can go back and revisit it.

I am out at dinner now so don't expect any updates tonight.

James
On Oct 28, 2011 8:53 PM, "Guilherme Salgado" <email address hidden>
wrote:

> Review: Resubmit
>
> Hi James,
> This one still has stuff that ought to have been removed, like the two js
> files and changes to project_create.html. Please go through the diff when
> you submit a MP so that the person who's reviewing the code can focus on the
> interesting things rather than trying to guess what's left over from
> previous commits and what's actually used in the current code.
> --
> https://code.launchpad.net/~dooferlad/offspring/ssh_ui_mods/+merge/80701
> You are the owner of lp:~dooferlad/offspring/ssh_ui_mods.
>

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-28: Posted in a previous version of this proposal

#

Oh, sorry, I mixed up project_create with project_edit. You're right that we want to allow people to enter the ssh key when creating a new project, but I don't see a reason for having the ssh_key/lp_user fields hidden initially, so we could probably get rid of the other js file. There's also the showAddAnotherPopup() function in the new template, which probably comes from the edit template that you used as a base, although I don't think is needed?

I'm not fond of using javascript (and specially a jquery placeholder plugin, as that's not the js library used in Offspring) to have the help text displayed inside the text area, but if you feel strong about it, that's fine.

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-29: Posted in a previous version of this proposal

#

No worries. I have got rid of the show/hide bit on the project add page and showAddAnotherPopup(). I have left the jquery placeholder plugin in because I think it improves the user experience and provides backwards compatibility for those using lesser browsers. I am not sure if it is a strong opinion though!

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-31: Posted in a previous version of this proposal

#

Download full text (28.1 KiB)

Here are the things we discussed in person today...

> === modified file 'lib/offspring/web/queuemanager/forms.py'
> --- lib/offspring/web/queuemanager/forms.py 2011-10-19 20:23:49 +0000
> +++ lib/offspring/web/queuemanager/forms.py 2011-10-31 15:48:00 +0000
> @@ -1,8 +1,11 @@
> # Copyright 2010 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> +import re
> +
> from django.contrib.auth.models import User
> from django.db import models
> +from django import forms
> from django.forms import (
> Form, ModelChoiceField, ModelForm, Textarea, TextInput, ValidationError)
> from django.forms import fields
> @@ -15,9 +18,81 @@
>
> from offspring.web.queuemanager.widgets import SelectWithAddNew
>
> -class ProjectBaseForm(ModelForm):
> - status = fields.CharField(max_length=200,
> - widget=fields.Select(choices=Project.STATUS_CHOICES), required=True)
> +class SSHPrivateKeyField(forms.Field):
> +
> + def validate(self, value):
> + "Check to see if the value looks like an SSH private key"
> +
> + # Use the parent's handling of required fields, etc.
> + super(SSHPrivateKeyField, self).validate(value)
> +
> + key_search_regexp = (r"-----BEGIN \w+ PRIVATE KEY-----"+
> + r".*"+
> + r"-----END \w+ PRIVATE KEY-----")
> + is_key = re.search(key_search_regexp, value, re.DOTALL | re.MULTILINE)
> +
> + if not is_key and not value == "":
> + msg = ("The key you entered doesn't appear to be valid. I am "+
> + "expecting a key in the form:\n "+
> + "-----BEGIN <type> PRIVATE KEY-----\n"+
> + "<ASCII key>\n"+
> + "-----END <type> PRIVATE KEY-----\n")
> + raise forms.ValidationError(msg)
> +
> +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)
> +
> + lp_fields_set = False
> + lp_ssh_set = False
> + lp_ssh_set_message = ("An SSH key is stored for this project. To replace "+
> + "it, paste a new SSH private key block here.")
> + lp_ssh_clear_meessage = ("To enable access to private repositories,"+
> + "enter an SSH private key here in the form of an "+
> + "ASCII key block.")
> +
> + def __init__(self, *args, **kwargs):
> + super(ProjectSSHCredForm, self).__init__(*args, **kwargs)
> + # Set the form fields based on the model object
> + if kwargs.has_key('instance'):
> + instance = kwargs['instance']
> + if instance.lp_ssh_key and instance.lp_ssh_key != "":

The second clause is redundant her...

Here are the things we discussed in person today...

> === modified file 'lib/offspring/web/queuemanager/forms.py'
> --- lib/offspring/web/queuemanager/forms.py	2011-10-19 20:23:49 +0000
> +++ lib/offspring/web/queuemanager/forms.py	2011-10-31 15:48:00 +0000
> @@ -1,8 +1,11 @@
>  # Copyright 2010 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
> +import re
> +
>  from django.contrib.auth.models import User
>  from django.db import models
> +from django import forms
>  from django.forms import (
>      Form, ModelChoiceField, ModelForm, Textarea, TextInput, ValidationError)
>  from django.forms import fields
> @@ -15,9 +18,81 @@
>  
>  from offspring.web.queuemanager.widgets import SelectWithAddNew
>  
> -class ProjectBaseForm(ModelForm):
> -    status = fields.CharField(max_length=200,
> -        widget=fields.Select(choices=Project.STATUS_CHOICES), required=True)
> +class SSHPrivateKeyField(forms.Field):
> +
> +    def validate(self, value):
> +        "Check to see if the value looks like an SSH private key"
> +
> +        # Use the parent's handling of required fields, etc.
> +        super(SSHPrivateKeyField, self).validate(value)
> +
> +        key_search_regexp = (r"-----BEGIN \w+ PRIVATE KEY-----"+
> +                             r".*"+
> +                             r"-----END \w+ PRIVATE KEY-----")
> +        is_key = re.search(key_search_regexp, value, re.DOTALL | re.MULTILINE)
> +
> +        if not is_key and not value == "":
> +            msg = ("The key you entered doesn't appear to be valid. I am "+
> +                   "expecting a key in the form:\n "+
> +                   "-----BEGIN <type> PRIVATE KEY-----\n"+
> +                   "<ASCII key>\n"+
> +                   "-----END <type> PRIVATE KEY-----\n")
> +            raise forms.ValidationError(msg)
> +
> +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)
> +
> +    lp_fields_set = False
> +    lp_ssh_set = False
> +    lp_ssh_set_message = ("An SSH key is stored for this project. To replace "+
> +                          "it, paste a new SSH private key block here.")
> +    lp_ssh_clear_meessage = ("To enable access to private repositories,"+
> +                             "enter an SSH private key here in the form of an "+
> +                             "ASCII key block.")
> +
> +    def __init__(self, *args, **kwargs):
> +        super(ProjectSSHCredForm, self).__init__(*args, **kwargs)
> +        # Set the form fields based on the model object
> +        if kwargs.has_key('instance'):
> +            instance = kwargs['instance']
> +            if instance.lp_ssh_key and instance.lp_ssh_key != "":

The second clause is redundant here.

> +                self.lp_ssh_set = True
> +            else:
> +                self.lp_ssh_set = False
> +
> +            self.lp_fields_set = instance.lp_ssh_key or instance.lp_user

I think it does not make sense to have one of them set but not the other,
right? Maybe we should have an extra validation step that requires both to be
set together?

> +
> +    class Meta:
> +        model = Project
> +        widgets = {
> +            'lp_user' : Textarea(attrs={'cols': 70, 'rows' : 1}),
> +        }
> +
> +class ProjectBaseForm(ProjectSSHCredForm):
> +    def save(self, commit=True):
> +        model = super(ProjectBaseForm, self).save(commit=False)
> +
> +        if 'lp_ssh_key_input' in self.cleaned_data:
> +            # Save the SSH key
> +            new_key = self.cleaned_data['lp_ssh_key_input']
> +
> +            if new_key == "":
> +                pass # No new key entered
> +
> +            else: # Save new key (validated in clean_lp_ssh_key_input)
> +                model.lp_ssh_key = new_key
> +
> +        if commit:
> +            model.save()
> +
> +        return model
>  
>      class Meta:
>          model = Project
> @@ -25,7 +100,8 @@
>              'name' : TextInput(attrs={'style': 'text-transform: lowercase;'}),
>              'series' : TextInput(attrs={'style': 'text-transform: lowercase;'}),
>              'config_url': TextInput(attrs={'size': 50}),
> -            'notes' : Textarea(attrs={'cols': 73, 'rows' : 4}),
> +            'notes' : Textarea(attrs={'cols': 70, 'rows' : 4}),
> +            'lp_user' : Textarea(attrs={'cols': 70, 'rows' : 1}),
>          }
>  
>      def clean_name(self):
> @@ -55,6 +131,15 @@
>          exclude = ('name', 'priority', 'is_active', 'access_groups')
>  
>  
> +class EditProjectSSHCredentialsForm(ProjectSSHCredForm):
> +    launchpad_project = ModelChoiceField(
> +        LaunchpadProject.objects, widget=SelectWithAddNew, required=False)

This doesn't seem to be used anywhere.

> +    class Meta(ProjectSSHCredForm.Meta):
> +        exclude = ('priority', 'is_active', 'suite', 'access_groups', 'arch',
> +                   'config_url', 'name', 'priority', 'series', 'status',
> +                   'title')
> +
> +
>  class AccessGroupMemberForm(Form):
>      new_user = ModelChoiceField(queryset=User.objects, required=False)
>  
> 
> === modified file 'lib/offspring/web/queuemanager/tests/test_views.py'
> --- lib/offspring/web/queuemanager/tests/test_views.py	2011-10-21 14:36:55 +0000
> +++ lib/offspring/web/queuemanager/tests/test_views.py	2011-10-31 15:48:00 +0000
> @@ -8,6 +8,8 @@
>      Template,
>      )
>  from django.test import TestCase
> +from django.test.client import Client
> +from django.contrib.auth.models import User
>  
>  from offspring.enums import ProjectBuildStates
>  from offspring.web.queuemanager.models import (
> @@ -71,7 +73,33 @@
>          self.assertEqual(404, response.status_code)
>  
>  
> -class ProjectDetailsViewTests(TestCase, ProjectViewTestsMixin):
> +class CreateProjectUsingWebMixin(object):
> +    def create_project_using_web(self, lp_ssh_key=""):
> +        user = User.objects.create_user('user', 'email@somewhere.com', 'pass')

factory.makeUser()

> +        user.is_staff = True
> +        user.is_superuser = True
> +        user.save()
> +        c = Client()
> +        c.login(username='user', password='pass')

self.client.login(username=user.username, password=user.username)

> +
> +        response = c.post('/projects/+add/',
> +                          {'_is_private': 0,
> +                           'owner': "1",
> +                           'name': "testcodename",
> +                           'title': "title",
> +                           'arch': "i386",
> +                           'series': "lucid",
> +                           'status': "experimental",
> +                           'config_url': "bla",
> +                           'lp_user': "lpuser",
> +                           'lp_ssh_key_input': lp_ssh_key,
> +                           }, follow=True)
> +
> +        return c, response
> +
> +
> +class ProjectDetailsViewTests(TestCase, ProjectViewTestsMixin,
> +                              CreateProjectUsingWebMixin):
>      view_path = 'offspring.web.queuemanager.views.project_details'
>  
>      def get_expected_page_heading(self, project):
> @@ -85,6 +113,45 @@
>              response, self.get_expected_page_heading(project),
>              status_code=200, msg_prefix=response.content)
>  
> +    def test_public_project_with_ssh_auth(self):
> +        project = factory.makeProject(is_private=False)
> +        project.lp_ssh_key = "1234"
> +        project.lp_user = "user"
> +        project.save()
> +        response = self.client.get(
> +            reverse(self.view_path, args=[project.name]))
> +        self.assertContains(
> +            response, self.get_expected_page_heading(project),
> +            status_code=200, msg_prefix=response.content)

This test is not testing anything useful that's not already tested in others;
just drop it.

> +
> +    def test_post_to_create_project_with_ssh_auth(self):
> +        key = ("-----BEGIN type PRIVATE KEY-----\n"+
> +               "12qwaszx34erdfcv\n"+
> +               "-----END type PRIVATE KEY-----\n")
> +
> +        c, response = self.create_project_using_web(key)
> +
> +        self.assertEqual(response.redirect_chain[0][0],
> +                         "http://testserver/projects/testcodename/")
> +        self.assertNotContains(response, "Select a valid choice")
> +        self.assertNotContains(response, "This field is required.")
> +        self.assertNotContains(response, "is not a valid choice.")
> +        self.assertNotContains(response, "The key you entered doesn")
> +        self.assertNotContains(response, "12qwaszx34erdfcv")
> +
> +    def test_post_to_create_project_with_ssh_auth_bad_key(self):
> +        key = ("-----BEGIN type PRVATE KEY-----\n"+
> +               "12qwaszx34erdfcv\n"+
> +               "-----END type PRIVATE KEY-----\n")
> +
> +        c, response = self.create_project_using_web(key)
> +
> +        self.assertNotContains(response, "Select a valid choice")
> +        self.assertNotContains(response, "This field is required.")
> +        self.assertNotContains(response, "is not a valid choice.")
> +        self.assertContains(response, "The key you entered doesn")
> +        self.assertNotContains(response, "12qwaszx34erdfcv")

These tests should be under the ProjectCreateViewTests instead of the one
they're at.

> +
>  
>  class ProjectEditViewTests(TestCase, ProjectViewTestsMixin):
>      view_path = 'offspring.web.queuemanager.views.project_edit'
> @@ -138,6 +205,114 @@
>              status_code=200, msg_prefix=response.content)
>  
>  
> +class ProjectEditSSHViewTests(TestCase,
> +                           CreateProjectUsingWebMixin):
> +    def test_post_to_edit_test_with_ssh_auth(self):
> +        c, response = self.create_project_using_web()

Here you can use factory.makeProject(is_private=False) instead.

> +
> +        self.assertNotContains(response, "Select a valid choice")
> +        self.assertNotContains(response, "This field is required.")
> +        self.assertNotContains(response, "is not a valid choice.")
> +        self.assertNotContains(response, "The key you entered doesn")
> +
> +        key = ("-----BEGIN type PRIVATE KEY-----\n"+
> +               "12qwaszx34erdfcv\n"+
> +               "-----END type PRIVATE KEY-----\n")
> +
> +        # Now add a valid SSH key
> +        response = c.post('/projects/testcodename/+editcredentials',

Here you could use reverse(self.view_path) like in the other test classes.

> +                          {'lp_user': "lpuser",
> +                           'lp_ssh_key_input': key,
> +                           }, follow=True)
> +
> +        self.assertNotContains(response, "Select a valid choice")
> +        self.assertNotContains(response, "This field is required.")
> +        self.assertNotContains(response, "is not a valid choice.")
> +        self.assertNotContains(response, "The key you entered doesn")
> +        self.assertNotContains(response, "12qwaszx34erdfcv")

And then here you can test that project.ssh_key is the value you defined
above. (you might have to use Project.objects.get(pk=project.pk) to refresh
the instance you have a reference to)

> +
> +    def test_post_to_edit_test_with_ssh_auth_bad_key(self):
> +        c, response = self.create_project_using_web()

Same about the makeProject().

> +
> +        self.assertNotContains(response, "Select a valid choice")
> +        self.assertNotContains(response, "This field is required.")
> +        self.assertNotContains(response, "is not a valid choice.")
> +        self.assertNotContains(response, "The key you entered doesn")
> +
> +        key = ("-----BEGIN type PRIVAE KEY-----\n"+
> +               "12qwaszx34erdfcv\n"+
> +               "-----END type PRIVATE KEY-----\n")
> +
> +        # Now add a valid SSH key
> +        response = c.post('/projects/testcodename/+editcredentials',
> +                          {'lp_user': "lpuser",
> +                           'lp_ssh_key_input': key,
> +                           }, follow=True)
> +
> +        self.assertNotContains(response, "Select a valid choice")
> +        self.assertNotContains(response, "This field is required.")
> +        self.assertNotContains(response, "is not a valid choice.")
> +        self.assertContains(response, "The key you entered doesn")
> +        self.assertNotContains(response, "12qwaszx34erdfcv")

Same as the previous test, but testing that the ssh key is not set.

> +
> +    def test_edit_ssh_auth(self):
> +        project = factory.makeProject(is_private=True)
> +        user = project.owner
> +        grant_permission_to_user(user, 'change_project')
> +        self.assertTrue(
> +            self.client.login(username=user.username, password=user.username))
> +
> +        key = ("-----BEGIN type PRIVATE KEY-----\n"+
> +               "12qwaszx34erdfcv\n"+
> +               "-----END type PRIVATE KEY-----\n")
> +
> +        data = {'lp_user': "lpuser",
> +                'lp_ssh_key_input': key,
> +                '_save': 1}
> +        response = self.client.post(
> +            reverse('offspring.web.queuemanager.views.project_editsshcredentials',
> +                    args=[project.name]),
> +            data, follow=True)
> +
> +        self.assertEqual(200, response.status_code)
> +
> +        # re-load the project from the database to pick up changes
> +        user_visible_objects = Project.all_objects.accessible_by_user(user)
> +        project = get_possibly_private_object(
> +                    user, user_visible_objects, pk=project.name)
> +
> +        self.assertEqual('lpuser', project.lp_user)
> +        self.assertEqual(key, project.lp_ssh_key)
> +
> +    def test_edit_ssh_auth_bad_key(self):
> +        project = factory.makeProject(is_private=True)
> +        user = project.owner
> +        grant_permission_to_user(user, 'change_project')
> +        self.assertTrue(
> +            self.client.login(username=user.username, password=user.username))
> +
> +        key = ("-----BEGIN type PR---VATE KEY-----\n"+
> +               "12qwaszx34erdfcv\n"+
> +               "-----END type PRIVATE KEY-----\n")
> +
> +        data = {'lp_user': "lpuser",
> +                'lp_ssh_key_input': key,
> +                '_save': 1}
> +        response = self.client.post(
> +            reverse('offspring.web.queuemanager.views.project_editsshcredentials',
> +                    args=[project.name]),
> +            data, follow=True)
> +
> +        self.assertEqual(200, response.status_code)
> +
> +        # re-load the project from the database to pick up changes
> +        user_visible_objects = Project.all_objects.accessible_by_user(user)
> +        project = get_possibly_private_object(
> +                    user, user_visible_objects, pk=project.name)
> +
> +        self.assertNotEqual('lpuser', project.lp_user)
> +        self.assertNotEqual(key, project.lp_ssh_key)

so these ones have my suggestions from above, so just delete the first two
ones.

We need a test to ensure the a private project's +editsshcredentials page
can't be seen by a user who's not allowed to see that project.

> +
>  class ProjectCreateViewTests(TestCase):
>      view_path = 'offspring.web.queuemanager.views.project_create'
>  
> 
> === 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-10-31 15:48:00 +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,67 @@
>  {% 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" or field.html_name == "erase_ssh_key" %}
> +
> +                {% else %}

The 'erase_ssh_key' field is gone, but we should leave a comment here
explaining that these fields are rendered at the bottom.

> +                    <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:
> +
> +            <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-10-31 15:48:00 +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"%}
> +                {% else %}

Most of the changes here can be reverted now; we only need the bits at the
bottom that adds the link to +editsshcredentials.

> +                    <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 }}
>                      </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 %}';"/>
> @@ -61,4 +78,7 @@
>  {% endblock %}
>  
>  {% block two-columns %}
> +<script>
> +    $('input[placeholder], textarea[placeholder]').placeholder();
> +</script>

This is no longer used, right?

>  {% endblock %}
> 
> === added file 'lib/offspring/web/templates/queuemanager/project_edit_credentials.html'
> --- lib/offspring/web/templates/queuemanager/project_edit_credentials.html	1970-01-01 00:00:00 +0000
> +++ lib/offspring/web/templates/queuemanager/project_edit_credentials.html	2011-10-31 15:48:00 +0000
> @@ -0,0 +1,64 @@
> +{% extends "base.html" %}
> +
> +{% block header_css %}
> +<link rel="stylesheet" type="text/css" href="/media/css/forms.css" />
> +{% endblock %}
> +
> +{% block header_js %}
> +<script type="text/javascript" src="/media/js/admin/RelatedObjectLookups.js"></script>

We don't need this js here, right?

> +<script type="text/javascript" src="/media/js/jquery.min.js"></script>
> +<script type="text/javascript" src="/assets/js/jquery.placeholder.js"></script>
> +{% endblock %}
> +
> +{% block title %}
> +Update details for {{project.title|capfirst}}

Care to pick a more meaningful title for this page?

> +{% endblock %}
> +
> +{% block content %}
> +    <form method="POST" action="" name="projsettings">{% csrf_token %}
> +        <div class="module aligned ">
> +            <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 }}
> +                                {% if form.lp_ssh_set %}
> +                                    Your saved key is not shown. To replace it, paste a new key in the above box.
> +                                {% else %}
> +                                    No key is stored for this project. Saved keys are not shown.
> +                                {% endif %}
> +                        </p>
> +                    {% endif %}
> +                </div>
> +                {{ form.lp_ssh_key_input.errors|linebreaksbr }}
> +            </div>
> +
> +            <br>
> +
> +            <div class="submit-row" style="overflow: auto;">
> +                <input type="submit" value="Save Updated Credentials" class="default" name="_save"/>
> +                <input type="submit" value="Delete Existing Credentials" class="default" name="delete"/>
> +                <input type="button" value="Cancel" class="default" name="_cancel" OnClick="window.location.href = '{% url offspring.web.queuemanager.views.project_details project.name %}';"/>
> +            </div>
> +        </div>
> +    </form>
> +{% endblock %}
> +
> +{% block two-columns %}
> +<script>
> +    $('input[placeholder], textarea[placeholder]').placeholder();
> +</script>
> +{% endblock %}

review: Needs Fixing

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-11-04:

#

Download full text (9.5 KiB)

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

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" %}
> +                    
> +                {% 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 %}';"/>
>

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-11-07:

#

Download full text (5.3 KiB)

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

On 4 November 2011 20:46, Guilherme Salgado
<guilherme.salgado@linaro.org> 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

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-11-08:

#

Download full text (6.6 KiB)

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

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?

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-11-08:

#

Download full text (8.0 KiB)

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

On 8 November 2011 15:46, Guilherme Salgado
<guilherme.salgado@linaro.org> 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

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-11-08:

#

On 8 November 2011 16:43, James Tunnicliffe <email address hidden> wrote:

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

Just confirmed that field.field.field.help_text doesn't work, so this
is a bug fix.

--
James Tunnicliffe

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-11-08:

#

Got it (I think). Multiple inheritance saves the day!

class ProjectBaseWithSSHForm(ProjectBaseForm, ProjectSSHCredForm):
pass

Using this as the form for ProjectAdmin and the base for
CreateProjectForm, leaving EditProjectForm as is, it creates the
correct combined forms when we need them. Will submit a new patch
after I have cleaned things up and verified the DB changes.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-11-08:

#

Download full text (8.0 KiB)

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

On Tue, 2011-11-08 at 16:43 +0000, James Tunnicliffe wrote:
> On 8 November 2011 15:46, Guilherme Salgado
> <guilherme.salgado@linaro.org> 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.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-11-08:

#

Ok, so after admitting this code was too complicated for my small brain I decided to try and simplify it a bit. lp:~salgado/offspring/ssh_ui_mods has a simpler version of this, with a much simpler form class, no need for the custom field and validation on the lowest possible level. As a bonus I've also fixed a bug in which the +editcredentials page was setting some attributes back to their default values because these fields were not in Meta.exclude, but also weren't rendered in the template (the list of fields rendered there are hard-coded to just lp_user and lp_ssh_key).

The key was using separate form classes depending on whether you're creating or editing a project and moving the validation to the model so that it's used in both forms. Please let me know if it looks good, James, or if you see any issues with it: https://code.launchpad.net/~salgado/offspring/ssh_ui_mods/+merge/81641

Offspring

Merge lp://staging/~dooferlad/offspring/ssh_ui_mods into lp://staging/~linaro-automation/offspring/private-builds

Commit message

Description of the change

Preview Diff

Subscribers