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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize fetch post list call #1364

Merged
merged 5 commits into from
Aug 27, 2019

Conversation

malinajirka
Copy link
Contributor

@malinajirka malinajirka commented Aug 23, 2019

This PR contains a couple of changes:

  • We stop storing most of AutoSave object subfields into PostListItem. The main reason for this change is that PostListItem should request only vital fields for determining whether the object was modified in the remote (we want to fetch the full object only when it was modified to save users' data). We currently request the whole AutoSave object, but we plan to optimize the call and request only autosave.lastModified very soon - Fetch only autosave.modifiedDate object instead of the whole meta object #1277 (PR on the API is almost ready to be merged).
  • It fixes an issue in handleFetchedPostList where isPostChanged field was comparing item.autoSave.revisionId != post.getAutoSaveRevisionId(). The issue was that autoSaveRevisionId doesn't change when the content is overridden with newer autosave. It's safer to compare autosave.lastModified instead which is always updated whenever something changes in the Autosave object.
  • It "simplifies" handleFetchedPostList -> the logic is following
  1. The post is not locally changed -> if the post or its autosave object changes, we want to fetch it.
  2. The post is locally changed and the post itself is changed -> we don't want to fetch the post and we want to update remoteLastModified (aka mark it as "in conflict with remote")
  3. The post is locally changed and its autosave object changes -> we don't want to fetch the post (this scenario will be handled in conflict resolution v2). We can simply ignore this case since the UI shows the Unpublished revision dialog only when there are no local changes.
  4. The post is locally changed and both the post itself and its autosave object changes -> We can proceed with 2. (the autosave conflict resolution will be handled in v2)

wordpress-mobile/WordPress-Android#10426 is targeting this branch so we'll test these changes as part of the PR in WPAndroid.

P.S. I'll add some test in another PR later today.

@shiki
Copy link
Member

shiki commented Aug 26, 2019

@malinajirka

wordpress-mobile/WordPress-Android#10426 is targeting this branch so we'll test these changes as part of the PR in WPAndroid.

Sorry, I'm having trouble understanding this. Should I wait to review until #10426 is merged? Or the other way around?

P.S. I'll add some test in another PR later today.

I'm hoping to see those first before diving in since, at first glance, this seems like a complicated change and the tests would help me understand this better. 😬

@malinajirka
Copy link
Contributor Author

malinajirka commented Aug 27, 2019

wordpress-mobile/WordPress-Android#10426 is targeting this branch so we'll test these changes as part of the PR in WPAndroid.
Sorry, I'm having trouble understanding this. Should I wait to review until #10426 is merged? Or the other way around?

You can test the functionality of this PR in wordpress-mobile/WordPress-Android#10426. However, this PR should be merged first.

I'm hoping to see those first before diving in since, at first glance, this seems like a complicated change and the tests would help me understand this better. 😬

It's actually not that complicated. I discussed this change with Maxime, but unfortunately he didn't have time to review it.
We merged https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/1357/files last week and this PR is just modifying some of the code which was added there. I'll add some tests, but they'll basically tests scenarios 1-4 described above.

Let me know if you rather want to jump on a quick call.

@malinajirka
Copy link
Contributor Author

Added unit tests in d4ceb54

@shiki shiki merged commit a546e39 into master-remote-auto-save Aug 27, 2019
@shiki shiki deleted the optimize-fetch-post-list-call branch August 27, 2019 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants