-
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
Add tracking for completed events #2207
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2207 +/- ##
===========================================
+ Coverage 59.5% 59.7% +0.2%
- Complexity 4129 4136 +7
===========================================
Files 453 454 +1
Lines 16482 16506 +24
===========================================
+ Hits 9807 9858 +51
+ Misses 6675 6648 -27
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 for working on this @mikkamp
I yes see the logs using the filter.
For example: For campaign creation I see all of this:
"gla_version": "2.5.15","
"gla_ads_id": "2675599762","
"gla_mc_id": "541848347","
"id": "20958455946","
"status": "enabled","
"name": "Campaign 2024-01-22 22:07:08","
"amount": "2","
"country": "ES","
"targeted_locations": "ES","
"_en": "wcadmin_gla_created_campaign","
"_ts": "1705961231444","
"_via_ua": "Mozilla\/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/120.0.0.0 Safari\/537.36","
"_lg": "en,es-ES;q=0.9,es;q=0.8,nl;q=0.7","
"_dr": "https:\/\/woomppellicer.jurassic.tube\/wp-admin\/admin.php?page=wc-admin&path=googlesetup-ads","
"_dl": "http:\/\/woomppellicer.jurassic.tube\/wp-json\/wc\/gla\/ads\/campaigns?_locale=user","
"_ut": "wpcom:user_id","
"_ui": "113366118","
"url": "https:\/\/woomppellicer.jurassic.tube","
"blog_lang": "en_US","
"blog_id": "225662403","
"store_id": "653e00fe-4f75-4c5d-b126-6f3fd6296c41","
"products_count": "27","
"wc_version": "8.5.0.40","
However, I don't see it in the Tracking History
src/API/Google/Settings.php
Outdated
* | ||
* @return array | ||
*/ | ||
public function get_tracked_settings() { |
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.
💅 I would change this to get_tracking_settings
or get_settings_for_tracking
.
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.
get_tracking_settings
implies it would be settings for the tracking system, which is not the case.
Changing the tense of the verb to "tracked" implies that it is "for tracking", but happy to change it to get_settings_for_tracking
if it's more explicit.
do_action( | ||
'woocommerce_gla_track_event', | ||
'edited_campaign', | ||
array_merge( | ||
[ | ||
'id' => $campaign_id, | ||
], | ||
$fields, | ||
) | ||
); |
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.
What do you think to wrap these functions? So we can avoid to repeat some code
public static function track_event( $event, $props ) {
do_action(
'woocommerce_gla_track_event',
$event,
$props
);
}
I see you created some Service for that. So potentially there is some reason to use an action instead of calling the function directly.
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.
The reason I didn't do this was because of dependencies, as well as consistency with the other tracking classes being setup to hook into an action to trigger the event.
My thought was that we would be adding tracking to many different classes, in order to be able to use a service class it would need to become a dependency of any class that wants to record a tracking event. However tracking is not a requirement for the code to function well within each of those classes, so I didn't want to add additional dependencies to multiple classes just for tracking.
The other option was to use a static class, but since the existing tracking implementations were using actions, I decided to stick with using a similar approach. The actions are also a little easier to unit test (using some helper functions) as opposed to sticking with a static class.
As for wrapping the function, the do_action
is just one function call, so I don't see a lot of repetition there, so if a dependency is needed for a utility class I don't see us gaining a lot of benefit from wrapping it.
Thanks for the review @puntope. I've gone ahead and renamed the function you suggested. I've also added an explanation why I don't think we should use a service class for tracking. Let me know if that makes sense or if there are any other suggestions there.
It can sometimes take a while till they are processed, I searched for the event and I can find one with the same properties you logged above: |
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.
Hi @mikkamp thanks for clarifying my question in regards the service.
I still don't see the track history for any event... Maybe I'm using the wrong username? Or some setup is wrong?
Since you see them, Im approving this in advance
Changes proposed in this Pull Request:
Project Thread: pcTzPl-20H-p2
This PR adds tracking from the backend for the following events:
Sending these events from the backend allows us to track when the event was successful (as opposed to when the button is clicked to submit the data), as well as return relevant properties.
Detailed test instructions:
Note
This snippet will only work for tracks that are sent through
wp_remote_get
(for example while doing REST API requests), tracks that are injected into the page footer will not be logged through this snippet.Changelog entry