-
Notifications
You must be signed in to change notification settings - Fork 286
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 site health checks for Ads module. #8379
Add site health checks for Ads module. #8379
Conversation
Build files for 9758b20 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 @benbowler LGTM.
I synced this branch with develop since the tag implementation was needed to confirm the behaviour.
I also updated the QAB to describe all cases, and take new tag implementation into account, which now brings the inline comment used to detect that tag is placed by Site Kit.
Note to the Merge reviewer: There is a weird CI only test failure for php 5.6, it might be some glitch, couldn't replicate it locally, and the code change here is very small to affect it
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, @benbowler. A few comments from me. Please, take a look.
includes/Modules/Ads.php
Outdated
use Module_With_Assets_Trait; | ||
use Module_With_Tag_Trait; |
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 not needed because it is already added to the class.
includes/Modules/Ads.php
Outdated
* | ||
* @since n.e.x.t | ||
* | ||
* @return array |
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 add a description for what is returned from this function to make it consistent with other method phpdocs.
|
||
$ads->get_settings()->set( array( 'adsConversionID' => 'AW-123456789' ) ); | ||
|
||
$this->assertEqualSets( |
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 also verify that the value is the same as we set in the settings.
…odule-site-health-checks.
Thanks @eugene-manuilov, updates pushed. |
Summary
Addresses issue:
Ads
Module Site Health Checks #8245Relevant technical choices
In order to implement
ModuleWithTag
I had to implement aregister_tag
function:site-kit-wp/includes/Modules/Ads.php
Lines 132 to 135 in 9a2dfa3
I also tested the regex used would capture the provided gtag config call.
This PR returns the Ads Conversion ID to the Site Health Checks but without the tag registered I was not able to see the tag registration check on this page as implemented.
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