Skip to content
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 REST and datastore APIs for audience caching #8486

Closed
38 tasks
techanvil opened this issue Apr 5, 2024 · 6 comments
Closed
38 tasks

Add REST and datastore APIs for audience caching #8486

techanvil opened this issue Apr 5, 2024 · 6 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Squad 2 (Team M) Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Apr 5, 2024

Feature Description

Add REST and datastore APIs for audience caching.

See audience caching in the design doc.


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

Acceptance criteria

Overview

  • The list of audiences retrieved from the audiences API should be cached in the DB.
  • The audiences should not be cached in their original format, rather each object should be mapped to an object with the following properties:
    • name:
      • The name property from the original audience object.
    • displayName:
      • The displayName property from the original audience object.
    • description:
      • The description property from the original audience object.
    • audienceType:
      • This is a field to indicate the audience type. It will have one of the following values:
        • DEFAULT_AUDIENCE:
          • Indicates the audience is a default audience.
          • Note that default audiences are a special case and their displayName field can’t be updated; this field can therefore be used to identify the default audiences.
        • SITE_KIT_AUDIENCE:
          • Indicates the audience is a Site Kit-created audience.
          • Note that the displayName and description audience fields are mutable for custom audiences. Therefore in order to identify the Site Kit-created audiences we can compare the immutable filterClauses field, within which we embed a descriptive identifier (see audience configuration in the design doc).
        • USER_AUDIENCE:
          • Indicates the audience is a user-created audience, i.e. not a default or Site Kit-created audience.
    • audienceSlug:
      • This is a field to identify the audience itself. It should be set for default and Site Kit-created audiences and can be omitted for user defined audiences which we don't need to distinguish further. It will have one of the following values:
        • all-users:
          • Set for the All Users default audience. Tracking slugs for the default audiences ensures we can continue to identify them audience if their name ever changes in a future Analytics update.
        • purchasers:
          • Set for the Purchasers default audience.
        • new-visitors:
          • Set for the "new visitors" Site Kit-created audience. This can be identified by the new_visitors value which is embedded in filterClauses as per the description for SITE_KIT_AUDIENCE.
        • returning-visitors:
          • Set for the "returning visitors" Site Kit-created audience, identified by the embedded returning_visitors value as above.
  • The cached audiences should be stored in a new availableAudiences setting in the Analytics module.
  • The last-synced time should be stored in a new availableAudiencesLastSyncedAt setting.

REST API

The following REST API endpoint should be added to the Analytics module:

  • POST:sync-audiences
    • Retrieve the list of audiences from the properties.audiences.list endpoint, map the list of objects as described above, store the result in the availableAudiences setting, and return it.
    • Update the value of availableAudiencesLastSyncedAt with the current timestamp.

Datastore API

The following datastore selectors should be added to the Analytics module:

  • getAvailableAudiences()
    • Return the list of available audiences which is stored in the availableAudiences setting.
  • getAvailableAudiencesLastSyncedAt()
    • Return the value of the availableAudiencesLastSyncedAt setting.
  • syncAvailableAudiences()
    • Invoke a sync of the audiences via the POST:sync-audiences endpoint, and return the result.

Implementation Brief

  • In includes/Modules/Analytics_4/Settings.php:
    • Update the get_default method to include two new module settings: availableAudiences which defaults to null, and availableAudiencesLastSyncedAt which defaults to 0.
    • Update the get_sanitize_callback to include sanitization for the above new module settings:
      • For availableAudiences, it should be verified that the value is an array, otherwise, it should be reset to its default.
      • For availableAudiencesLastSyncedAt, it should be verified that the value is an integer, otherwise, it should be reset to its default.
  • In assets/js/modules/analytics-4/datastore/base.js:
    • Include availableAudiences and availableAudiencesLastSyncedAt as setting slugs.
  • In includes/Modules/Analytics_4.php:
    • Update the get_datapoint_definitions method to include a new POST:sync-audiences data point if the audienceSegmentation feature flag is enabled. Its service should be analyticsaudiences.
    • Update the create_data_request method to include a case for POST:sync-audiences. It should be a duplicate of the existing GET:audiences case where it calls PropertiesAudiences::listPropertiesAudiences.
    • Update the parse_data_response to include a case for POST:sync-audiences. It should use $response->getAudiences() to get the list of audience, and then map the audiences in a way where it returns an array of objects, with each object being an audience but mapped in a structure as described in the ACs.
      • The mapping and data handling can be done within a new separate method, e.g. set_available_audiences.
      • To set the audienceType property in each object, the following logic can be used:
        • DEFAULT_AUDIENCE: If the displayName property of the audience is either All Users or Purchasers.
        • SITE_KIT_AUDIENCE: If the original audience object has a filterClauses property, where there is a groupID field, value for which includes the text created_by_googlesitekit.
        • USER_AUDIENCE: If the audience is neither a DEFAULT_AUDIENCE nor a SITE_KIT_AUDIENCE.
      • To set the audienceSlug property in each object, the following logic can be used:
        • all-users: If the displayName property of the audience is All Users.
        • purchasers: If the displayName property of the audience is Purchasers.
        • new-visitors: If the original audience object has a filterClauses property, where there is a groupId field, value for which includes the text created_by_googlesitekit:new_visitors.
        • returning-visitors: If the original audience object has a filterClauses property, where there is a groupId field, value for which includes the text created_by_googlesitekit:returning-visitors.
      • $this->get_settings()->merge() should be used to save:
        • the resulting array of objects in the availableAudiences module setting.
        • the current time() in the availableAudiencesLastSyncedAt module setting.
      • Return the array of objects as the data point response.
  • In assets/js/modules/analytics-4/datastore/audiences.js:
    • Add a new action named syncAvailableAudiences that should make an API call to the new POST:sync-audiences endpoint.
      • Add a relevant fetch store for the API call.

