Open nfmohit opened 1 month ago
Hi @ankitrox, thanks for drafting this IB. I have a few thoughts, please see what you think.
- [ ] In
assets/js/modules/analytics-4/datastore/audiences.js
, create a new selectorgetSiteKitAudiencesUserCountReportOptions
which should return the report options for Site Kit user count. RefergetAudiencesUserCountReportOptions
selector which is similar one.
- [ ] This selector should accept th
audiences
as parameter which would be audience resource names for Site Kit audiences i.e.New visitors
andReturning visitors
.- [ ]
getSiteKitAudiencesUserCountReportOptions
should build the options same as we do inAudienceItems
component here. The only additional thing isdimensionFilters
will be added to these options based on the audiences passed as mentioned in above point. Refer how it is done forgetAudiencesUserCountReportOptions
selector.- [ ] In
assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItems.js
, make change innewVsReturningReport
so that it should callgetReport
function by passinggetSiteKitAudiencesUserCountReportOptions( siteKitAudiences )
as parameter, just like it is done for audienceResourceNameReport here.
This isn't quite right, and is a little overcomplicated. The getSiteKitAudiencesUserCountReportOptions()
selector doesn't need to take an argument at all. It simply needs to replicate the current report used to retrieve the SK audience user counts:
- [ ] Add a new selector
getSiteKitAudiences
inassets/js/modules/analytics-4/datastore/audiences.js
.
- [ ] Logic to get Site Kit audiences is already available here which can be ported to selector and be reused in
AudienceItems
component. This is useful step because this selector can also be used in #8144.
Let's go a step further and make the selector getConfiguredSiteKitAndOtherAudiences()
and just extract this block of code, returning [ siteKitAudiences, otherAudiences ]
. We are going to need both of these values for this IB, as I'll discuss below. This selector would still be useful for #8144, which we should make a dependency of this issue and update its IB accordingly.
Let's also create a new selector hasAudiencePartialData( audiences )
which extracts and makes generic this block of code, this will also be useful below, and can also be used in #8144.
- [ ] Add a new parameter
siteKitAudiencesUserCountError
to selectorgetAudiencesUserCountReportError
. Default value of it should befalse
. When it is true, it will look forgetReport
errors withgetSiteKitAudiencesUserCountReportOptions
instead ofgetAudiencesUserCountReportOptions
.
- [ ] If
siteKitAudiencesUserCountError
is true, call thegetSiteKitAudiencesUserCountReportOptions
selector and store it insiteKitAudiencesUserCountReportOptions
.- [ ] Update the return statement to pass
siteKitAudiencesUserCountReportOptions
ifsiteKitAudiencesUserCountError
istrue
instead of passingaudiencesUserCountReportOptions
which is present currently.
Let's not modify getAudiencesUserCountReportError()
to accept a boolean argument (which is a bit of a code smell).
Actually, looking at the current implementation of getAudiencesUserCountReportError()
, it's evident that the arguments passed to getAudiencesUserCountReportOptions()
don't match what's being passed when we actually retrieve the report:
Compare getAudiencesUserCountReportError()
:
https://github.com/google/site-kit-wp/blob/2ced71984a4fa365c621be4cb62dca121f51909d/assets/js/modules/analytics-4/datastore/audiences.js#L625-L632
Clearly, we need to apply the same conditionality to the argument for getAudiencesUserCountReportOptions()
when retrieving the error as we do when requesting the report, in order to ensure we correctly retrieve the error for the report.
What I'd suggest is renaming getAudiencesUserCountReportError()
to getAudienceUserCountReportErrors()
, and update the selector to make use of getConfiguredSiteKitAndOtherAudiences()
and hasAudiencePartialData( audiences )
to reproduce enough logic from AudienceItems
to determine which reports have been run, returning one or two errors as appopropriate.
- [ ] In
assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/ErrorNotice.js
, create a new constantsiteKitUserCountReportError
and callgetAudiencesUserCountReportError
with parametertrue
.
- [ ] If it returns the object, add it to the
errors
list present in component.- [ ] Update this condition to also check for
siteKitUserCountReportError
.
This last part of the IB won't be needed if we are taking the getAudienceUserCountReportErrors()
approach described above.
Thank you @techanvil for your valuable feedback and time on reviewing the IB.
I've updated the IB as per the suggested points. Can you please let me know if it all looks good?
Thank you.
Hi @ankitrox, thanks for the update. This IB is looking good. There's just one additional aspect that it would be good to address here.
I've noticed that this getReport()
should be conditionally invoked; we should not fetch the report if isSiteKitAudiencePartialData
is true but otherAudiences
is empty, as there is no point retrieving a report for an empty list of audiences:
Although it might look out of scope for this issue, I would say it's within the scope because it affects the error handling - we also want to make sure we don't attempt to retrieve an error for this report within getAudienceUserCountReportErrors()
when the above condition is met.
Please can you add a note or point(s) to the IB to make sure this is captured?
Thank you @techanvil . I have added the mentioned point as the last pointer of the IB.
Thanks @ankitrox! I've tweaked the point to make it clear that we should avoid retrieving the error within getAudienceUserCountReportErrors()
.
IB :white_check_mark:
Hey @ankitrox, please can you add a QAB for this issue?
@techanvil I've added the QAB. Unassigning myself.
Thanks @ankitrox! Note that I've given it a little tweak - particularly because without backtick quotes the regular expression looked like this:
./wp-json/google-site-kit/v1/modules/analytics-4/data/report.newVsReturning.*
It should however display like this:
.*/wp-json/google-site-kit/v1/modules/analytics-4/data/report.*newVsReturning.*
Note how the 2nd and 3rd *
are not showing up without the backticks, without these the regex wouldn't match. One to watch out for! :sweat_smile:
This is tested good overall. I only could not verify whether the request is intercepted by tweak. How do we actually do that? Anyhow, most likely this would not matter too much as the end result is as expected.
Verified good
I was able to see the error in the selection panel and dashes in place of user counts. ✅
After clicking re-try, the data at the selection panel appeared again: ✅
Hi @kelvinballoo ,
You shall see the number of requests intercepted by the extension as shown in screenshot and its details when you open the extension panel.
Thank you.
Feature Description
This is a follow-up of #8161 which introduced an error notice above the Audience Selection Panel footer that shows an error notice when the report request to obtain user count for audiences fails. However, as part of #8160, the mechanism to obtain user count was changed such that if any of the Site Kit audiences were in a partial data state, it would make an additional report request using the
newVsReturning
dimension to obtain user count for Site Kit audiences.The error handling established in #8161 should be updated to cover this additional report as well only in the case where one or more of the Site Kit audiences are in a partial data state.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
newVsReturning
dimension (see #8160):Implementation Brief
assets/js/modules/analytics-4/datastore/audiences.js
, create a new selectorgetSiteKitAudiencesUserCountReportOptions
which should return the report options for Site Kit user count. RefergetAudiencesUserCountReportOptions
selector which is similar one.getSiteKitAudiencesUserCountReportOptions
should build the options same as we do inAudienceItems
component here.assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItems.js
, make change innewVsReturningReport
so that it should callgetReport
function by passinggetSiteKitAudiencesUserCountReportOptions( siteKitAudiences )
as parameter, just like it is done foraudienceResourceNameReport
here.hasAudiencePartialData( audiences )
inassets/js/modules/analytics-4/datastore/audiences.js
.AudienceItems
component.getConfiguredSiteKitAndOtherAudiences
inassets/js/modules/analytics-4/datastore/audiences.js
.AudienceItems
component. This is useful step because this selector can also be used in #8144.getAudiencesUserCountReportError
togetAudienceUserCountReportErrors
inassets/js/modules/analytics-4/datastore/audiences.js
.getConfiguredSiteKitAndOtherAudiences
selector instead ofgetConfigurableAudiences
.hasAudiencePartialData
selector to check if Site Kit audiences are in partial data state.getConfiguredSiteKitAndOtherAudiences
) and the other for Site Kit audiences (if Site Kit audiences are in partial data state) like following.assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItems.js
, replacegetAudiencesUserCountReportError
withgetAudienceUserCountReportErrors
and destructure response intoconst [ otherUserCountReportError, siteKitUserCountReportError ]
.errors
list present in component.assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItems.js
isSiteKitAudiencePartialData
istrue
, butotherAudiences
is empty (returned bygetConfiguredSiteKitAndOtherAudiences
selector), then this report should not be invoked.getAudienceUserCountReportErrors
when the above condition is met.Test Coverage
Update tests for the
ErrorNotice
component to also include scenarios where a Site Kit audiences user count error occurs.Update tests for the new
getAudienceUserCountReportErrors
selector inassets/js/modules/analytics-4/datastore/audiences.test.js
.QA Brief
In the Tester Settings (tester plugin), go to
Analytics Partial Data > Force Analytics audience partial data state
, setlast-7-days
and save.Using the Tweak Chrome extension, create a rule as follows.
.*/wp-json/google-site-kit/v1/modules/analytics-4/data/report.*newVsReturning.*
(.*)
GET
400
Make sure audience segmentation feature is enabled and Site Kit audiences are created in the Google Analytics. Also, these Site Kit audiences (new visitors and returning visitors) are available in audience selection panel.
Clear the reports from session storage in browser.
Enable the Tweak rule with regular expression matching. Make sure browser's developer tool is not open.
Refresh the Site Kit dashboard. Open the audience selection panel. You should be able to see the error in the selection panel and dashes in place of user counts. Also, observe that request is intercepted by tweak.
Changelog entry