• 
      

    Add Security Center Widget on Admin Dashboard

    Review Request #10189 — Created Oct. 1, 2018 and updated

    Information

    Review Board
    master
    f2d9531...

    Reviewers

    Add a new Security Center Widget in the Admin Dashboard which gets data
    from SecurityCheckRunner() and displays the count of errors as well
    as the short error messages from each one. The widget also has a 'More'
    button which goes to the Security Checklist page and a 'Reload' button
    which reruns SecurityCheckRunner() and refreshes the widget data.

    • Added and registered the widget and confirmed it appears on dashboard
    • Tested by appending datetime.now() to the error_msgs returned by
      SecurityCheckRunner and ensuring it updated on hitting Reload
    • Used the above method to also confirm refreshes on the dashboard

    Description From Last Updated

    Can you add a screenshot?

    brennie brennie

    W293 blank line contains whitespace

    reviewbot reviewbot

    E501 line too long (80 > 79 characters)

    reviewbot reviewbot

    E712 comparison to False should be 'if cond is False:' or 'if not cond:'

    reviewbot reviewbot

    E501 line too long (85 > 79 characters)

    reviewbot reviewbot

    F401 'reviewboard.admin.security_checks.SecurityCheckRunner' imported but unused

    reviewbot reviewbot

    E302 expected 2 blank lines, found 1

    reviewbot reviewbot

    W292 no newline at end of file

    reviewbot reviewbot

    E501 line too long (80 > 79 characters)

    reviewbot reviewbot

    Can you reformat as: results = [ item['err_msg'] for item in runner.run() if not item['result'] ] Also, since we are …

    brennie brennie

    Undo this.

    brennie brennie

    Swap these so the keys are in alphabetical order.

    brennie brennie

    Django template variables follow the same naming conventions as Python, so this variable name should be in lower_snake_case.

    brennie brennie

    I don't see you using {% static %} anywhere in the template so you shouldn't need to load the staticfiles …

    brennie brennie

    Comments should be full sentences (i.e., be sentence case and end in a period).

    brennie brennie

    Indentation is off here. This should be indented as: $('#reload-security-widget').click(function(evt) { $('.security-center-content-widget').load('feed/security/?reload=1'); evt.preventDefault(); });

    brennie brennie

    Also, this should go first in case there is an exception that occurs within $() or $.load. We also probably …

    brennie brennie

    We are using this variable immediately so let's just inline it.

    brennie brennie

    This should be a rule in a stylesheet.

    brennie brennie

    This should be flush with the first column. We only indent inside the {% block %}.

    brennie brennie

    And here.

    brennie brennie

    And here.

    brennie brennie

    How about "Run security checks and render the result with the given template." ?

    brennie brennie

    Missing args, returns.

    brennie brennie

    "This widget displays ..."

    brennie brennie

    nit: blank line between these.

    brennie brennie

    We shouldn't need feedtags (or rather, I don't see any tags in use).

    brennie brennie

    error is not even passed into this template. Can an error occur?

    brennie brennie

    We use HTML5 which does not have self closing tags. ie, no / here.

    brennie brennie

    Should not be indented relative to the above line, i.e., they should start on the same column.

    brennie brennie

    Only one space of indentation here.

    brennie brennie

    These need to be marked for translation: {% if count == 1 %} {% blocktrans %} 1 security issue was …

    brennie brennie

    This shouldn't render correctly? .count isn't a thing. Also why the nbsp? And this shouldn't need <br> since <div> is …

    brennie brennie

    Only one space of indentation here.

    brennie brennie

    Only one space of indentation here.

    brennie brennie

    class="entry"

    brennie brennie

    This has no matching {% if %}. This template shouldn't even be rendering. Have you verified it still works? You …

    brennie brennie

    Nit: no blank line here.

    brennie brennie

    This will not work. If you attempt to interpolate a variable var inside a {% blocktrans %} block without doing …

    brennie brennie

    Since we're not doing any interpolation anymore, we can use {% trans "1 security issue was found:" %} instead.

    brennie brennie

    There is an extra space between blocktrans and %

    brennie brennie

    There is an extra space between blocktrans and %

    brennie brennie

    If we use {# ... #} we can have the comment not render to the client, which means less bytes …

    brennie brennie

    Can you make it so has-errors is the red one?

    brennie brennie

    Can we add a case where count == 0 to be something like "No security issues were found". In that …

    brennie brennie

    In your widgets.py you have commas between your list items in your actions list but here you don't. Keep your …

    cdkushni cdkushni
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    gojeffcho
    Review request changed
    Change Summary:

    Fix formatting errors.

    Commit:
    a4871139eb12e4e9d4f4467915255bfe1df59671
    5c915a188f08ce98b1f2ecbacab52ad528f3a3dd

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    gojeffcho
    brennie
    1. 
        
    2. reviewboard/admin/views.py (Diff revision 3)
       
       
       
      Show all issues

      Can you reformat as:

      results = [
          item['err_msg']
          for item in runner.run()
          if not item['result']
      ]
      

      Also, since we are explicitly not collecting the results and instead collecting the error messages, we should probably call this error_messages or err_msgs.

    3. reviewboard/admin/widgets.py (Diff revision 3)
       
       
      Show all issues

      Undo this.

    4. reviewboard/admin/widgets.py (Diff revision 3)
       
       
       
      Show all issues

      Swap these so the keys are in alphabetical order.

    5. Show all issues

      Django template variables follow the same naming conventions as Python, so this variable name should be in lower_snake_case.

    6. Show all issues

      I don't see you using {% static %} anywhere in the template so you shouldn't need to load the staticfiles template library.

    7. Show all issues

      Comments should be full sentences (i.e., be sentence case and end in a period).

    8. Show all issues

      Indentation is off here. This should be indented as:

          $('#reload-security-widget').click(function(evt) {
              $('.security-center-content-widget').load('feed/security/?reload=1');
              evt.preventDefault();
          });
      
    9. Show all issues

      Also, this should go first in case there is an exception that occurs within $() or $.load.

      We also probably want to do evt.stopPropagation() as well.

    10. Show all issues

      We are using this variable immediately so let's just inline it.

      1. I did not inline this as it's used twice.

      2. I shouldn't review before I've had my coffee.

    11. 
        
    gojeffcho
    brennie
    1. 
        
    2. Show all issues

      This should be a rule in a stylesheet.

    3. Show all issues

      This should be flush with the first column. We only indent inside the {% block %}.

    4. Show all issues

      And here.

    5. Show all issues

      And here.

    6. 
        
    gojeffcho
    brennie
    1. 
        
    2. reviewboard/admin/views.py (Diff revision 5)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      How about "Run security checks and render the result with the given template." ?

    3. reviewboard/admin/views.py (Diff revision 5)
       
       
       
      Show all issues

      Missing args, returns.

      1. Could I get more fidelity on what you mean by this? Again, comparing this code to the others in the same file, it looks to be in-style but I may be missing or overlooking something.

      2. It is in style for the file, but we don't want to add new code without proper documentation even if the surrounding file is lacking.

        The docstring should look something like this:

        """Run security checks and pass error messages to template.
        
        Args:
            request (django.http.HttpRequest):
                The HTTP request from the client.
        
            template_name (unicode):
                The template to render the result of the security
                checks with.
        
        Returns:
            django.http.HttpResponse:
            The rendered response.
        """
        
      3. It should be noted we use these docstrings to autogenerate our documentation (see here). So if Args/Returns are missing the documentation will be incomplete.

    4. reviewboard/admin/widgets.py (Diff revision 5)
       
       
      Show all issues

      "This widget displays ..."

      1. I left this in its current format, because it seems to follow the format and voice of the other widget descriptions in this file. If you feel that it should be changed to the style of your suggestion, I can implement this.

      2. It is technically a sentence fragment, since there is no subject, and we don't like to add new things that don't match our current documentation style, even if it clashes with the rest of the documentation in the file. It creates more work down the line to migrate docs that we could have written in our new style in the first place.

    5. Show all issues

      nit: blank line between these.

    6. Show all issues

      We shouldn't need feedtags (or rather, I don't see any tags in use).

    7. reviewboard/templates/admin/security_feed.html (Diff revision 5)
       
       
       
       
      Show all issues

      error is not even passed into this template. Can an error occur?

    8. Show all issues

      We use HTML5 which does not have self closing tags. ie, no / here.

    9. 
        
    gojeffcho
    gojeffcho
    brennie
    1. 
        
    2. reviewboard/admin/views.py (Diff revision 7)
       
       
      Show all issues

      Should not be indented relative to the above line, i.e., they should start on the same column.

    3. Show all issues

      Only one space of indentation here.

    4. reviewboard/templates/admin/security_feed.html (Diff revision 7)
       
       
       
       
       
       
       
      Show all issues

      These need to be marked for translation:

      {% if count == 1 %}
      {%  blocktrans  %}
      1 security issue was found:
      {%  endblocktrans %}
      {% else %}
      {%  blocktrans with count=count %}
      {{count}} security issues were found:
      {%  endblocktrans %}
      {% endif %}
      
    5. Show all issues

      This shouldn't render correctly? .count isn't a thing.

      Also why the nbsp?

      And this shouldn't need <br> since <div> is display: block by default.

    6. Show all issues

      Only one space of indentation here.

    7. Show all issues

      Only one space of indentation here.

    8. Show all issues

      class="entry"

    9. Show all issues

      This has no matching {% if %}. This template shouldn't even be rendering. Have you verified it still works?

      You should re-run all testing with each revision.

    10. 
        
    gojeffcho
    brennie
    1. 
        
    2. Show all issues

      Can you add a screenshot?

    3. reviewboard/admin/views.py (Diff revision 8)
       
       
      Show all issues

      Nit: no blank line here.

    4. reviewboard/templates/admin/security_feed.html (Diff revision 8)
       
       
       
       
      Show all issues

      This will not work. If you attempt to interpolate a variable var inside a {% blocktrans %} block without doing {% blocktrans with var=... %} it will not render.

      Also, we know its 1. Let's just use 1 instead of forcing an interpolation.

    5. Show all issues

      There is an extra space between blocktrans and %

    6. Show all issues

      There is an extra space between blocktrans and %

    7. Show all issues

      If we use {# ... #} we can have the comment not render to the client, which means less bytes over the wire.

    8. 
        
    gojeffcho
    gojeffcho
    gojeffcho
    brennie
    1. Teeny tiny nits :)

    2. reviewboard/templates/admin/security_feed.html (Diff revisions 8 - 9)
       
       
       
       
      Show all issues

      Since we're not doing any interpolation anymore, we can use {% trans "1 security issue was found:" %} instead.

    3. reviewboard/templates/admin/security_feed.html (Diff revision 9)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Can we add a case where count == 0 to be something like "No security issues were found". In that case we don't need to iterate over err_msgs.

      In this case the font colour probably shouldn't be red. I would suggest doing something like:

      <div class="summary{% if count > 0 %} has-errors{% endif %}">
      ...
      </div>
      

      and add a .summary { &.has-errors: { ... } } rule in the stylesheet that overrides the colour.

    4. 
        
    gojeffcho
    brennie
    1. 
        
    2. Show all issues

      Can you make it so has-errors is the red one?

    3. 
        
    gojeffcho
    Review request changed
    Change Summary:

    i r dum

    Commit:
    901d2ef85b84a19b47fa88b4d689c4eee3525981
    f2d9531688ee6be578bfa2922011b63f20d4b1da

    Checks run (2 succeeded)

    flake8 passed.
    JSHint passed.
    cdkushni
    1. 
        
    2. reviewboard/admin/views.py (Diff revision 11)
       
       
      Show all issues

      In your widgets.py you have commas between your list items in your actions list but here you don't. Keep your syntax consistent.

      1. This is a list comprehension, so commas are not required.

    3.