DataViews: Register the permanentlyDelete action like any third-party action. #62647
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
|
|
||
|
|
||
| .edit-site-editor__editor-interface { | ||
| opacity: 1; | ||
| transition: opacity 0.1s ease-out; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { store as coreStore } from '@wordpress/core-data'; | ||
| import { store as noticesStore } from '@wordpress/notices'; | ||
| import { __, sprintf } from '@wordpress/i18n'; | ||
| import type { Action } from '@wordpress/dataviews'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { getItemTitle } from './utils'; | ||
| import type { Post } from '../types'; | ||
| import { | ||
| TEMPLATE_POST_TYPE, | ||
| TEMPLATE_PART_POST_TYPE, | ||
| } from '../../store/constants'; | ||
|
|
||
| type CoreDataError = { | ||
| reason: { message?: string }; | ||
| }; | ||
|
|
||
| const permanentlyDeletePostAction: Action< Post > = { | ||
|
There was a problem hiding this comment. Hi @youknowriad, in order to know if the user has the right permissions to delete or trash a post, we need to call some selectors. Currently, we have regressions and are showing trash action even when we should not. I'm fixing that for Trash action at #62589 (which we should include for 6.6). We also need to do the same to delete permanently but we could not use a selector to check permissions in the API I guess we need to fix that at the API level. There was a problem hiding this comment. Also, something where we would check permissions to delete before registering the action would not work, the user may be able to delete some items (e.g.: the items they created or draft items) but not other items. There was a problem hiding this comment. The "callback" can receive the "registry" object, we might want to do the same for the isElligible. There was a problem hiding this comment. Hi @youknowriad, I think simply passing the registry to isElligible is not a solution and will not work as expected. In the beginning, while checks are being done canUser will return false, and if isEligible is false then canUser may say true but isEligible is not re-executed because there is nothing that causes a React rerender. This is not a theoretical issue it was happening on #62589 and that was the reason I used the hacky solution of getCachedResolvers around useMemo to trigger the rerender. I see two possible ways forward:
There was a problem hiding this comment. What if we just call isEligible within useSelect and pass "select". I'm not sure if I like it but it's a solution as well. There was a problem hiding this comment.
It is also a possible solution, it is going to be called a lot e.g: in a list with 50 times we will have 50 useSelects calling is eligible running on each store change. That may be ok. Another downside is that it is not very flexible it forces people to use WordPress data e.g.: I can not check if something is eligible on my own API without using resolvers etc. |
||
| id: 'permanently-delete', | ||
| label: __( 'Permanently delete' ), | ||
| supportsBulk: true, | ||
| isEligible( { status, type }: Post ) { | ||
| const isTemplateOrTemplatePart = [ | ||
| TEMPLATE_POST_TYPE, | ||
| TEMPLATE_PART_POST_TYPE, | ||
| ].includes( type ); | ||
|
|
||
| return ! isTemplateOrTemplatePart && status === 'trash'; | ||
| }, | ||
| async callback( posts, { registry } ) { | ||
| const { createSuccessNotice, createErrorNotice } = | ||
| registry.dispatch( noticesStore ); | ||
| const { deleteEntityRecord } = registry.dispatch( coreStore ); | ||
| const promiseResult = await Promise.allSettled( | ||
| posts.map( ( post ) => { | ||
| return deleteEntityRecord( | ||
| 'postType', | ||
| post.type, | ||
| post.id, | ||
| { force: true }, | ||
| { throwOnError: true } | ||
| ); | ||
| } ) | ||
| ); | ||
| // If all the promises were fulfilled with success. | ||
| if ( promiseResult.every( ( { status } ) => status === 'fulfilled' ) ) { | ||
| let successMessage; | ||
| if ( promiseResult.length === 1 ) { | ||
| successMessage = sprintf( | ||
| /* translators: The posts's title. */ | ||
| __( '"%s" permanently deleted.' ), | ||
| getItemTitle( posts[ 0 ] ) | ||
| ); | ||
| } else { | ||
| successMessage = __( 'The posts were permanently deleted.' ); | ||
| } | ||
| createSuccessNotice( successMessage, { | ||
| type: 'snackbar', | ||
| id: 'permanently-delete-post-action', | ||
| } ); | ||
| } else { | ||
| // If there was at lease one failure. | ||
| let errorMessage; | ||
| // If we were trying to permanently delete a single post. | ||
| if ( promiseResult.length === 1 ) { | ||
| const result = promiseResult[ 0 ] as CoreDataError; | ||
| if ( result.reason?.message ) { | ||
| errorMessage = result.reason.message; | ||
| } else { | ||
| errorMessage = __( | ||
| 'An error occurred while permanently deleting the post.' | ||
| ); | ||
| } | ||
| // If we were trying to permanently delete multiple posts | ||
| } else { | ||
| const errorMessages = new Set(); | ||
| const failedPromises = promiseResult.filter( | ||
| ( { status } ) => status === 'rejected' | ||
| ); | ||
| for ( const failedPromise of failedPromises ) { | ||
| const result = failedPromise as CoreDataError; | ||
youknowriad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if ( result.reason?.message ) { | ||
| errorMessages.add( result.reason.message ); | ||
| } | ||
| } | ||
| if ( errorMessages.size === 0 ) { | ||
| errorMessage = __( | ||
| 'An error occurred while permanently deleting the posts.' | ||
| ); | ||
| } else if ( errorMessages.size === 1 ) { | ||
| errorMessage = sprintf( | ||
| /* translators: %s: an error message */ | ||
| __( | ||
| 'An error occurred while permanently deleting the posts: %s' | ||
| ), | ||
| [ ...errorMessages ][ 0 ] | ||
| ); | ||
| } else { | ||
| errorMessage = sprintf( | ||
| /* translators: %s: a list of comma separated error messages */ | ||
| __( | ||
| 'Some errors occurred while permanently deleting the posts: %s' | ||
| ), | ||
| [ ...errorMessages ].join( ',' ) | ||
| ); | ||
| } | ||
| } | ||
| createErrorNotice( errorMessage, { | ||
| type: 'snackbar', | ||
| } ); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| export default permanentlyDeletePostAction; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useEffect } from '@wordpress/element'; | ||
| import { useDispatch } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import deletePermanently from './delete-permanently'; | ||
| import { unlock } from '../../lock-unlock'; | ||
| import { store as editorStore } from '../../store'; | ||
|
|
||
| export default function useDefaultActions() { | ||
| const { registerEntityAction, unregisterEntityAction } = unlock( | ||
| useDispatch( editorStore ) | ||
| ); | ||
|
|
||
| useEffect( () => { | ||
| registerEntityAction( 'postType', '*', deletePermanently ); | ||
|
|
||
| return () => { | ||
| unregisterEntityAction( 'postType', '*', deletePermanently.id ); | ||
| }; | ||
| }, [ registerEntityAction, unregisterEntityAction ] ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { decodeEntities } from '@wordpress/html-entities'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import type { Post } from '../types'; | ||
|
|
||
| export function getItemTitle( item: Post ) { | ||
| if ( typeof item.title === 'string' ) { | ||
| return decodeEntities( item.title ); | ||
| } | ||
| return decodeEntities( item.title?.rendered || '' ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,22 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { createSelector } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import type { State } from './reducer'; | ||
|
|
||
| export const getEntityActions = createSelector( | ||
| ( state: State, kind: string, name: string ) => { | ||
| return [ | ||
| ...( state.actions[ kind ]?.[ name ] ?? [] ), | ||
| ...( state.actions[ kind ]?.[ '*' ] ?? [] ), | ||
|
There was a problem hiding this comment. I implemented the |
||
| ]; | ||
| }, | ||
| ( state: State, kind: string, name: string ) => [ | ||
| state.actions[ kind ]?.[ name ], | ||
| state.actions[ kind ]?.[ '*' ], | ||
| ] | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import type { AnyItem } from '@wordpress/dataviews'; | ||
|
|
||
| type PostStatus = | ||
| | 'published' | ||
| | 'draft' | ||
| | 'pending' | ||
| | 'private' | ||
| | 'future' | ||
| | 'auto-draft' | ||
| | 'trash'; | ||
|
|
||
| export interface Post extends AnyItem { | ||
|
There was a problem hiding this comment. Ideally these "Model" types should be in core-data but it's fine for now here. I think ultimately the whole "dataviews" folder here will move to "core-data". |
||
| status: PostStatus; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably not the right place to use this hook.
Because ideally, this only run ones and third-parties should be able to unregister the core actions even before they are actually rendered. So I'm not sure what the best path forward here:
@gziolo maybe you have better ideas here since you worked on similar things for blocks registrations..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "Move this call to the top level component in both post and site editors?" is the better approach. Could we put on some top-level editor component used by both edit site and edit post so we avoid the duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The site editor is not just an "Editor", it's the equivalent of the "whole admin". The editor can be unmounted there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point in that case I would still opt for "Move this call to the top level component in both post and site editors?" and would duplicate the call on edit post and edit site, it is just one line of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding block registration, we put some similar code in all top-level components of the usual apps: editor, post editor, widgets editor, etc.
Site editor:
gutenberg/packages/edit-site/src/index.js
Lines 37 to 41 in 1050efa
Post editor:
gutenberg/packages/edit-post/src/index.js
Lines 73 to 86 in 1050efa
There are some legacy implications, so it might be more complex than necessary there. However, the biggest challenge with filters is that they might get applied before or after the top-level component gets mounted, or even before or after the individual store action implementing a filter gets executed. In effect, we decided in the past to account for it. The comment in the code might better illustrate that:
gutenberg/packages/blocks/src/store/actions.js
Lines 33 to 46 in 1050efa
As with everything, it all comes with defining the overall strategy knowing that 3rd party depending on the defined script dependencies might load before or after the code that defines the default data view action handlers. The biggest challenge with unregistering the action handlers is also the fact that you need to ensure that the script gets executed after it gets registered, which is sometimes problematic when some other code has the opposite requirement. I'm mostly illustrating the considerations that we have been witnessing in the bug reports over time with the extensibility model we iterated upon for block registration. I'm happy to discuss the implications of the approach you prefer to take here and help to think about potential edge cases.