site-kit-wp: Adjust verbiage from 'Cancel' to 'Back' where user is presented an expected error message

Bug Description

While testing https://github.com/google/site-kit-wp/issues/433 I noticed during the error flow the verbiage on the lower call to action is labeled ‘Cancel’ although the user’s action was already canceled due to an error.

image

Steps to reproduce

  1. Adjust tag manager permissions after placing snipped on FE
  2. Run through Tag Manager service setup

Notice the expected error occurs and the user is presented a cancel button


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

Acceptance criteria

  • The “Cancel” link in the footer of the module set up wrapper (ModuleSetup) should use alternate text content in the case where the user is blocked from proceeding (such as when there is an existing tag that the user does not have access to)
  • In this case, the link should have the text content “Back”
  • This should be implemented via a new piece of centralized infrastructure that could be used for every module. However, for now it should only be implemented specifically for the AdSense module, when an ad blocker is active and therefore prevents the setup.

Implementation Brief

  • Using assets/js/googlesitekit/data/utils.js,
    • Update createValidationSelector, add a second parameter which is an object with negate : false as default value, which indicates whether to negate the boolean result or not.
      • Instead of returning true or false, return the appopriate boolean value based on negate.
  • Using assets/js/googlesitekit/modules/create-module-store.js,
    • Update createModuleStore:
      • to add another key, validateIsSetupBlocked for any options which is a function to validate whether module setup is terminally blocked from continuing.
      • If requiresSetup is true and validateIsSetupBlocked is defined, call createValidationSelector, with negate: true and add the results to the list of selectors. They should be:
        • isSetupBlocked for the safe selector
        • __dangerousIsSetupBlocked for the unsafe one.
  • All those changes above can be viewed in the following branch: poc/1045-is-setup-blocked
  • Using assets/js/modules/adsense/datastore/base.js,
    • When creating the module store, i.e when calling Modules.createModuleStore, define validateIsSetupBlocked in the options object and using the adsense data store.
      • check if there is an ad blocker active via the isAdBlockerActive selector. If this is the case, throw an error.
  • Create assets/js/components/setup/ModuleSetupFooter.js which renders the same markup within googlesitekit-setup__footer in assets/js/components/setup/ModuleSetup.js with the following changes:
    • It accepts the module prop.
    • Use Grid, Row and Cell for the grid layout.
    • Query whether the setup is blocked via the isSetupBlocked selector. The module store name can be obtained via module.storeName and since not all modules have this selector defined, it can be queried as such:
    const isSetupBlocked = useSelect( ( select ) =>
    	select( module.storeName )?.isSetupBlocked?.()
    )
    
    • If the selector returns true, display Back, else display Cancel as it is currently.
  • Update assets/js/components/setup/ModuleSetup.js:
    • to use ModuleSetupFooter.
    • to use Grid, Row and Cell for the grid layout.

Test Coverage

  • No new tests to be added.

Visual Regression Changes

QA Brief

  • Disconnect AdSense module.
  • Enable your AdBlocker and proceed to setup AdSense module.
  • Instead of displaying “Cancel”, the verbiage should be “Back” when the AdSense AdBlocker error is displayed.
  • Disable your AdBlocker, the link should be: “Cancel”

Changelog entry

  • Update the cancel button on the module setup form to have a Back label when the setup process can’t proceed.

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Comments: 20 (7 by maintainers)

Most upvoted comments

@aaemnnosttv Good point that we can probably still move parts of this forward already. However, I think the only module-specific integration that we should cover in here for now is the AdSense setup part of having an ad blocker, as that will remain relevant in the future. The Tag Manager and Analytics cases where the setup is currently blocked will be drastically simplified, and therefore it’s not worth covering that here just to remove it again later. The AdSense setup flow itself will change quite a bit too.

I’ve therefore added a third point to the ACs that clarify that the actual implementation of that new API infrastructure should only be done for the ad blocker part of AdSense as part of this issue. @asvinb If that sounds good, can you please update the IB accordingly and remove all the parts regarding Tag Manager and Analytics? Other than that, this should be good to go.

@eclarke1 – the IB needs to be rewritten as we got off track after the initial review here but it can be done by anyone, using https://github.com/google/site-kit-wp/compare/poc/1045-is-setup-blocked as a starting point.

@aaemnnosttv Hmm, is it really overkill? I see how it may feel so since the goal of this issue is to only change the button label, but whether a module setup is blocked is something that in principle can be relevant for any module, so I think integrating the implementation into “core” makes sense. Right now we already have a need for it in GA and GTM, and you could argue a similar change would make sense in AdSense for the cases when the user is in a scenario where they just have to wait for AdSense review etc.

Hey @danielgent, can you look at this IB again outline the approach taken in the PR and the one that you’d recommend here? The existing IB text isn’t really a brief and seems more like discovery text used to outline the current/proposed flow, but I’m not quite sure which.

It’s worth noting that the linked branch doesn’t have the “Back” string translated, but passed directly into the rendered output as 'Back', not __( 'Back' ).

But it also looks like the child component is… causing a side effect that changes what the parent renders? This is a bit confusing—even if it’s a logic flow that doesn’t have bugs it’s a bit hard to understand. Maybe we want to store a piece of data in the CORE_UI data store (something we use as a sort of “catch-all” data store to store transitory values) that marks the text as needing to change? The current flow just strikes me as a bit roundabout. 🤷🏻‍♂️

Let me know if you want to chat about it more or correct a mistake I made, maybe I’m missing something. 🙂