Remove entity edits when deleting an entity #36030
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
7 tasks
|
Size Change: +471 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Oct 28, 2021
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.
Why a new action, why not just remove the edits on REMOVE_ITEMS action?
There was a problem hiding this comment.
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.
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.
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.
talldan
commented
Oct 29, 2021
Comment on lines
+512
to
+514
| return state.filter( | ||
| ( undoRecord ) => undoRecord.recordId !== action.recordId | ||
| ); |
There was a problem hiding this comment.
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.
talldan
commented
Oct 29, 2021
Comment on lines
+349
to
+356
| 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.
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.
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Alternative to #36027
If an entity is deleted in an editor, the entity saving panel tries to show it as an
undefinedthing 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.jsfiles for terms that need renaming or removal).