Skip to content
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

Create Ads Module Site Health Checks #8245

Closed
2 tasks done
10upsimon opened this issue Feb 13, 2024 · 13 comments
Closed
2 tasks done

Create Ads Module Site Health Checks #8245

10upsimon opened this issue Feb 13, 2024 · 13 comments
Labels
Good First Issue Good first issue for new engineers Module: Ads Google Ads module related issues P0 High priority PHP Type: Feature New feature

Comments

@10upsimon
Copy link
Collaborator

10upsimon commented Feb 13, 2024

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

  • 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-
  • 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 the Ads module settings.
  • 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 and placed by Site Kit."
  • Connect Analytics 4, and ensure that Place Google Analytics code setting is enabled
    • Go to Tools > Site Health, then click Passed Tests and expand the Tag Placement accordion
    • It should show the same as above: "Tag detected and placed by Site Kit."
  • Disabling the Place Google Analytics code setting for Analytics 4 module, should still confirm tha Tag placement for Ads module and show the same message "Tag detected and placed by Site Kit."

Changelog entry

  • Add Site Health information for the Ads module.
@10upsimon 10upsimon added Module: Ads Google Ads module related issues P0 High priority PHP Type: Feature New feature labels Feb 13, 2024
@eugene-manuilov eugene-manuilov self-assigned this Feb 13, 2024
@eugene-manuilov
Copy link
Collaborator

@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).

@10upsimon
Copy link
Collaborator Author

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

@eugene-manuilov
Copy link
Collaborator

Yeah, AC 🌶️

@eugene-manuilov eugene-manuilov removed their assignment Feb 19, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Feb 23, 2024
@eugene-manuilov eugene-manuilov self-assigned this Feb 26, 2024
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Feb 26, 2024
@wpdarren wpdarren added the Good First Issue Good first issue for new engineers label Mar 11, 2024
@benbowler benbowler self-assigned this Mar 11, 2024
@benbowler benbowler removed their assignment Mar 12, 2024
@hussain-t hussain-t self-assigned this Mar 13, 2024
@hussain-t
Copy link
Collaborator

hussain-t commented Mar 15, 2024

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

@10upsimon 10upsimon added QA: Eng Requires specialized QA by an engineer and removed QA: Eng Requires specialized QA by an engineer labels Mar 15, 2024
@hussain-t hussain-t assigned 10upsimon and unassigned hussain-t Mar 15, 2024
@10upsimon
Copy link
Collaborator Author

10upsimon commented Mar 20, 2024

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.

@mxbclang
Copy link

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

@mxbclang mxbclang assigned zutigrm and unassigned 10upsimon Mar 21, 2024
@zutigrm zutigrm removed their assignment Apr 1, 2024
@eugene-manuilov eugene-manuilov removed their assignment Apr 2, 2024
@wpdarren wpdarren self-assigned this Apr 3, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 3, 2024

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.

@benbowler
Copy link
Collaborator

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

@benbowler benbowler removed their assignment Apr 3, 2024
@kuasha420 kuasha420 self-assigned this Apr 3, 2024
@kuasha420
Copy link
Contributor

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

@kuasha420 kuasha420 assigned wpdarren and unassigned kuasha420 Apr 3, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 3, 2024

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 wpdarren removed their assignment Apr 3, 2024
@aaemnnosttv
Copy link
Collaborator

@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.

@kuasha420
Copy link
Contributor

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. 🎉

@kuasha420 kuasha420 removed their assignment Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers Module: Ads Google Ads module related issues P0 High priority PHP Type: Feature New feature
Projects
None yet
Development

No branches or pull requests

9 participants