Skip to content

Conversation

@mzorz
Copy link
Contributor

@mzorz mzorz commented Jan 10, 2019

Fixes #8932 by bringing back the functionality first introduced in #3001

The changes are in the corresponding FluxC PR wordpress-mobile/WordPress-FluxC-Android#1064

This PR makes sure to not overwrite posts that are marked as locally changed. Whenever the post list refreshes, local changes are retained.

This PR only brings back what was working before. Now we have revisions and are in a better position (in regards to the information we've got available) to implement something better. I've left further changes for other PRs to get a better UX (for example by expressly telling the user about the changes and such), waiting for design flow input in #5984 (comment) - it may be good to go along the lines of what @oguzkocer suggested in #5984 (comment)

Also citing an obvious implication of this PR here (but important to note nevertheless), taken from #3001 description:

This does mean that if the user edits a post outside of the app, the app won't show those edits if it has local changes. This isn't ideal, but I think is still far better than the current situation.

To test:

1. Create a draft while I am online in the app. That'll create the draft in remote.
2. Go offline
3. Make changes to the post and go back. This will save the changes as local draft
4. Go to web or any other client and make changes to the post.
5. Publish the post.
5.bis: now turn airplane mode off on the device and pull to refresh on the Posts list.
6. Observe the post, that was marked with "local changes" doesn't get updated and is still accessible.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@mzorz mzorz added this to the 11.6 milestone Jan 10, 2019
@mzorz mzorz requested a review from oguzkocer January 10, 2019 21:14
@oguzkocer oguzkocer self-assigned this Jan 11, 2019
Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mzorz We can't merge this as is (FluxC PR needs to be merged first and hash needs to be updated), but I still wanted to try out the fix. The local post is not overwritten with the remote post as we wanted to, however when I try to discard the changes in the post, it actually overwrites the remote post which is more dangerous in my opinion.

I realize this is not this PR's fault, but it's an important issue and I think we'll need to fix them both at the same time. Could you also test this out and see how it works for you? The testing steps are the same, except at the end of it, just select the post and do Discard Local Changes from the editor menu.

@mzorz
Copy link
Contributor Author

mzorz commented Jan 14, 2019

I realize this is not this PR's fault, but it's an important issue and I think we'll need to fix them both at the same time.

100% agreed, let me check it out here and see what we can do to fix that 👍

@mzorz
Copy link
Contributor Author

mzorz commented Jan 14, 2019

@oguzkocer the changes to single post fetch were removed from the FluxC branch which is now merged, so I updated the FluxC hash reference here. This one should be good to merge to close #8932. We can continue with the efforts being discussed in #5984 separately 👍

Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes what it's supposed to, so I think we should merge it. Having said that, we do have some issues regarding discarding local changes as mentioned in this PR, as discussed in #5984 and as described in #8957. I think we should try to address these issues sooner rather than later.

:shipit:

@oguzkocer oguzkocer merged commit 23a5218 into develop Jan 14, 2019
@oguzkocer oguzkocer deleted the issue/8932-dont-overwrite-local-changes branch January 14, 2019 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants