Add enhanced Thank with Google setting validation

[felixarntz]

The Thank with Google settings should be more appropriately validated in JS.

---

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

Acceptance criteria

• Iterating on Add foundation for correct Thank with Google settings #5366, the JS setting validation for the TwG module should be enhanced to be more strict and specific to the actual settings:
  • colorTheme must be a string and one of the following values (use an array already since this will later become more than 1 value):
    • blue
  • buttonPlacement must be a string and one of the following values:
    • dynamic_low
    • dynamic_high
    • static_auto
    • static_above-content
    • static_below-content
    • static_below-first-paragraph
  • buttonPostTypes must be a list/array with one or more non-empty string values
• The TwG Settings class should be updated to implement its sanitize_callback similar to other modules, essentially stripping out invalid values and sanitizing new values
  • The get method should be enhanced to ensure the buttonPostTypes key only includes valid public post type slugs (a.k.a. names)
    • This same filtering should be applied in the sanitize_callback as well

Implementation Brief

In assets/js/modules/thank-with-google/util/validation.js:

• Define validColorThemes array which will contain the valid color theme values from the AC.
• Define validButtonPlacements array which will contain the valid button placement values from the AC.
• Update the isValidColorTheme function to make sure the colorTheme is a validColorThemes.
• Update the isValidButtonPlacement function to make sure the buttonPlacement is a validButtonPlacements.

In Google\Site_Kit\Modules\Thank_With_Google\Settings class:

• Implement get_sanitize_callback.
  • get all publicly available post types available in the site using get_post_types function with array( 'public' => true ) args.
  • Make sure that the buttonPostTypes only contains valid public post types and filter out the rest.
  • Also make sure that the buttonPlacement and colorTheme values are also valid as par AC.
• Implement get method that also makes sure no invalid settings are returned.
  • This logic should be similar to the previous get_sanitize_callback. We can extract out the filtering/validation logic and use those in both places.

Test Coverage

• Add Tests for the enhanced Thank_With_Google\Settings.
• Update any failing jest or storybook tests.

QA Brief

This is hard to QA without actual Settings Edit UI being completed. For now, is would be adequate for an engineer to make sure the related tests are correct and try to set the settings via browser console.

Changelog entry

• Improve validation for Thank with Google settings.

[aaemnnosttv]

Thanks @kuasha420 – a few comments for you here:

• Further simplify the output by only keeping the returned post type slug as a plain array of strings.

This is the default output of the function. It calls them "names" which is consistent with the corresponding property on the WP_Post_Type class – the display name is the label. However, we need both the slug/name and the label as a displayName similar to how we handle WP Roles for dashboard sharing in the choices for the input.

This issue is specific to changes to the settings validation though, so we may need another issue for this if we don't have one already. We shouldn't need this yet (see below)

• Inline the isValidButtonPostTypes validation function to make sure all buttonPostTypes values are a isValidButtonPostTypes.

I'm not sure we need to inline this, why not simply add publicPostTypes as a parameter? More importantly though –

Regarding valid post types, consider this situation:

• User adds plugin that introduces a new public post type
• User enables the TwG button on the plugin's post type
• User later disables/uninstalls the plugin
• User attempts to save settings for TwG

In this scenario, the saved value would include invalid post types meaning that if this validation were done before saving, the user would be blocked from making changes.

In this case, I think the client side validation related to submit changes should remain more basic, essentially ensuring the shape is correct or enforcing required values. Then on the server side, we can filter out any invalid post types in the sanitize_callback. We could also do the same filtering on the saved value in the 'option_' . self::OPTION filter to ensure any invalid post types are not returned by the setting.

[kuasha420]

@aaemnnosttv Your comment makes sense. So, should we just check that the values inside the buttonPostTypes array is non empty string and check the settings serverside?

Regarding the Public Post Types, if answer to the earlier question is yes, then we will not need it here. But it will be needed for #5456, should I add that point in the IB for #5456 or we need a separate issue?

Cheers!

[aaemnnosttv]

So, should we just check that the values inside the buttonPostTypes array is non empty string and check the settings serverside?

Yes, you got it 👍

Regarding the Public Post Types, if answer to the earlier question is yes, then we will not need it here. But it will be needed for #5456, should I add that point in the IB for #5456 or we need a separate issue?

This would be infrastructure needed for implementing the UI so while we could include it there, I think it makes for more clearly defined scope to keep them separate. Please open a new issue for this and then we can update #5456 to depend on it.

The server side settings sanitization and filtering can be done as part of this issue then, I'll update the ACs to include this.

[hussain-t]

@felixarntz, @aaemnnosttv, @kuasha420, should we rename the buttonPlacement to ctaPlacement in the AC and IB, which was introduced in #5533? Or at least it would be good to add a note mentioning 5533 in the IB.

[aaemnnosttv]

Good shout @hussain-t – I'm going to make #5533 dependent on this issue as well so that the changes here will be updated in that one as it already depends on another dependency of this one. No changes should be needed here.

[aaemnnosttv]

IB ✅

[tofumatt]

As per the QA brief here, there's nothing directly to QA yet. Given this already has unit tests covering the logic implemented, moving directly to Approval.