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

Jump to bottom

Add idSegments as SQL comment in segment archiving SQL #8809

Merged
merged 12 commits into from Sep 28, 2015
Merged

Add idSegments as SQL comment in segment archiving SQL #8809

merged 12 commits into from Sep 28, 2015

Conversation

diosmosis
Copy link
Member

This PR adds a comment like /* idSegments = [1, 2, 3] */ to all segment archiving SQL, if the requested segment matches the definition of a stored segment.

The addition is made non-intrusively by decorating LogQueryBuilder, instead of referencing SegmentEditor in core. This change should function, even if the SegmentEditor plugin is disabled.

Fixes #8752

Additional Changes

  • LogQueryBuilder was turned into a stateless service class and put in DI.
  • SegmentEditor\Model was put in DI.
  • SegmentEditor\Model::getSegmentsToAutoArchive is exposed in an API method and modified so we can query for all sites. The result is stored in the transient cache.

TODO

  • Add unit test for SegmentQueryDecorator (using mocks).
  • Add integration test for SegmentQueryDecorator (which adds stored segments to the database).

@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. 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 Sep 17, 2015
@diosmosis diosmosis added this to the 2.15.0 milestone Sep 17, 2015
@diosmosis diosmosis removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 17, 2015
tsteur
tsteur reviewed Sep 21, 2015
View reviewed changes
core/Segment/SegmentExpression.php
@@ -49,6 +49,11 @@ public function __construct($string)
$this->tree = $this->parseTree();
}

public function getSegmentString()
Copy link
Member

Choose a reason for hiding this comment

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

Could there be maybe a better name instead of ...String? It's not important is it's not API though so can be ignored. Also not sure what to use instead. getSegmentExpression could be misleading possibly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change it to getSegmentDefinition().

@tsteur
Copy link
Member

tsteur commented Sep 21, 2015

Left a few comments maybe you can have a look if something needs to be done or not

@mattab
Copy link
Member

mattab commented Sep 22, 2015

Noticed there's an integration test failing: https://travis-ci.org/piwik/piwik/jobs/81498450#L703
The UI tests failures are un-related to this PR/can be ignored

Do not use doAsSuperUser in order to use API in SegmentQueryDecorator…
055481d 
…. Instead use new service-layer class that is not accessible from API and does not do access checking.
@diosmosis
Copy link
Member Author

Ready for review and/or merge.

mattab
mattab reviewed Sep 24, 2015
View reviewed changes
plugins/SegmentEditor/tests/Integration/SegmentQueryDecoratorTest.php
$segmentEditorApi->add('segment 3', 'visitCount<2', 1, $autoArchive = true);
$segmentEditorApi->add('segment 4', 'visitCount<2', 2, $autoArchive = true);

// test that segments w/ auto archive == false aren't included
Copy link
Member

Choose a reason for hiding this comment

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

segments w/ auto archive === false should also be included, since it's useful to know those segment IDs...

optional: display trigger = archive in the comment when core:archive triggered the query

To provide better diagnostics to sysadmins, decorate segment SQL w/ a…
a0e37cb 
…ny matching stored segment ID, even if not set to auto-archive and also decorate w/ special message if the SQL was triggered by core:archive.
@diosmosis
Copy link
Member Author

@mattab Ready for another review.

mattab pushed a commit that referenced this pull request Sep 28, 2015
Merge pull request #8809 from piwik/8752_segment_id_in_sql
a581017 
Add idSegments as SQL comment in segment archiving SQL
@mattab mattab merged commit a581017 into master Sep 28, 2015
@mattab
Copy link
Member

mattab commented Sep 28, 2015

👍

@mattab mattab deleted the 8752_segment_id_in_sql branch October 2, 2015 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
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
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@diosmosis @tsteur @mattab