support security (and other) alerts #72
Conversation
NightJar
force-pushed
the
pulls/1.0/support-security-alerts
branch
5 times, most recently
from
5760340 to
04f2849
Compare
css/sitesummary.css
Outdated
| /* SS4 colours */ | ||
| .package-summary__badge--good { background: #3fa142; } | ||
| .package-summary__badge--warning {background: #ff7f22;} | ||
| .package-summary__badge--bad {background: #d40404;} |
There was a problem hiding this comment.
Please format this a little bit - two spaces for indentation would be ideal
src/Model/Package.php
Outdated
| public function getBadges() | ||
| { | ||
| $badgeDefinitions = []; | ||
| $this->extend('updageBadges', $badgeDefinitions); |
There was a problem hiding this comment.
Typo* also might be good to put the extension point after the logic below that assembles the list
There was a problem hiding this comment.
lol, somewhere between 'update' and 'upgrade', ahaha
|
|
||
| /** | ||
| * Return a list of alerts to display in a message box above the report | ||
| * A combination of free text fields - combined alerts as opposed to a message box per alert. |
There was a problem hiding this comment.
Plz add a return phpdoc
There was a problem hiding this comment.
This doesn't auto-resolve because the line it's on hasn't changed.
But there is now a return php doc 2 lines down (after an empty one) in this file.
templates/Package_summary.ss
Outdated
| <% if $Description %><span class="package-summary__description">$Description.XML</span><% end_if %> | ||
|
|
||
| <% if $Badges %> | ||
| <% loop $Badges %> |
There was a problem hiding this comment.
We could just loop Badges right? At worst it'll be empty
There was a problem hiding this comment.
In theory, yes. There are issues in v3 with looping empty lists resulting in at least one pass of the internal HTML (with no variables filled in).
In at least... there were. Call it habit if has changed at some point.
There was a problem hiding this comment.
Can you reference a framework issue please if this is a valid issue
There was a problem hiding this comment.
Sweet, tested. Guess it was fixed at some point in the past. Call it habit, now no longer needed (thank goodness!)
templates/Package_summary.ss
Outdated
|
|
||
| <% if $Badges %> | ||
| <% loop $Badges %> | ||
| <span class="package-summary__badge<% if $Type %> package-summary__badge--$Type.ATT<% end_if %>">$Title</span> |
There was a problem hiding this comment.
May as well add $Title.XML, unless it's already escaped
css/sitesummary.css
Outdated
| .package-summary table.ss-gridfield-table tr td.col-Summary { | ||
| padding: 0; | ||
| } | ||
|
|
||
| a.package-summary__anchor { |
There was a problem hiding this comment.
Considering package-summary has been changed to site-summary above you should remain consistent.
There was a problem hiding this comment.
No, this is a separate selector. It summarises a package (as the block*), not the site (as the gridfield does) - the site level summary being the one I updated.
*Block as the B in BEM.
NightJar
force-pushed
the
pulls/1.0/support-security-alerts
branch
4 times, most recently
from
bfbca70 to
8ce9ca0
Compare
NightJar
force-pushed
the
pulls/1.0/support-security-alerts
branch
from
8ce9ca0 to
31861fc
Compare
src/Model/Package.php
Outdated
| */ | ||
| public function getBadges($badgeDefinitions = []) | ||
| { | ||
| $this->extend('updateBadges', $badgeDefinitions); |
There was a problem hiding this comment.
See previous comment - I think this should go after the foreach.
| if ($alerts) { | ||
| $summaryInfo = '<div class="site-summary message warning">' . implode("\n", $alerts) . '</div>'; | ||
| $fields->unshift(LiteralField::create('AlertSummary', $summaryInfo)); | ||
| } |
There was a problem hiding this comment.
If you were to put this into a beforeUpdateCMSFields callback then extensions could remove or adjust it if they wanted to. They won't be able to at the moment
There was a problem hiding this comment.
Is that a thing in 3?
There's a rather rudimentary extension hook in getAlerts that allows modification (that's how the security module adds its one).
There was a problem hiding this comment.
Is that a thing in 3?
Yep:
public function getCMSFields()
{
$this->beforeUpdateCMSFields(function (FieldList $fields) {
// ... modify field list here
});
return parent::getCMSFields();
}There was a problem hiding this comment.
Yeah nah, this is a SS_Report though, not a DataObject (the only place the function is defined) :>
Me test give us an error:
Exception: Object->__call(): the method 'beforeupdatecmsfields' does not exist on 'SiteSummary', or the method is not public.
Hooray for tests! :D
There was a problem hiding this comment.
Ah yep, you can still use beforeExtending callbacks if you want here but it’s ok :-D
In order to display information from `bringyourownideas/silverstripe-composer-security-updates` we need to be able to add various information to the summary, and badges which were previously 'not a thing'. This commit makes badges 'a thing', makes them extensible, and provides a rudimentary way to alter the summary information for the on scren report. Also added were namespaces for the GridFieldComponents, but this should affect nothing (should not affect anything).
NightJar
force-pushed
the
pulls/1.0/support-security-alerts
branch
from
31861fc to
3e724b2
Compare
Required for #39 - but does not close it. Dependency of bringyourownideas/silverstripe-composer-security-checker#34
Needs a rebase yet, as includes a pre-rebase of #65