@diosmosis opened this Pull Request on February 7th 2019 Member

Added new ViewDataTable property that allows reports to add a suffix to the segment used in the segmented visitor log.

Note: I think for row evolution users we should just include the other metrics, like nb_orders, etc. Might be strange to see Visits = 50 in the table, then Visits = 5 in the row evolution popup.

Also I noticed a strange discrepancy between the segmented visitor log and the ecommerce orders metrics. They don't seem to match up for me locally (w/ VisitorGenerator test data).

Fixes #10989

@diosmosis commented on February 9th 2019 Member

I'm not sure if a UI test would be a good idea here, we've already got a lot, and one for this wouldn't be testing very much... A unit test for datatable => segmented visitor log row action would be good, perhaps that can be added later when moving to Angular.

@diosmosis commented on February 11th 2019 Member

@tsteur can you review https://github.com/matomo-org/matomo/pull/14079/commits/b9dcc04cfb32bbd89ec64b1506ecda652bcbc551 quickly? It's to remove characters like == (in, eg, browserCode==ff) from the unique ID which I think is used as-is in id parameters in HTML.

@tsteur commented on February 11th 2019 Member

@diosmosis I guess that makes sense (as long as it isn't double encoded or so) not sure though in general... are there any tests failing? it could break dashboards etc but not sure where we would use such characters... is there a chance maybe to ignore segmented_visitor_log_segment_suffix in unique id? I reckon if we did, we would maybe have some duplicated IDs? I suppose if it doesn't break anything, should be fine.

@diosmosis commented on February 11th 2019 Member

I reckon if we did, we would maybe have some duplicated IDs?

Not currently since it's only used in one place, but conceivably in the future.

it could break dashboards etc but not sure where we would use such characters...

Maybe if a segment is set, will check...

@diosmosis commented on February 11th 2019 Member

Seems to only be an issue if a report/widget sets a parameter value w/ a non-alphanumeric letter. Looking through our uses, don't see anything.

@diosmosis commented on February 11th 2019 Member

I just thought of something, since the parameter is added to the ecommerce log, I guess the dashboard will break for users that use the ecommerce log? Will try to add an update...

@tsteur commented on February 11th 2019 Member

In this case we should ideally add the fixed ecommerce segment query server side if possible. Not sure if easily doable though. if we can workaround otherwise and keep original ID that be good.

@diosmosis commented on February 12th 2019 Member

Thinking about this incorrectly, it only affects the child widgets of the ecommerce sales bydimension widget container. I think we're fine on that point.

@tsteur commented on February 12th 2019 Member

👍

This Pull Request was closed on February 12th 2019
Powered by GitHub Issue Mirror