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

Some posts with local changes are never uploaded. #12000

Closed
diegoreymendez opened this issue Jun 25, 2019 · 5 comments
Closed

Some posts with local changes are never uploaded. #12000

diegoreymendez opened this issue Jun 25, 2019 · 5 comments

Comments

@diegoreymendez
Copy link
Contributor

diegoreymendez commented Jun 25, 2019

Expected behavior

Posts with local changes are uploaded when you have a connection.

Actual behavior

If, while you're in the editor with local changes, you force the App to close (without really closing the editor first), when you open the back again, you can see a post with local changes that is never uploaded.

Steps to reproduce the behavior

If, while you're in the editor with local changes, you force the App to close, when you open the back again, you can see a post with local changes that is never uploaded.

More information:

This probably happens because the PostCoordaintor currently looks for failed posts only:

func resume() {
let service = PostService(managedObjectContext: mainContext)
service.getFailedPosts { [weak self] posts in
guard let self = self else {
return
}
posts.forEach() { self.retrySave(of: $0 ) }
}
}

Failed posts are posts that we attempted to save / upload, but that failed due to not having connectivity.

We need to improve this logic someway to include any posts with local changes (but we also need to make sure this is not triggered for posts that are currently being edited).

We should also consider what to do with published posts and scheduled posts with local changes.

Tested on iPhone XS, iOS 12.3.1, WPiOS 12.7 (DEV)
@diegoreymendez
Copy link
Contributor Author

/cc @wordpress-mobile/ravenclaw

@maxme - Can I ask you to provide some insights on what Android does? This could be useful to implement something similar in iOS.

@maxme
Copy link
Contributor

maxme commented Jul 16, 2019

@maxme - Can I ask you to provide some insights on what Android does? This could be useful to implement something similar in iOS.

Sorry I just noticed your ping here. In wpandroid, we don't check for failed uploads. We list post with local modifications (currently we do only this only for drafts, but this should be extended to any post status).

The logic is described there. You'll notice some extra checks (is the post publishable? is the post being uploaded? etc.)

@diegoreymendez
Copy link
Contributor Author

diegoreymendez commented Jul 16, 2019

I'm starting to think that it may make sense not to auto-upload posts when there was no explicit request by the user to "save" the changes. The reason is that to do this properly, we'd need to:

  1. Check the previous status of the post vs the current one (to prevent an unwanted status change)
  2. Even if the status wasn't changed, make sure it's not a published or scheduled post (because we can't assume the user wanted those changes published at all)
  3. For scheduled posts, compare the previous publish date vs the new one (because we don't want that date changed without explicit confirmation).

It is my understanding that auto-uploading would ONLY be useful in the case where the post was and still is a Draft (which is something we need to be able to verify somehow).

Also, some related discussion in Android here: wordpress-mobile/WordPress-Android#10021 (comment)

/cc @malinajirka since you may have some thoughts about this.

@malinajirka
Copy link

malinajirka commented Jul 17, 2019

I created this table yesterday, which might be useful - the comment seems more complicated than it actually is wordpress-mobile/WordPress-Android#10174 (comment)

I agree this solution doesn't cover the following scenario

  1. User edits a draft in offline
  2. User edits the same draft in Calypso and publishes it
  3. The app auto-uploads the draft from step 1 - it overrides the remote changes and changes the status of the post back to "draft"

This issue has two solution imho

  1. The optimal solution -> update the API. There is a lot of options how to fix the implementation of the API. For example introducing "remoteLastModified" query parameter -> if the lastModified date in the remote is different there is a possible conflict which the user needs to resolve on the client.
  2. Non-optimal but decent solution -> never use auto-upload unless the changes were explicitly confirmed by the user. We could use auto-save even for drafts (I'm not 100% about this). The only thing which would change in the table from my comment is

Screenshot 2019-07-17 at 09 05 36

This is basically what Diego proposed I'm starting to think that it may make sense not to auto-upload posts when there was no explicit request by the user to "save" the changes.. This "non-optimal" solution wouldn't solve the case when the user edits the post in remote after they already confirmed the changes in the app - they need to manually go back to the app and cancel the action.

Update - wordpress-mobile/WordPress-Android#10174 (comment)

@diegoreymendez
Copy link
Contributor Author

I'm closing this issue as we shouldn't auto-upload posts that didn't receive confirmation to do so by the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants