Skip to content
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

Group different writings of Instagram in reports #14436

Merged
merged 3 commits into from May 12, 2019
Merged

Group different writings of Instagram in reports #14436

merged 3 commits into from May 12, 2019

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented May 9, 2019

fixes #14433

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels May 9, 2019
@sgiehl sgiehl added this to the 3.10.0 milestone May 9, 2019
@@ -365,6 +365,13 @@ public function getSocials($idSite, $period, $date, $segment = false, $expanded

$dataTable = Archive::createDataTableFromArchive(Archiver::SOCIAL_NETWORKS_RECORD_NAME, $idSite, $period, $date, $segment, $expanded, $flat);

$dataTable->filter('GroupBy', ['label', function($value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do this only if one or the other label exists? Typically the GroupBy filter is quite slow, and checking if a label exists in the report is really fast usually.

Not too important but could be worth thinking about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the branch

@@ -365,6 +365,35 @@ public function getSocials($idSite, $period, $date, $segment = false, $expanded

$dataTable = Archive::createDataTableFromArchive(Archiver::SOCIAL_NETWORKS_RECORD_NAME, $idSite, $period, $date, $segment, $expanded, $flat);

$groupIntagramLabel = function($dataTable) use (&$groupIntagramLabel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgiehl might be better to create a filter for this? would remove some complexity and be better to read etc. like $dataTable->filter('Piwik\Plugins\Referrers\DataTable\Filter\Group') then you don't need to check if it's a map etc and in theory it would be even possible to write a unit test for the group filter but wouldn't really be needed.

@tsteur tsteur merged commit 7b56ad8 into 3.x-dev May 12, 2019
@tsteur tsteur deleted the groupinsta branch May 12, 2019 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Social networks report shows instagram twice
2 participants