Do not use idGoal param for reports with goal metrics #12195
Conversation
sgiehl
added
Needs Review
plugins/API/RowEvolution.php
Outdated
| @@ -317,6 +317,13 @@ private function getRowEvolutionMetaData($idSite, $period, $date, $apiModule, $a | |||
| $reportMetadata = API::getInstance()->getMetadata($idSite, $apiModule, $apiAction, $apiParameters, $language, | |||
| $period, $date, $hideMetricsDoc = false, $showSubtableReports = true); | |||
|
|
|||
| // ignore idGoal if no report can be found with it | |||
| if (empty($reportMetadata) && isset($apiParameters['idGoal'])) { | |||
There was a problem hiding this comment.
if we go with this "hack", be good to leave a comment about it for what it fixes. Wonder if it could lead to some "wrong" results? Probably not?
Did you have a chance to think about what a "proper" fix might look like? Can't think of anything right now apart from having another report instance of all the regular reports with a "idGoal" parameter but this might have other downsides? Like in Reports.addReports event we could maybe add for all "Goal" reports another report instance and set idGoal parameter? Might be just as well a different hack thought and bit more complicated but might enable then other features as well. Really just a random thought and not even sure if it is possible.
There was a problem hiding this comment.
Did you have a chance to think about what a "proper" fix might look like?
No, but I don't think adding additional reports to the reports list would be good. Maybe instead we shouldn't pass a idGoal to the rowevolution in this case, but another param like showGoalMetricsFor or similar to be able to show other metrics within the same report?
There was a problem hiding this comment.
Passing idGoal if needed in rowevolution would be a really good solution I reckon, did not think of that. If it is easy possible 👍
There was a problem hiding this comment.
Will have a look as soon as I have some time. Maybe that would a better solution than going with the changes provided in this PR
sgiehl
added
the
Pull Request WIP
sgiehl
force-pushed
the
rowevolutiongoals
branch
from
0cc4731
to
2fa971b
Compare
sgiehl
changed the title
sgiehl
force-pushed
the
rowevolutiongoals
branch
from
2fa971b
to
25dd3cd
Compare
|
I've now updated the implementation. Instead of ignoring possible incorrect idGoal param, I have now renamed the param used to trigger displaying correct goal metrics in goal visualization from |
sgiehl
removed
the
Pull Request WIP
|
I wonder if previous implementation was maybe better as it could break some things. For example when such a report is rendered, in https://github.com/piwik/piwik/pull/12356/files#diff-c337b15b90f57588992ba972d3a21e08R148 I added a check based on the |
|
I'll issue a new PR, that will handle that in rowevolution js. Maybe that's the simplest solution right now. |
mattab
modified the milestones:
3.5.0,
Priority Backlog (Help wanted),
Backlog (Help wanted)
This is kind of a "quick" fix for #11526. But Row Evolution will show the same as when clicking on it in a non "Goal" report. Alternatively we could also simply deactivate Row Evolution for those Goals reports.
Note: I guess it would be awesome to be able to select the goal metrics for row evolution in this case, but that needs a bit more work.