Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core Data: Test entity undo reducer. #18642

Merged
merged 3 commits into from Nov 23, 2019
Merged

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 20, 2019

Closes #17434

Description

This PR adds unit tests for core-data's new undo reducer.

It also fixes a minor edge case where we were updating a reference to the last edit action even if the new action only contained transient edits. This was causing us to skip/delete an undo level when explicitly marking the last changes as persistent.

How has this been tested?

They are tests 馃槃

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
lastEdits = {};
undoState = undefined;
expectedUndoState = [];
expectedUndoState.offset = 0;

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 21, 2019

Member

This is interesting to see in a state, as normally the state should be serialisable. Could you comment on this in the reducer? Why can't it be nested?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 21, 2019

Author Contributor

It could be nested. I just followed from the previous iteration and I think I've seen other places where this is done.

I guess we should change it in another PR if we need to serialize it.

let action = {};
if ( args[ 0 ] === 'isUndo' || args[ 0 ] === 'isRedo' ) {
// We need to "apply" the undo level here and build
// the action to move the offset.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 21, 2019

Member

Why does the action need to be build based on the state? Isn't that something that can be done in the reducer?

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 21, 2019

Member

Previously we had UNDO and REDO actions.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 21, 2019

Author Contributor

Because the editor grabs the undo edit and applies it, the undo reducer just keeps track of the offset.

// Don't update the last edit cache if the new one only has transient edits.
// Transient edits don't create new levels so updating the cache would make
// us skip an edit later when creating levels explicitly.
Object.keys( action.edits ).some( ( key ) => ! action.transientEdits[ key ] )

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 21, 2019

Member

Is there an e2e test case that could fail without this change?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 21, 2019

Author Contributor

No, it's not a flow that happens in the editor, but it could happen with other third party usage.

transientEdits,
};
action = {
type: 'EDIT_ENTITY_RECORD',

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 21, 2019

Member

I really liked how the previous history reduces could work with any state and actions. This one seems to handle EDIT_ENTITY_RECORD. Is there any way it could be more abstracted? Cc @aduth.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 21, 2019

Author Contributor

What other actions does it need to handle for entities?

I don't think this is how this reducer will look like in the long term, specially after adding support for collaborative editing.

But, for now it gives us a lot of control over details that allow us to meet requirements for things like explicit levels and other nuances that the editor and block editor need. A more generalized abstraction could make that harder.

Copy link
Member

ellatrix left a comment

Thanks! It's great to have some tests :)

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 21, 2019

@ellatrix My change to the reducer needed a tweak. I would appreciate another look 馃槃

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Nov 22, 2019

In 3bcddbe, I see changed and added code, but no extra tests. Would it make sense to have tests for this part?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 22, 2019

@ellatrix Yeah, but it fixed an e2e test that was failing, so it's already covered.

@epiqueras epiqueras force-pushed the add/core-data-undo-reducer-unit-tests branch from e817ff6 to f3ac6ba Nov 23, 2019
@epiqueras epiqueras force-pushed the add/core-data-undo-reducer-unit-tests branch from f3ac6ba to 05e4845 Nov 23, 2019
@epiqueras epiqueras merged commit ba8cd71 into master Nov 23, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras deleted the add/core-data-undo-reducer-unit-tests branch Nov 23, 2019
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.0 Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.