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

Handle the special case for “new visitors” and “returning visitors” to avoid the “partial data” state in the Audience Tile

Open techanvil opened this issue 1 year ago • 9 comments

Feature Description

Implement the special case for “new visitors” and “returning visitors” on the Audience Tile whereby their metrics are retrieved using the newVsReturning dimension instead of via the audience until the corresponding audiences are out of the "partial data" state.

See special case to avoid "partial data" state in the design doc.


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

Acceptance criteria

  • If either of the “new visitors” and “returning visitors” audiences are determined to be in the "partial data" state (see #8141):
    • The Audience Tile metrics for “new visitors” and “returning visitors” should be retrieved via a report that uses the newVsReturning dimension rather than audienceResourceName.
    • These metrics should reflect the full data available for the property over the current date range rather than the limited data available for the corresponding audiences.
    • The "partial data" badge at the top of these Audience Tiles should not be displayed.

Implementation Brief

Note: This issue may have some implementation shared with the related issue #8160.

Inside AudienceTilesWidget component assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js.

  • [ ] Partial data states are gathered in partialDataStates inside AudienceTilesWidget component. Check if the "new visitors" or "returning visitors" audiences are in "partial data" state. Set isSiteKitAudiencePartialData constant to true/false depending on whether these audiences are in partial state.

  • [ ] Use getAvailableAudiences selector and map the audienceResourceName to audienceSlug to form a new list audiencesNameSlugMap.

  • [ ] If isSiteKitAudiencePartialData is true:

    • [ ] Remove the "new visitors" and "returning visitors" from configuredAudiences list, if present. It's better to create a new list (without these corresponding audiences) so that it can be passed as dimensionFilters in reportOptions. isSiteKitAudience selector can be used to to determine if the audience is one of the Site Kit audiences i.e. "new visitors" or "returning visitors".
    • [ ] Create a new constant siteKitAudienceResourceNames which should contain the name for "new visitors" and "returning visitors"
    • [ ] Create a new report for siteKitAudiencesReport which should be fetched with same reportOptions, but the only difference being the dimensionFilters set to newVsReturning.
    • [ ] Fetch the top cities, top content and top page titles for siteKitAudienceResourceNames separately with dimensionFilters set to newVsReturning.
    • [ ] Create a function inside AudienceTilesWidget which should be called when we are iterating over the visibleAudiences to return AudienceTile component. It should return the following data as array elements which can be destructured and passed to AudienceTile component (either via spread or individually).
       1. reportRow
       2. previousReportRow
       3. topCitiesReport
       4. topContentReport
       5. topContentTitlesReport
       6. totalPageviewsReport
      
    • [ ] This function should accept audienceResourceName as argument. It should check if isSiteKitAudiencePartialData is true.
      • [ ] If isSiteKitAudiencePartialData is true and the audienceSlug (from audiencesNameSlugMap mentioned avove) is new-visitors or returning-visitors (check this using audiencesNameSlugMap map), return the data from the report fetched for site kit audiences (siteKitAudiencesReport and pivot reports).
  • [ ] In AudienceTile component inside assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTile/index.js

    • [ ] While determining isAudiencePartialData, check if the audienceResourceName belongs to "new visitors" or "returning visitors".
    • [ ] If property is not in partial state and audience belongs to one of the above, return false.
    • [ ] If property is not in partial state and audience is non Site-Kit audience, and in partial state, then return true.

Test Coverage

  • Add the test coverage for the scenario where "new visitors" and "returning visitors" are in partial data states.

QA Brief

Changelog entry

techanvil avatar Jan 24 '24 17:01 techanvil

Note that I've moved this to the Backlog as we have an open discussion on the "collecting data" terminology in the design doc.

techanvil avatar Feb 06 '24 14:02 techanvil

The aforementioned discussion has been resolved, and this is ready for AC.

techanvil avatar Feb 12 '24 12:02 techanvil

Hi @ankitrox, thanks for drafting this IB.

You raise an interesting concern as to how this issue relates to #8726. On balance, I think it would in fact be preferable to wait for #8726 to be implemented before tackling this issue, as #8726 will result in some significant refactoring of the AudienceTilesWidget and AudienceTile components, which I think would further impact this issue (for example I don't think the specced getMetric() would be relevant any more, and the data extraction aspect this addresses would need to be tackled in AudienceTile instead).

It's also the case that if we implement this issue before #8726, then #8726 itself will be impacted and require additional work that we haven't accounted for in its IB.

In terms of execution efficiency I think the refactor specced in #8726 would be better done first because it will simplify the data extraction, and lead to a fairly straightforward implementation for this issue, whereas if we implement this issue first we'll add complexity to the data extraction side of things which will be a bit harder to subsequently refactor in #8726.

So, as a result I've added #8726 as a blocker to this issue, and would suggest revisiting the IB here with this in mind.

Here are a few additional points on the current IB:

  • A minor one, I'd suggest a couple of renames:
    • isSiteKitAudiencePartial -> isSiteKitAudiencePartialData
    • siteKitAudiencesResourceNames -> siteKitAudienceResourceNames, if we keep the variable. It's not entirely clear what this variable will be used for, so please can you either clarify or remove this from the spec?

  • [ ] While determining isAudiencePartialData, check if the audienceResourceName belongs to "new visitors" or "returning visitors".
  • This is OK, but it could be worth a mention of the isSiteKitAudience() selector which we can use for an easy check here.

  • [ ] If property is not in partial state or audience is non Site-Kit audience, and in partial state, then return true.
  • It looks like the "or" here should be an "and", i.e. "If property is not in partial state and the audience is non Site-Kit audience"...

techanvil avatar May 31 '24 10:05 techanvil

Thank you @techanvil .

As we have pretty much implemented #8484 and #8726 is also in CR, I've written the IB for this one. Requesting you to kindly review the same.

Thanks

ankitrox avatar Jun 25 '24 10:06 ankitrox

Thanks @ankitrox! The IB is most of the way there. A couple of smallish points:

  • The audienceResourceName property is the name of an audience and looks something like properties/433726123/audiences/8044127456. The IB has it mixed up with audienceSlug which will have the values new-visitors, returning-visitors etc.
    • This means that in AudienceTilesWidget we'll need to do an extra lookup to retrieve the audienceSlug for an audience rather than comparing audienceResourceName directly as specified.
    • In AudienceTiles we can use the isSiteKitAudience() selector which accepts an audienceResourceName argument.
  • Please update AudienceTiles to be AudienceTilesWidget which is the name of the component.

techanvil avatar Jun 27 '24 11:06 techanvil

Thank you @techanvil . I have updated the IB for the points mentioned.

Assigning to you for review.

ankitrox avatar Jun 27 '24 12:06 ankitrox

Thanks @ankitrox! One more thing -

Inside the function call the getAvailableAudiences selector and map the audienceResourceName to audienceSlug to form a new list to lookup in the next step mentioned.

I would suggest we can lift this out the function so we create a map of audienceResourceName to audienceSlug within the body of the render function. Then we can avoid iterating over availableAudiences multiple times, and we can then potentially also use this map to identify the audiences when removing them from configuredAudiences instead of having to call isSiteKitAudience() for each audience.

techanvil avatar Jun 27 '24 14:06 techanvil

Thank for the update, @ankitrox! The IB LGTM. :white_check_mark:

techanvil avatar Jul 01 '24 11:07 techanvil

Hi @ankitrox, in light of the fact we're reverting the change for this issue's dependency https://github.com/google/site-kit-wp/issues/8726, please can you review the IB for this issue and adjust it as necessary?

techanvil avatar Jul 05 '24 15:07 techanvil

Hi @techanvil ,

Thanks for the heads up regarding revert of #8726 . I've adjusted the IB according to the current state of AudienceTilesWidget and AudienceTile component.

Assigning over to you for review.

Thanks.

ankitrox avatar Jul 09 '24 05:07 ankitrox

Thanks @ankitrox. The IB was mostly looking good - I've given it a bit of a tweak, fixing a couple of plural names and some minor grammatical errors, and making the logic more explicit in a couple of places. I've also bumped the estimate to a 19 to make some allowances for testing.

Please review the amended IB and if you're happy this can go straight to the EB.

In the meantime I'll give this the old IB :white_check_mark:.

techanvil avatar Jul 09 '24 10:07 techanvil

Hey @ankitrox, I've sent this back to you in IB for an update to reuse the selectors we are defining in https://github.com/google/site-kit-wp/issues/8923.

techanvil avatar Jul 11 '24 10:07 techanvil

Thanks @techanvil

Once #8923 IB is approved, I will update the IB for this one.

ankitrox avatar Jul 12 '24 16:07 ankitrox

Thanks @ankitrox! The IB for #8923 has now been approved, so this should be good for an update.

techanvil avatar Jul 19 '24 11:07 techanvil

Thanks @ankitrox, that's great; note that I also made a couple of minor tweaks to the IB.

Once again, this IB LGTM. :white_check_mark:

techanvil avatar Jul 25 '24 15:07 techanvil

Hussain flagged this will go over the estimate a bit due to increased complexity.

ivonac4 avatar Sep 10 '24 13:09 ivonac4

QA Update: ✅

Verified:

  • Audience Tiles for new visitors and returning visitors display the data from the newVsReturning dimension. I ran through the report in GA4 and compared this with data for 7, 14, 28 and 90 days.
  • Audience Tiles for new visitors and returning visitors are not displaying the partial data badge for the report.
  • The Top content by pageviews sections has the partial data badge displayed if in partial data state.
  • The two errors (full and single) appeared as expected.. I tested this on another live site, and also checked to make sure that new and returning visitors functioned the same with different setup in Tweak extension. See screenshots.
Screenshots

image image image image image

wpdarren avatar Sep 18 '24 07:09 wpdarren