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

Issue / 9284 Refactor AuthError Error Notification to Use New Notifications Datastore #9818

Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a86e4ca
Refactor AuthError to use new notification implementations.
10upsimon Dec 4, 2024
1b433fd
Remove AuthError from ErrorNotifications component.
10upsimon Dec 4, 2024
9982b6a
Register new notification implementatiion of AuthError.
10upsimon Dec 4, 2024
d7dce24
Add AuthError story.
10upsimon Dec 5, 2024
1a4919a
Add VRT reference images for AuthError story.
10upsimon Dec 5, 2024
1307e04
Merge in branch develop.
10upsimon Dec 5, 2024
48ce5b8
Change priority to 120 for auth-error notification.
10upsimon Dec 5, 2024
660d1ae
Merge in branch develop.
10upsimon Dec 19, 2024
92ee5ca
Bump priority of notice.
10upsimon Dec 19, 2024
58d5451
Move order of auth-error notification component during registration.
10upsimon Jan 6, 2025
d3fb110
Remove resolveAll call within auth-error checkRequirements given that…
10upsimon Jan 6, 2025
f82c01a
Refactor AuthError story and move registry setup hanled in args to ma…
10upsimon Jan 6, 2025
226b8dd
Merge branch 'develop' into issue/9284-refactor-AuthError-error-notif…
aaemnnosttv Jan 16, 2025
918f070
Merge branch 'develop' of github.com:google/site-kit-wp into develop.
10upsimon Jan 21, 2025
3256d8d
Merge branch 'develop' of github.com:google/site-kit-wp into develop.
10upsimon Jan 21, 2025
478005d
Merge branch 'issue/9284-refactor-AuthError-error-notification-use-ne…
10upsimon Jan 21, 2025
3455ac6
Merge branch 'develop' into issue/9284-refactor-AuthError-error-notif…
10upsimon Jan 21, 2025
102dbd6
Remove AuthError story scneario label.
10upsimon Jan 21, 2025
f66e775
Remove unnecessary call to getAuthentication resolver.
10upsimon Jan 21, 2025
c329711
Reinstate scenario as empty object.
10upsimon Jan 21, 2025
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
38 changes: 23 additions & 15 deletions assets/js/components/notifications/AuthError.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,24 +26,32 @@ import { __ } from '@wordpress/i18n';
*/
import { useSelect } from 'googlesitekit-data';
import { CORE_USER } from '../../googlesitekit/datastore/user/constants';
import BannerNotification from './BannerNotification';
import NotificationError from '../../googlesitekit/notifications/components/layout/NotificationError';
import CTALink from '../../googlesitekit/notifications/components/common/CTALink';
import Description from '../../googlesitekit/notifications/components/common/Description';

export default function AuthError() {
export default function AuthError( { id, Notification } ) {
const error = useSelect( ( select ) => select( CORE_USER ).getAuthError() );
if ( ! error ) {
return null;
}

return (
<BannerNotification
id="autherror"
title={ __(
'Site Kit can’t access necessary data',
'google-site-kit'
) }
description={ error.message }
ctaLink={ error.data.reconnectURL }
ctaLabel={ __( 'Redo the plugin setup', 'google-site-kit' ) }
/>
<Notification className="googlesitekit-publisher-win googlesitekit-publisher-win--win-error">
<NotificationError
title={ __(
'Site Kit can’t access necessary data',
'google-site-kit'
) }
description={ <Description text={ error.message } /> }
actions={
<CTALink
id={ id }
ctaLabel={ __(
'Redo the plugin setup',
'google-site-kit'
) }
ctaLink={ error.data.reconnectURL }
/>
}
/>
</Notification>
);
}
73 changes: 73 additions & 0 deletions assets/js/components/notifications/AuthError.stories.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* AuthError Component Stories.
*
* Site Kit by Google, Copyright 2024 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.
*/

/**
* Internal dependencies
*/
import WithRegistrySetup from '../../../../tests/js/WithRegistrySetup';
import { provideUserAuthentication } from '../../../../tests/js/utils';
import { CORE_USER } from '../../googlesitekit/datastore/user/constants';
import AuthError from './AuthError';
import { withNotificationComponentProps } from '../../googlesitekit/notifications/util/component-props';

const NotificationWithComponentProps =
withNotificationComponentProps( 'auth-error' )( AuthError );

function Template() {
return <NotificationWithComponentProps />;
}

export const AuthenticationError = Template.bind( {} );
AuthenticationError.storyName = 'AuthError';
AuthenticationError.args = {
setupRegistry: ( registry ) => {
provideUserAuthentication( registry );

const authError = {
code: 'missing_delegation_consent',
status: 401,
message:
'Looks like your site is not allowed access to Google account data and can’t display stats in the dashboard.',
data: {
reconnectURL: 'https://example.com/',
},
};

registry.dispatch( CORE_USER ).setAuthError( authError );
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this file contains only one scenario data provided here covers the default one, we can move this down to the main export, above the args.setupRegistry( registry ); or instead, since we do not expect more stories, and this can be added in the future if there is a need to expand on scenarios. So this scenario does not need to pass any arguments as they will be handled in the defaul export

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @zutigrm , this has been moved into the default export in my latest commits.

};
AuthenticationError.scenario = {
label: 'Components/Notifications/Banners/AuthError',
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
};

export default {
title: 'Components/Notifications/Banners',
decorators: [
( Story, { args } ) => {
const setupRegistry = ( registry ) => {
args.setupRegistry( registry );
};

return (
<WithRegistrySetup func={ setupRegistry }>
<Story />
</WithRegistrySetup>
);
},
],
};
2 changes: 0 additions & 2 deletions assets/js/components/notifications/ErrorNotifications.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import { Fragment } from '@wordpress/element';
/**
* Internal dependencies
*/
import AuthError from './AuthError';
import InternalServerError from './InternalServerError';
import Notifications from './Notifications';
import { NOTIFICATION_AREAS } from '../../googlesitekit/notifications/datastore/constants';
Expand All @@ -33,7 +32,6 @@ export default function ErrorNotifications() {
return (
<Fragment>
<InternalServerError />
<AuthError />
<Notifications areaSlug={ NOTIFICATION_AREAS.ERRORS } />
</Fragment>
);
Expand Down
24 changes: 24 additions & 0 deletions assets/js/googlesitekit/notifications/register-defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
import { isZeroReport } from '../../modules/analytics-4/utils';
import { MODULES_SEARCH_CONSOLE } from '../../modules/search-console/datastore/constants';
import { READ_SCOPE as TAGMANAGER_READ_SCOPE } from '../../modules/tagmanager/datastore/constants';
import AuthError from '../../components/notifications/AuthError';
import UnsatisfiedScopesAlert from '../../components/notifications/UnsatisfiedScopesAlert';
import UnsatisfiedScopesAlertGTE from '../../components/notifications/UnsatisfiedScopesAlertGTE';
import GatheringDataNotification from '../../components/notifications/GatheringDataNotification';
Expand Down Expand Up @@ -566,6 +567,29 @@ export const DEFAULT_NOTIFICATIONS = {
);
},
},
'auth-error': {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's group errors together, this one should go more up, just after the setup_plugin_error one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @zutigrm , this has been moved in my latest commits.

Component: AuthError,
priority: 130,
areaSlug: NOTIFICATION_AREAS.ERRORS,
viewContexts: [
VIEW_CONTEXT_MAIN_DASHBOARD,
VIEW_CONTEXT_MAIN_DASHBOARD_VIEW_ONLY,
VIEW_CONTEXT_ENTITY_DASHBOARD,
VIEW_CONTEXT_ENTITY_DASHBOARD_VIEW_ONLY,
VIEW_CONTEXT_SETTINGS,
],
checkRequirements: async ( { select, resolveSelect } ) => {
await Promise.all( [
// The getAuthError() selector relies on the resolution of getAuthentication().
resolveSelect( CORE_USER ).getAuthentication(),
] );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since getAuthentication resolver is the only one that needs to be resolved, we do not need to use Promise.all, we can just resolve it directly:

Suggested change
await Promise.all( [
// The getAuthError() selector relies on the resolution of getAuthentication().
resolveSelect( CORE_USER ).getAuthentication(),
] );
await resolveSelect( CORE_USER ).getAuthentication();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @zutigrm , this has been change to a single resolve in my latest commits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zutigrm this actually wasn't needed. See #9818 (comment)


const error = select( CORE_USER ).getAuthError();

return !! error;
},
isDismissible: false,
},
};

/**
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading