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
Define four segments that search over all custom variable name/value columns instead of one per column #8754
Conversation
It is still possible to apply a segment for a specific one right? Can we maybe show a title in the UI that says that we're search in all custom variables for one segment and that results can be inaccurate if the search matches values from different custom variables? If I understand this feature right one has to be very careful using it, maybe even allowing only |
// Select also flattened keys (custom variables "page" scope, page URLs for one visit, page titles for one visit) | ||
$valuesBis = $table->getColumnsStartingWith($segmentName . ColumnDelete::APPEND_TO_COLUMN_NAME_TO_KEEP); | ||
$values = array_merge($values, $valuesBis); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This long method needs a refactoring one day :) not related to your PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed many methods needing refactoring :) I even added a TODO for the first one. Then I saw the rest and lost hope :)
I don't understand everything but looks ok to me :) Are there any changes re performance or so? Of course using the CustomVariableName/Page segment will be slower and I think the changes in AutoSuggestAPI also only affect this segment right? |
Yes, still possible. The other ones are added through an event.
This should only be a problem if doing a segment like
There shouldn't be, I tried to make the old behavior the default in every change. I was most worried about accidental BC breaks.
That's the idea. My changes should always use the fastest code path. The changes to AutoSuggestAPI will partially affect any segment that uses |
@mattab This is waiting for review and/or merge. |
@@ -279,6 +279,12 @@ | |||
<row> | |||
<type>dimension</type> | |||
<category>Custom Variables</category> | |||
<name>Custom Variable name</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be Custom Variable name (scope visit)
Good to merge after fixing the Segment names! LGTM |
…n instead of Dimension so we can define segments in them.
…n be matched in any of those fields, and use to define two new segments: customVariableName & customVariableValue.
…ested values by allowing the suggestedValuesCallback to accept Live.getLastVisitDetails data.
…s & the database.
…dle methods and free functions.
ea7abdf
to
ca6b7e7
Compare
Define four segments that search over all custom variable name/value columns instead of one per column
This PR creates four new segments: customVariableName, customVariableValue, customVariablePageName, customVariablePageValue. Each one searches over every custom variable slot so it is not necessary to manually string the different custom variable searches together (eg,
customVariableName1=...,customVariableName2=...,customVariableName3=...
).To accomplish this, I modified the
Segment::setSqlSegment
field so it can be an array (though this isn't documented). If found,Piwik\Segment\SegmentExpression
will automatically insertOR
s between conditions matching the value on each specified table column.To get the auto suggest API to work, I had to allow the suggestedValuesCallback property Segments to be non-strings and to allow them to accept a DataTable argument. If it does accept such an argument, the Live.getLastVisitDetails data (collected in API::getSuggestedValuesForSegment) is passed to the method.
Also includes a change of base classes for CustomVariableName/CustomVariableValue dimensions so they will be included in the
Dimension::getAllDimensions()
result. Not sure if this will cause side effects.The build may not pass yet.
Fixes #6031