Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display change metrics feature tour. #7661

Merged
merged 27 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
2bc796f
Add shared key metrics tour and it's trigger.
zutigrm Oct 3, 2023
d68eaa7
Change Key_Metrics_Setup_Completed to integer.
zutigrm Oct 3, 2023
514d7d8
Add keyMetricsSetupCompletedByUserID to preloaded data.
zutigrm Oct 3, 2023
8ea9326
Edit key_metrics_setup_completed to save current user id.
zutigrm Oct 3, 2023
33f7de7
Include keyMetricsSetupCompletedByUserID to datastore.
zutigrm Oct 3, 2023
ca64103
Update jest test.
zutigrm Oct 3, 2023
ccf78bc
Update php integration tests.
zutigrm Oct 3, 2023
ac7ffb7
Add e2e user mocking plugin.
zutigrm Oct 3, 2023
1a5c0aa
Add e2e share modules plugin.
zutigrm Oct 3, 2023
9584b40
Add e2e test and utility functions.
zutigrm Oct 3, 2023
27ead2f
Mock get_blogs_of_user to bypass notice in admin bar.
zutigrm Oct 3, 2023
fe8d33b
Update per CR feedback.
zutigrm Oct 4, 2023
4348f61
Remove e2e, it will be moved to another branch.
zutigrm Oct 4, 2023
0bf9abb
Remove unused class, after method removal.
zutigrm Oct 4, 2023
0eb476d
Update per CR feedback.
zutigrm Oct 5, 2023
28c6cfd
Revert a few changes to site/info store.
aaemnnosttv Oct 5, 2023
492a5b0
Rename single state key for KM setup completed by.
aaemnnosttv Oct 5, 2023
78a27c5
Update siteInfo state key.
aaemnnosttv Oct 5, 2023
fba5794
Update completed and ByUserID be completedBy.
aaemnnosttv Oct 5, 2023
882d893
Rename to Key_Metrics_Setup_Completed_By for consistency.
aaemnnosttv Oct 5, 2023
0bb67ad
Update call to keyMetricsSetupCompletedBy when saving KMW settings.
aaemnnosttv Oct 5, 2023
18d095c
Update doc block.
aaemnnosttv Oct 5, 2023
e5c9d67
Update action dispatched to new name.
aaemnnosttv Oct 5, 2023
0808bf8
Fix tour conflict with no tour for view.
aaemnnosttv Oct 5, 2023
4581d46
Update test for saveKeyMetricsSettings.
aaemnnosttv Oct 5, 2023
9a0ca0f
Move isUserEligibleForTour outside of hook.
zutigrm Oct 6, 2023
7e4e26b
Update assets/js/components/KeyMetrics/hooks/useChangeMetricsFeatureT…
eugene-manuilov Oct 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion assets/js/components/KeyMetrics/ChangeMetricsLink.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import Link from '../Link';
import PencilIcon from '../../../svg/icons/pencil-alt.svg';
import { trackEvent } from '../../util';
import useViewContext from '../../hooks/useViewContext';
import { useChangeMetricsFeatureTourEffect } from './hooks/useChangeMetricsFeatureTourEffect';
const { useSelect, useDispatch } = Data;

export default function ChangeMetricsLink() {
Expand All @@ -48,7 +49,12 @@ export default function ChangeMetricsLink() {
trackEvent( `${ viewContext }_kmw`, 'change_metrics' );
}, [ setValue, viewContext ] );

if ( ! Array.isArray( keyMetrics ) || ! keyMetrics?.length > 0 ) {
const renderChangeMetricLink =
Array.isArray( keyMetrics ) || keyMetrics?.length > 0;

useChangeMetricsFeatureTourEffect( renderChangeMetricLink );

if ( ! renderChangeMetricLink ) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* Change metrics feature tour hook.
*
* Site Kit by Google, Copyright 2022 Google LLC
eugene-manuilov marked this conversation as resolved.
Show resolved Hide resolved
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* External dependencies
*/
import { useEffect } from '@wordpress/element';

/**
* Internal dependencies
*/
import Data from 'googlesitekit-data';
import { CORE_SITE } from '../../../googlesitekit/datastore/site/constants';
import { CORE_USER } from '../../../googlesitekit/datastore/user/constants';
import sharedKeyMetrics from '../../../feature-tours/shared-key-metrics';
const { useSelect, useDispatch } = Data;

/**
* Triggers on demand tour for shared key metrics if all conditions are met.
*
* @since n.e.x.t
*
* @param {boolean} renderChangeMetricLink If metric link meets the conditions to render.
*/
export const useChangeMetricsFeatureTourEffect = ( renderChangeMetricLink ) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of adding this new hook? I don't see it is required in IB. I understand that you try to encapsulate the logic but extracting it into a separate hook makes sense only when there are multiple components that use it. In this situation we have just one place where we use this hook and there is no potential to have another component that needs this in the future. Let's merge this hook into the ChangeMetricsLink component to avoid bloating up our code base with extra files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eugene-manuilov It wasn't required in IB, it was a suggestion on previous CR, it was originally in ChangeMetricsLink, but since it was adding certain number of lines for this logic, it was moved to separate hook - comment for reference - #7661 (comment)

Let me know what you think

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... okay, then let's keep it as is although that is less appropriate IMO because:

  1. It makes us have more files in the codebase than we really need.
  2. It makes the logic in the component harder to read and understand because we need to find and open an additional file to understand what happens in that hook.
  3. Webpack will bundle component and hook as separate modules thus the size of the final bundle will be slightly bigger than it could be if we not extract that logic into a custom hook.

cc @aaemnnosttv

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review @eugene-manuilov ! A few thoughts in response to your comments above.

  • It makes us have more files in the codebase than we really need.

We shouldn't have more files for no reason of course, but we often use more files to keep each of them smaller and easier to understand and extract complexity. I don't think that it only makes sense to extract components/hooks if there is more than one usage of them, there are plenty of instances where we extract parts to keep the overall size and complexity down, e.g. BannerNotification components.

  • It makes the logic in the component harder to read and understand because we need to find and open an additional file to understand what happens in that hook.

I find this point a bit hard to see, as I feel that it's harder to understand the component with a large portion of its code only needed for the tour effect which is itself easier to understand in isolation IMO. In the context of the component, we see that it has a named effect about its tour, so you could argue that it's easier to understand in this way.

  • Webpack will bundle component and hook as separate modules thus the size of the final bundle will be slightly bigger than it could be if we not extract that logic into a custom hook.

This is true, but the increase in size is probably only a handful of bytes which isn't a concern. There are likely many other things we could optimize to reduce the overall size of our assets in more impactful ways, so I don't think bundle size is an issue here. We have the compressed size action to help highlight changes to our bundle size which reported didn't show any substantial change here.

const keyMetricsSetupCompletedByUserID = useSelect( ( select ) =>
select( CORE_SITE ).getKeyMetricsSetupCompletedByUserID()
);
const currentUserID = useSelect( ( select ) =>
select( CORE_USER ).getID()
);
const { triggerOnDemandTour } = useDispatch( CORE_USER );

useEffect( () => {
const isUserEligibleForTour =
Number.isInteger( keyMetricsSetupCompletedByUserID ) &&
Number.isInteger( currentUserID ) &&
keyMetricsSetupCompletedByUserID > 0 &&
currentUserID !== keyMetricsSetupCompletedByUserID;

if ( renderChangeMetricLink && isUserEligibleForTour ) {
triggerOnDemandTour( sharedKeyMetrics );
}
}, [
renderChangeMetricLink,
keyMetricsSetupCompletedByUserID,
currentUserID,
triggerOnDemandTour,
] );
};
56 changes: 56 additions & 0 deletions assets/js/feature-tours/shared-key-metrics.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* Site Kit by Google, Copyright 2023 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

/*
* Internal dependencies
*/
import {
VIEW_CONTEXT_MAIN_DASHBOARD,
VIEW_CONTEXT_MAIN_DASHBOARD_VIEW_ONLY,
VIEW_CONTEXT_ENTITY_DASHBOARD,
VIEW_CONTEXT_ENTITY_DASHBOARD_VIEW_ONLY,
} from '../googlesitekit/constants';
import { isFeatureEnabled } from '../features';

const sharedKeyMetrics = {
slug: 'sharedKeyMetrics',
contexts: [
VIEW_CONTEXT_MAIN_DASHBOARD,
VIEW_CONTEXT_MAIN_DASHBOARD_VIEW_ONLY,
VIEW_CONTEXT_ENTITY_DASHBOARD,
VIEW_CONTEXT_ENTITY_DASHBOARD_VIEW_ONLY,
],
gaEventCategory: ( viewContext ) => `${ viewContext }_shared_key-metrics`,
checkRequirements: () => isFeatureEnabled( 'userInput' ),
steps: [
{
target: '.googlesitekit-km-change-metrics-cta',
title: __( 'Personalize your key metrics', 'google-site-kit' ),
content: __(
'Another admin has set up these tailored metrics for your site. Click here to personalize them.',
'google-site-kit'
),
placement: 'bottom-start',
},
],
};
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved

export default sharedKeyMetrics;
34 changes: 29 additions & 5 deletions assets/js/googlesitekit/datastore/site/info.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,16 @@ import { normalizeURL, untrailingslashit } from '../../../util';

const { createRegistrySelector } = Data;

function getSiteInfoProperty( propName ) {
function getSiteInfoProperty( propName, castToBool = false ) {
return createRegistrySelector( ( select ) => () => {
const siteInfo = select( CORE_SITE ).getSiteInfo() || {};

// Added to convert keyMetricsSetupCompleted user ID to
// boolean, for flexibility it is made as parameter.
if ( castToBool && undefined !== siteInfo[ propName ] ) {
return !! siteInfo[ propName ];
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessary since we can do the same in the one selector as this isn't a common need.


return siteInfo[ propName ];
} );
}
Expand Down Expand Up @@ -112,14 +119,15 @@ export const actions = {
* Sets the `keyMetricsSetupCompleted` boolean value.
*
* @since 1.108.0
* @since n.e.x.t Changed boolean to number since value is holding user ID.
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
*
* @param {boolean} keyMetricsSetupCompleted Whether key metrics setup is completed.
* @param {number} keyMetricsSetupCompleted Positive integer if key metrics setup is completed, otherwise 0.
* @return {Object} Redux-style action.
*/
setKeyMetricsSetupCompleted( keyMetricsSetupCompleted ) {
invariant(
typeof keyMetricsSetupCompleted === 'boolean',
'keyMetricsSetupCompleted must be a boolean.'
typeof keyMetricsSetupCompleted === 'number',
'keyMetricsSetupCompleted must be a number.'
);

return {
Expand Down Expand Up @@ -162,6 +170,7 @@ export const reducer = ( state, { payload, type } ) => {
pluginBasename,
productBasePaths,
keyMetricsSetupCompleted,
keyMetricsSetupCompletedByUserID,
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
} = payload.siteInfo;

return {
Expand Down Expand Up @@ -194,6 +203,7 @@ export const reducer = ( state, { payload, type } ) => {
pluginBasename,
productBasePaths,
keyMetricsSetupCompleted,
keyMetricsSetupCompletedByUserID,
},
};
}
Expand Down Expand Up @@ -271,6 +281,7 @@ export const resolvers = {
pluginBasename,
productBasePaths,
keyMetricsSetupCompleted,
keyMetricsSetupCompletedByUserID,
} = global._googlesitekitBaseData;

const {
Expand Down Expand Up @@ -308,6 +319,7 @@ export const resolvers = {
pluginBasename,
productBasePaths,
keyMetricsSetupCompleted,
keyMetricsSetupCompletedByUserID,
} );
},
};
Expand Down Expand Up @@ -769,6 +781,17 @@ export const selectors = {
*/
getPluginBasename: getSiteInfoProperty( 'pluginBasename' ),

/**
* Get the `userID` of the user who setup Key Metrics widget if present.
*
* @since n.e.x.t
*
* @return {number} `userID` of the user who did initial setup of the Key Metrics widget, if setup was done, otherwise `0`.
*/
getKeyMetricsSetupCompletedByUserID: getSiteInfoProperty(
'keyMetricsSetupCompleted'
),

/**
* Determines whether the current WordPress site has the minimum required version.
* Currently, the minimum required version is 5.2.
Expand Down Expand Up @@ -816,7 +839,8 @@ export const selectors = {
* @return {(boolean)} `true` if the Key Metrics widget has been setup, otherwise `false`.
*/
isKeyMetricsSetupCompleted: getSiteInfoProperty(
'keyMetricsSetupCompleted'
'keyMetricsSetupCompleted',
true
),
};

Expand Down
30 changes: 14 additions & 16 deletions assets/js/googlesitekit/datastore/site/info.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,53 +147,47 @@ describe( 'core/site site info', () => {

describe( 'setKeyMetricsSetupCompleted', () => {
it( 'sets the keyMetricsSetupCompleted property', () => {
registry
.dispatch( CORE_SITE )
.setKeyMetricsSetupCompleted( true );
registry.dispatch( CORE_SITE ).setKeyMetricsSetupCompleted( 1 );

expect(
registry.select( CORE_SITE ).isKeyMetricsSetupCompleted()
).toBe( true );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is how it was before but we shouldn't be using selectors to make assertions about actions. These should test the effect of the action (state change, request, etc).


registry
.dispatch( CORE_SITE )
.setKeyMetricsSetupCompleted( false );
registry.dispatch( CORE_SITE ).setKeyMetricsSetupCompleted( 0 );

expect(
registry.select( CORE_SITE ).isKeyMetricsSetupCompleted()
).toBe( false );
} );

it( 'requires a boolean argument', () => {
it( 'requires a number argument', () => {
expect( () => {
registry
.dispatch( CORE_SITE )
.setKeyMetricsSetupCompleted();
} ).toThrow( 'keyMetricsSetupCompleted must be a boolean.' );
} ).toThrow( 'keyMetricsSetupCompleted must be a number.' );

expect( () => {
registry
.dispatch( CORE_SITE )
.setKeyMetricsSetupCompleted( undefined );
} ).toThrow( 'keyMetricsSetupCompleted must be a boolean.' );
} ).toThrow( 'keyMetricsSetupCompleted must be a number.' );

expect( () => {
registry
.dispatch( CORE_SITE )
.setKeyMetricsSetupCompleted( 0 );
} ).toThrow( 'keyMetricsSetupCompleted must be a boolean.' );
.setKeyMetricsSetupCompleted( true );
} ).toThrow( 'keyMetricsSetupCompleted must be a number.' );

expect( () => {
registry
.dispatch( CORE_SITE )
.setKeyMetricsSetupCompleted( true );
.setKeyMetricsSetupCompleted( 1 );

registry
.dispatch( CORE_SITE )
.setKeyMetricsSetupCompleted( false );
} ).not.toThrow(
'keyMetricsSetupCompleted must be a boolean.'
);
.setKeyMetricsSetupCompleted( 0 );
} ).not.toThrow( 'keyMetricsSetupCompleted must be a number.' );
} );
} );
} );
Expand Down Expand Up @@ -371,6 +365,10 @@ describe( 'core/site site info', () => {
[ 'getUpdateCoreURL', 'updateCoreURL' ],
[ 'getTimezone', 'timezone' ],
[ 'getPostTypes', 'postTypes' ],
[
'getKeyMetricsSetupCompletedByUserID',
'keyMetricsSetupCompleted',
],
[ 'isUsingProxy', 'usingProxy' ],
[ 'isAMP', 'ampMode' ],
[ 'isPrimaryAMP', 'ampMode' ],
Expand Down
4 changes: 2 additions & 2 deletions assets/js/googlesitekit/datastore/user/key-metrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ const baseActions = {
// TODO: We should find a better way of keeping this value synced.
yield registry
.dispatch( CORE_SITE )
.setKeyMetricsSetupCompleted( true );
.setKeyMetricsSetupCompleted( 1 ); // Return positive integer
}

return { response, error };
Expand Down Expand Up @@ -400,7 +400,7 @@ const baseSelectors = {

if (
! isAuthenticated &&
module.shareable &&
module?.shareable &&
! canViewSharedModule( slug )
) {
return false;
Expand Down
3 changes: 2 additions & 1 deletion includes/Core/Key_Metrics/Key_Metrics.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,13 @@ public function register() {
* Adds the status of the Key Metrics widget setup to the inline JS data.
*
* @since 1.108.0
* @since n.e.x.t Return casted to integer.
*
* @param array $data Inline JS data.
* @return array Filtered $data.
*/
private function inline_js_base_data( $data ) {
$data['keyMetricsSetupCompleted'] = (bool) $this->key_metrics_setup_completed->get();
$data['keyMetricsSetupCompleted'] = (int) $this->key_metrics_setup_completed->get();

return $data;
}
Expand Down
5 changes: 3 additions & 2 deletions includes/Core/Key_Metrics/Key_Metrics_Setup_Completed.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,17 @@ class Key_Metrics_Setup_Completed extends Setting {
/**
* The option_name for this setting.
*/
const OPTION = 'googlesitekit_key_metrics_setup_completed';
const OPTION = 'googlesitekit_key_metrics_setup_completed_by_user_id';

/**
* Gets the expected value type.
*
* @since 1.108.0
* @since n.e.x.t Type changed to integer.
*
* @return string The type name.
*/
protected function get_type() {
return 'boolean';
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
return 'integer';
}
}
8 changes: 7 additions & 1 deletion includes/Core/Key_Metrics/REST_Key_Metrics_Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,13 @@ protected function get_rest_routes() {
array( 'status' => 400 )
);
}
$this->key_metrics_setup_completed->set( true );

$key_metrics_setup_already_done_by_user = $this->key_metrics_setup_completed->get();
if ( empty( $key_metrics_setup_already_done_by_user ) ) {
$current_user_id = get_current_user_id();

$this->key_metrics_setup_completed->set( $current_user_id );
}
}

$this->settings->merge( $data['settings'] );
Expand Down
8 changes: 7 additions & 1 deletion includes/Core/User_Input/REST_User_Input_Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,13 @@ private function get_rest_routes() {
$answers = $this->user_input->set_answers( $data['settings'] );

if ( ! empty( $answers['purpose']['values'] ) ) {
$this->key_metrics_setup_completed->set( true );
$key_metrics_setup_already_done_by_user = $this->key_metrics_setup_completed->get();

if ( empty( $key_metrics_setup_already_done_by_user ) ) {
$current_user_id = get_current_user_id();

$this->key_metrics_setup_completed->set( $current_user_id );
}
}

$response = rest_ensure_response( $answers );
Expand Down
Loading
Loading