Module disconnect modal wording assumes module has features

[aaemnnosttv]

Bug Description

All modules have the option to declare a list of features as user-facing short descriptions, e.g. "Intelligent, automatic ad placement" is one for AdSense. Features are not a required element of module registration though so components that use this should not assume that this list will always be present, even if all Site Kit modules provide it.

Steps to reproduce

1. Activate Idea Hub
2. Deactivate Idea Hub

Screenshots

---

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

Acceptance criteria

• If a module has no available features defined, the message that would otherwise appear above the list of features in the disconnect dialog should not appear.
• Note that Site Kit modules should always provide features to clarify the meaning of disconnecting said modal. This tweak is only so that eventual third-party modules are not technically required to provide such a list.

Implementation Brief

See working branch here https://github.com/google/site-kit-wp/tree/bug/3691-Module-disconnect-modal-no-features

Using assets/js/components/settings/SettingsActiveModule/ConfirmDisconnect.js

• If no features are passed as props e.g. feature prop is null/undefined/empty array, do not pass in subtitle string to inner Dialog component (e.g. pass in empty string or null)

Create a .stories.js file for the ConfirmDisconnect component

• Add a story where the features are populated (current behaviour)
• Add a new story where there are no new features

Test Coverage

• No changes

Visual Regression Changes

• No changes

QA Brief

Check two new stories in Storybook

• with features https://google.github.io/site-kit-wp/storybook/develop/?path=/story/components-confirmdisconnect--confirm-disconnect-with-features
  This is a new story, but the functionality should be the same as before this task

• without features https://google.github.io/site-kit-wp/storybook/develop/?path=/story/components-confirmdisconnect--confirm-disconnect-without-features
  This should be this task implemented

Changelog entry

• Improve module disconnect screen when a module doesn't have listed features.

[danielgent]

@aaemnnosttv Presumably a lot has changed in the last two weeks because Site Kit has a list of features now 😄

When will there be third-party modules that don't provide lists of features? 🤔

[danielgent]

The fix looks pretty simple though @aaemnnosttv . The aim is to have the modal look like this right? (obviously not for Idea Hub though because it has features 😉 )

This component assets/js/components/settings/SettingsActiveModule/ConfirmDisconnect.js instantiates this assets/js/components/Dialog.js as

<Dialog
    dialogActive
    handleDialog={ handleDialog }
    title={ title }
    subtitle={ subtitle }
    provides={ provides }
    handleConfirm={ handleDisconnect }
    dependentModules={ dependentModulesText }
    inProgress={ isDeactivating }
    danger
/>

So it's just a case of not passing in subtitle if provides is empty (it's an array: so empty array or null/undefined)

Then the modal will appear like the screenshot here

Happy to write an IB for this but if we can't reproduce this then it's gonna be odd for CR and QA! 😄

[danielgent]

I created a branch here to show this https://github.com/google/site-kit-wp/compare/bug/3691-Module-disconnect-modal-no-features

[aaemnnosttv]

The aim is to have the modal look like this right?

@danielgent yes – if a module does not have features, the message above the list of features should not appear.

Presumably a lot has changed in the last two weeks because Site Kit has a list of features now 😄

Yes, Idea Hub features were added in #3692

When will there be third-party modules that don't provide lists of features?

It's possible even now, we just don't advertise/encourage it since our APIs are still maturing. Registering a module does not require features to be provided so we can't assume they will be present elsewhere, even if all bundled modules all have (and always should have) features defined.

[danielgent]

@aaemnnosttv So what should be QA'd for this task? I could add <ConfirmDisconnect /> to storybook with examples with and without features?

If it's QA:Eng I could write tests (although new stories would be easier)

[aaemnnosttv]

@danielgent I agree stories would be the easiest. A more robust test would be adding tests for SettingsActiveModule using a custom module registered in the test (or using provideModuleRegistrations with override) but I don't think it's necessary here.

I could add <ConfirmDisconnect /> to storybook with examples with and without features?

SGTM 👍

[danielgent]

Putting 3 hours as I've already done the implementation. Only the stories need adding!

[aaemnnosttv]

IB ✅

[cole10up]

QA ✅

Confirmed the following plugin modals:

Reset SK:

Analytics:

Adsense:

PSI:

Optimize:

Tag Manager:

Ideahub:

Sending to testing approval.

[aaemnnosttv]

Thanks @cole10up – just a heads up that your screenshots above don't cover the problem this issue addresses which is an edge case that isn't present in any of the current modules in SK: an empty list of features for a module. See the QAB which references a few stories – one in particular which is needed to test this as the state doesn't exist in the plugin any more (see #3692). I've verified things are looking correct here but just wanted to bring it to your attention.

[cole10up]

Thanks for having a look @aaemnnosttv. While I understood the ticket and the problem as well as the fact that we no longer have a scenario to cover this specific update. My testing efforts above were to ensure that with this update, we retained the functionality and content in each of our modals.

Thanks again!