Enhancement/2074 allow registering module setup components #2129

caseymorrisus · 2020-10-05T16:26:19Z

Summary

Addresses issue #2074

Relevant technical choices

Created DefaultModuleSetup component
Added registerModule action with setupComponent argument with DefaultModuleSetup as the default value
Refactored SetupWrapper to be a functional component and grab module data from registry via getModule
Update modules to register components via registerModule rather than using a filter via googlesitekit.ModuleSetup-${ slug }
Fixed stories to account for modules coming from the store

Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

caseymorrisus · 2020-10-07T02:25:01Z

Tests pass locally, failing on server due to flaky tests therefore I am moving this out of draft as I cannot manually re-run Travis CI jobs without dummy commits. Whoever ends up reviewing this will need to re-run Travis to get checks to pass.

eugene-manuilov

Nice work, @caseymorrisus! Left a few comments for you. Could you, please, address it?

assets/js/googlesitekit/modules/components/DefaultModuleSetup.js

assets/js/components/setup/setup-wrapper.js

eugene-manuilov

@caseymorrisus LGTM! Just left a final suggestion, let's update it and I'll approve this PR.

assets/js/googlesitekit/modules/components/DefaultModuleSetup.js

eugene-manuilov · 2020-10-08T15:10:11Z

@caseymorrisus I have approved this PR, but could you please resolve merge conflicts?

…unctional component, and add registerModule action.

…e module object.

…mponents.

felixarntz

The production code mostly LGTM, but this seems to break some Tag Manager stories.

felixarntz · 2020-10-12T22:16:59Z

assets/js/googlesitekit/modules/components/DefaultModuleSetup.js

+ */
+import Button from '../../../components/button';
+
+export default function DefaultModuleSetup( { finishSetup, module = {} } ) {


I think we should use minimal props here like we usually do and only pass the moduleSlug which is sufficient to identify the module. Then this component can look up the module object to get module.name from the datastore.

felixarntz · 2020-10-12T22:18:09Z

assets/js/googlesitekit/modules/components/DefaultModuleSetup.js

+	module: PropTypes.shape( {
+		name: PropTypes.string,
+		slug: PropTypes.string,
+	} ),


See above. Passing a slug is more straightforward here, I think it's a bit odd to expect an object here, especially since the shape provided here is not the full module object - it shouldn't depend too much on the implementation of the component, let's require the module slug, from there we can get anything else we could possibly need via the datastore.

felixarntz · 2020-10-12T22:18:29Z

assets/js/googlesitekit/modules/datastore/modules.js

@@ -221,6 +223,7 @@ const baseActions = {
 	 * @param {number}    [settings.order]             Optional. Numeric indicator for module order. Default 10.
 	 * @param {string}    [settings.homepage]          Optional. Module homepage URL. Default empty string.
 	 * @param {WPElement} [settings.settingsComponent] React component to render the settings panel. Default is the DefaultModuleSettings component.
+	 * @param {WPElement} [settings.setupComponent]    React component to render the setuppanel. Default is the DefaultModuleSetup component.


Suggested change

* @param {WPElement} [settings.setupComponent] React component to render the setuppanel. Default is the DefaultModuleSetup component.

* @param {WPElement} [settings.setupComponent] React component to render the setup panel. Default is the DefaultModuleSetup component.

felixarntz · 2020-10-12T22:21:34Z

assets/js/modules/adsense/index.js

+		{
+			setupComponent: SetupMain,
+		},
+	);


Super-nitpick, but maybe we should put the registerModule call above the widget registrations, just because module registration is the more general step. Of course this doesn't technically change anything, but without module registration widget registration is mostly useless (most widgets only are active if their respective module is registered and active).

This also applies to the other similar files.

felixarntz · 2020-10-12T22:30:37Z

stories/module-adsense-setup.stories.js

+	.addDecorator( ( storyFn ) => {
+		global._googlesitekitLegacyData.setup.moduleToSetup = 'adsense';
+		const registry = createTestRegistry();
+		registry.dispatch( CORE_MODULES ).receiveGetModules( [


I think it would be worth switching this and similar calls to using the new provideModules utility function.

felixarntz · 2020-10-12T22:44:31Z

stories/modules-tagmanager-setup.stories.js

+		registry.dispatch( CORE_MODULES ).receiveGetModules( [
+			{
+				slug: 'tagmanager',
+				active: true,
+				connected: true,
+			},
+		] );


felixarntz · 2020-10-12T22:45:35Z

stories/modules-tagmanager-setup.stories.js

@@ -21,37 +21,22 @@
 */


Something here seems to mess up several Tag Manager setup stories, e.g.:

stories/utils/generate-gtm-analytics-property-story.js

felixarntz · 2020-10-12T22:47:42Z

assets/js/components/setup/setup-wrapper.js

@@ -96,77 +68,75 @@ class SetupWrapper extends Component {
 		delay( function() {
 			global.location.replace( redirectURL );
 		}, 500, 'later' );
+	}, [] );


I think here we should pass moduleToSetup from above as callback dependency and then use it in the callback, instead of relying on the same global again, which may in theory cause inconsistent callbacks.

felixarntz · 2020-10-12T22:52:00Z

assets/js/components/setup/setup-wrapper.js

-	}
+export default function SetupWrapper() {
+	const settingsPageURL = useSelect( ( select ) => select( CORE_SITE ).getAdminURL( 'googlesitekit-settings' ) );
+	const { moduleToSetup } = global._googlesitekitLegacyData.setup;


This was explicitly not required as part of the ACs, but maybe it would be worth moving this legacy dependency out of this component and require it as a prop instead (moduleSlug). This would for example simplify the Storybook configuration so that we no longer need to mess with the global there which is error-prone.

We could instead do the global._googlesitekitLegacyData.setup.moduleToSetup lookup in the components using SetupWrapper and pass the value as prop, so that at least this component is fully refactored (let's not worry about the other ones):

DashboardSplashApp

GoogleSitekitDashboard

GoogleSitekitModule

caseymorrisus · 2020-10-18T15:46:59Z

@felixarntz Failed CI tests have been confirmed to pass (multiple times) locally. Please re-run tests when re-reviewing to verify.

…mponents.

felixarntz

@caseymorrisus Against the ACs, this is good to go. However, based on some changes in the approach we decided on for the related #1623, I suggested some changes here (see specific comments). @aaemnnosttv What do you think about my alternative approach suggested here?

felixarntz · 2020-10-20T02:06:19Z

assets/js/googlesitekit/modules/datastore/modules.js

@@ -230,6 +233,7 @@ const baseActions = {
 		order,
 		homepage,
 		settingsComponent = DefaultModuleSettings,
+		setupComponent = DefaultModuleSetup,


Revisiting this, we may want to diverge from the ACs here in that there should not be any default value for the setupComponent argument. Per #1623, this will be changed for the settingsComponent replacements as well. Providing a default here prevents us from checking whether the module has a setup component attached or not - which will come in handy in the future, e.g. when we need to determine whether we need to link to the module setup screen or the regular module page (linking to the setup screen when there is no setup component would not make sense). There is currently some hard-coded exception in place for PageSpeed Insights (which doesn't require a setup) for that very reason - which we can eliminate by checking whether the module has a setup component.

Yeah, I think that makes sense. A DefaultModuleSetup component should be capable of setting up the module, but setup is very module-specific so I don't think it's possible to have such a thing in our case as we can't really assume anything about how every module works to have a generic setup component.

setupComponent can default to null if not passed and is what I have in #1623 for settings components so far (almost ready for review, but you can see it here: https://github.com/google/site-kit-wp/pull/2202/files#diff-51079bbb133e20fa794c0e1fd5fd30b551dda818e0cfe0ba4162502670cf133a)

felixarntz · 2020-10-20T02:06:39Z

assets/js/googlesitekit/modules/datastore/modules.js

@@ -221,6 +223,7 @@ const baseActions = {
 	 * @param {number}    [settings.order]             Optional. Numeric indicator for module order. Default 10.
 	 * @param {string}    [settings.homepage]          Optional. Module homepage URL. Default empty string.
 	 * @param {WPElement} [settings.settingsComponent] React component to render the settings panel. Default is the DefaultModuleSettings component.
+	 * @param {WPElement} [settings.setupComponent]    React component to render the setup panel. Default is the DefaultModuleSetup component.


Based on my above comment, this could then say "Default none.".

felixarntz · 2020-10-20T02:06:56Z

assets/js/googlesitekit/modules/datastore/modules.js

@@ -59,6 +60,7 @@ const moduleDefaults = {
 	order: 10,
 	icon: null,
 	settingsComponent: DefaultModuleSettings,
+	setupComponent: DefaultModuleSetup,


See above, the default should be undefined.

I chose to go with null for the settings components to indicate that it is intentionally empty as we do elsewhere rather than undefined which is more of an initial state or unresolved value. Perhaps we're not consistent with this, but both values should work for our needs. If you would prefer to use undefined instead, that's an easy change to make for me.

felixarntz · 2020-10-20T02:11:07Z

assets/js/components/setup/setup-wrapper.js

-			'googlesitekit-settings',
-			{}
-		);
+	const { setupComponent: SetupComponent } = module;


Based on my above comment, we could either remove the DefaultModuleSetup component entirely (it strictly isn't needed), or we could use it here internally to render it if SetupComponent is undefined. Since this screen would in the future never be linked to if there is no setupComponent, it may also be okay just rendering nothing in that case.

…mponents.

…mponents

…r if setup component does not exist.

PR #2129.

…mponents

…mponents.

…mponents

felixarntz

Awesome work!

google-cla bot added the cla: yes label Oct 5, 2020

caseymorrisus marked this pull request as ready for review October 7, 2020 02:25

eugene-manuilov requested changes Oct 7, 2020

View reviewed changes

caseymorrisus requested a review from eugene-manuilov October 8, 2020 06:43

eugene-manuilov requested changes Oct 8, 2020

View reviewed changes

assets/js/googlesitekit/modules/components/DefaultModuleSetup.js Outdated Show resolved Hide resolved

eugene-manuilov previously approved these changes Oct 8, 2020

View reviewed changes

caseymorrisus added 4 commits October 8, 2020 09:17

Create default module setup component, refactor setup wrapper to be f…

73e5e76

…unctional component, and add registerModule action.

Implement settings pages via dispatch, modify stories to use .

7e71de3

Remove unused import variables.

4dfd3d5

Update prop types, pass page to , load/finish setup refactors, and us…

6657420

…e module object.

caseymorrisus dismissed eugene-manuilov’s stale review via 6657420 October 8, 2020 16:27

caseymorrisus force-pushed the enhancement/2074-register-module-setup-components branch from 08e6898 to 6657420 Compare October 8, 2020 16:27

eugene-manuilov previously approved these changes Oct 8, 2020

View reviewed changes

Merge branch 'develop' into enhancement/2074-register-module-setup-co…

2f77581

…mponents.

felixarntz dismissed eugene-manuilov’s stale review via 2f77581 October 12, 2020 22:12

felixarntz requested changes Oct 12, 2020

View reviewed changes

Use moduleSlug prop, re-order registerModule calls, & fix stories.

63812e0

caseymorrisus requested a review from felixarntz October 18, 2020 15:46

Merge branch 'develop' into enhancement/2074-register-module-setup-co…

865ff23

…mponents.

felixarntz reviewed Oct 20, 2020

View reviewed changes

felixarntz and others added 3 commits October 19, 2020 19:41

Merge branch 'develop' into enhancement/2074-register-module-setup-co…

028f780

…mponents.

Merge branch 'develop' into enhancement/2074-register-module-setup-co…

4d79b9e

…mponents

Remove default value for setupComponent & skip rendering setup wrappe…

c21e59d

…r if setup component does not exist.

aaemnnosttv added a commit that referenced this pull request Oct 27, 2020

Port Modules.registerModule from #2074.

d4beafb

PR #2129.

caseymorrisus and others added 3 commits October 27, 2020 11:21

Merge branch 'develop' into enhancement/2074-register-module-setup-co…

044f697

…mponents

Fix merge issues.

e234667

Merge branch 'develop' into enhancement/2074-register-module-setup-co…

14af92e

…mponents.

felixarntz and others added 3 commits October 29, 2020 11:27

Merge branch 'develop' into enhancement/2074-register-module-setup-co…

be4ef9e

…mponents

Remove unnecessary call to old undefined function.

472ef64

Fix broken Analytics setup story.

b8b6e5b

felixarntz approved these changes Oct 29, 2020

View reviewed changes

felixarntz merged commit 893f6c6 into develop Oct 29, 2020

felixarntz deleted the enhancement/2074-register-module-setup-components branch October 29, 2020 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/2074 allow registering module setup components #2129

Enhancement/2074 allow registering module setup components #2129

caseymorrisus commented Oct 5, 2020

caseymorrisus commented Oct 7, 2020

eugene-manuilov left a comment

eugene-manuilov left a comment

eugene-manuilov commented Oct 8, 2020

felixarntz left a comment

felixarntz Oct 12, 2020

felixarntz Oct 12, 2020

felixarntz Oct 12, 2020

felixarntz Oct 12, 2020

felixarntz Oct 12, 2020

felixarntz Oct 12, 2020

felixarntz Oct 12, 2020 •

edited

felixarntz Oct 12, 2020

felixarntz Oct 12, 2020

caseymorrisus commented Oct 18, 2020

felixarntz left a comment

felixarntz Oct 20, 2020

aaemnnosttv Oct 20, 2020

felixarntz Oct 20, 2020

felixarntz Oct 20, 2020

aaemnnosttv Oct 20, 2020

felixarntz Oct 20, 2020

felixarntz left a comment

	* @param {WPElement} [settings.setupComponent] React component to render the setuppanel. Default is the DefaultModuleSetup component.
	* @param {WPElement} [settings.setupComponent] React component to render the setup panel. Default is the DefaultModuleSetup component.

Enhancement/2074 allow registering module setup components #2129

Enhancement/2074 allow registering module setup components #2129

Conversation

caseymorrisus commented Oct 5, 2020

Summary

Relevant technical choices

Checklist

caseymorrisus commented Oct 7, 2020

eugene-manuilov left a comment

Choose a reason for hiding this comment

eugene-manuilov left a comment

Choose a reason for hiding this comment

eugene-manuilov commented Oct 8, 2020

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz Oct 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseymorrisus commented Oct 18, 2020

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz Oct 12, 2020 •

edited