-
Notifications
You must be signed in to change notification settings - Fork 21
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
[API Pull] Add Track Events to the Auth API responses #2452
[API Pull] Add Track Events to the Auth API responses #2452
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/google-api-project #2452 +/- ##
==============================================================
+ Coverage 64.6% 64.8% +0.1%
Complexity 4552 4552
==============================================================
Files 473 473
Lines 17757 17783 +26
==============================================================
+ Hits 11478 11516 +38
+ Misses 6279 6267 -12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@jorgemd24 I yes get the blog ID (in the logs using the snippets. In tracks I didn0't check) |
|
||
/** | ||
* @event revoke_wpcom_api_auth | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add here the property directives? Also, I miss the event description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I add them here: fb8b49c
/** | ||
* @event revoke_wpcom_api_auth | ||
*/ | ||
do_action( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @puntope for the review!
Yes, the Can you take another look to this PR? |
src/API/WP/OAuthService.php
Outdated
* @event revoke_wpcom_api_authorization | ||
* @property string status The status of the request. | ||
* @property string error The error message. | ||
* @property mixed blog_id The blog ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of mixed WDYT about number|null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, maybe we can consider sending 400 or 500 for example or get the error code to have consistency between both woocommerce_gla_track_event
tracks (in some is a number, the request status, and in this one the string "error" )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error seems to be optional
src/API/WP/OAuthService.php
Outdated
* When the WPCOM token has been revoked with errors. | ||
* | ||
* @event revoke_wpcom_api_authorization | ||
* @property string status The status of the request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status is number in this case right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jorgemd24 for adding the docs. I left some additional comments
Hi @puntope, I addressed your suggestions. Can u have another look? Thanks |
src/API/WP/OAuthService.php
Outdated
* When the WPCOM token has been revoked successfully. | ||
* | ||
* @event revoke_wpcom_api_authorization | ||
* @property string status The status of the request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing to refactor the status, is also int here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx @jorgemd24 approved in advance. However, I miss some doc tweak in one of the revoke_wpcom_api_authorization
Changes proposed in this Pull Request:
Part of #2146 .
Currently, we have two tracking events,
gla_disable_product_sync_click
andgla_enable_product_sync_click
. However, it would also be useful to track the status of the Auth flow and when merchants revoke access.This PR adds the following server-side event tracking:
revoke_wpcom_api_authorization
-> When the token is revoked, this event will include the status, blog id, and any error if it exists.update_wpcom_api_authorization
-> When the Auth flow is completed, this event will include the status, blog id, and any error if it exists.Screenshots:
Detailed test instructions:
wcadmin_gla_update_wpcom_api_authorization
andwcadmin_gla_revoke_wpcom_api_auth
. It may take some time for the data to appear.Ref: #2207
add_filter( 'woocommerce_gla_notifications_enabled','__return_true' )
;Disable product data fetch
:Additional details:
blog_id
because I thought it might help with debugging issues, but I can't see it in Tracks. I'm not sure why that is.Changelog entry