Adds segmented log feature to more reports #13939
Conversation
sgiehl
added
the
Pull Request WIP
sgiehl
added
not-in-changelog
| $msg = 'AddSegmentByRangeLabel is called without having any segment defined'; | ||
| Development::error($msg); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Would it be better to do this in the constructor? Or is there a benefit to doing it here?
There was a problem hiding this comment.
Also is this something that could happen in production? If so maybe the log should appear all the time instead of just in development mode.
There was a problem hiding this comment.
Didn't have too much thoughts on that. Actually it's the same as in https://github.com/matomo-org/matomo/blob/3.x-dev/core/DataTable/Filter/AddSegmentByLabel.php
The error should not happen in production, as it should be fixed while development if it ever occurs.
| if ($url) { | ||
| $row->setMetadata('segmentValue', urldecode($url)); | ||
| } else if ($this->isPageTitleType && $label != -1) { | ||
| $row->setMetadata('segmentValue', trim(urldecode($label))); |
There was a problem hiding this comment.
Is the -1 for ID_SUMMARY_ROW, maybe it should also check for the new totals row? Also not sure if it's really necessary, but should we check if $label is not false?
There was a problem hiding this comment.
The check is required. Otherwise the summary row will have a segmented log feature, which can't work. Will add that for totals row as well....
There was a problem hiding this comment.
Ok. No need to check for totals row, as that row will be added afterwards. Summary row might exist before.
|
This doesn't seem to work in the "Visits by Days Since Last Visit" report. Clicking on the segment for "0 days" since last visit selects all visits. There's also no segment icon for the "New Visits" row. |
|
"Page Titles Following a Site Search"/"Pages Following a Site Search"/"Search Categories" don't have segment icons. Not sure if they're supposed to be added in this PR. |
This should be fixed now.
The segment should be set correctly. Maybe the segment isn't working correctly?
Are there any segments we could use for this? |
Doesn't look like it, guess they'd have to be new if adding them. |
|
wasn't mentioned in #7212, so wouldn't include it here |
|
Tested locally, works for me. |
fixes #7212