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

Create flag and update label for a conflicted post #14104

Conversation

embryoconcepts
Copy link

@embryoconcepts embryoconcepts commented May 12, 2020

  • Create a BOOL flag for a post that has a conflict in the dateModified property between the local and web versions
  • Update the status on the PostCard and PostCompact cells to display ‘Version Conflictinerror` coloring, when a post has a version conflict

NOTES:

  • Android uses isConflicted, but I felt hasVersionConflict was more precise, and more future-proof (ie, there may be other types of conflicts in the future, and this leaves room for those names
  • I opted to only update the local status, ala “Local Changes”, and not the post status, since this is something that is restricted to local - please let me know if I should incorporate it as a proper Post Status, and I can do that

Fixes (in part) #13683

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.
  6. on the app, you should be on the posts list. turn airplane mode off
  7. pull to refresh.
  8. you should see now the post has a “Conflicted” state.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

- Create a BOOL flag for a post that has a conflict in the `dateModified` property between the local and web versions
- Update the status on the PostCard and PostCompact cells to display ‘Version Conflict` in `error` coloring, when a post has a version conflict
Copy link
Contributor

@leandroalonso leandroalonso left a comment

Choose a reason for hiding this comment

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

Good work here @embryoconcepts.

Left a few suggestions for unit tests and a question about the conflict logic.

WordPress/WordPressTest/PostTests.swift Outdated Show resolved Hide resolved
WordPress/Classes/ViewRelated/Post/PostCardCell.xib Outdated Show resolved Hide resolved
WordPress/Classes/Models/AbstractPost.m Show resolved Hide resolved
@embryoconcepts embryoconcepts marked this pull request as ready for review May 12, 2020 21:50
Copy link
Contributor

@leandroalonso leandroalonso left a comment

Choose a reason for hiding this comment

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

Thank you so much for the changes @embryoconcepts!

Almost there!

WordPress/WordPressTest/PostTests.swift Outdated Show resolved Hide resolved
WordPress/Classes/Models/AbstractPost.m Show resolved Hide resolved
@embryoconcepts embryoconcepts force-pushed the issue/13683_conflict-resolution-dialog branch from be0c1fd to 0d13cda Compare May 13, 2020 19:49
Copy link
Contributor

@leandroalonso leandroalonso left a comment

Choose a reason for hiding this comment

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

Just minor changes @embryoconcepts and we would be good to go! o/

WordPress/Classes/Models/AbstractPost.m Outdated Show resolved Hide resolved
WordPress/WordPressTest/PostTests.swift Outdated Show resolved Hide resolved
WordPress/WordPressTest/PostTests.swift Outdated Show resolved Hide resolved
WordPress/WordPressTest/PostTests.swift Outdated Show resolved Hide resolved
WordPress/WordPressTest/PostTests.swift Outdated Show resolved Hide resolved
WordPress/WordPressTest/PostTests.swift Outdated Show resolved Hide resolved
@leandroalonso leandroalonso added 0 0 point estimation and removed 0 0 point estimation labels May 14, 2020
Copy link
Contributor

@leandroalonso leandroalonso left a comment

Choose a reason for hiding this comment

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

@embryoconcepts All good to go. Just one detail:

Can you please remove Localizable.strings? We should not change this file (you can see a warning above about it).

@leandroalonso leandroalonso merged commit 7701696 into wordpress-mobile:issue/13683_conflict-resolution-dialog May 14, 2020
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.

None yet

4 participants