Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored the system check to allow plugins to add new diagnostics #7646

Merged
merged 10 commits into from Apr 14, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Apr 8, 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:

    '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

capture d ecran 2015-04-09 a 11 05 14

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.

@mnapoli mnapoli added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review labels Apr 8, 2015
@mnapoli mnapoli added this to the Piwik 2.13.0 milestone Apr 8, 2015
@mattab
Copy link
Member

mattab commented Apr 9, 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!)


class Updates_2_13_0_b2 extends Updates
{
static function update()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the non-static Updates::doUpdate method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

$this->assertEquals(DiagnosticResult::STATUS_ERROR, $results[0]->getStatus());
$this->assertEquals('Warning', $results[1]->getLabel());
$this->assertEquals(DiagnosticResult::STATUS_WARNING, $results[1]->getStatus());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to also add an Ok diagnostic to the check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@diosmosis
Copy link
Member

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
Copy link
Contributor Author

mnapoli commented Apr 14, 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
Copy link
Member

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
Copy link
Member

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

@mattab
Copy link
Member

mattab commented Apr 14, 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 (Some UI screenshot tests fail at random  #6693))

@mnapoli
Copy link
Contributor Author

mnapoli commented Apr 14, 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.

diosmosis added a commit that referenced this pull request Apr 14, 2015
Refactored the SystemCheck class into new Diagnostics API that allows plugins to add new checks via dependency injection config. The new API is located in the new Diagnostics plugin and plugin specific diagnostics are included in their respective plugins. Also includes new CLI command to run diagnostics.
@diosmosis diosmosis merged commit ccb001d into master Apr 14, 2015
@diosmosis diosmosis deleted the diagnostics branch April 14, 2015 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants