Merge into private-builds : ssh_ui_mods : Code : Offspring

Status:	Superseded
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 (community)	2011-10-29	Needs Fixing on 2011-10-31
James Tunnicliffe		Pending
Review via email: mp+80730@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:

#

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:

#

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

lp://staging/~dooferlad/offspring/ssh_ui_mods updated on 2011-11-03

86. By James Tunnicliffe on 2011-11-03: * Require both or neither ssh user and key.
* Removed bad tests
* Added test to ensure we can't edit a projects SSH credentials if it is private and we don't have permissions
* Code clean up and tidy.
87. By James Tunnicliffe on 2011-11-03: Updated and fixed logic for requiring SSH user and key to be both set, or neither set.

Offspring

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers