-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove entity edits when deleting an entity #36030
Conversation
Size Change: +471 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
packages/core-data/src/actions.js
Outdated
@@ -203,6 +203,13 @@ export const deleteEntityRecord = ( | |||
} ); | |||
|
|||
await dispatch( removeItems( kind, name, recordId, true ) ); | |||
|
|||
dispatch( { | |||
type: 'REMOVE_ENTITY_RECORD_EDITS', |
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.
Why a new action, why not just remove the edits on REMOVE_ITEMS
action?
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.
removeItems
is in the queried-data actions file, which made it feel like it should be specific to that reducer.
But I see now RECEIVE_ITEMS
is used in both reducers.
Maybe both receiveItems
and removeItems
should be moved to the base actions.js
file.
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 an action inside the "queried-data" can be used outside but the opposite shouldn't be the case.
In other words, I see the dependency as: "core store" depends on "queried data store" and not the other way around.
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.
Ok, I thought of it more like a hierarchy where queried-data
was a sub-reducer, but I see how it could be considered a dependency.
I've switched it to use removeItems
.
return state.filter( | ||
( undoRecord ) => undoRecord.recordId !== action.recordId | ||
); |
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.
This works, but unfortunately after the removal a new edit is pushed onto the stack for the deleted entity. It isn't undo-able though, the undo button is greyed out. Not sure why, but at least it prevents a buggy state.
I think we also need to avoid pushing edits
on the stack for removed items to solve this.
it( 'removes undo records when an entity is deleted', () => { | ||
undoState = createNextUndoState(); | ||
undoState = createNextUndoState( { value: 1 } ); | ||
undoState = createNextUndoState( { value: 2 } ); | ||
undoState = createNextUndoState( { value: 3 } ); | ||
undoState = createNextUndoState( 'isRemoveItems' ); | ||
expect( undoState ).toEqual( [] ); | ||
} ); |
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.
Test would ideally push a different record onto the undo stack to assert that only the deleted record is removed, but it took me so long to understand how these tests work, and it looks like the whole 'framework' for these tests would need to be re-written to support more than one record.
Maybe after WordPress 5.9 I'll take some time to look at it, but right now time is something I don't have.
Description
Alternative to #36027
If an entity is deleted in an editor, the entity saving panel tries to show it as an
undefined
thing that needs to be saved:Since a delete is immediate, that shouldn't be the case.
This change removes 'edits' and 'undo' records for deleted entities so that they don't appear in the entity panel.
How has this been tested?
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).