Issue / 9284 Refactor AuthError Error Notification to Use New Notifications Datastore #9818
Conversation
|
Build files for c329711 have been deleted. |
|
Size Change: -472 B (-0.02%) Total Size: 1.99 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @10upsimon almost there, I left you few small comments
| @@ -566,6 +567,29 @@ export const DEFAULT_NOTIFICATIONS = { | |||
| ); | |||
| }, | |||
| }, | |||
| 'auth-error': { | |||
There was a problem hiding this comment.
Let's group errors together, this one should go more up, just after the setup_plugin_error one
There was a problem hiding this comment.
Thanks @zutigrm , this has been moved in my latest commits.
| await Promise.all( [ | ||
| // The getAuthError() selector relies on the resolution of getAuthentication(). | ||
| resolveSelect( CORE_USER ).getAuthentication(), | ||
| ] ); |
There was a problem hiding this comment.
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:
| await Promise.all( [ | |
| // The getAuthError() selector relies on the resolution of getAuthentication(). | |
| resolveSelect( CORE_USER ).getAuthentication(), | |
| ] ); | |
| await resolveSelect( CORE_USER ).getAuthentication(); |
There was a problem hiding this comment.
Thanks @zutigrm , this has been change to a single resolve in my latest commits.
There was a problem hiding this comment.
@zutigrm this actually wasn't needed. See #9818 (comment)
| 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 ); | ||
| }, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks @zutigrm , this has been moved into the default export in my latest commits.
… there is only one promise to resolve.
…in default export, as no extra stories are intended to be added here.
…ication-use-new-notifications-datastore.
There was a problem hiding this comment.
Thanks @10upsimon ! A few minor things to address here but this overall LGTM 👍
Just as a reminder, please also keep your branch up to date whenever you're iterating on it. I sync'd it here to also run E2E again (as they're all failing) but it was quite far behind
| await resolveSelect( CORE_USER ).getAuthentication(); | ||
| const error = select( CORE_USER ).getAuthError(); |
There was a problem hiding this comment.
getAuthError doesn't depend on data from getAuthentication, so the resolveSelect isn't needed/relevant here.
See
site-kit-wp/assets/js/googlesitekit/datastore/user/authentication.js
Lines 283 to 286 in 226b8dd
All selectors that depend on data from the server-side authentication state should use
site-kit-wp/assets/js/googlesitekit/datastore/user/authentication.js
Lines 31 to 36 in 226b8dd
A quick search through the code shows that authError only exists on the client side which is only called here
site-kit-wp/assets/js/googlesitekit/api/index.js
Lines 95 to 108 in 226b8dd
There was a problem hiding this comment.
Thanks @aaemnnosttv , I've since removed the call to the resolver, there are now none 👍
…w-notifications-datastore' of github.com:google/site-kit-wp into issue/9284-refactor-AuthError-error-notification-use-new-notifications-datastore.
…ication-use-new-notifications-datastore.
aaemnnosttv
deleted the
issue/9284-refactor-AuthError-error-notification-use-new-notifications-datastore
branch
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist