site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Create `Ads` Module Site Health Checks

Open 10upsimon opened this issue 1 year ago • 7 comments

Feature Description

As part of the new Ads module work being implemented, new Site Health checks are required to be added. These Site Health checks will be present in the Site Health > Info section, and appear within the Site Kit by Google health check group.

Checks will include the following:

  • Display of the Ads Conversion ID field value
  • An indication of whether the Ads Conversion ID script is being rendered/output on the site

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The following Site Health checks will appear within the Site Health > Info section, and appear within the Site Kit by Google health check group:
    • Output of the currently store Ads Conversion ID field value
    • Indication of whether the Ads Conversion ID snippet is being rendered on the frontend

Implementation Brief

  • [x] Add includes/Modules/Ads/Tag_Matchers.php
    • Use includes/Modules/AdSense/Tag_Matchers.php as a starting point.
    • Modify the regex array to match the ads code pattern, which is added in the gtag as gtag('config','AW-12345'). Ads conversion ID will always start with AW-
  • [x] Update Google\Site_Kit\Modules\Ads class:
    • Class should implement Module_With_Debug_Fields and Module_With_Tag interfaces
    • It should use Module_With_Tag_Trait trait
    • Add get_debug_fields method, it should output adsConversionID setting. You can see the example of implementation in any other module, for example Google\Site_Kit\Modules\AdSense::get_debug_fields
    • Add get_tag_matchers method, which should return the new Ads module Tag_Matchers class

Test Coverage

  • Update tests/phpunit/integration/Modules/AdsTest.php, to include test case forget_debug_fields

QA Brief

  • Enable the adsModule feature flag and connect the Ads module in the SK settings.
  • Add the Conversion ID to both the Ads module settings and the Analytics 4 settings.
  • Enable the Ads tag placement.
  • Go to Tools > Site Health, select the Info tab and open the Site Kit by Google accordion
    • Check the Ads Conversion ID is shown
  • Go to Tools > Site Health, then click Passed Tests and expand the Tag Placement accordion
    • Check the Tag Placement is shown. It will currently show: "Tag detected but could not verify that Site Kit placed the tag." because the Analytics module owns the tag not the Ads module.

Changelog entry

10upsimon avatar Feb 13 '24 08:02 10upsimon

@10upsimon, the current AC is more like IB. We don't need to go into that much details there. Just top level requirements should be added that describe the final result instead of how we need to do it.

In the context of the current ticket, the final results are that we display site health information for the Ads module and it shows conversion id and whether the snippet information. I think we don't even need to declare which class we need to implement in the module class because that is a part of IB after all (i know that we did it like this in other tickets, but that is excessive i think).

eugene-manuilov avatar Feb 13 '24 18:02 eugene-manuilov

@eugene-manuilov this is back with you, greatly simplified

10upsimon avatar Feb 19 '24 11:02 10upsimon

Yeah, AC 🌶️

eugene-manuilov avatar Feb 19 '24 12:02 eugene-manuilov

IB ✔️

eugene-manuilov avatar Feb 26 '24 17:02 eugene-manuilov

Hi @10upsimon, As discussed on Slack, the QAB needs to be updated. Can you please update it and set the QA:Eng label?

Update: Let's set this ticket blocked by #8348. Hence we don't need QA:Eng. cc: @benbowler

hussain-t avatar Mar 15 '24 09:03 hussain-t

Flagging that this is still blocked until we have #8251 merged. It's pending merge of that purely from a setup perspective so that we do not need a QA:ENG approach. The code is good to go, hence it's still in CR.

10upsimon avatar Mar 20 '24 14:03 10upsimon

@zutigrm With #8251 now merged, I think this should be good to go? Reassigning to you.

mxbclang avatar Mar 21 '24 13:03 mxbclang

QA Update: ❌

@benbowler we have now renamed Ads Conversion ID to Conversion Tracking ID so we should rename the title in the Info tab to this new title.

wpdarren avatar Apr 03 '24 04:04 wpdarren

Thanks @wpdarren, I've updated this title and will move to CR once checks pass.

benbowler avatar Apr 03 '24 09:04 benbowler

@wpdarren The follow up PR is now merged. Remember to test on main. Cheers.

kuasha420 avatar Apr 03 '24 11:04 kuasha420

QA Update: ✅

Verified:

  • When I select the Info tab and open the Site Kit by Google accordion, the ID is shown
  • When I click Passed Tests and expand the Tag Placement accordion, the Tag Placement is shown.
  • When I connect Analytics and on Site Health, then click Passed Tests and expand the Tag Placement accordion It shows the same as above: Tag detected and placed by Site Kit.
  • When I disable the Place Google Analytics code setting for the Analytics module and the Tag placement for the Ads module, it shows the same message: Tag detected and placed by Site Kit.
Screenshots

image image image image image

wpdarren avatar Apr 03 '24 15:04 wpdarren

@benbowler we have now renamed Ads Conversion ID to Conversion Tracking ID so we should rename the title in the Info tab to this new title.

@wpdarren @kuasha420 it's correct to update the field for consistency, but all module fields should be prefixed with the module name in the Site Health table. It's less obvious with the Ads module since it only has one field but when in doubt, it's good to look at other modules 👍

Also, the label and field key (used in debug output copied to clipboard) should match.

I'll open a quick PR for this fix.

aaemnnosttv avatar Apr 04 '24 20:04 aaemnnosttv

No additional QA needed here as it's just a string change + the masking of the value. I've verified the change in Local while Code Reviewing. Moving back to approval. :tada:

kuasha420 avatar Apr 04 '24 21:04 kuasha420