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

Fix metadata problem for goal reports #9993

Merged
merged 1 commit into from Apr 7, 2016
Merged

Fix metadata problem for goal reports #9993

merged 1 commit into from Apr 7, 2016

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Apr 3, 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: #9697 (comment)
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

@sgiehl sgiehl added the Needs Review PRs that need a code review label Apr 3, 2016
@sgiehl sgiehl added this to the 2.16.1 milestone Apr 3, 2016
@tsteur
Copy link
Member

tsteur commented Apr 3, 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
Copy link
Member Author

sgiehl commented Apr 4, 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
Copy link
Member

tsteur commented Apr 6, 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
Copy link
Member

tsteur commented Apr 6, 2016

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

@sgiehl
Copy link
Member Author

sgiehl commented Apr 6, 2016

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

@tsteur
Copy link
Member

tsteur commented Apr 6, 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
Copy link
Member Author

sgiehl commented Apr 6, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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