`validateHaveSettingsChanged` Should Be Updated To Align With Existing Semantic Convention about site-kit-wp

[tofumatt]

The ACs needed more clarity around what the outcome should be here, so mentioning the explicit selector names and where they'd come from I think clears things up.

Moving to IB 🙂

[binnieshah]

@tofumatt any chance you can review this one please?

[tofumatt]

@zutigrm Which instances of validateHaveSettingsChanged should be changed here?

I see at least two separate instances:

1. This one in the Ads datastore:

   site-kit-wp/assets/js/modules/ads/datastore/settings.js

   Line 97 in 81fdd2f

   export function validateHaveSettingsChanged( select, state, keys ) {

2. This one in the Analytics datastore:

   site-kit-wp/assets/js/modules/analytics-4/datastore/settings.js

   Line 304 in 81fdd2f

   export function validateHaveSettingsChanged( select, state, keys ) {

There's the default created for this argument:

site-kit-wp/assets/js/googlesitekit/data/create-settings-store.js

Line 85 in 81fdd2f

validateHaveSettingsChanged = makeDefaultHaveSettingsChanged(),

, but I don't think that is relevant here.

properly validate selector for function throw, and use returned selectors

What does this mean exactly? It seems like it means that it should try/catch a callback and return its value if it doesn't catch, but I'm not clear on what this means. Are there other functions you can supply as examples?

[zutigrm]

@tofumatt I see your point, I will make it clearer, it is basically about how the function is used in the store. Will update the AC

[zutigrm]

@tofumatt AC updated

[tofumatt]

switching the conversion tracking toggle on edit settings screen of Ads and Analytics module should change be detected in the save button

Can you rephrase this? I don't understand what this means specifically.

---

The IB here doesn't go into any details about what should change really, aside from what's already outlined in the ACs. Can you go into at least a bit of conceptual detail on how this would be implemented/how it should function? Right now the IB just reads like "implement the ACs", which isn't specific enough to review.

For example, what should be tested in the tests for these selectors? What is the behaviour we want to test? That would be good to outline in the IB.

[zutigrm]

@tofumatt Thanks. Since it straightforward and AC covered good part of the details with pointing to the example, I thought it won't need much details in IB as well. I updated IB now to have more info and expanded on test section, let me know what you think

[tofumatt]

IB ✅

[mohitwp]

QA Update ⚠️

• Tested on dev environment.
• Verified settings edit screen for Ads, Analytics, Search Console, Tag manager and AdSense Module.
• Verified save/confirm changes button behaves as expected/before.
• Verified the toggle for conversion tracking on Ads and Analytics module when 'conversionInfra' Feature flag is enabled. It is working as expected.

@zutigrm I completed QA according to QAB but AC of this ticket is more technical. Should we tag QA:Eng to verify points mentioned under AC ?

Analytics module

Recording.1143.mp4

Ads module

Recording.1144.mp4

Conversion tracking Toggle

Recording.1145.mp4

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified settings edit screen for Ads, Analytics, Search Console, Tag manager and AdSense Module.
• Verified save/confirm changes button behaves as expected/before.
• Verified the toggle for conversion tracking on Ads and Analytics module when 'conversionInfra' Feature flag is enabled. It is working as expected.

Analytics module

Recording.1143.mp4

Ads module

Recording.1144.mp4

Conversion tracking Toggle

Recording.1145.mp4