site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard
verified publisher icon google

`validateHaveSettingsChanged` Should Be Updated To Align With Existing Semantic Convention

Open zutigrm opened this issue 1 year ago • 10 comments

Feature Description

validateHaveSettingsChanged should be used with createValidationSelector so it can be properly validated agains functions throw, and also test coverage for this utility should be added. More details in the comment here

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

Acceptance criteria

  • The haveSettingsChanged selector in assets/js/googlesitekit/data/create-settings-store.js should utilise the createValidationSelector utility function to properly validate the validateHaveSettingsChanged function.
  • This means the selectors in assets/js/googlesitekit/data/create-settings-store.js should be updated to have both haveSettingsChanged and __dangerousHaveSettingsChanged selectors, as done in other files that use createValidationSelector.
    • The haveSettingsChanged selector should be the safeSelector returned from createValidationSelector( validateHaveSettingsChanged ), while the __dangerousHaveSettingsChanged should be the dangerousSelector returned from createValidationSelector( validateHaveSettingsChanged ).
  • Test coverage for validateHaveSettingsChanged should be added.
  • Test coverage should ensure validateHaveSettingsChanged is used by both selectors (see https://github.com/google/site-kit-wp/blob/3b3ebf91cdb01a0670e30a5cfd2afba0778a9a11/assets/js/googlesitekit/modules/create-submit-changes-store.test.js#L152-L165 for an example of how to test this).

Implementation Brief

  • [ ] In assets/js/googlesitekit/data/create-settings-store.js
    • Replace the current implementation of haveSettingsChanged selector https://github.com/google/site-kit-wp/blob/3b3ebf91cdb01a0670e30a5cfd2afba0778a9a11/assets/js/googlesitekit/data/create-settings-store.js#L292-L296 which uses passed prop validateHaveSettingsChanged directly without validating it, to follow the existing semantic convention we have in other places, by utilising the createValidationSelector which will catch any errors thrown by the selector.
    • Pass validateHaveSettingsChanged prop to the createValidationSelector util function, and extract returned selectors
      • createValidationSelector will return safeSelector - which catches exceptions, and dangerousSelector which does not catch exceptions (eg. will throw with errors). Use alias haveSettingsChanged for safeSelector and __dangerousHaveSettingsChanged for dangerousSelector and add them to the main selectors object . You can see an example usage here https://github.com/google/site-kit-wp/blob/3e1657a3a3a10572812617bc45e91de4752bdac1/assets/js/googlesitekit/modules/create-submit-changes-store.js#L146-L153
  • Ensure that the selector works properly:
    • Conversion Tracking Toggle: When toggling the conversion tracking toggle on the Settings > Edit screen of the Ads and Analytics module, this change should be detected in the "Save" button. The "Save" button should change its label accordingly same as for any other option belonging to the module (conversion tracking toggle has its own datastore, but should behave same as if it was part of the current module datastore)
    • Other Options: All other options outside the toggle should continue to work as expected, with no changes in their behaviour; editing them should be detected by the save button.

Test Coverage

  • Add tests for haveSettingsChanged. You can use assets/js/googlesitekit/modules/create-submit-changes-store.test.js canSubmitChanges as an example, to verify that haveSettingsChanged and __dangerousHaveSettingsChanged are functions and that they are using validateCanSubmitChanges https://github.com/google/site-kit-wp/blob/3b3ebf91cdb01a0670e30a5cfd2afba0778a9a11/assets/js/googlesitekit/modules/create-submit-changes-store.test.js#L143-L165

QA Brief

  • Setup Site Kit with Ads, Analytics and any other module
  • Verify that on settings edit screens of each module save/confirm changes button behaves as expected/before
  • Changing the toggle for conversion tracking on Ads and Analytics module should also be detected and button should display "confirm changes" label

Changelog entry

  • N/A

zutigrm avatar Jun 19 '24 09:06 zutigrm

@tofumatt any chance you can review this one please?

binnieshah avatar Jun 19 '24 13:06 binnieshah

@zutigrm Which instances of validateHaveSettingsChanged should be changed here?

I see at least two separate instances:

  1. This one in the Ads datastore: https://github.com/google/site-kit-wp/blob/81fdd2fb31d7d9bd529bfcde292754db1d01c00a/assets/js/modules/ads/datastore/settings.js#L97
  2. This one in the Analytics datastore: https://github.com/google/site-kit-wp/blob/81fdd2fb31d7d9bd529bfcde292754db1d01c00a/assets/js/modules/analytics-4/datastore/settings.js#L304

There's the default created for this argument: https://github.com/google/site-kit-wp/blob/81fdd2fb31d7d9bd529bfcde292754db1d01c00a/assets/js/googlesitekit/data/create-settings-store.js#L85, 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?

tofumatt avatar Jun 19 '24 19:06 tofumatt

@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 avatar Jun 20 '24 07:06 zutigrm

@tofumatt AC updated

zutigrm avatar Jun 20 '24 08:06 zutigrm

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 🙂

tofumatt avatar Jun 20 '24 12:06 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.

tofumatt avatar Jun 20 '24 14:06 tofumatt

@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

zutigrm avatar Jun 20 '24 16:06 zutigrm

IB ✅

tofumatt avatar Jun 24 '24 13:06 tofumatt

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

https://github.com/google/site-kit-wp/assets/94359491/4cd61e80-3daa-41ae-9434-b8f337f0725c

Ads module

https://github.com/google/site-kit-wp/assets/94359491/57bdffd8-fd0f-4fab-90f8-ba3ebb1f8244

Conversion tracking Toggle

https://github.com/google/site-kit-wp/assets/94359491/d820759c-123d-43d7-b821-15cc3bfd3361

mohitwp avatar Jul 03 '24 13:07 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

https://github.com/google/site-kit-wp/assets/94359491/4cd61e80-3daa-41ae-9434-b8f337f0725c

Ads module

https://github.com/google/site-kit-wp/assets/94359491/57bdffd8-fd0f-4fab-90f8-ba3ebb1f8244

Conversion tracking Toggle

https://github.com/google/site-kit-wp/assets/94359491/d820759c-123d-43d7-b821-15cc3bfd3361

mohitwp avatar Jul 03 '24 13:07 mohitwp