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

Fix inconsistent usage of segment idSites #15746

Merged
merged 1 commit into from Apr 1, 2020
Merged

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Mar 29, 2020

In all other cases that method Rules::shouldProcessReportsAllPlugins is called with the idSite we are currently archiving vs in these cases it was potentially called with children idSites (when roll up reporting is used with a specific setting).

Problem?

  • Roll up reporting idSite 3 has a segment defined. The roll up has 2 children sites 1 and 2.
  • There is a segment configured to be auto archived for idSite 3 (the roll up). The same segment is not configured in idSite 1 and 2.
  • When then the segment for idSite was about to be archived, it checked whether the segment is configured to be prearchived in Rules::isSegmentPreProcessed() -> Rules::getSegmentsToProcess()
  • Before it was calling Rules::isSegmentPreProcessed(1,2) but then it thinks the segment for idSite 3 is not prearchived.
  • If neither idSite 1 or 2 had the very same segment defined, it would only archive visits summary details but no other report for the roll up.

I've done some extensive testing and it seems this change alone fixes the issue and no change in roll up reporting is needed. I can only assume the reason this was never noticed is:

  • View people have segment browser archiving disabled enforced
  • Often the segment might still be configured for all websites in which case this issue is not happening
  • The same segment exists for another site as well

@tsteur tsteur added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Mar 29, 2020
@tsteur tsteur added this to the 3.13.5 milestone Mar 29, 2020
@tsteur tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Mar 30, 2020
@mattab
Copy link
Member

mattab commented Mar 30, 2020

Great find!
Question: There is another call to shouldProcessReportsAllPlugins in getArchiveGroupOfPlugin, does this one need to be changed as well or not?

@tsteur
Copy link
Member Author

tsteur commented Mar 30, 2020

No, it uses a totally different parameter class Piwik\Archive\Parameters vs Piwik\ArchiveProcessor\Parameters

@diosmosis diosmosis merged commit e89ebdf into 3.x-dev Apr 1, 2020
@diosmosis diosmosis deleted the segmentconsarch branch April 1, 2020 10:32
jonasgrilleres pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 22, 2020
jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants