Remove no longer used ActivateModuleCTA and CompleteModuleActivationCTA components

[techanvil]

trafficstars

Feature Description

Following on from https://github.com/google/site-kit-wp/issues/5148, the ActivateModuleCTA and CompleteModuleActivationCTA components are no longer in active use and can be removed.

---

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

Acceptance criteria

• Confirm that ActivateModuleCTA and CompleteModuleActivationCTA are not in active use, i.e. they are no longer being rendered by any component.
• If so, remove them and all related, no longer needed code.

Implementation Brief

Remove ActivateModuleCTA component and its usages

• Git remove the assets/js/components/ActivateModuleCTA.js file.
• Git remove the stories/activate-module.stories.js file.
• Git remove the assets/js/googlesitekit/widgets/components/WidgetActivateModuleCTA.js file if it was not removed as part of #5148.
• Git remove the assets/js/googlesitekit/widgets/components/WidgetActivateModuleCTA.test.js file.
• Remove the usages of the ActivateModuleCTA component from the following files:
  • assets/js/components/adminbar/AdminBarWidgets.js,
  • assets/js/components/wp-dashboard/WPDashboardWidgets.js,
  • assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/index.js,
  • assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/Overview.js,
  • assets/js/googlesitekit/widgets/util/constants.js,
  • assets/js/googlesitekit/widgets/util/get-widget-layout.test.js,
  • assets/js/googlesitekit/widgets/util/is-inactive-widget-state.test.js

Remove CompleteModuleActivationCTA component and its usages

• Git remove the assets/js/components/CompleteModuleActivationCTA.js file.
• Git remove the stories/complete-module-activation.stories.js file.
• Git remove the assets/js/googlesitekit/widgets/components/WidgetCompleteModuleActivationCTA.js file if it was not removed as part of #5148.
• Git remove the assets/js/googlesitekit/widgets/components/WidgetCompleteModuleActivationCTA.test.js file.
• Remove the usages of the CompleteModuleActivationCTA component from the following files:
  • assets/js/components/adminbar/AdminBarWidgets.js,
  • assets/js/components/wp-dashboard/WPDashboardWidgets.js,
  • assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/index.js,
  • assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/Overview.js,
  • assets/js/googlesitekit/widgets/util/constants.js
  • assets/js/googlesitekit/widgets/util/combine-widgets.test.js

Test Coverage

• Update VRT images and E2E tests if necessary.
• Ensure all the existing Jest tests are passing.

QA Brief

Changelog entry

[techanvil]

@aaemnnosttv I have added this issue along with the AC, please could you take a look?

[aaemnnosttv]

@techanvil this could probably be done in #5148 as well if this is where the last remaining usage is for those as that issue is also about removing unused code. We can follow up with this one if needed but seems potentially unnecessary. Otherwise, this LGTM, we'd just want to mark 5148 as a dependency.

[eugene-manuilov]

IB ✔️

[mohitwp]

@hussain-t Can you please provide more information under QAB here apart from the smoke test of whole plugin like if there is any module, component, or functionality can get impact due to the changes introduced under this PR. Thank you !

cc @wpdarren

[wpdarren]

@mohitwp @hussain-t looking at this ticket, it seems to be a change related to activating modules and also cancelling the activation. Is this correct? I feel the QAB is very sparse and we could do with some more direction here, especially since this ticket is only a 7 and a smoke test of the entire plugin is going to take considerably longer.

[hussain-t]

@mohitwp @wpdarren, thanks for bringing this up. The PR removes the legacy code. So, there is nothing specific to be tested. However, I have updated the QAB. LMK if that sounds helpful. Cheers!

[mohitwp]

QA Update ✅

Verified-

• Verified this using oi.ie site and dev plugin.
• Verified this for zero data states - zero and gathering.
• Verified and compared all widgets data with latest release.
• Verified WP dashboard widgets.
• All widgets are rendering fine.

[felixarntz]

Approval ⚠️

@techanvil @hussain-t The PR now adds RecoverableModules (and thus a WidgetRecoverableModules). I'm not sure why that was decided, but it doesn't make sense to me, and it is also neither part of the ACs nor IB. This has production code / API implications, hence it has to be considered. It may also not in fact work as expected because we pass an array to it rather than a string, which works differently in comparisons in JS. Anyway, the latter is not really my point.

If this was only done to replace the components in unit tests, shouldn't we rather simply remove those unit tests since they are no longer applicable? Having taken a look at #5779, I think some of the unit test modifications there don't make sense and should rather be removed (since the related components were removed as well). We still have coverage for the overall combineWidgets logic through ReportZero.

cc @aaemnnosttv

[techanvil]

Hi @felixarntz, thanks for raising this.

To be clear, the RecoverableModules and WidgetRecoverableModules components were added via a previous issue and we don't want to remove those. However there is an unwanted addition here of WidgetRecoverableModules to assets/js/googlesitekit/widgets/util/get-widget-component-props.js that was missed in Code Review.

@hussain-t, would you be happy filing a followup PR here to remove WidgetRecoverableModules from assets/js/googlesitekit/widgets/util/get-widget-component-props.js and update/remove unit tests/stories that reference it?

[felixarntz]

@techanvil Thanks for clarifying, I wasn't aware of that. In that case, I would almost reverse my concern and ask, why wasn't that component included in getWidgetComponentProps prior? Apologies as I don't have the full context here, but just want to make sure we're making the right call there.

[aaemnnosttv]

why wasn't that component included in getWidgetComponentProps prior?

@felixarntz because RecoverableModules is rendered by WidgetRenderer rather than the widget itself since we already know it depends on a recoverable module, there's no point in rendering it. Adding it to the widget's component props wouldn't make sense because it would require every single module to implement the same behavior. See https://github.com/google/site-kit-wp/issues/5376 for more context.

[felixarntz]

Thanks @aaemnnosttv. Okay, in that case I think we're good to close this one. Thanks everyone for clarifying.