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
PPCDEV-379 Added possibility to modify visualizations of VisitorLog widget/view and Visitors Real-time widget. Refactored getLastVisitsStart() action to render report instead of using an API. #9672
PPCDEV-379 Added possibility to modify visualizations of VisitorLog widget/view and Visitors Real-time widget. Refactored getLastVisitsStart() action to render report instead of using an API. #9672
Conversation
16ef838
to
d82bd19
Compare
* @param array &$templateFile The original template file, the overriding one should extend the original one | ||
* @param array &$templateFile Template variables, you can add new or modify the ones already present | ||
*/ | ||
Piwik::postEvent('Visualization.customizeTemplate', array(&$templateFile, &$templateVars)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would make our template file names API. Instead it would be better to use Visualization ID. Also it would make the template vars API which is not ideal. Can we instead post an instance of the Visualization? Otherwise we make any template variable an API and won't be able to make any changes anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should actually completely remove this part and instead use specific events in the template files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do so, but wouldn't this be useful to change Visualization or at least extend it, inject additional vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is, it would be come API and we wouldn't be able to change it anymore.
Also a problem is that we do not really have a way yet, to document existing template variables etc
I'm a bit confused by the description of the PR since it also adds a widget etc. and not only posts an event. What is the widget supposed to do? |
@@ -0,0 +1,32 @@ | |||
<?php | |||
/** | |||
* Copyright (C) Piwik PRO - All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header should be Piwik.org everywhere
This PR doesn't add any widget. It just changes existing one. The way this widget is rendered. With this PR it is rendered as a Report. Changes that are not in description of PR, are in ticket PPCDEV-379 |
1733f97
to
ac56f02
Compare
ac56f02
to
ad75dd3
Compare
@@ -0,0 +1,412 @@ | |||
<?xml version="1.0" encoding="utf-8" ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess those files are not needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, will see at the test outcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were right we don't need them any more.
PR looks good. Need to wait for test results |
ad75dd3
to
a3387be
Compare
FYI: UI tests succeeded apart from 2 failing tests: http://builds-artifacts.piwik.org/piwik/piwik/master/17920 I don't know if it's a random build failure or due to the new visualization addition (I expect it is because of adding it). The tests may be fixed by adding public static function canDisplayViewDataTable(ViewDataTable $view)
{
return ($view->requestConfig->getApiModuleToRequest() === 'Live');
} to the visualization |
…idget/view and Visitors Real-time widget. Refactored getLastVisitsStart() action to render report instead of using an API.
a3387be
to
1ab7e73
Compare
@tsteur I've added it, but test still fail. |
Quick question, in 2.16.0, would it be enough to only have the events in the templates and not the widget changes? I guess for this feature the visualization is not really needed for now and we could have it in 2.16.1? |
I guess that would be fine too. So do you want me to split this into two commits? |
Do you mind preparing quickly another PR that only contains the postEvents in the template? We would merge immediately and release 2.16.0. We can merge this one then for 2.16.1. We need to release 2.16.0 soon and problem is this change was not included in any beta yet. So we want to limit possible regressions. |
Let me know if you can't do it now and I will issue the PR |
Done here #9693 |
Thank you, I had to go to sleep yesterday. |
Closing as necessary changes should be in 2.16.0 (#9693) |
… to customize vizualization template. Refactored getLastVisitsStart() action to render report instead of using an API.