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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

sebastianpiskorski
Copy link
Contributor

… to customize vizualization template. Refactored getLastVisitsStart() action to render report instead of using an API.

@sebastianpiskorski sebastianpiskorski force-pushed the bugfix_PPCDEV-379 branch 4 times, most recently from 16ef838 to d82bd19 Compare February 1, 2016 14:45
tsteur
tsteur reviewed Feb 1, 2016
View reviewed changes
core/Plugin/Visualization.php
* @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));
Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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

@tsteur
Copy link
Member

tsteur commented Feb 1, 2016

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?

tsteur
tsteur reviewed Feb 1, 2016
View reviewed changes
plugins/Live/Reports/GetLastVisitsDetailsWidget.php
@@ -0,0 +1,32 @@
<?php
/**
* Copyright (C) Piwik PRO - All rights reserved.
Copy link
Member

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

@sebastianpiskorski
Copy link
Contributor Author

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

@sebastianpiskorski sebastianpiskorski force-pushed the bugfix_PPCDEV-379 branch 3 times, most recently from 1733f97 to ac56f02 Compare February 2, 2016 22:30
@sebastianpiskorski sebastianpiskorski changed the title PPCDEV-379 added 'Visualization.customizeTemplate' event, that allows… 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. Feb 2, 2016
tsteur
tsteur reviewed Feb 2, 2016
View reviewed changes
...st_periodIsRange_dateIsLastN_MetadataAndNormalAPI__Live.getLastVisitsDetailsWidget_range.xml
@@ -0,0 +1,412 @@
<?xml version="1.0" encoding="utf-8" ?>
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@tsteur
Copy link
Member

tsteur commented Feb 2, 2016

PR looks good. Need to wait for test results

@tsteur
Copy link
Member

tsteur commented Feb 3, 2016

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

@sebastianpiskorski
PPCDEV-379 Added possibility to modify visualizations of VisitorLog w…
1ab7e73 
…idget/view and Visitors Real-time widget. Refactored getLastVisitsStart() action to render report instead of using an API.
@sebastianpiskorski
Copy link
Contributor Author

@tsteur I've added it, but test still fail.

@tsteur
Copy link
Member

tsteur commented Feb 3, 2016

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?

@sebastianpiskorski
Copy link
Contributor Author

I guess that would be fine too. So do you want me to split this into two commits?

@tsteur
Copy link
Member

tsteur commented Feb 4, 2016

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.

@tsteur
Copy link
Member

tsteur commented Feb 4, 2016

Let me know if you can't do it now and I will issue the PR

@tsteur tsteur mentioned this pull request Feb 4, 2016
Merged
@tsteur
Copy link
Member

tsteur commented Feb 4, 2016

Done here #9693

@tsteur tsteur added this to the 2.16.1 milestone Feb 4, 2016
@sebastianpiskorski
Copy link
Contributor Author

Thank you, I had to go to sleep yesterday.

@mattab
Copy link
Member

mattab commented Feb 6, 2016

Closing as necessary changes should be in 2.16.0 (#9693)

@mattab mattab closed this Feb 6, 2016
@mattab mattab added the duplicate For issues that already existed in our issue tracker and were reported previously. label Feb 6, 2016
@mattab mattab modified the milestones: 2.16.x, 2.16.1 Feb 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
duplicate For issues that already existed in our issue tracker and were reported previously.
Projects
None yet
Milestone
2.16.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sebastianpiskorski @tsteur @mattab