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

Improve experience with recoverable modules for view-only users. #5465

Merged
merged 18 commits into from
Jun 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
84 changes: 84 additions & 0 deletions assets/js/components/RecoverableModules.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/**
* RecoverableModules component.
*
* Site Kit by Google, Copyright 2022 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.
*/

/**
* External dependencies
*/
import PropTypes from 'prop-types';

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

/**
* Internal dependencies
*/
import Data from 'googlesitekit-data';
import { CORE_MODULES } from '../googlesitekit/modules/datastore/constants';
import CTA from './notifications/CTA';

const { useSelect } = Data;

export default function RecoverableModules( { moduleSlugs } ) {
const moduleNames = useSelect( ( select ) => {
const modules = select( CORE_MODULES ).getModules();

if ( modules === undefined ) {
return undefined;
}

return moduleSlugs.map( ( moduleSlug ) => modules[ moduleSlug ].name );
} );

if ( moduleNames === undefined ) {
return null;
}

const description =
moduleNames.length === 1
? sprintf(
/* translators: %s: Module name */
__(
'%s data was previously shared by an admin who no longer has access. Please contact another admin to restore it.',
'google-site-kit'
),
moduleNames[ 0 ]
)
: sprintf(
/* translators: %s: List of module names */
__(
'The data for the following modules was previously shared by an admin who no longer has access: %s. Please contact another admin to restore it.',
'google-site-kit'
),
moduleNames.join(
_x( ', ', 'Recoverable modules', 'google-site-kit' )
)
);

return (
<CTA
title={ __( 'Data Unavailable', 'google-site-kit' ) }
description={ description }
/>
);
}

RecoverableModules.propTypes = {
moduleSlugs: PropTypes.arrayOf( PropTypes.string ).isRequired,
};
37 changes: 24 additions & 13 deletions assets/js/googlesitekit/modules/datastore/modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,29 @@ const normalizeModules = memize( ( serverDefinitions, clientDefinitions ) => {
}, {} );
} );

/**
* Gets a memoized object mapping recoverable module slugs to their corresponding
* module objects.
*
* @since n.e.x.t
*
* @param {Object} modules Module definitions.
* @param {Array} recoverableModules Array of recoverable module slugs.
* @return {Object} Map of recoverable module slugs to their corresponding module objects.
*/
const calculateRecoverableModules = memize( ( modules, recoverableModules ) =>
Object.values( modules ).reduce( ( recoverable, module ) => {
if ( recoverableModules.includes( module.slug ) ) {
return {
...recoverable,
[ module.slug ]: module,
};
}

return recoverable;
}, {} )
Comment on lines +114 to +123
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be more efficient to iterate over the recoverable slugs as there will always be less of these than all the modules (simply because not all modules are shareable). We could also then check if modules[recoverableSlug] rather than the list search when reducing.

The sets are so small that this probably doesn't matter at all but worth mentioning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this did cross my mind too. As I was simply moving this code, I thought it might cloud the issue if I also rewrote it. Besides, as you've pointed out the sets are small, and likely to stay that way, so it would arguably be a case of premature optimization to rewrite it...

);

const fetchGetModulesStore = createFetchStore( {
baseName: 'getModules',
controlCallback: () => {
Expand Down Expand Up @@ -1244,19 +1267,7 @@ const baseSelectors = {
return undefined;
}

return Object.values( modules ).reduce(
( recoverableModules, module ) => {
if ( state.recoverableModules.includes( module.slug ) ) {
return {
...recoverableModules,
[ module.slug ]: module,
};
}

return recoverableModules;
},
{}
);
return calculateRecoverableModules( modules, state.recoverableModules );
} ),

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
* limitations under the License.
*/

/**
* External dependencies
*/
import { getByText } from '@testing-library/dom';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be necessary as this query is returned by render

Copy link
Collaborator Author

@techanvil techanvil Jun 29, 2022

Choose a reason for hiding this comment

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

This is a deliberate choice, as I want to use the version of getByText which accepts a container as the first argument, rather than the bound version returned by render.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I'm aware of the difference, the part that isn't obvious is why this would be intentionally used/needed instead?

Copy link
Collaborator Author

@techanvil techanvil Jun 29, 2022

Choose a reason for hiding this comment

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

Well, in the test where it's used, I am asserting that the visible widget contains the text rendered by RecoverableModules:

expect(
getByText(
container.firstChild.querySelector( visibleWidgetSelector ),
'Search Console data was previously shared by an admin who no longer has access. Please contact another admin to restore it.'
)
).toBeInTheDocument();

If I use the bound version and omit the container, this test fails, because RecoverableModules is in fact rendered three times into the DOM, twice within the scope of a .googlesitekit-hidden class (as evident in the corresponding snapshot), and getByText throws an error if there is more than one match for the text.

So, I want to use the non-bound version with the container to scope getByText to the visible widget.

There are of course other ways to code this assertion, but this seemed good enough to me at the time of writing the test...


/**
* Internal dependencies
*/
Expand All @@ -27,6 +32,7 @@ import {
WIDGET_AREA_STYLES,
} from '../datastore/constants';
import { CORE_SITE } from '../../../googlesitekit/datastore/site/constants';
import { CORE_MODULES } from '../../modules/datastore/constants';
import {
createTestRegistry,
render,
Expand Down Expand Up @@ -90,6 +96,7 @@ describe( 'WidgetAreaRenderer', () => {

beforeEach( async () => {
registry = createTestRegistryWithArea( areaName );

const connection = { connected: true };
await registry.dispatch( CORE_SITE ).receiveGetConnection( connection );
} );
Expand Down Expand Up @@ -702,4 +709,67 @@ describe( 'WidgetAreaRenderer', () => {
)
).toHaveLength( 1 );
} );

it( 'should combine multiple widgets in RecoverableModules state with the same metadata into a single widget', () => {
provideModules( registry );
registry
.dispatch( CORE_MODULES )
.receiveRecoverableModules( [ 'search-console' ] );

provideUserCapabilities( registry, {
[ PERMISSION_VIEW_DASHBOARD ]: true,
[ `${ PERMISSION_READ_SHARED_MODULE_DATA }::["search-console"]` ]: true,
} );

createWidgets( registry, areaName, [
{
Component: WidgetComponent,
slug: 'one',
modules: [ 'search-console' ],
},
{
Component: WidgetComponent,
slug: 'two',
modules: [ 'search-console' ],
},
] );

const { container } = render(
<WidgetAreaRenderer slug={ areaName } />,
{
registry,
viewContext: VIEW_CONTEXT_DASHBOARD_VIEW_ONLY,
features: [ 'dashboardSharing' ],
}
);

const visibleWidgetSelector =
'.googlesitekit-widget-area-widgets > .mdc-layout-grid__inner > .mdc-layout-grid__cell > .googlesitekit-widget';

// There should be a single visible widget.
expect(
container.firstChild.querySelectorAll( visibleWidgetSelector )
).toHaveLength( 1 );

// The visible widget should be rendered as the RecoverableModules component.
expect(
getByText(
container.firstChild.querySelector( visibleWidgetSelector ),
'Search Console data was previously shared by an admin who no longer has access. Please contact another admin to restore it.'
)
).toBeInTheDocument();

// There should also be a hidden widget.
expect(
container.firstChild.querySelectorAll(
'.googlesitekit-widget-area-widgets .googlesitekit-hidden .googlesitekit-widget'
)
).toHaveLength( 1 );

expect(
container.firstChild.querySelector(
'.googlesitekit-widget-area-widgets'
)
).toMatchSnapshot();
} );
} );
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/**
* WidgetRecoverableModules component.
*
* Site Kit by Google, Copyright 2022 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.
*/

/**
* External dependencies
*/
import PropTypes from 'prop-types';

/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';

/**
* Internal dependencies
*/
import useWidgetStateEffect from '../hooks/useWidgetStateEffect';
import RecoverableModules from '../../../components/RecoverableModules';

// The supported props must match `RecoverableModules` (except `widgetSlug`).
export default function WidgetRecoverableModules( {
widgetSlug,
moduleSlugs,
...props
} ) {
const metadata = useMemo(
() => ( {
// Here we serialize to `moduleSlug` for compatibility with the logic in
// `combineWidgets()`. In future we may wish to take a less "hacky" approach.
// See https://github.com/google/site-kit-wp/issues/5376#issuecomment-1165771399.
moduleSlug: [ ...moduleSlugs ].sort().join( ',' ),
// We also store `moduleSlugs` in the metadata in order for it to be passed back
// into RecoverableModules as a prop.
// See https://github.com/google/site-kit-wp/blob/c272c20eddcca61aae24c9812b6b11dbc15ec673/assets/js/googlesitekit/widgets/components/WidgetAreaRenderer.js#L171.
moduleSlugs,
} ),
[ moduleSlugs ]
);
useWidgetStateEffect( widgetSlug, RecoverableModules, metadata );

return <RecoverableModules moduleSlugs={ moduleSlugs } { ...props } />;
}

WidgetRecoverableModules.propTypes = {
widgetSlug: PropTypes.string.isRequired,
...RecoverableModules.propTypes,
};
33 changes: 32 additions & 1 deletion assets/js/googlesitekit/widgets/components/WidgetRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,30 +20,52 @@
* External dependencies
*/
import PropTypes from 'prop-types';
import intersection from 'lodash/intersection';

/**
* WordPress dependencies
*/
import { Fragment } from '@wordpress/element';
import { useMemo, Fragment } from '@wordpress/element';

/**
* Internal dependencies
*/
import Data from 'googlesitekit-data';
import { CORE_WIDGETS } from '../datastore/constants';
import { CORE_MODULES } from '../../modules/datastore/constants';
import BaseWidget from './Widget';
import WidgetRecoverableModules from './WidgetRecoverableModules';
import { getWidgetComponentProps } from '../util';
import { HIDDEN_CLASS } from '../util/constants';
import useViewOnly from '../../../hooks/useViewOnly';
import { useFeature } from '../../../hooks/useFeature';

const { useSelect } = Data;

const WidgetRenderer = ( { slug, OverrideComponent } ) => {
const dashboardSharingEnabled = useFeature( 'dashboardSharing' );

const widget = useSelect( ( select ) =>
select( CORE_WIDGETS ).getWidget( slug )
);
const widgetComponentProps = getWidgetComponentProps( slug );
const { Widget, WidgetNull } = widgetComponentProps;

const recoverableModules = useSelect(
( select ) =>
dashboardSharingEnabled &&
select( CORE_MODULES ).getRecoverableModules()
);

const viewOnly = useViewOnly();
const widgetRecoverableModules = useMemo(
() =>
widget &&
recoverableModules &&
intersection( widget.modules, Object.keys( recoverableModules ) ),
[ recoverableModules, widget ]
);

if ( ! widget ) {
return <WidgetNull />;
}
Expand All @@ -52,6 +74,15 @@ const WidgetRenderer = ( { slug, OverrideComponent } ) => {

let widgetElement = <Component { ...widgetComponentProps } />;

if ( viewOnly && widgetRecoverableModules?.length ) {
widgetElement = (
<WidgetRecoverableModules
widgetSlug={ slug }
moduleSlugs={ widgetRecoverableModules }
/>
);
}

if ( OverrideComponent ) {
// If OverrideComponent passed, render it instead of the actual widget.
// It always needs to be wrapped as it is expected to be a
Expand Down
Loading