@sgiehl opened this Pull Request on January 31st 2019 Member

In some cases it might be useful to join tables to visitor data using other columns than idVisit and idAction.

There is already a method LogTable::getWaysToJoinToOtherLogTables, which makes it possible to define ways to join the table other than using idVisit and idAction.
But currently that method is only used in PrivacyManager in order to remove related data.

This change should also make it possible to use tables joined by getWaysToJoinToOtherLogTables as dimensions and in segmentation.

@tsteur commented on February 3rd 2019 Member

Those PRs are always tricky :) We would need to run all system tests of all premium features and for custom reports on top the integration tests to make sure everything will still work fine. (I suppose it will as they mainly define arrays in the archiving anyway so this logic should be ignored). Can you run them or should I?

@sgiehl commented on February 4th 2019 Member

@tsteur Updated the branch. Regarding running the tests. Would be nice if you could run them as well. Would need to set up a new vm to do that properly, as my current setup uses php 5.5. Some of the plugins have failing system tests due to sorting issues...

@sgiehl commented on February 9th 2019 Member

@tsteur I've now implemented the automatic join up the hierarchy if it's joinable with log_visit or log_action. To prove that works as expected I've added a new plugin ExampleLogTables which includes two custom log tables. One that joins to log_visit using the user_id column, and another log table that joins the other custom table using another custom column. Also there is a custom dimension/segment for both log tables.
The plugin includes various system tests to check that all APIs/archiving still work when using a segment of one of the custom log tables.

@sgiehl commented on February 28th 2019 Member

@tsteur do you have some time for another review? Would be awesome if we could get that merged for 3.9, as I need that for a plugin

@tsteur commented on March 10th 2019 Member

Added one more comment otherwise looks fine if all the tests pass.

I reckon there might be edge cases where it doesn't work depending on various combinations of joins for example or when multiple log tables are defined as a way to join etc. But those are not too important right now as we're not using it and can fix the issues as usual once we run into issues.

@sgiehl commented on March 11th 2019 Member

test failures are unrelated to this PR, so will merge it now.

This Pull Request was closed on March 11th 2019
Powered by GitHub Issue Mirror