Bugfix: Issue 2743, allow users to remove ssh from admin settings

Review Request #3758 — Created Feb. 2, 2013 and submitted

Information

Review Board
master

Reviewers

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 28 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 75 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 76 E502 the backslash is redundant between brackets

reviewbotreviewbot

This makes an assumption that might not be true down the line when we have multiple keys; it assumes that …

mike_conleymike_conley

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

mike_conleymike_conley

Can you add docstrings to this and delete?

chipx86chipx86

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

chipx86chipx86

No blank line here.

chipx86chipx86

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

chipx86chipx86

HTML should use 1 space indentation.

chipx86chipx86

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

reviewbotreviewbot

Blank line between these.

chipx86chipx86

Test functions should be more like: test_generate_request (Some older tests use testBlah, but we're trying to move away from that.) …

chipx86chipx86

Test docstrings should be sort of namespaced, since there's so many of them. Something like: """Testing SSHSettingsForm POST with generate_key=1"""

chipx86chipx86

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

chipx86chipx86

ssh_client

chipx86chipx86

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

chipx86chipx86

Nitpick, but this should look more like: response = client.post(..., { 'generate_key': 'True', }) I'm not sure "True" is correct, …

chipx86chipx86

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

chipx86chipx86

Like above, I'd recommend: """Testing SSHSettingsForm POST with delete_key=1"""

chipx86chipx86

Should use self.client instead.

chipx86chipx86

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

chipx86chipx86

Same comment about the {}

chipx86chipx86

I'm leaning toward saying we should remove this function. Reachable is an iffy description, because it may mean different things …

chipx86chipx86

You won't need to default this.

chipx86chipx86

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

chipx86chipx86

Should be assertNotEqual

chipx86chipx86
reviewbot
  1. 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
    
    
  2. reviewboard/admin/forms.py (Diff revision 1)
     
     
    Show all issues
    Col: 52
     W291 trailing whitespace
    
  3. reviewboard/admin/forms.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/admin/forms.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (88 > 79 characters)
    
  7. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  9. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 47
     E702 multiple statements on one line (semicolon)
    
  10. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 28
     W291 trailing whitespace
    
  11. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 20
     E711 comparison to None should be 'if cond is None
  12. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  13. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  14. reviewboard/ssh/client.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  15. 
      
GR
reviewbot
  1. 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
    
    
  2. reviewboard/ssh/client.py (Diff revision 2)
     
     
    Show all issues
    Col: 75
     W291 trailing whitespace
    
  3. reviewboard/ssh/client.py (Diff revision 2)
     
     
    Show all issues
    Col: 47
     E702 multiple statements on one line (semicolon)
    
  4. reviewboard/ssh/client.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  5. 
      
GR
GR
reviewbot
  1. 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
    
    
  2. reviewboard/ssh/client.py (Diff revision 3)
     
     
    Show all issues
    Col: 76
     E502 the backslash is redundant between brackets
    
  3. 
      
mike_conley
  1. Hey Greg,
    
    Can you post a screenshot of how this looks?
    
    -Mike
  2. 
      
GR
GR
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 
      
  2. reviewboard/admin/views.py (Diff revision 4)
     
     
     
     
     
     
     
     
    Show all issues
    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
  3. 
      
GR
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 
      
  2. reviewboard/ssh/client.py (Diff revision 5)
     
     
    Show all issues
    Move the check for key is not None after the try/except.
  3. 
      
GR
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 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.
    1. Agree. 
      I had a look into the admin/forms.py, but I didn't find any other form class has done any test similar. Is there an example I can follow for this kind of unit test? 
      And a little bit google I did tells me to derive the `unittest.TestCase` or django's `TestCase`. I guess it is not the thing you wish me to do. 
  2. reviewboard/admin/forms.py (Diff revision 6)
     
     
    Show all issues
    Can you add docstrings to this and delete?
  3. reviewboard/ssh/client.py (Diff revision 6)
     
     
     
    Show all issues
    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."
  4. reviewboard/ssh/client.py (Diff revision 6)
     
     
     
     
    Show all issues
    No blank line here.
  5. reviewboard/ssh/client.py (Diff revision 6)
     
     
    Show all issues
    PasswordRequiredException is a subclass of SSHException, so it's not needed here.
  6. reviewboard/templates/admin/ssh_settings.html (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues
    HTML should use 1 space indentation.
  7. What happens if this is left out? Does the alignment break?
    1. If this <label> is left out, the <input> will appear with no indentation. But I think the `class="required"` is not necessary here. I will fix that. Thank you. 
  8. 
      
GR
reviewbot
  1. 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
    
    
  2. 
      
GR
reviewbot
  1. 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
    
    
  2. reviewboard/admin/tests.py (Diff revision 8)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
GR
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/admin/tests.py (Diff revision 9)
     
     
     
    Show all issues
    Blank line between these.
  3. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    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.
  4. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    Test docstrings should be sort of namespaced, since there's so many of them. Something like:
    
    """Testing SSHSettingsForm POST with generate_key=1"""
  5. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    You shouldn't instantiate this. Instead, use self.client.
  6. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    ssh_client
  7. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    Comments should end with periods. Same with the others in this file.
  8. reviewboard/admin/tests.py (Diff revision 9)
     
     
     
    Show all issues
    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.
  9. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    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.
  10. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    Like above, I'd recommend:
    
    """Testing SSHSettingsForm POST with delete_key=1"""
  11. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    Should use self.client instead.
  12. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    ssh_client.
    
    ACtually, stuff like this is best done in a testcase's setUp() function, since it's common amongst tests.
  13. reviewboard/admin/tests.py (Diff revision 9)
     
     
    Show all issues
    Same comment about the {}
  14. reviewboard/ssh/client.py (Diff revision 9)
     
     
    Show all issues
    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.
  15. reviewboard/ssh/client.py (Diff revision 9)
     
     
     
    Show all issues
    You won't need to default this.
  16. 
      
GR
reviewbot
  1. 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
    
    
  2. 
      
GR
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 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 :)
  2. reviewboard/admin/tests.py (Diff revision 11)
     
     
    Show all issues
    These assertTrue calls should be assertEqual calls, comparing against None.
  3. reviewboard/admin/tests.py (Diff revision 11)
     
     
    Show all issues
    Should be assertNotEqual
  4. 
      
GR
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
GR
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.7.x (2c75a8b). Thanks!