Add Security Center Widget on Admin Dashboard
Review Request #10189 — Created Oct. 1, 2018 and updated
Add a new Security Center Widget in the Admin Dashboard which gets data
fromSecurityCheckRunner()
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 rerunsSecurityCheckRunner()
and refreshes the widget data.
- Added and registered the widget and confirmed it appears on dashboard
- Tested by appending
datetime.now()
to theerror_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 | |
W293 blank line contains whitespace |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E712 comparison to False should be 'if cond is False:' or 'if not cond:' |
reviewbot | |
E501 line too long (85 > 79 characters) |
reviewbot | |
F401 'reviewboard.admin.security_checks.SecurityCheckRunner' imported but unused |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
W292 no newline at end of file |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
Can you reformat as: results = [ item['err_msg'] for item in runner.run() if not item['result'] ] Also, since we are … |
brennie | |
Undo this. |
brennie | |
Swap these so the keys are in alphabetical order. |
brennie | |
Django template variables follow the same naming conventions as Python, so this variable name should be in lower_snake_case. |
brennie | |
I don't see you using {% static %} anywhere in the template so you shouldn't need to load the staticfiles … |
brennie | |
Comments should be full sentences (i.e., be sentence case and end in a period). |
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 | |
Also, this should go first in case there is an exception that occurs within $() or $.load. We also probably … |
brennie | |
We are using this variable immediately so let's just inline it. |
brennie | |
This should be a rule in a stylesheet. |
brennie | |
This should be flush with the first column. We only indent inside the {% block %}. |
brennie | |
And here. |
brennie | |
And here. |
brennie | |
How about "Run security checks and render the result with the given template." ? |
brennie | |
Missing args, returns. |
brennie | |
"This widget displays ..." |
brennie | |
nit: blank line between these. |
brennie | |
We shouldn't need feedtags (or rather, I don't see any tags in use). |
brennie | |
error is not even passed into this template. Can an error occur? |
brennie | |
We use HTML5 which does not have self closing tags. ie, no / here. |
brennie | |
Should not be indented relative to the above line, i.e., they should start on the same column. |
brennie | |
Only one space of indentation here. |
brennie | |
These need to be marked for translation: {% if count == 1 %} {% blocktrans %} 1 security issue was … |
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 | |
Only one space of indentation here. |
brennie | |
Only one space of indentation here. |
brennie | |
class="entry" |
brennie | |
This has no matching {% if %}. This template shouldn't even be rendering. Have you verified it still works? You … |
brennie | |
Nit: no blank line here. |
brennie | |
This will not work. If you attempt to interpolate a variable var inside a {% blocktrans %} block without doing … |
brennie | |
Since we're not doing any interpolation anymore, we can use {% trans "1 security issue was found:" %} instead. |
brennie | |
There is an extra space between blocktrans and % |
brennie | |
There is an extra space between blocktrans and % |
brennie | |
If we use {# ... #} we can have the comment not render to the client, which means less bytes … |
brennie | |
Can you make it so has-errors is the red one? |
brennie | |
Can we add a case where count == 0 to be something like "No security issues were found". In that … |
brennie | |
In your widgets.py you have commas between your list items in your actions list but here you don't. Keep your … |
cdkushni |
- Change Summary:
-
Fix formatting errors.
- Commit:
-
a4871139eb12e4e9d4f4467915255bfe1df596715c915a188f08ce98b1f2ecbacab52ad528f3a3dd
- Change Summary:
-
One last fix for a line that was too long still.
- Commit:
-
5c915a188f08ce98b1f2ecbacab52ad528f3a3ddab48ee7c790f55898234d7a78c56e035e9543e96
Checks run (2 succeeded)
-
-
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
orerr_msgs
. -
-
-
Django template variables follow the same naming conventions as Python, so this variable name should be in
lower_snake_case
. -
I don't see you using
{% static %}
anywhere in the template so you shouldn't need to load thestaticfiles
template library. -
-
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(); });
-
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. -
- Change Summary:
-
Fixes as described by @brennie
- Commit:
-
ab48ee7c790f55898234d7a78c56e035e9543e96067a103c8333cebab77004bac9344862f447bdc1
Checks run (2 succeeded)
- Change Summary:
-
Reworked table into divs, moved styling to stylesheet.
- Commit:
-
067a103c8333cebab77004bac9344862f447bdc10b402e3e00b26cda1066f0223ca7725ac1f3b8bb
Checks run (2 succeeded)
- Change Summary:
-
Additional changes based on Barret's review.
- Commit:
-
0b402e3e00b26cda1066f0223ca7725ac1f3b8bb13f08f543393e2cda4813839918f0b8e280ee19e
Checks run (2 succeeded)
- Change Summary:
-
Docstrings updated.
- Commit:
-
13f08f543393e2cda4813839918f0b8e280ee19eea017bb49f81b6cf01e56fa75db86609a5706f42
Checks run (2 succeeded)
-
-
-
-
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 %}
-
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. -
-
-
-
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.
- Change Summary:
-
Additional changes based on feedback; all RB tests passed.
- Commit:
-
ea017bb49f81b6cf01e56fa75db86609a5706f424ae33b3b02b643550cb02cc32a4f6c7aae6a9c55
Checks run (2 succeeded)
-
-
-
-
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.
-
-
-
If we use
{# ... #}
we can have the comment not render to the client, which means less bytes over the wire.
- Change Summary:
-
pls approv
- Commit:
-
4ae33b3b02b643550cb02cc32a4f6c7aae6a9c551d947a3b0606688d85908b3707899bf61f799450
Checks run (2 succeeded)
- Change Summary:
-
Screenshot of Security Center Widget added to Description.
- Description:
-
Add a new Security Center Widget in the Admin Dashboard which gets data
from SecurityCheckRunner()
and displays the count of errors as wellas 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.+ +
- Change Summary:
-
Added it as a file instead of putting it in the Description.
- Description:
-
Add a new Security Center Widget in the Admin Dashboard which gets data
from SecurityCheckRunner()
and displays the count of errors as wellas 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 Files:
-
Teeny tiny nits :)
-
Since we're not doing any interpolation anymore, we can use
{% trans "1 security issue was found:" %}
instead. -
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.
- Commit:
-
1d947a3b0606688d85908b3707899bf61f799450901d2ef85b84a19b47fa88b4d689c4eee3525981
Checks run (2 succeeded)
- Change Summary:
-
i r dum
- Commit:
-
901d2ef85b84a19b47fa88b4d689c4eee3525981f2d9531688ee6be578bfa2922011b63f20d4b1da