Feat/callbacks error and success #7946

dhoko · 2015-05-20T07:50:08Z

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)
    }
  });

mattab · 2015-05-21T03:27:02Z

Hi @dhoko

Thanks for the PR. I don't know if we need this (it's quite complicated).
There is already a callback parameter to the sendRequest function (added in #5850).
Maybe we could do a simpler change like this:


--- js/piwik.js (revision 530ff25b5cb5b6de89ff9e45f7c04757fd3cf26e)
+++ js/piwik.js (revision )
@@ -3714,7 +3714,7 @@
             /*
              * Log the event
              */
-            function logEvent(category, action, name, value, customData)
+            function logEvent(category, action, name, value, customData, callback)
             {
                 // Category and Action are required parameters
                 if (String(category).length === 0 || String(action).length === 0) {
@@ -3726,7 +3726,7 @@
                         'event'
                     );

-                sendRequest(request, configTrackerPause);
+                sendRequest(request, configTrackerPause, callback);
             }

             /*
@@ -5147,9 +5147,9 @@
                  * @param string name (optional) The Event's object Name (a particular Movie name, or Song name, or File name...)
                  * @param float value (optional) The Event's value
                  */
-                trackEvent: function (category, action, name, value) {
+                trackEvent: function (category, action, name, value, callback) {
                     trackCallback(function () {
-                        logEvent(category, action, name, value);
+                        logEvent(category, action, name, value, null, callback);
                     });
                 },

(I didn't test it). would it work for you?

if so feel free to update your PR

dhoko · 2015-05-21T07:04:57Z

The default solution does not work as expected, it's triggered too many times and we cannot detect if the request is a success or an error cf:

I can change the implementation by using too arguments instead of one object:

trackEvent(category, action, name, value, {success,error});

// to
trackEvent(category, action, name, value, onSuccess, onError);

What do you think ?

tsteur · 2015-05-24T21:54:37Z

js/piwik.js

@@ -5148,9 +5219,9 @@ if (typeof Piwik !== 'object') {
                 * @param string name (optional) The Event's object Name (a particular Movie name, or Song name, or File name...)
                 * @param float value (optional) The Event's value
                 */
-                trackEvent: function (category, action, name, value) {
+                trackEvent: function (category, action, name, value, callbacks) {


Is callbacks a common name for that? Reading that parameter I expected to pass an array of callbacks. Also it needs to be documented as a parameter. Shall we maybe rather still use callback, especially since a function callback would work as well? Or is it maybe worth to use a settings object to allow {success: function()..., error: function ()...} that would be okay for me as it is optional anyway. Just to avoid having to add many more optional parameters in the future.

There are methods like trackLink that currently expect callback. We'd need to make sure to use consistent naming and behaviour and to maintain backwards compatibility.

Yup we can use onSuccess, onError 1 more argument to the function. I doesn't break the compatibility cf L2513

// Compatibility for eventLink if('function' === typeof cb) {

mattab · 2015-05-24T22:21:49Z

The default solution does not work as expected, it's triggered too many times and we cannot detect if the request is a success or an error.

I didn't look, but maybe there is a way to "fix" the default solution so it can do what you need as well as keeping BC for those who may be using the callback already?

dhoko · 2015-05-29T14:58:17Z

The meaning of this PR wasn't to rewrite the current callback system. I try to improve it and fix it.

Today with Piwik:

You cannot detect if the request is a success or a failure.
You cannot detect if the call was from a XHR or a GET.
The callback is triggered too many times

Bonus: it doesn't break the compatibility.

Why ?

Because I am working with a mobile app, I cannot do a request for each event (it's expensive) so I need to put these items to a cache (localStorage) and send my events after a .
To prevent any errors on mobile i need to know if I am online or not and if the request is a success. Piwik cannot do that so, that's why there are these PR ( #7949 too). (and I have a third one coming)

mattab · 2015-09-11T10:59:16Z

Hi @dhoko - Do you still think we should merge this PR? if so please leave a comment, and we will take another look... For sure, we would need to have automated tests as well (tests/javascript/index.php) before we could merge it

dhoko · 2015-09-11T13:10:48Z

Yup, I will try to write some tests.
First. I must understand how do you write tests with piwik.

I don't know Qunit yet, i work with Jasmine.

…otifications

class already extends BaseFactory with class name & error msg overrides

…e abstract class

Fix pixelRatio

Fix piwiki

Fix cb declaration

dhoko · 2015-09-14T08:05:48Z

Something is fucked up with my fork. Sorry for the mess, I will create a new PR.

dhoko force-pushed the feat/callbacksErrorAndSuccess branch from f2ebaa8 to b6f8a6f Compare May 20, 2015 10:28

mattab mentioned this pull request May 21, 2015

Piwik.js JS tracker: allow developer to pass a 'callback' argument #5850

Merged

tsteur reviewed May 24, 2015
View reviewed changes

dhoko force-pushed the feat/callbacksErrorAndSuccess branch from b6f8a6f to 29b697e Compare August 25, 2015 09:11

dhoko force-pushed the feat/callbacksErrorAndSuccess branch from 29b697e to 596fc7e Compare September 10, 2015 07:36

sgiehl and others added 9 commits September 14, 2015 10:02

adds SMS Provider for development, that shows all sent SMS as Piwik n…

23bdcfe

…otifications

refs matomo-org#4615 fix layout for displayed phone numbers

0ca54a5

Refs matomo-org#8777 tweak summary log messages output for more clarity

a2b8530

Submodule update for matomo-org/matomo-php-tracker#12

e0462a0

Remove un-needed factory()

1f56854

class already extends BaseFactory with class name & error msg overrides

Display a clear error message when the SMS provider doesn't extend th…

6ba7b88

…e abstract class

Add ability to set a callback onSuccess and onError for GET and POST

1c45c95

Fix pixelRatio

Add compatibility for logLink callback

a6961e4

Fix piwiki

Fix sendRequest callbacks generation

0aa454c

Fix cb declaration

dhoko force-pushed the feat/callbacksErrorAndSuccess branch from 596fc7e to 0aa454c Compare September 14, 2015 08:03

dhoko closed this Sep 14, 2015

dhoko mentioned this pull request Sep 14, 2015

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

Closed

This was referenced Jul 31, 2022

[Snyk] Security upgrade puppeteer from 8.0.0 to 13.3.0 dwayne45/matomo#15

Open

[Snyk] Security upgrade puppeteer from 10.4.0 to 13.3.0 dwayne45/matomo#16

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/callbacks error and success #7946

Feat/callbacks error and success #7946

dhoko commented May 20, 2015

mattab commented May 21, 2015

dhoko commented May 21, 2015

tsteur May 24, 2015

tsteur May 24, 2015

dhoko May 29, 2015

mattab commented May 24, 2015

dhoko commented May 29, 2015

mattab commented Sep 11, 2015

dhoko commented Sep 11, 2015

dhoko commented Sep 14, 2015

Feat/callbacks error and success #7946

Feat/callbacks error and success #7946

Conversation

dhoko commented May 20, 2015

mattab commented May 21, 2015

dhoko commented May 21, 2015

tsteur May 24, 2015

Choose a reason for hiding this comment

tsteur May 24, 2015

Choose a reason for hiding this comment

dhoko May 29, 2015

Choose a reason for hiding this comment

mattab commented May 24, 2015

dhoko commented May 29, 2015

mattab commented Sep 11, 2015

dhoko commented Sep 11, 2015

dhoko commented Sep 14, 2015