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] - Create a callback for error or success #8782

Closed
wants to merge 1 commit into from
Closed

[Piwik.js] - Create a callback for error or success #8782

wants to merge 1 commit into from

Conversation

dhoko
Copy link

@dhoko dhoko commented Sep 14, 2015

Hi,

We cannot add a callback to detect both error and success for GET and POST.
Now, with trackEvent we can set an object as the fifth argument to set callbacks.

tracker
  .trackEvent('test',null,null,null,{
    success: function(isXhr) {
      console.log('Success', isXhr)
    },
    error: function(isXhr) {
      console.log('Error', isXhr)
    }
  });

cf PR #7946

@dhoko dhoko changed the title [piwik.js] - Create a callback for error or success [Piwik.js] - Create a callback for error or success Sep 14, 2015
tsteur
tsteur reviewed Sep 21, 2015
View reviewed changes
js/piwik.js

/*
* Send image request to Piwik server using GET.
* The infamous web bug (or beacon) is a transparent, single pixel (1x1) image
*/
function getImage(request, callback) {
var cb = getCallBacksRequest(callback);
var image = new Image(1, 1);

image.onload = function () {
iterator = 0; // To avoid JSLint warning of empty block
Copy link
Member

Choose a reason for hiding this comment

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

iterator = 0 is possibly no longer needed I think, same in onerror

@tsteur
Copy link
Member

tsteur commented Sep 21, 2015

I'd need to do a manual test but looks good in general. For our piwik.js tests it is quite important that we always add automated tests. Are you familiar with our piwik.js tests in https://github.com/piwik/piwik/tree/master/js ? I know it's quite strange, do you think you could add a test and somehow verify the callback(s) are triggered? It probably won't be possible to test the error case I presume (haven't thought about it) but at least to make sure the success gets triggered either via the old way cb=function and the new way cb.success=function

@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 Oct 13, 2015
@arul-prasad
Copy link

Hi,

It is pretty good change which i was also looking for to have a cb on Error.

My doubt is have you tested with HTTP 204 with no content ? because Piwik tracker call piwik.php will always have HTTP 204 no content.

I manually made the param send_image=1 to have the HTTP 200 response. because image.onError will be called even in http 204.

thanks.
Arul

arul-prasad
arul-prasad reviewed Dec 30, 2015
View reviewed changes
js/piwik.js
cb.success();
};

image.onerror = function () {
Copy link

Choose a reason for hiding this comment

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

this callback will be called when we have other than HTTP 200, even it is HTTP 204 no content.

Copy link
Author

Choose a reason for hiding this comment

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

204 no-content is a success, so it's ok.

Copy link

Choose a reason for hiding this comment

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

Image object consider 204 as error. So onError executed when we have 204
instead onSuccess.
On 11-Jan-2016 3:36 pm, "dhoko" notifications@github.com wrote:

In js/piwik.js
#8782 (comment):

             var image = new Image(1, 1);

             image.onload = function () {
                 iterator = 0; // To avoid JSLint warning of empty block

  •                if (typeof callback === 'function') { callback(); }

  •                cb.success();

  •            };

  •            image.onerror = function () {

204 no-content is a success, so it's ok.

—
Reply to this email directly or view it on GitHub
https://github.com/piwik/piwik/pull/8782/files#r49304629.

@arul-prasad
Copy link

One More thing, can we add the same error cb on the trackPageview method too ?

@tsteur
Copy link
Member

tsteur commented Jan 10, 2016

As the PR is quite useful is there maybe someone who wants to work on finishing this PR? That would be really helpful :)

@mattab
Copy link
Member

mattab commented Feb 1, 2016

we'd be very interested to merge this PR for Piwik 3.0.0 - if someone wants to help we would be grateful!

@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:39
@mattab mattab modified the milestones: 3.0.0-b2, 3.0.0 Sep 23, 2016
@mattab
Copy link
Member

mattab commented Oct 30, 2016

Hi @dhoko or anyone else interested in this PR,

Please leave a comment to get the PR re-opened if you'd like to finish it and have it merged in Piwik. Currently we don't have time to finish it ourselves but we'd love to help getting it merged when someone else will be interested 👍

@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 @tsteur @arul-prasad @mattab