This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

Col: 52
 W291 trailing whitespace

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 80
 E501 line too long (88 > 79 characters)

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

Col: 47
 E702 multiple statements on one line (semicolon)

Col: 28
 W291 trailing whitespace

Col: 20
 E711 comparison to None should be 'if cond is None

Col: 80
 E501 line too long (84 > 79 characters)

Col: 1
 W293 blank line contains whitespace

Col: 1
 W293 blank line contains whitespace

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

Col: 75
 W291 trailing whitespace

Col: 47
 E702 multiple statements on one line (semicolon)

Col: 80
 E501 line too long (84 > 79 characters)

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

Col: 76
 E502 the backslash is redundant between brackets

Hey Greg,

Can you post a screenshot of how this looks?

-Mike

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

This makes an assumption that might not be true down the line when we have multiple keys; it assumes that if we already have a key, then if we POSTed, then we're deleting.

This will no longer be true when we have multiple keys. I know we'd kinda re-architect all this stuff in that case, but let's just be clear.

We should ensure that we're actually asking for a delete.

What I'm suggesting is something like:

if form.wants_delete() and client.has_reachable_user_key():
  # do delete
else:
  # don't delete

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

Move the check for key is not None after the try/except.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

I need to see the Testing Done section filled out, and the following cases covered:

1) Creating a key from scratch still works
2) Clicking Save when there's a key doesn't delete the key
3) Deleting works.

In fact, this is probably a good first exercise for learning to write unit tests. Tests that covered those three cases would ensure that this does work and never breaks. You'd want to edit admin/forms.py and add a new test suite (a class) and simulate data going into the URL for the form. You can read about how the Django test client (self.client) works for this. Basically, programatic simulation of a browser doing GETs and POSTs to a form. You'd set up the expected environment in code (creating a key), perform the repro steps (loading the form, performing an action), and then check the result.

Can you add docstrings to this and delete?

The first line of a docstring can't wrap. See if it can be reworded, and go into more detail in the description.

It should cover what is meant by "reachable."

No blank line here.

PasswordRequiredException is a subclass of SSHException, so it's not needed here.

HTML should use 1 space indentation.

What happens if this is left out? Does the alignment break?

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
    reviewboard/admin/tests.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
    reviewboard/admin/tests.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

Col: 80
 E501 line too long (80 > 79 characters)

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/ssh/client.py
    reviewboard/admin/views.py
    reviewboard/admin/tests.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

Blank line between these.

Test functions should be more like:

test_generate_request

(Some older tests use testBlah, but we're trying to move away from that.)

In this case, I'd call this test_generate_key. Similar with others. The "request" part isn't really what we're generating.

Test docstrings should be sort of namespaced, since there's so many of them. Something like:

"""Testing SSHSettingsForm POST with generate_key=1"""

You shouldn't instantiate this. Instead, use self.client.

ssh_client

Comments should end with periods. Same with the others in this file.

Nitpick, but this should look more like:

    response = client.post(..., {
        'generate_key': 'True',
    })

I'm not sure "True" is correct, though. I believe a form will serialize as 1. If you put 'False' in there, it'd probably turn into 'true' due to how it'd be compared.

One last thing that applies to both tests. Don't hard-code URLs. Instead, use reverse(). This allows the tests to work if the URL scheme is different at any point.

This comment is just saying what the code already says, so it's not very useful. What would be more useful is knowing why it's 302.

In general, only have a comment when the code is not necessarily obvious, and the comment is there to explain it better.

Like above, I'd recommend:

"""Testing SSHSettingsForm POST with delete_key=1"""

Should use self.client instead.

ssh_client.

ACtually, stuff like this is best done in a testcase's setUp() function, since it's common amongst tests.

Same comment about the {}

I'm leaning toward saying we should remove this function.

Reachable is an iffy description, because it may mean different things depending on the SSH key backend. It's also not something we want to ignore, because it could mean there's a problem.

For example, the default with RB is to use filesystem-based keys, so reachable here is assuming 1) existence, and 2) access permissions. What if there's a valid key, but on a network share that goes away? Or there's just a permissions issue, but the file is where we expect it (which could cause it to be overwritten when we generate a new one, and fail).

The backend can be database-backed, which means that "unreachable" could mean whole new things. Network problems, connectivity problems.

I think if we have a failure to reach the key when deleting (the only place that uses this function), we should be explicit that there was a problem in the form.

In that case, I'm not sure this function is  ideal. I think the form should be calling get_user_key (which means we'll log the errors) and then display the error if there's a failure.

You won't need to default this.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/admin/views.py
    reviewboard/admin/tests.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/admin/views.py
    reviewboard/admin/tests.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

This is good to go, with one small exception. You're using the wrong assertions, which will make it harder to debug if anything goes wrong. We're comparing values, so you want the assertEqual/assertNotEqual calls, which will show us both values when it fails.

After this, we'll commit it :)

These assertTrue calls should be assertEqual calls, comparing against None.

Should be assertNotEqual

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/admin/views.py
    reviewboard/admin/tests.py
  Ignored Files:
    reviewboard/templates/admin/ssh_settings.html

Ship It!