Add new autosave fields to the PostModel #1357
Conversation
There was a problem hiding this comment.
Thanks @maxme! I've left some comments. Let me know what you think.
| @@ -708,7 +711,8 @@ private void handleFetchedPostList(FetchPostListResponsePayload payload) { | |||
| // will not be updated. | |||
| boolean isPostChanged = | |||
| !post.getLastModified().equals(item.lastModified) | |||
| || !post.getStatus().equals(item.status); | |||
| || !post.getStatus().equals(item.status) | |||
| || item.autoSaveRevisionId > 0; | |||
There was a problem hiding this comment.
Can you pls elaborate why we compare item.autoSaveRevisionId > 0 instead of comparing item.autoSaveRevisionId != post.autoSaveRevisionId? It seems the code will mark the post as "potential conflict/isPostChanged" whenever the post contains a remoteAutoSave object. Is that what we want?
Imagine the following scenario
- We edit a published post and leave the editor
- The app remote-auto-saves it
- We fetch the post list ->
item.autoSaveRevisionId > 0is true since the post has unpublished revision -> a conflict is detected even though the current local copy of the post is equal to theAutoSavedcopy.
Wdyt?
There was a problem hiding this comment.
Right, I was a bit confused, the idea was to always get the "unhandled auto save" unless we have some local modifications (which happens when you push a remote-auto-save from the app).
I'll follow your suggestion, this won't change the current behavior, but this might be less confusing if we update the logic later.
| @@ -736,6 +740,7 @@ private void handleFetchedPostList(FetchPostListResponsePayload payload) { | |||
| // both locally and on the remote), so flag the local version of the Post so the | |||
| // hosting app can inform the user and the user can decide and take action | |||
| post.setRemoteLastModified(item.lastModified); | |||
| post.setAutoSaveRevisionId(item.autoSaveRevisionId); | |||
There was a problem hiding this comment.
I'd suggest introducing a new field for this scenario -> something like autoSaveRemoteRevisionId or another options is to update all the auto-save fields here. As otherwise we might end up in an inconsistent state.
Eg. we would update autoSaveRevisionId but all the other fields of the AutoSave object would still contain content related to the previous revisionId. Wdyt?
There was a problem hiding this comment.
Btw I think we might want to remove mRemoteAutoSaveModified since we don't use it for conflict detection anymore.
There was a problem hiding this comment.
Ok, do you see any future usage of mRemoteAutoSaveModified? For a version 2 of the conflict dialog?
There was a problem hiding this comment.
Sidenote: would be great to add tests for this.
There was a problem hiding this comment.
Sidenote: would be great to add tests for this.
We have some tests for this method
Ok, do you see any future usage of mRemoteAutoSaveModified? For a version 2 of the conflict dialog?
Yes, I think we'll need it for v2.
There was a problem hiding this comment.
Thanks @maxme! I've left one comment. And as we discussed on the call my suggestion to update all the sub-fields of the autosave object wasn't ideal. I'll create an extra PR which will introduce remoteAutoSaveRevisionId and we'll update only that field instead of updating all the autoSave object subfields.
| @@ -721,8 +730,7 @@ private void handleFetchedPostList(FetchPostListResponsePayload payload) { | |||
| * was updated. However, if we know the last modified date is equal to the date we have in local | |||
| * autosave object we are sure that our invocation of /autosave updated the post directly. | |||
| */ | |||
| if (isPostChanged && item.lastModified.equals(post.getAutoSaveModified()) | |||
| && item.autoSaveModified == null) { | |||
| if (isPostChanged && item.lastModified.equals(post.getAutoSaveModified())) { | |||
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
Not intentional 🤦♂ but I actually found why it was removed (removed a field from PostListItem). Did an extra commit (quite useless since you'll open a followup) 57cc768
Add 3 new fields to the PostModel + migration plan, to be used in wpandroid for restoring title/content/excerpt.
Also updated the
PostStoreandPostRestClientto update the AutoSaveRevisionId when fetching the post list (needed to detect unhandled auto saves in the post list).Autosave restoration logic will be done on the client side.