@sgiehl opened this issue on April 3rd 2016

Tried to fix #9697. I don't think that it is the best solution, but I guess it shouldn't have any side effects this way.

Note regarding: https://github.com/piwik/piwik/issues/9697#issuecomment-179637988 Removing lines 159-161 completely would fix this issue aswell, but that might create problems for other goal reports where the idGoal is part of the metadata

fixes #9697

@tsteur commented on April 3rd 2016

With the steps you mentioned I can reproduce it as well. I'm not sure if it's the best fix though as it might find a report when an idGoal is given when it should not find one etc. It might be a not so good workaround as well but maybe

if (!empty($request['idGoal']) && in_array($this->apiModule, array('Goals', 'Ecommerce'))) {

This would be needed everywhere though so we would need to rather handle this in ProcessedReport::getMetadata() and unset idGoal when needed like

if (!empty($apiParameters['idGoal']) && in_array($apiModule, array('Goals', 'Ecommerce'))) {
unset($apiParameters['idGoal']);

I bet this has side effects though as well and this workaround would be kind of against the Piwik platform thought as other plugins might use idGoal as well.

If we always had the viewDataTable=tableGoals there (I think this is the case as it's hardcoded in https://github.com/piwik/piwik/blob/2.16.1-rc1/plugins/Goals/Controller.php#L457) we could do something like:

if (!empty($request['idGoal']) && $viewDataTable !== 'tableGoals') {

The problem is that in this case the idGoal is not really meant for the report but for the tableGoals visualization.

Another fix could be to not use idGoal parameter name for tableGoals visualization but eg idGoalView or similar somewhere here: https://github.com/piwik/piwik/blob/2.16.1-rc1/plugins/Goals/Controller.php#L461 and then use this parameter name here: https://github.com/piwik/piwik/blob/2.16.1-rc1/plugins/Goals/Visualizations/Goals.php#L61

Not sure about side effects there though. I think I would prefer not to fix it in DataTableManipulator as this logic seems right as it is implemented (more or less) and could introduce more side effects. I'd probably prefer a fix in Goals controller/visualization or by making it dependent on visualization (I haven't actually tested if it breaks something else). In general I think the problem here is the visualization and not the DataTableManipulator.

So many things to fix this but there's not really a perfect solution for this I presume

@sgiehl commented on April 4th 2016

Had another thought on that. But I guess you're right:

So many things to fix this but there's not really a perfect solution for this I presume

Using tableGoals won't help in this case I think, as it would still be possible in that view to switch to another visualization. Haven't tested that yet, but I guess it might throw the same error then.

Using the apiModule to check if idGoal should be used is as you mentioned kind of against the Piwik platform. But I presume for now maybe the easiest and best "quick" fix. But we should let the issue open and maybe try to find a better solution for Piwik 3.x.

The only other "fix" that came to my mind would be to disable the flattening feature for those reports displayed for specific goals. But imho that's not better aswell and I'm not sure if the error might occur in other cases, aswell.

What do you think?

@tsteur commented on April 6th 2016

Using tableGoals won't help in this case I think, as it would still be possible in that view to switch to another visualization. Haven't tested that yet, but I guess it might throw the same error then.

I think the other visualizations do not need the idGoal and just show the plain report. Actually, I would even consider it a bug that one can switch to other visualisations there as they don't show the information goal related. Maybe it's not a bug but the UI could be simpler there. Even the export links are broken I believe:

{"result":"error","message":"Invalid dimension 'null'."}
@tsteur commented on April 6th 2016

I reckon we could merge this fix as suggested and then work on a fix around the visualization TableGoals I would say

@sgiehl commented on April 6th 2016

@tsteur Ok. Changed it to use apiModule to check if idGoal should be used or not.

@tsteur commented on April 6th 2016

Sorry, I meant we can merge as it is without checking for API Module. By checking for module I'm not sure if it eg breaks Funnel plugin etc.

@sgiehl commented on April 6th 2016

Ah ok. Reverted my last rebase. Should be as before now.

This issue was closed on April 7th 2016
Powered by GitHub Issue Mirror