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.23k stars 284 forks source link

Add `adSenseLinked ` and `adSenseLinkedLastSyncedAt` settings to `modules/analytics-4` data store #8048

Closed tofumatt closed 7 months ago

tofumatt commented 8 months ago

Feature Description

The new settings added in #8047 should be available in the Analytics 4 datastore in JS.


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

techanvil commented 8 months ago

IB :white_check_mark:

hussain-t commented 7 months ago

Hi @tofumatt,

Could you give me some clarifications regarding the implementation details outlined in this ticket? There seems to be a discrepancy between the Feature Description and the AC, alongside some naming conventions and testing requirements:

  1. Discrepancy Between Feature Description and AC:

Feature Description:

We should fetch the AdSense linked accounts retrieved with the REST API route created in https://github.com/google/site-kit-wp/issues/8046 and save them to the new settings added in https://github.com/google/site-kit-wp/issues/8047 using a new action in the Analytics 4 datastore in JS.

Acceptance Criteria:

The adsenseLinked and adSenseLinkedLastSyncedAt settings from https://github.com/google/site-kit-wp/issues/8047 should be available in the MODULES_ANALYTICS_4 data store.

This raises a question: Should we implement a fetch resolver, a selector, and an action to save the settings, as indicated in the Feature Description, or follow the AC and IB, which suggest adding the settings in the base datastore? Based on #8046, I think we should update the AC according to the description.

  1. Naming Convention for adsenseLinked: Should this be renamed adSenseLinked with a capital ‘S’?

  2. Inclusion of Test Coverage in the IB: IB proposes adding test coverage, which seems unnecessary given the current implementation strategy. If we include a fetch resolver and an action according to the Feature Description, adding tests would then be relevant and beneficial.

Thanks!

cc: @aaemnnosttv @nfmohit

tofumatt commented 7 months ago
  1. I've clarified things, but all we need is to add the settings to the JS datastore 😄
  2. Yes, it should be adSenseLinked.
  3. I've clarified the tests changes here, thanks 😄
hussain-t commented 7 months ago

Thanks, @tofumatt

mohitwp commented 7 months ago

QA Update ✅

image

image