-
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 section #8181
Comments
Hi @ankitrox, thanks for drafting this IB. However, the approach you've suggested is not really necessary - we can add to the For example, see the "available custom dimensions" entry: site-kit-wp/includes/Modules/Analytics_4.php Lines 428 to 440 in e023adf
|
Thanks @ankitrox, this is looking better, one thing though - we try to avoid including literal code blocks in IBs. I would say this IB can be written without the code block, please can you remove/rewrite that aspect? You can simply say to take the existing custom dimension entry as a reference, for example and permalink to the entry. |
Thank you @techanvil for reviewing the IB. I agree that adding the literal code block is not good idea as the execution should be at the engineer's discretion. As per your suggestion, I have removed the code block and added the list wide pointers for the implementation. Assigning it to you for the review again. Thank you. |
Thanks @ankitrox! I've made some additional changes to the IB. Notably I've removed the explicit definition for the new method and left this optional, and also removed the point about calling Please review the changes and if you are happy feel free to move this to the EB. IB ✅ |
Looking everything good. Thanks @techanvil Moving this to EB. |
Hi @ankitrox, as well as addressing the feedback left on the PR, please can you update the QAB for this issue as follows:
|
Hi @techanvil , Thank you for the review. I have addressed your feedback on the PR and also updated QAB so that we create Assigning this to you for the re-review. Thank you. |
Thanks @ankitrox! I've left a few further comments on the PR, please take a look. I've also tweaked the code example in the QAB slightly to follow our usual naming conventions (not essential, but might as well!), and to be slightly more verbose - we don't need to aim for DRY code in QABs, it's generally better if it's slightly more straightforward. |
Thank you @techanvil for reviewing the PR. I have addressed the comments in the PR and QAB code is looking good. Assigning this back to you for review. Thanks. |
Thanks @ankitrox! The PR needs one last update and this should be good to go. |
Thank you @techanvil for reviewing the PR. I've made the suggested change and assigning this to you for the review. Thanks. |
QA Update ❌Hi @ankitrox , tested this and I was able to do all the steps but in the end, there was not section with the values 'New visitors' and 'Returning visitors' in the Tools > Info section. The details are below:
Assigning ticket to you for further check. |
QA Update ✅After reviewing together with @ankitrox , it seems like my branch was not properly connected to Develop.
Moving this ticket to Approval |
Feature Description
Add the new “Analytics 4 site created audiences” section to Site Health.
See Site Health in the design doc.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
audienceSegmentation
feature flag is enabled, the Site Health section for Site Kit should contain a new section.Implementation Brief
includes/Modules/Analytics_4.php
, inside theget_debug_fields
method:audienceSegmentation
feature flag is enabled.$debug_fields
with following:label
asAnalytics site created audiences
.value
as comma separated string, containing thedisplayName
fields from the audiences stored in theavailableAudiences
setting whoseaudienceType
is'SITE_KIT_AUDIENCE'
. Feel free to add an additional method to retrieve these, or it can be inlined.debug
should be the same value.Test Coverage
tests/phpunit/integration/Modules/Analytics_4Test.php
fix thetest_get_debug_fieldss
test.QA Brief
Ensure that you have access to analytics 4 property where you can create the audiences. This will require write permission. Connect such a property in Google analytics in site kit.
Run the following command in browser console.
This will trigger two requests to
modules/analytics-4/data/create-audience
in the browser which should create two new audiencesNew visitors
andReturning visitors
in Google Analytics.It will sync audiences from Google Analytics to Site Kit.
Tools > Site Health > Info > Site Kit by Google
, you must see the sectionAnalytics site created audiences
with valueNew visitors, Returning visitors
.Changelog entry
The text was updated successfully, but these errors were encountered: