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

De-duplicate blogs after syncing #12377

Merged
merged 6 commits into from Aug 23, 2019

Conversation

@koke
Copy link
Member

commented Aug 23, 2019

This doesn't prevent the site duplication from happening, but if it has happened it will clean it up in the next blog sync

To help with #7886

To test:

To set up the scene, check out release/13.1 and run the app, but before set up a breakpoint in -[BlogService updateBlogWithRemoteBlog:account:] so it always creates a duplicate when syncing:

Screen Shot 2019-08-23 at 11 32 52

Then go to the sites list and pull to refresh until you start seeing duplicates.

Add some local drafts in different blogs by creating new posts and relaunching the app before they get saved on the server.

localDrafts

Now checkout fix/deduplicate-blogs and run the app. Go to the sites list and wait for a bit, it should sync automatically. If not, pull to refresh. The duplicates should be gone. If you go to the posts list, every local draft should be there.

localDraftsRecovered

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@koke koke added the Network/Sync label Aug 23, 2019

@koke koke added this to the 13.1 ❄️ milestone Aug 23, 2019

@koke koke requested review from yaelirub, jklausa and diegoreymendez Aug 23, 2019

koke added 3 commits Aug 23, 2019
@diegoreymendez
Copy link
Contributor

left a comment

Works as described. The code looks fine and the tests run fine.

@jklausa
Copy link
Member

left a comment

Some tiny style nitpicks and questions, but overall I think this is good to go!

let blogsById = Dictionary(grouping: account.blogs, by: { $0.dotComID?.intValue ?? 0 })
// For any group with more than one blog, remove duplicates
for (blogID, group) in blogsById where group.count > 1 {
assert(blogID > 0, "There should not be a Blog without ID if it has an account")

This comment has been minimized.

Copy link
@jklausa

jklausa Aug 23, 2019

Member

Wouldn't this crash in cases where a XML-RPC blog would get duplicated? They will have a dotComID of nil, which would get coalesced to 0 in the line above and then trigger this assert.

This comment has been minimized.

Copy link
@jklausa

jklausa Aug 23, 2019

Member

Ah, but this is based on Account anyway, so they have to be REST blogs anyway. Nevermind.

let candidate = group[candidateIndex]

// We look through every other blog
for (index, blog) in group.enumerated() where index != candidateIndex {

This comment has been minimized.

Copy link
@jklausa

jklausa Aug 23, 2019

Member

I had no idea you could do a where together with for... in! That's so cool!

// The original predicate from PostService.countPostsWithoutRemote() was:
// "postID = NULL OR postID <= 0"
// Swift optionals make things a bit more verbose, but this should be equivalent
return blog.posts?.filter({ (post) -> Bool in

This comment has been minimized.

Copy link
@jklausa

jklausa Aug 23, 2019

Member

There's a isDraft property on AbstractPost you might be able to use here, though it uses different logic — not sure which is more applicable.

This comment has been minimized.

Copy link
@koke

koke Aug 23, 2019

Author Member

I follow the same logic we use to show an alert warning that you have local drafts when signing out

More context here:
https://github.com/wordpress-mobile/WordPress-iOS/issues/7886#issuecomment-524221031
*/
[self deduplicateBlogsForAccount:account];

This comment has been minimized.

Copy link
@jklausa

jklausa Aug 23, 2019

Member

Should we do it on every syncBlogsForAccount: call? Seems like once during an app startup should be enough?

This comment has been minimized.

Copy link
@koke

koke Aug 23, 2019

Author Member

We don't really know for sure how blogs get duplicated, and in any case I think it belongs conceptually in the sync method: when you call sync you expect the local list to reflect what's on the server.

The implementation will bail early if it detects no duplicates, so it shouldn't be too much overhead.

I wouldn't mind revisiting this once we have all the other fixes in place though.

@koke koke merged commit 4d3f5c5 into release/13.1 Aug 23, 2019

6 checks passed

Hound No violations found. Woof!
Peril All green. Well done.
Details
ci/circleci: Build UI Tests Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPad 6th generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone Xs) Your tests passed on CircleCI!
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

@koke koke deleted the fix/deduplicate-blogs branch Aug 23, 2019

@koke koke referenced this pull request Aug 28, 2019
1 of 1 task complete

@jkmassel jkmassel restored the fix/deduplicate-blogs branch Sep 5, 2019

@jkmassel jkmassel deleted the fix/deduplicate-blogs branch Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.