DataViews: Register the permanentlyDelete action like any third-party action. #62647
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
mcsf,
sirreal,
jorgefilipecosta,
ntsekouras and
fabiankaegy
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| @@ -1007,6 +918,7 @@ export const duplicateTemplatePartAction = { | |||
| }; | |||
|
|
|||
| export function usePostActions( { postType, onActionPerformed, context } ) { | |||
| useDefaultActions(); | |||
There was a problem hiding this comment.
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:
- Move this call to the top level component in both post and site editors? Not ideal to have to duplicate this call.
- Avoid using a hook and just call this top level, when loading the module.
- Use a "filter" instead of suggesting to third-parties to use the unregister functions?
@gziolo maybe you have better ideas here since you worked on similar things for blocks registrations..
There was a problem hiding this comment.
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.
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.
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.
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.
| ( state: State, kind: string, name: string ) => { | ||
| return [ | ||
| ...( state.actions[ kind ]?.[ name ] ?? [] ), | ||
| ...( state.actions[ kind ]?.[ '*' ] ?? [] ), |
There was a problem hiding this comment.
I implemented the * to register actions for all post types.
| | '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".
|
Size Change: +860 B (+0.05%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
| 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:
- Support hook-based actions on the API.
- Make is eligible something async and then it is up to the component users of isEligible to make sure the component rerender after isEligible finishes the computation.
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.
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.
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.
|
So this PR is actually blocked. I can't rebase this PR unless we find a solution for this first. #62647 (comment) |
|
Going to close this PR for now, let's get back to this when we have a good way to access stores in |
Related #61084
What?
In #62052 an API to register and unregister dataviews actions has been implemented. But in order to allow third-party developers to be able to unregister these actions, we need to be using the same actions in Core to register the core actions.
The current PR explore the possibility to use the API to register one action: "permanently delete". Check the inline comments for more details about the decisions made...
Testing Instructions
1- Open the "trash" pages dataviews.
2- You should be able to see the "permanently delete" action in the actions dropdown menu
3- you can try to use the action.