-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: add ga4 user registered handler #2281
Conversation
foreach ( self::$watched_events as $event_name ) { | ||
Data_Events::register_handler( [ __CLASS__, 'handle_' . $event_name ], $event_name ); | ||
} |
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.
Why not make it a global handler that watches every event? A global handler exists mostly for analytics integrations like this one.
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'll explore it
I want to have each event handler in its own callback for the sake of organization, I didn't want to have one big callback with a switch case or anything like it. But let me give it a go
/** | ||
* Initialize the class and registers the handlers | ||
* | ||
* @return void | ||
*/ | ||
public static function init() { | ||
Data_Events::register_handler( [ __CLASS__, 'handle_reader_logged_in' ], 'reader_logged_in' ); | ||
foreach ( self::$watched_events as $event_name ) { | ||
Data_Events::register_handler( [ __CLASS__, 'handle_' . $event_name ], $event_name ); |
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.
This feels like a very fragile implementation. If the watched_events
are modified it will call for an undefined method.
$params['registration_method'] = $data['metadata']['registration_method'] ?? ''; | ||
|
||
$event = new Event( 'reader_registered', $params ); | ||
self::send_event( $event, $client_id, $timestamp, $user_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.
In the context of a global handler for analytics, it can look like this:
Being a global hanler, this suggestion assumes the first argument of the method is the $action_name
.
$params['registration_method'] = $data['metadata']['registration_method'] ?? ''; | |
$event = new Event( 'reader_registered', $params ); | |
self::send_event( $event, $client_id, $timestamp, $user_id ); | |
$event = new Event( $action_name, $data ); | |
self::send_event( $event, $client_id, $timestamp, $user_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.
I cant' simply pass whatever is in $data
to GA4. Each event will have its own handler to add the appropriate parameters
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 wish we could. The listeners' documentation would be the source of truth on what and how we collect and we avoid having another layer of support for each new trackable thing. The more we track the better, there's no downside to that IMO.
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.
There's a limit of 25 parameters an event can have in GA, so I think we want to carefully choose what we will track.
Also, some events have arrays as parameters, that need to be treated...
@miguelpeixe This is ready for another review. I slightly changed the implementation, using a global handler, but I kept one method for each event. Each event will have a very unique way of setting up its parameters. (In fact, if this gets too big, I might refactor it into having each event handling in one simple child class with only one method) Please note that the Note that I've updated the test instructions. |
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 registration metadata is being logged as described, except for the campaign popup metadata, which I was not able to test:
add_filter( 'newspack_data_events_dispatch_body', [ __CLASS__, 'filter_event_body' ], 10, 2 ); | ||
} | ||
|
||
/** | ||
* Handler for the reader_logged_in event. |
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.
This is no longer a specific handler method
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.
fixed
if ( ! empty( $data['metadata']['newspack_popup_id'] ) ) { | ||
$params = array_merge( $params, Popups_Events::get_popup_metadata( $data['metadata']['newspack_popup_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.
Don't know the reason, but the newspack_popup_id
is not being populated to my registration through a campaigns popup. This is the resulting params from a center overlay
campaign:
(
[logged_in] => yes
[is_reader] => yes
[registration_method] => registration-block
[referer] => /
)
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.
Check if you have blocks and popups plugins in the latest master and that they are built
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.
It was the popups plugin that was outdated!
After getting all params working from a campaigns' prompt the
This error message is not too helpful because it can be any of:
Perhaps the In this case, this is the culprit: |
I also suggest making the I suggest the parsing to:
|
Yes, I know. I struggled a bit when I was doing this and decided to keep it simple for starters because I didn't want to over complicate. But I agree with you, let's make it correct and sanitize the parameters and just throw a warning log in case something is invalid. |
@miguelpeixe done! I want to keep the Event class simple, as the source of truth of the limitations that GA4 has to events. And I didn't want to transform validation methods into validate & sanitize... So I made the event values a little more flexible, accepting data types we can easily convert to strings, but still throwing error otherwise. And then added the validating and error handling to our class. I think it looks ok now. In the future we might want to surface these errors, and the events api errors in general, somehow.... |
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.
Thank you for the tweaks and improved debugging! It's working great now 🎉:
Non-blocking NIT, by instantiating a new Event()
with name and params, isn't non-static methods a better practice for this case?
$event = new Event( $name, $params );
if ( ! $event->has_valid_name() ) {
throw new \Exception( 'Invalid event name' );
}
Not necessarily. If the method does not interact or depend on anything from the instanced object, it could be static. It could be non-static if it was, for example, writing the errors to a By keeping it static we are saying "If you provide me invalid params, I will throw exceptions, use my static methods to validate them before instantiating an object. It's also super easy to test those methods as well. |
# [1.106.0-alpha.1](v1.105.0...v1.106.0-alpha.1) (2023-03-03) ### Bug Fixes * **ads:** gam api availability according to error type ([#2289](#2289)) ([024fe08](024fe08)) ### Features * add a Add new button to subscription lists ([#2314](#2314)) ([9543ad2](9543ad2)) * add ga4 user registered handler ([#2281](#2281)) ([5eb2336](5eb2336)) * add pid to Logger ([#2290](#2290)) ([fd3011c](fd3011c)) * Add popup info to donations ([#2300](#2300)) ([7ea800b](7ea800b)) * allow external links in dashboard via a filter ([#2279](#2279)) ([3943b1a](3943b1a)) * campaigns listeners for the data events api ([#2291](#2291)) ([ab407d4](ab407d4)) * disable save button for unchanged settings ([#2259](#2259)) ([e06d72f](e06d72f)), closes [#1531](#1531) * **donate-block:** support modal checkout ([#2256](#2256)) ([34226dd](34226dd)) * Normalize donation events ([#2299](#2299)) ([2624d53](2624d53)) * **perfmatters:** improve config ([267306e](267306e)) * prevent homepage from being unpublished ([#2307](#2307)) ([a151d53](a151d53)) * Remove the campaign rendered event ([#2301](#2301)) ([23caa1d](23caa1d)) * Stripe Subscriptions to WC subscriptions migrator ([#2298](#2298)) ([6904356](6904356)), closes [#2251](#2251) * **wc:** force allowing subscription switching ([#2305](#2305)) ([c13e741](c13e741))
🎉 This PR is included in version 1.106.0-alpha.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.106.0](v1.105.1...v1.106.0) (2023-03-14) ### Bug Fixes * **ads:** gam api availability according to error type ([#2289](#2289)) ([024fe08](024fe08)) * show handoff to finish Newspack setup only if setup is incomplete ([#2343](#2343)) ([1173b5b](1173b5b)) ### Features * add a Add new button to subscription lists ([#2314](#2314)) ([9543ad2](9543ad2)) * add ga4 user registered handler ([#2281](#2281)) ([5eb2336](5eb2336)) * add pid to Logger ([#2290](#2290)) ([fd3011c](fd3011c)) * Add popup info to donations ([#2300](#2300)) ([7ea800b](7ea800b)) * allow external links in dashboard via a filter ([#2279](#2279)) ([3943b1a](3943b1a)) * campaigns listeners for the data events api ([#2291](#2291)) ([ab407d4](ab407d4)) * disable save button for unchanged settings ([#2259](#2259)) ([e06d72f](e06d72f)), closes [#1531](#1531) * **donate-block:** support modal checkout ([#2256](#2256)) ([34226dd](34226dd)) * Normalize donation events ([#2299](#2299)) ([2624d53](2624d53)) * **perfmatters:** improve config ([267306e](267306e)) * prevent homepage from being unpublished ([#2307](#2307)) ([a151d53](a151d53)) * Remove the campaign rendered event ([#2301](#2301)) ([23caa1d](23caa1d)) * Stripe Subscriptions to WC subscriptions migrator ([#2298](#2298)) ([6904356](6904356)), closes [#2251](#2251) * **wc:** force allowing subscription switching ([#2305](#2305)) ([c13e741](c13e741))
🎉 This PR is included in version 1.106.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Closes # .
How to test the changes in this Pull Request:
Get the GA4 credentials. You will need your measurement ID and an API secret.
Admin > Data Streams > choose your stream > Measurement Protocol > Create
Admin > Data Streams > choose your stream > Measurement ID
Store this info in the database:
Now, let's test:
reader_registered
event being fired in the logsreader_registered
GA event:reader_registered
events coming inregistration_method
attribute set asregistration-block
for the times you registered via the registration block.referer
attribute is correct(you can also add a debug to the
handle_reader_registered
method to see these params being set).Note that this PR also fixes kind of a bug. Something that was working by accident.
In the line:
wp_get_referer()
always returnsfalse
because the referer is the same page as the current page. (It's a post to itself). The accident is that if the second parameter toadd_query_arg
isfalse
, it will use the current URL instead. And since this is a post to itself, the result is the same...But my code fixes it and we now actually read the value from the
_wp_http_referer
in the form.Other information: