Bugfix: Issue 2743, allow users to remove ssh from admin settings
Review Request #3758 — Created Feb. 2, 2013 and submitted
Bugfix: Issue 2743, allow users to remove ssh from admin settings
Both manually and unit test passed, includes: * Creating a key from scratch still works * Deleting works Manually tested: * Clicking Save when there's a key, doesn't trigger any event
| Description | From | Last Updated |
|---|---|---|
|
Col: 52 W291 trailing whitespace |
 reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (88 > 79 characters) |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 47 E702 multiple statements on one line (semicolon) |
reviewbot
|
|
|
Col: 28 W291 trailing whitespace |
reviewbot
|
|
|
Col: 20 E711 comparison to None should be 'if cond is None |
reviewbot
|
|
|
Col: 80 E501 line too long (84 > 79 characters) |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 75 W291 trailing whitespace |
reviewbot
|
|
|
Col: 47 E702 multiple statements on one line (semicolon) |
reviewbot
|
|
|
Col: 80 E501 line too long (84 > 79 characters) |
reviewbot
|
|
|
Col: 76 E502 the backslash is redundant between brackets |
reviewbot
|
|
|
This makes an assumption that might not be true down the line when we have multiple keys; it assumes that … |
 mike_conley
|
|
|
Move the check for key is not None after the try/except. |
mike_conley
|
|
|
Can you add docstrings to this and delete? |
 chipx86
|
|
|
The first line of a docstring can't wrap. See if it can be reworded, and go into more detail in … |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
PasswordRequiredException is a subclass of SSHException, so it's not needed here. |
chipx86
|
|
|
HTML should use 1 space indentation. |
chipx86
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Blank line between these. |
chipx86
|
|
|
Test functions should be more like: test_generate_request (Some older tests use testBlah, but we're trying to move away from that.) … |
chipx86
|
|
|
Test docstrings should be sort of namespaced, since there's so many of them. Something like: """Testing SSHSettingsForm POST with generate_key=1""" |
chipx86
|
|
|
You shouldn't instantiate this. Instead, use self.client. |
chipx86
|
|
|
ssh_client |
chipx86
|
|
|
Comments should end with periods. Same with the others in this file. |
chipx86
|
|
|
Nitpick, but this should look more like: response = client.post(..., { 'generate_key': 'True', }) I'm not sure "True" is correct, … |
chipx86
|
|
|
This comment is just saying what the code already says, so it's not very useful. What would be more useful … |
chipx86
|
|
|
Like above, I'd recommend: """Testing SSHSettingsForm POST with delete_key=1""" |
chipx86
|
|
|
Should use self.client instead. |
chipx86
|
|
|
ssh_client. ACtually, stuff like this is best done in a testcase's setUp() function, since it's common amongst tests. |
chipx86
|
|
|
Same comment about the {} |
chipx86
|
|
|
I'm leaning toward saying we should remove this function. Reachable is an iffy description, because it may mean different things … |
chipx86
|
|
|
You won't need to default this. |
chipx86
|
|
|
These assertTrue calls should be assertEqual calls, comparing against None. |
chipx86
|
|
|
Should be assertNotEqual |
chipx86
|
-
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 -
-
-
GR
- Change Summary:
-
Add bug tracker number
- Bugs:
GR
- Change Summary:
-
Remove redundant backslash. Remove 'Delete Key' tag, but extend the button label. Update screenshot.
- Diff:
-
Revision 4 (+50 -6)
- Removed Files:
- Added Files:
-
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
GR
- Change Summary:
-
Explicitly check whether delete button was pressed. This fix avoid the potential risk that may delete ssh key upon wrong assumption.
-
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 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.
-
-
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."
-
-
-
-
GR
- Change Summary:
-
1. Fix HTML Template indentation 2. Tried to write TestCase, but failed. Having login issue.
- Testing Done:
-
+ All manual test passed, includes:
+ * Creating a key from scratch still works + * Clicking Save when there's a key, doesn't trigger any event + * Deleting works + + WIP: Tending to build a unit test TestCase, but failed.
+ The main reason is login issue when using the Client to access /admin/settings/ssh/page. Login asadmindoesn't change the situation.
-
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
GR
- Change Summary:
-
* Fix the unit test for SSHSettingsFormTestCase.testDeleteRequest * Add testGenerateRequest unit test to ensure the key generation still works
- Testing Done:
-
~ All manual test passed, includes:
~ Both manually and unit test passed, includes:
* Creating a key from scratch still works - * Clicking Save when there's a key, doesn't trigger any event * Deleting works ~ WIP: Tending to build a unit test TestCase, but failed.
~ The main reason is login issue when using the Client to access /admin/settings/ssh/page. Login asadmindoesn't change the situation.~ Manually tested:
~ * Clicking Save when there's a key, doesn't trigger any event
-
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
-
-
-
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"""
-
-
-
-
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.
-
-
-
ssh_client. ACtually, stuff like this is best done in a testcase's setUp() function, since it's common amongst tests.
-
-
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.
-
GR
- Change Summary:
-
Revise SSHSettingFormTestCase to use self.* and local_site_reverse() Remove has_reachable_user_key(), use get_user_key() instead.
-
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 :)
-
-
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