Add Security Center Widget on Admin Dashboard

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

Review Board
reviewboard, students

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 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
Loading file attachments...

  • 0
  • 0
  • 44
  • 2
  • 46
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.


Review request changed

Change Summary:

Fix formatting errors.




Revision 2 (+87 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.


  2. reviewboard/admin/ (Diff revision 3)

    Can you reformat as:

    results = [
        for item in
        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/ (Diff revision 3)

    Undo this.

  4. reviewboard/admin/ (Diff revision 3)

    Swap these so the keys are in alphabetical order.

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

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

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

  8. Indentation is off here. This should be indented as:

        $('#reload-security-widget').click(function(evt) {
  9. 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. 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.

  2. This should be a rule in a stylesheet.

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

  2. reviewboard/admin/ (Diff revision 5)

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

  3. reviewboard/admin/ (Diff revision 5)

    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.
          request (django.http.HttpRequest):
              The HTTP request from the client.
          template_name (unicode):
              The template to render the result of the security
              checks with.
          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/ (Diff revision 5)

    "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. nit: blank line between these.

  6. 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)

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

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

  2. reviewboard/admin/ (Diff revision 7)

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

  3. Only one space of indentation here.

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

    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. 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. Only one space of indentation here.

  7. Only one space of indentation here.

  8. class="entry"

  9. 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.

  2. Can you add a screenshot?

  3. reviewboard/admin/ (Diff revision 8)

    Nit: no blank line here.

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

    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. There is an extra space between blocktrans and %

  6. There is an extra space between blocktrans and %

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

  1. Teeny tiny nits :)

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

    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)

    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 %}">

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

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

Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
  2. reviewboard/admin/ (Diff revision 11)

    In your 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.