Test Coverage:

  • In tests/phpunit/integration/Modules/Analytics_4/SettingsTest.php:
    • Update the list of module settings to ensure tests don't fail due to the addition of the new module settings.
  • In tests/phpunit/integration/Modules/Analytics_4Test.php:
    • Update the list of module settings to ensure tests don't fail due to the addition of the new module settings.
    • Update the list of data points to ensure tests don't fail due to the addition of the new data point.
    • Add a test case test_sync_audiences to verify the behaviour of the new POST:sync-audiences endpoint.
      • Verify that the relevant availableAudiences and availableAudiencesLastSyncedAt module settings are populated, and the audiences are returned.
      • Verify that the saved and returned audiences are in the desired format.
  • In assets/js/modules/analytics-4/datastore/audiences.test.js:
    • Add test coverage for the new syncAvailableAudiences action.

QA Brief

  1. Set up some audiences at https://analytics.google.com/.
    i. Follow this part in the design doc to set up "New visitors" and "Returning visitors" audiences (these are SK audiences, which will later be created by SK itself).
    ii. Also, create a different audience with other random details.
  2. Set up SK with Analytics, and connect the same property with the set audiences.
  3. Go to the SK dashboard and run the following script in the browser console to observe the Analytics module settings:
googlesitekit.data.select( 'modules/analytics-4' ).getSettings()

Verify that the returned object includes availableAudiences & availableAudiencesLastSyncedAt settings with null and 0 default values respectively.
4. Run the following in the browser console:

googlesitekit.data.dispatch( 'modules/analytics-4' ).syncAvailableAudiences()
  1. Verify that a network request is made to POST:sync-audiences, and its response should return an array of audiences that you see in https://analytics.google.com/.
  2. Verify that the items in the array have the same structure as described in the ACs.
  3. Verify that the audienceType and audienceSlug properties are set correctly according to the ACs.
  4. Refresh the page and perform step 3 again. Verify that the settings availableAudiences & availableAudiencesLastSyncedAt have been populated correctly.

Changelog entry

  • Add REST and datastore APIs for audience caching.
@techanvil techanvil added Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature labels Apr 5, 2024
@nfmohit nfmohit self-assigned this Apr 5, 2024
@ivonac4 ivonac4 added Squad 2 (Team M) Issues for Squad 2 Next Up Issues to prioritize for definition labels Apr 5, 2024
@nfmohit nfmohit removed their assignment Apr 8, 2024
@techanvil techanvil self-assigned this Apr 8, 2024
@techanvil
Copy link
Collaborator Author

techanvil commented Apr 8, 2024

Hey @nfmohit, nice work here on the IB.

Two points occur to me.

  • Reviewing the IB has made me realise that we've missed something in the design doc/AC. We need to include a way to identify the Site Kit-created audiences, as for example we need to show different tooltips for the new vs returning visitors audiences. I've added an audienceSlug property to the AC (the design doc is now approved so I've mentioned this in the changes during engineering section there rather than update the config in the doc, although I've added a comment there too). It also occurs to me it would be useful to provide slugs for the default audiences, to insulate us from any potential future changes to the names, and avoid having to directly test for All Users and Purchasers everywhere, so I've included that too in the AC. Please can you take a look at the AC, see/lmk what you think, and update the IB if you like the change.
  • A minor point on the testing front: you've specified including a test for set_available_audiences() if that exists. However, there's no reason to make this method public as it won't be called outside of its class. I know we do test private/protected methods in some cases, but this is not really best practice, and I think we should keep this to a minimum and only where it's absolutely necessary for some reason. From a testing perspective, private/protected are essentially "black box" implementation details that are subject to change whereas the public interface is what is "known", should be more stable and is what we should be testing. We can test the behaviour of set_available_audiences() via the test for the POST:sync-audiences endpoint so there's no need to explicitly test the underlying private/protected method.

