Add the datastore API for determining audience type (default, SK-created or user-created)

[techanvil]

Feature Description

Add the datastore API for determining the type of an audience. Specifically that means the following selectors:

• isDefaultAudience( audienceResourceName )
• isSiteKitAudience( audienceResourceName )
• isUserAudience( audienceResourceName )

Note that the criteria for determining the SK vs user audience type is still to be confirmed at the time of writing, but the shape of the API should not change. This implies that the AC can be drafted immediately, but the IB will need to wait until the aforementioned criteria have been fully determined.

See datastore configuration in the design doc for more details.

---

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

Acceptance criteria

• The following selectors should be added to the audiences datastore partial within the modules/analytics-4 datastore:
  • isDefaultAudience( audienceResourceName ): Returns true if the audience is one of the two Analytics created audiences, identified by their names, Purchasers or All Users. Otherwise false or undefined when making an API request.
  • isSiteKitAudience( audienceResourceName ): Returns true if the audience is one of the two Site Kit created audiences, identified by their immutable filterClauses field.
  • isUserAudience( audienceResourceName ): Returns true if the audience is neither a Default Audience nor a Site Kit created audience.

Implementation Brief

• [ ]

Test Coverage

QA Brief

Changelog entry

[jimmymadon]

@techanvil - Added some preliminary ACs here. But I think moving this along without defining criteria/definitions of what differentiates a SiteKit audience would be incomplete. It also can't be picked up for an IB so wouldn't quite make sense. So will leave this with me until we come to a conclusion. I've added a tiny suggestion to the thread in the document too.

[techanvil]

Thanks @jimmymadon, sounds sensible. Your suggestion on the design doc SGTM too.

[techanvil]

@jimmymadon I'm moving this to the Backlog to help clarify which issues need to be finalised in the design doc.

[jimmymadon]

Unassigning myself from this one due to my upcoming break. c.c @ivonac4 @bethanylang @techanvil

[techanvil]

As we've now finalised the definition for the audience configuration, along with the criteria for differentiating the audience types, I've completed the AC and moved this to IB.

[techanvil]

Thanks for the IB @nfmohit. This LGTM.

It's worth noting that due to the audience caching aspect of the feature, which is currently being finalised in the design doc, we're going to be switching out the use of getAudiences() for getAvailableAudiences().

However, I think we can and should still move forward with implementing this issue as it stands. Switching out the use of getAudiences() for getAvailableAudiences() should be pretty trivial and we can cover it in the issue which will created to address other existing uses of getAudiences(). And, moving ahead with this issue will unblock https://github.com/google/site-kit-wp/issues/8138 which is a significant point of integration so I feel it's worthwhile moving ahead with this one as it stands.

So, TL;DR that's an IB :white_check_mark:, but I just wanted to add a bit of extra context here for the sake of visibility.

[techanvil]

Hmm having iterated further on the audience caching section of the design doc, it's worth noting that the definitions for the isDefaultAudience(), isSiteKitAudience() and isUserAudience() in the doc have been (provisionally, at the time of writing, but most likely) tweaked a bit, so bear that in mind if and when comparing the AC or implementation to the doc.

This does imply a bit more involvement to the switch mentioned in my previous comment; however, I think we can still proceed to execute on this issue in the meantime, in the interest of keeping issues downstream of this issue unblocked while we continue to define the caching related updates.

[techanvil]

@nfmohit, sorry to do a bit of a 180 here but as the audience caching infrastructure issue #8486 has now been defined with ACs, and considering where we're at with regard this issue and those downstream (i.e. it's not been picked up for implementation yet), it will be more efficient to implement that issue first and rework the IB for this one. Therefore I've made #8486 a dependency for this issue, which I've moved back to IB.

[nfmohit]

@nfmohit, sorry to do a bit of a 180 here but as the audience caching infrastructure issue #8486 has now been defined with ACs, and considering where we're at with regard this issue and those downstream (i.e. it's not been picked up for implementation yet), it will be more efficient to implement that issue first and rework the IB for this one. Therefore I've made #8486 a dependency for this issue, which I've moved back to IB.

Thank you @techanvil. I've updated the IB to reflect the usage of the audience caching mechanism. Please let me know if this looks good now, thank you!

[techanvil]

Thanks @nfmohit, the IB LGTM! :white_check_mark:

Please can you add an estimate, and move it to the EB. Cheers!

[nfmohit]

Thanks @nfmohit, the IB LGTM! ✅

Please can you add an estimate, and move it to the EB. Cheers!

Oops, sorry! Added an estimate of 7 and moved to EB, thanks @techanvil !

[kelvinballoo]

QA Update ⚠️

Tested this and it's looking good overall, though I'll need some guidance:

• I ran through with the steps and was able to follow the QAB mostly. The scripts ran were as expected as per the following screenshot.

  

• When all the values are right, the selectors will return 'True' for valid values of matching default/site kit/user audience. If the property ID and audience IDs are wrong, then the selector returns 'False'

• @techanvil I want to ask more about the last point : Additionally, the selectors should return undefined when run prior to the first audience sync.

  • Does that mean that once I execute the first 2 scripts to sync and get available audience, I won't be able to test this?
  • If let's say I ran all the scripts, how do I confirm this point again? Should I be creating another environment for this?

[techanvil]

Hi @kelvinballoo, thanks for asking.

• @techanvil I want to ask more about the last point : Additionally, the selectors should return undefined when run prior to the first audience sync.
  • Does that mean that once I execute the first 2 scripts to sync and get available audience, I won't be able to test this?

That's right - although, in practice you should be able to test this by checking the return values the first time around. I realise in hindsight I should have mentioned this point toward the start of the QAB rather than as a note at the end. Sorry about that!

• If let's say I ran all the scripts, how do I confirm this point again? Should I be creating another environment for this?

You don't need to create a new environment. Getting into the details a bit - as a convention, Site Kit's selectors return undefined to indicate that their data is loading. The three selectors mentioned in the AC all internally call the getAvailableAudiences() selector to retrieve the availableAudiences Analytics module setting.

So, clearing the Analytics module settings is the easiest way to reset things so you can retest this point. You can achieve this in a number of ways, including:

• Reset Site Kit.
• Disconnect Analytics.
• Clear the googlesitekit_analytics-4_settings option from the database, which in turn can be done in a couple of ways
  • Via the WP CLI: wp option delete googlesitekit_analytics-4_settings
  • Directly in the DB - if using InstaWP, you can access the database through the provided web interface, and delete the row from the wp_options table where option_name = 'googlesitekit_analytics-4_settings'.

Please let me know if you'd like any additional info on the above, if there are aspects you're not yet familiar with I'd be happy to jump on a call to help clarify things or maybe @wpdarren could do the same?

[kelvinballoo]

QA Update ✅

Thanks for the updates around this and the update to the QAB @techanvil .

Based on this updated QAB, we are good to go on this ticket as the scripts ran were as expected.

Moving this ticket to 'Approval'.