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: Add support for autosaving entities. #16903

Merged
merged 3 commits into from Aug 6, 2019

Conversation

@epiqueras
Copy link
Contributor

commented Aug 5, 2019

Continues #16867

Description

This PR adds support for autosaving entities in Core Data and makes the recently created "current undo offset" selector private.

How has this been tested?

It has not been tested 馃槵

Types of Changes

New Feature: Add autosaving support to Core Data entities.

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.
'getAutosave',
persistedRecord.type,
persistedRecord.id,
currentUserId

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

Any reason we need the currentUserId to fetch the autosaved post?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

Autosaves are author-specific.

return acc;
}, {} );
const autosave = yield apiFetch( {
path: `${ path }/autosaves`,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

Do you think this is a convention for all entities autosaves? Do we need to check whether a given entity supports autosaves?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

I thought it was. Should it be part of the entity config?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

We can start this way if it's consistent across WP entities, I was just asking.

);
let data = { ...persistedRecord, ...autosavePost, ...record };
data = Object.keys( data ).reduce( ( acc, key ) => {
if ( key in [ 'title', 'excerpt', 'content' ] ) {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

Is this something to be configured per entity (autosaveable fields or something)

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

Yeah, good idea.

kind,
name,
record,
{ isAutosave = false } = { isAutosave: false }

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

Based on the code changes, it feels that the common code is very small between autosaves and saves, which makes me wonder if they should be two separate action creators.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

I thought that maybe future options could make them share more code.

Should we separate them for now?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

I'm fine either way, you're in a better position to judge as you worked on it :)

const currentUser = yield select( 'getCurrentUser' );
const currentUserId = currentUser ? currentUser.id : undefined;
const autosavePost = yield select(
'getAutosave',

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

Where is this selector defined?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

export function getAutosave( state, postType, postId, authorId ) {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

oh so it was defined before hand? Do you think a user can access autosaves of another user?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

Probably, they are just posts right?

persistedRecord.id,
currentUserId
);
let data = { ...persistedRecord, ...autosavePost, ...record };

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

I'm having trouble understanding which data we're sending. Maybe a comment could help. Like do we send just specific record properties, everything, why are we merging these three records, and why that order.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

I've added this comment:

// Autosaves need all expected fields to be present.
// So we fallback to the previous autosave and then
// to the actual persisted entity if the edits don't
// have a value.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

I guess I'm wondering why we need the three steps merge. If I'm undersanding properly, we don't really need autosavePost here since the edits will most likely contain these as well right?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

Not always.

It has 3 items because we have 2 fallbacks. First, the previous autosave, and then the persisted values.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

Really? The UI only shows the edits and not the autosaved content so I'm having trouble thinking of use-cases where autosaves have more data than edits.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

you would but that's an explicit action. saveEntityRecord( getAutosave() )

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

That would not work, you would need to merge the properties like in this PR.

Why complicate things when this will be the only usage of the API. When would you want to clear autosaved properties without changes?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

How does it work today in the editor screen without the refactoring? Say I have an autosave but I ignore it and I continue editing my post, another autosave will trigger right without the updates from the previous autosave?

How would you do that in this new approach?

To be honest, I don't want to complicate anything. I actually think I'm simplifying things. Anyway, I won't block that PR for this, I feel we can tweak this later if needed but still cuirious about the answer to the question above.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 5, 2019

Author Contributor

We use this fallback mechanism in the editor store right now.

It works like this right now.

Say I have an autosave but I ignore it and I continue editing my post, another autosave will trigger right without the updates from the previous autosave?

It will have the updates from the previous autosave.

toSend = {
...pick( post, AUTOSAVE_PROPERTIES ),
...mappedAutosavePost,
...toSend,
};

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 6, 2019

Contributor

It works like this right now.

Ok let's keep it but it feels like a bug to me. cc @adamsilverstein if you know whether an autosave should be created based on the previous autosave or based on the things that are shown in the UI (previous saved state + edits)

@epiqueras epiqueras merged commit 0b853e8 into master Aug 6, 2019

1 of 4 checks passed

Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@epiqueras epiqueras added this to Done in Phase 2 via automation Aug 6, 2019

@epiqueras epiqueras deleted the add/core-data-support-for-autosaving branch Aug 6, 2019

@epiqueras epiqueras referenced this pull request Aug 6, 2019
5 of 5 tasks complete

@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.3 Aug 9, 2019

gziolo added a commit that referenced this pull request Aug 29, 2019
Core Data: Add support for autosaving entities. (#16903)
* Core Data: Make current offset selector private.

* Core Data: Add support for autosaving entities.

* Core Data: Clarify why and how autosave payload data is gathered, with a comment.
gziolo added a commit that referenced this pull request Aug 29, 2019
Core Data: Add support for autosaving entities. (#16903)
* Core Data: Make current offset selector private.

* Core Data: Add support for autosaving entities.

* Core Data: Clarify why and how autosave payload data is gathered, with a comment.
@epiqueras epiqueras referenced this pull request Aug 29, 2019
5 of 5 tasks complete
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.