google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 284 forks source link

Create `Ads` Module Site Health Checks #8245

Closed 10upsimon closed 5 months ago

10upsimon commented 7 months ago

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:


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

eugene-manuilov commented 7 months ago

@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 commented 7 months ago

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

eugene-manuilov commented 7 months ago

Yeah, AC 🌶️

eugene-manuilov commented 6 months ago

IB ✔️

hussain-t commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

wpdarren commented 5 months ago

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 commented 5 months ago

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

kuasha420 commented 5 months ago

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

wpdarren commented 5 months ago

QA Update: ✅

Verified:

Screenshots ![image](https://github.com/google/site-kit-wp/assets/73545194/a54f54fb-ba5b-4e5b-b32e-0de2d11553af) ![image](https://github.com/google/site-kit-wp/assets/73545194/b2a15536-08d9-4e7c-8f6d-736c94ea067d) ![image](https://github.com/google/site-kit-wp/assets/73545194/ef29c5c4-97e3-4a78-8ff1-9ec51764d62c) ![image](https://github.com/google/site-kit-wp/assets/73545194/044d88be-7289-4342-933f-83f519201896) ![image](https://github.com/google/site-kit-wp/assets/73545194/446cd0f5-1bb9-4bbf-8b61-b56e16196ce8)
aaemnnosttv commented 5 months ago

@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 commented 5 months ago

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: