-
Notifications
You must be signed in to change notification settings - Fork 277
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
Issue / 8777 Prevent Conversion Tracking When Toggled Off #8779
Issue / 8777 Prevent Conversion Tracking When Toggled Off #8779
Conversation
…rsion tracking toggled off.
…ion_Tracking::register().
…pt enqueued when conversion tracking disabled.
Build files for a461c55 have been deleted. |
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 @10upsimon I left you 1 comment
…scripts callback.
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.
@10upsimon Nice one, LGTM
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 @10upsimon – this LGTM, just a few small changes to clean this up.
@@ -96,7 +96,7 @@ public function register() { | |||
'wp_enqueue_scripts', | |||
function() { | |||
// Do nothing if neither Ads nor Analytics snippet has been inserted. | |||
if ( ! did_action( 'googlesitekit_ads_init_tag' ) && ! did_action( 'googlesitekit_analytics-4_init_tag' ) ) { | |||
if ( ! $this->conversion_tracking_settings->is_conversion_tracking_enabled() || ( ! did_action( 'googlesitekit_ads_init_tag' ) && ! did_action( 'googlesitekit_analytics-4_init_tag' ) ) ) { |
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.
Let's split these onto separate lines as it's quite hard to read as one long line especially with extra parenthesis.
Also, the implementation is getting a bit large to be inline here, let's extract it to a method to keep register
focused on just registration.
$original_conversion_tracking_settings = array( | ||
'enabled' => true, | ||
); | ||
|
||
$this->conversion_tracking_settings->set( $original_conversion_tracking_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.
This shouldn't be needed since changes to options shouldn't persist between tests as WP will rollback the transaction. Feel free to double check to make sure though 👍
$original_conversion_tracking_settings = array( | ||
'enabled' => false, | ||
); | ||
|
||
$this->conversion_tracking_settings->set( $original_conversion_tracking_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.
How about this?
$original_conversion_tracking_settings = array( | |
'enabled' => false, | |
); | |
$this->conversion_tracking_settings->set( $original_conversion_tracking_settings ); | |
$this->conversion_tracking_settings->set( array( 'enabled' => false ) ); |
Summary
Addresses issue:
Relevant technical choices
Conversion_TrackingTest
to add test that asserts scripts are not enqueued when conversion tracking is disabled.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist