@mnapoli opened this issue on April 8th 2015

Addresses #7235 and #6726

I have refactored the system check: each check goes into a separate class, thus breaking the big (500 lines long) SystemCheck class + big (400 lines) Twig template into small classes (the twig template is now dead simple).

It allows plugins to provide their own Diagnostics, as they can be configured through the container (this is a good illustration of the possibilities of the DI config, if there are issues with this we could consider an alternative solution). To give you an idea, here is how you create a new diagnostic: - implement the Diagnostic interface - add it to the diagnostics.mandatory or diagnostics.optional list, for example:

php 'diagnostics.optional' => DI\add(array( DI\link('Piwik\Plugins\CoreUpdater\Diagnostic\HttpsUpdateCheck'), )),

As such I have moved a few diagnostics to the plugin they belong to (e.g. "update over HTTPS" check is in the CoreUpdater now, as shown in the example above).

Given the logic is no longer split between the controller + view + SystemCheck class, it was very easy to implement a console command: diagnostic:run that runs the system check in the command line.

$ ./console diagnostic:run        # will show only the warnings and errors
$ ./console diagnostic:run --all  # will show all diagnostics

I have also discussed with Michal and that command will replace the existing enterpriseadmin:systemcheck (BC kept with an alias) which was kind of hacking the current implementation to allow it to run in CLI. I have a pull request ready for EnterpriseAdmin.

@mattab commented on April 9th 2015

(I didn't look at the code but all of Piwik developers have thought one day that a new solution was needed and this is a good example of removing technical debt with high value!)

@diosmosis commented on April 14th 2015

FYI, the SystemCheck UI test appears to have some small styling differences (font stuff + an italics change) & regression (Filesystem check isn't Ok). See http://builds-artifacts.piwik.org/ui-tests.diagnostics/11706.7/screenshot-diffs/diffviewer.html

@mnapoli commented on April 14th 2015

Yep it's expected. Each line in the table had different styling (e.g. some stuff was in italic, some wasn't) or inconsistent output (e.g. "Filesystem check isn't Ok" is actually OK, it was just redundantly showing "OK" next to the green check, which was inconsistent with all the other checks).

So all is good I have just made things consistent, if we want to improve the design to make it look better let's do it in a separate pull request.

@diosmosis commented on April 14th 2015

Ok, I get the Filesystem change is expected. I think the styling changes are ok, the italics, eg, don't seem to add anything. @mattab can you confirm the styling changes to the system check page here: http://builds-artifacts.piwik.org/ui-tests.diagnostics/11706.7/screenshot-diffs/diffviewer.html are not important?

@diosmosis commented on April 14th 2015

Follow up issue for after this is merged: In QueuedTracking use diagnostics API instead of the existing SystemCheck class

@mattab commented on April 14th 2015

can you confirm the styling changes to the system check page here: http://builds-artifacts.piwik.org/ui-tests.diagnostics/11706.7/screenshot-diffs/diffviewer.html are not important?

Feedback: - Installation_system_check -> the font used in the list of "Directories with write access" looks different in the processed - maybe the font family was lost? - UIIntegrationTest_admin_plugins -> add a description to the plugin - (UIIntegrationTest_ecommerce_log is a random failure (#6693))

@mnapoli commented on April 14th 2015

Installation_system_check -> the font used in the list of "Directories with write access" looks different in the processed - maybe the font family was lost?

That's a good thing right? Currently the directory list has a different font than the rest, with the PR it becomes consistent.

UIIntegrationTest_admin_plugins -> add a description to the plugin

Done.

This issue was closed on April 14th 2015
Powered by GitHub Issue Mirror