Site-kit-wp: Refactor `canSubmitChanges` selectors and tests to use validation utilities

[aaemnnosttv]

Feature Description

In #1984, several enhancements were introduced with the changes made to the Tag Manager datastore, particularly around the canSubmitChanges selector.

This selector often selects many different things and has complex logic for determining if changes can be submitted, ultimately resulting in a boolean result. The problem with this is that it is difficult to accurately test that the expected value is returned for the right reason; particularly in the case of a false return.

For this reason, the logic was extracted to a new internal validateCanSubmitChanges utility function which would simply throw an error for any case where the return value should be false.

Before

https://github.com/google/site-kit-wp/blob/c2693a862016cc4750b04f48c320c7f8527393c7/assets/js/modules/tagmanager/datastore/settings.js#L151-L207

After

https://github.com/google/site-kit-wp/blob/d0604fa339d56dcec1055871f2a13c1a9f816e8a/assets/js/modules/tagmanager/datastore/settings.js#L159-L173
https://github.com/google/site-kit-wp/blob/d0604fa339d56dcec1055871f2a13c1a9f816e8a/assets/js/modules/tagmanager/datastore/settings.js#L188-L257

Additionally, a new createStrictSelect utility was introduced as a kind of decorated registry.select so that "strict" selectors could be used inside validation functions like this. They essentially work the same as normal bound selectors with the exception that they will throw an error if undefined is returned which should only happen when a selector's data is not resolved yet.

This ensures that canSubmitChanges will always return false until all selectors used in the validation function are resolved.

Other noteworthy benefits of this refactoring are closely related:

• Multi-line conditionals simplified into single line validation using invariant
• The ability to add a validation message/reason to a failing condition (via invariant)
• The ability to assert that canSubmitChanges returns false for the expected reason by using validateCanSubmitChanges in tests

---

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

Acceptance criteria

• All stores should implement a consistent canSubmitChanges selector that uses an internal validateCanSubmitChanges function
• All selectors used within validateCanSubmitChanges functions should be "strict" selectors created by createStrictSelect
• All tests for canSubmitChanges should be updated to use the new validation function to ensure that all required data is received/resolved in tests and that false values are returned for the expected reasons

Implementation Brief

• In the assets/js/googlesitekit/data/utils.js file:

  • [x] Create a new function createValidationSelector that looks like this:

  function createValidationSelector( validate ) {
      const safeSelector = createRegistrySelector( ( select ) => ( state, ...args ) => {
          try {
              validate( select, state, ...args );
              return true;
          } catch {
              return false;
          }
      } );
  
      const dangerousSelector = createRegistrySelector( ( select ) => ( state, ...args ) => {
          validate( select, state, ...args );
      } );
  
      return {
          safeSelector,
          dangerousSelector,
      };
  }
  
  



• Update stores of the following modules to use createValidationSelector and createStrictSelect helpers to create canSubmitChanges and __dangerousCanSubmitChanges selectors:
  
  
  
  • [x] [Adsense](https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/adsense/datastore/settings.js#L319)
  
  
  • [x] [Analytics](https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/analytics/datastore/settings.js#L158)
  
  
  • [x] [Optimize](https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/optimize/datastore/settings.js#L123)
  
  
  • [x] [Tag Manager](https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/tagmanager/datastore/settings.js#L166) - this module already has canSubmitChanges refactored but we need to rework it to use the new approach with createValidationSelector helper.
  
  
• Update javascript tests for the following modules to test the __dangerousCanSubmitChanges selector to ensure that all required data is received/resolved in tests and that appropriate errors are thrown for the expected reasons:
  
  
  
  • [x] [Adsense](https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/adsense/datastore/settings.test.js#L249)
  
  
  • [x] [Analytics](https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/analytics/datastore/settings.test.js#L349)
  
  
  • [x] [Optimize](https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/optimize/datastore/settings.test.js#L135)
  
  
  • [x] [Tag Manager](https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/tagmanager/datastore/settings.test.js#L335)
  
  

QA Brief

• No _specific_ testing required, but make sure that the setup flows for the following modules can still be completed as before:
  
  
  
  • AdSense
  
  
  • Analytics
  
  
  • Optimize
  
  
  • Tag Manager
  
  

Changelog entry

• Make canSubmitChanges selectors throughout different module datastores more testable and consistent.

[eugene-manuilov]

@aaemnnosttv IB has been added. I have slightly extended your idea and added the createCatchableSelector helper function to avoid repeating the same code in all modules. Let me know if it works for you, then we would probably need to slightly amend AC as well.

[aaemnnosttv]

@eugene-manuilov I like the idea in general, but I think we can improve it a bit.

The first parameter to the actual selector must be state – this is missing, and rethrow is in its place. Even though we don't really use it, it's needed for rethrow to come from the argument the selector is called with.

Rather than using a parameter for rethrow I think it would be better to have two separate selectors, one that throws and one that doesn't. The same utility could generate both:

function createValidationSelect( validate ) {
    const safeSelector = createRegistrySelector( ( select ) => ( state, ...args ) => {
        try {
            validate( select, ...args );
            return true;
        } catch {
            return false;
        }
    } );
    const dangerousSelector = createRegistrySelector( ( select ) => ( state, ...args ) => {
        validate( select, ...args );
    } );

    return {
        safeSelector,
        dangerousSelector,
    };
}

I chose "createValidationSelect" because validation is true/false. A "catchable" select sounds like a selector that could return anything.

Then when registering in the datastore, we could register them both like so:

selectors.canSubmitChanges = safeSelector;
selectors.__dangerousCanSubmitChanges = dangerousSelector;

I don't know how I feel about adding selectors for the purpose of testing only, but I suppose it could be useful in the app too. I definitely prefer it over _parameters_ for the purpose of testing though.

With that said, the duplication we're avoiding by using an abstraction here is rather minimal (a try/catch in the selector). Given that we're already going to be introducing a utility for generating these selectors in #2136, perhaps this should be a follow-up to that issue.

Thoughts @felixarntz?

[eugene-manuilov]

@aaemnnosttv yes, i like your idea too. IB is updated.

[aaemnnosttv]

@eugene-manuilov – looks good to me but I'd like to have @felixarntz weigh in here since it's closely related to #2136 .

[felixarntz]

The current approach and IB look good to me ✅

[felixarntz]

@aaemnnosttv @eugene-manuilov Actually, I think we should make one minor change here:

The first parameter to the actual selector must be state – this is missing, [...]. Even though we don't really use it, [...]

I think we should probably pass state to the validate function too because it might be useful depending on the implementation. While we currently only use select in all our implementations, there might be simpler cases that can just use state properties to determine the outcome. So I think we should make the validate functions require a signature like validate( select, state, ...args ).

[eugene-manuilov]

Yep, makes sense. IB is updated.

[felixarntz]

@eugene-manuilov IB ✅

Technically this is all good, but I'd suggest for the code to actually write it out like state, ...args instead of just ...args - it results in the same behavior, but writing out state there clarifies that _that_ is the first parameter. I think args makes most sense to be used for the "unpredictable" parameters, but state will _always_ be there, so let's write it out for clarity.

[eugene-manuilov]

@felixarntz agree, it makes sense. Updated IB.

[tofumatt]

QA ✅