Cc @kuasha420 @aaemnnosttv

@techanvil techanvil assigned nfmohit and unassigned techanvil Apr 8, 2024
@nfmohit
Copy link
Collaborator

nfmohit commented Apr 8, 2024

Thank you for the kind review, @techanvil.

  • Reviewing the IB has made me realise that we've missed something in the design doc/AC. We need to include a way to identify the Site Kit-created audiences, as for example we need to show different tooltips for the new vs returning visitors audiences. I've added an audienceSlug property to the AC (the design doc is now approved so I've mentioned this in the changes during engineering section there rather than update the config in the doc, although I've added a comment there too). It also occurs to me it would be useful to provide slugs for the default audiences, to insulate us from any potential future changes to the names, and avoid having to directly test for All Users and Purchasers everywhere, so I've included that too in the AC. Please can you take a look at the AC, see/lmk what you think, and update the IB if you like the change.

I think this makes sense. I have updated the IB accordingly.

  • A minor point on the testing front: you've specified including a test for set_available_audiences() if that exists. However, there's no reason to make this method public as it won't be called outside of its class. I know we do test private/protected methods in some cases, but this is not really best practice, and I think we should keep this to a minimum and only where it's absolutely necessary for some reason. From a testing perspective, private/protected are essentially "black box" implementation details that are subject to change whereas the public interface is what is "known", should be more stable and is what we should be testing. We can test the behaviour of set_available_audiences() via the test for the POST:sync-audiences endpoint so there's no need to explicitly test the underlying private/protected method.

I agree. I have removed this point from the IB.

Please let me know if this looks good now, thank you!

@nfmohit nfmohit assigned techanvil and unassigned nfmohit Apr 8, 2024
@techanvil
Copy link
Collaborator Author

That's great, thanks for the update @nfmohit. IB LGTM! ✅

@techanvil techanvil removed their assignment Apr 9, 2024
@nfmohit nfmohit self-assigned this Apr 9, 2024
@ivonac4 ivonac4 removed the Next Up Issues to prioritize for definition label Apr 10, 2024
@nfmohit nfmohit mentioned this issue Apr 13, 2024
18 tasks
@nfmohit nfmohit removed their assignment Apr 13, 2024
@techanvil techanvil mentioned this issue Apr 15, 2024
18 tasks
@techanvil techanvil assigned techanvil and nfmohit and unassigned techanvil Apr 16, 2024
@nfmohit nfmohit removed their assignment Apr 16, 2024
@techanvil techanvil assigned nfmohit and techanvil and unassigned techanvil and nfmohit Apr 17, 2024
techanvil added a commit that referenced this issue Apr 17, 2024
@techanvil techanvil removed their assignment Apr 17, 2024
@wpdarren wpdarren self-assigned this Apr 23, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 23, 2024

QA Update: ⚠️

@nfmohit this is looking good based on the QAB, but I do have one observation. When I ran googlesitekit.data.select( 'modules/analytics-4' ).getSettings()for the 2nd time, I saw that the availableAudiences was now populated with the audiences, but, availableAudiencesLastSyncedAt is still showing as 0 - should it still be zero?

@nfmohit
Copy link
Collaborator

nfmohit commented Apr 23, 2024

Hi @wpdarren.

This is what I see on my end:
image

Just to confirm, did you refresh the page before running googlesitekit.data.select( 'modules/analytics-4' ).getSettings() for the second time? The availableAudiencesLastSyncedAt setting does not update in the realtime in the client-side, so it needs a refresh to update the value from the server.

Thanks!

@wpdarren
Copy link
Collaborator

QA Update: ✅

@nfmohit Ah, I thought I had refreshed but just ran through the test again and that did the trick! Thank you.

Verified:

  • The returned object includes availableAudiences & availableAudiencesLastSyncedAt settings with null and 0 default values respectively.
  • A network request is made to POST:sync-audiences, and its response returns an array of audiences that you see in my Analytics account.
  • The items in the array have the same structure as described in the ACs.
  • The audienceType and audienceSlug properties are set correctly according to the ACs.
  • The settings availableAudiences & availableAudiencesLastSyncedAt have been populated correctly.
Screenshots

image
image
image
image
image
image
image
image
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Squad 2 (Team M) Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants