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.22k stars 278 forks source link

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

Open techanvil opened 7 months ago

techanvil commented 7 months ago

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

Implementation Brief

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

Test Coverage

QA Brief

Prerequisites

Data Verification

Testing the Error State

Changelog entry

techanvil commented 7 months ago

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

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

techanvil commented 3 months ago

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:


  • [ ] While determining isAudiencePartialData, check if the audienceResourceName belongs to "new visitors" or "returning visitors".

  • [ ] If property is not in partial state or audience is non Site-Kit audience, and in partial state, then return true.
ankitrox commented 2 months ago

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

techanvil commented 2 months ago

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

ankitrox commented 2 months ago

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

Assigning to you for review.

techanvil commented 2 months ago

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

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

techanvil commented 2 months ago

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?

ankitrox commented 2 months ago

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.

techanvil commented 2 months ago

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 commented 1 month ago

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.

ankitrox commented 1 month ago

Thanks @techanvil

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

techanvil commented 1 month ago

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

techanvil commented 1 month ago

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: