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

[Piwik.js] - Enable bulk tracking #8699

Closed
wants to merge 2 commits into from
Closed

[Piwik.js] - Enable bulk tracking #8699

wants to merge 2 commits into from

Conversation

dhoko
Copy link

@dhoko dhoko commented Sep 2, 2015

cf #7949

tracker.activeBulkTracking(true);
tracker.setBulkTrackingDelay(5000);

As soon as we have an event/etc., a list is created with this request. Every 300ms (default) we send dat list to piwik.

@dhoko dhoko mentioned this pull request Sep 2, 2015
Closed
@dhoko dhoko changed the title Enable bulk tracking [Piwik.js] - Enable bulk tracking Sep 2, 2015
@dhoko
Copy link
Author

dhoko commented Sep 2, 2015

Testing with my app, it works. I will try to add some tests.

@pat841
Copy link

pat841 commented Nov 6, 2015

Seems to work with my testing too.

tsteur
tsteur reviewed Nov 8, 2015
View reviewed changes
js/piwik.js
* Send each bulk tracks to the server
* @return {void}
*/
function sendBulkTrackingRequests() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if the method sendBulkRequest() can be used here? Or maybe the name could be bit different like sendBulkTrackingCollection() and inside this method call sendBulkRequest(bulkTrackingCollection)?

@tsteur
Copy link
Member

tsteur commented Nov 8, 2015

Sorry that it took me so long to get back to it. Would be a nice improvement to have! I left a few comments. Ideally there would be also some tests in https://github.com/piwik/piwik/tree/master/tests/javascript for this but I must admit I'm not even sure how I would test it myself as it's surely not trivial. Probably it would need a new section

test("tracking", function() {
  expect(NumberOfExpectedTests);

  //enable bulk tracking
  // send a few tracking requests
  // check whether they were sent together similar to https://github.com/piwik/piwik/blob/2.15.0/tests/javascript/index.php#L3019
}

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jan 12, 2016
@mattab
Copy link
Member

mattab commented Feb 1, 2016

Hi @dhoko if you'd like this this PR merged in Piwik 3.0.0 please reply to @tsteur feedback above. it would be very useful feature to have bulk tracking in piwik.js!

@mattab mattab added this to the 3.0.0 milestone Feb 1, 2016
@dhoko
Copy link
Author

dhoko commented Feb 2, 2016

I wanted to upgrade this PR, but since the beginning I have this #9492 It's hard to work with it :/

@tsteur tsteur changed the base branch from master to 2.x-dev September 1, 2016 06:40
@mattab
Copy link
Member

mattab commented Sep 23, 2016

Hello @dhoko - do you still experience the issue in #9492 ? if so, please let me know and i'll try to help you find a solution, as i'm sure it's a problem for many people and not just you. It would be great if you would finish your nice Pull requests for 3.0.0-b2 release! 👍

@mattab mattab modified the milestones: 3.0.0-b2, 3.0.0 Sep 23, 2016
@mattab
Copy link
Member

mattab commented Sep 23, 2016

Moving tentatively to 3.0.0-b2 milestone. If we don't merge @dhoko PRs in this release, we'll close the PR for now. Hopefully it will happen, as these PRs look very useful 👍

@mattab
Copy link
Member

mattab commented Oct 30, 2016

Haven't heard from @dhoko so closing this PR for now

@mattab mattab closed this Oct 30, 2016
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
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Milestone
3.0.0-b2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dhoko @pat841 @tsteur @mattab