De-duplicate blogs after syncing #12377
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import Foundation | ||
|
|
||
| extension BlogService { | ||
| /// Removes any duplicate blogs in the given account | ||
| /// | ||
| /// We consider a blog to be a duplicate of another if they have the same dotComID. | ||
| /// For each group of duplicate blogs, this will delete all but one, giving preference to | ||
| /// blogs that have local drafts. | ||
| /// | ||
| /// If there's more than one blog in each group with local drafts, those will be reassigned | ||
| /// to the remaining blog. | ||
| /// | ||
| @objc(deduplicateBlogsForAccount:) | ||
| func deduplicateBlogs(for account: WPAccount) { | ||
| // Group all the account blogs by ID so it's easier to find duplicates | ||
| 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") | ||
| guard blogID > 0 else { | ||
| DDLogError("Found one or more WordPress.com blogs without ID, skipping de-duplication") | ||
| continue | ||
| } | ||
| DDLogWarn("Found \(group.count - 1) duplicates for blog with ID \(blogID)") | ||
| deduplicate(group: group) | ||
| } | ||
| } | ||
|
|
||
| private func deduplicate(group: [Blog]) { | ||
| // If there's a blog with local drafts, we'll preserve that one, otherwise we pick up the first | ||
| // since we don't really care which blog to pick | ||
| let candidateIndex = group.firstIndex(where: { !localDrafts(for: $0).isEmpty }) ?? 0 | ||
| let candidate = group[candidateIndex] | ||
|
|
||
| // We look through every other blog | ||
| for (index, blog) in group.enumerated() where index != candidateIndex { | ||
|
There was a problem hiding this comment. I had no idea you could do a |
||
| // If there are other blogs with local drafts, we reassing them to the blog that | ||
| // is not going to be deleted | ||
| for draft in localDrafts(for: blog) { | ||
| DDLogInfo("Migrating local draft \(draft.postTitle ?? "<Untitled>") to de-duplicated blog") | ||
| draft.blog = candidate | ||
| } | ||
| // Once the drafts are moved (if any), we can safely delete the duplicate | ||
| DDLogInfo("Deleting duplicate blog \(blog.logDescription())") | ||
| managedObjectContext.delete(blog) | ||
| } | ||
| } | ||
|
|
||
| private func localDrafts(for blog: Blog) -> [AbstractPost] { | ||
| // 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 | ||
|
There was a problem hiding this comment. There's a There was a problem hiding this comment. I follow the same logic we use to show an alert warning that you have local drafts when signing out |
||
| if let postID = post.postID?.intValue, | ||
| postID > 0 { | ||
| return false | ||
| } | ||
| return true | ||
| }) ?? [] | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| import CoreData | ||
| import XCTest | ||
| @testable import WordPress | ||
|
|
||
| class BlogServiceDeduplicationTests: XCTestCase { | ||
| var contextManager: TestContextManager! | ||
| var blogService: BlogService! | ||
| var context: NSManagedObjectContext { | ||
| return contextManager.mainContext | ||
| } | ||
|
|
||
| override func setUp() { | ||
| super.setUp() | ||
|
|
||
| contextManager = TestContextManager() | ||
| blogService = BlogService(managedObjectContext: contextManager.mainContext) | ||
| } | ||
|
|
||
| override func tearDown() { | ||
| super.tearDown() | ||
|
|
||
| ContextManager.overrideSharedInstance(nil) | ||
| contextManager.mainContext.reset() | ||
| contextManager = nil | ||
| blogService = nil | ||
| } | ||
|
|
||
| func testDeduplicationDoesNothingWhenNoDuplicates() { | ||
| let account = createAccount() | ||
| let blog1 = createBlog(id: 1, url: "blog1.example.com", account: account) | ||
| let blog2 = createBlog(id: 2, url: "blog2.example.com", account: account) | ||
| createDraft(title: "Post 1 in Blog 2", blog: blog2, id: 1) | ||
| createDraft(title: "Draft 2 in Blog 2", blog: blog2) | ||
| let blog3 = createBlog(id: 3, url: "blog3.example.com", account: account) | ||
|
|
||
| XCTAssertEqual(account.blogs.count, 3) | ||
| XCTAssertEqual(blog1.posts?.count, 0) | ||
| XCTAssertEqual(blog2.posts?.count, 2) | ||
| XCTAssertEqual(blog3.posts?.count, 0) | ||
|
|
||
| deduplicateAndSave(account) | ||
|
|
||
| XCTAssertEqual(account.blogs.count, 3) | ||
| XCTAssertEqual(blog1.posts?.count, 0) | ||
| XCTAssertEqual(blog2.posts?.count, 2) | ||
| XCTAssertEqual(blog3.posts?.count, 0) | ||
| } | ||
|
|
||
| func testDeduplicationRemovesDuplicateBlogsWithoutDrafts() { | ||
| let account = createAccount() | ||
| createBlog(id: 1, url: "blog1.example.com", account: account) | ||
| createBlog(id: 2, url: "blog2.example.com", account: account) | ||
| createBlog(id: 2, url: "blog2.example.com", account: account) | ||
|
|
||
| XCTAssertEqual(account.blogs.count, 3) | ||
|
|
||
| deduplicateAndSave(account) | ||
|
|
||
| XCTAssertEqual(account.blogs.count, 2) | ||
| } | ||
|
|
||
| func testDeduplicationPrefersCandidateWithLocalDrafts() { | ||
| let account = createAccount() | ||
|
|
||
| let blog1 = createBlog(id: 1, url: "blog1.example.com", account: account) | ||
|
|
||
| let blog2a = createBlog(id: 2, url: "blog2.example.com", account: account) | ||
| createDraft(title: "Post 1 in Blog 2", blog: blog2a, id: 1) | ||
| createDraft(title: "Draft 2 in Blog 2", blog: blog2a) | ||
| let blog2b = createBlog(id: 2, url: "blog2.example.com", account: account) | ||
|
|
||
| let blog3a = createBlog(id: 3, url: "blog3.example.com", account: account) | ||
| let blog3b = createBlog(id: 3, url: "blog3.example.com", account: account) | ||
| createDraft(title: "Post 1 in Blog 3", blog: blog3b, id: 1) | ||
| createDraft(title: "Draft 2 in Blog 3", blog: blog3b) | ||
|
|
||
| XCTAssertEqual(account.blogs.count, 5) | ||
| XCTAssertEqual(blog1.posts?.count, 0) | ||
| XCTAssertEqual(blog2a.posts?.count, 2) | ||
| XCTAssertEqual(blog2b.posts?.count, 0) | ||
| XCTAssertEqual(blog3a.posts?.count, 0) | ||
| XCTAssertEqual(blog3b.posts?.count, 2) | ||
|
|
||
| deduplicateAndSave(account) | ||
|
|
||
| XCTAssertEqual(account.blogs.count, 3) | ||
| XCTAssertEqual(account.blogs, Set(arrayLiteral: blog1, blog2a, blog3b)) | ||
|
|
||
| XCTAssertFalse(isDeleted(blog1)) | ||
| XCTAssertFalse(isDeleted(blog2a)) | ||
| XCTAssertTrue(isDeleted(blog2b)) | ||
| XCTAssertTrue(isDeleted(blog3a)) | ||
| XCTAssertFalse(isDeleted(blog3b)) | ||
|
|
||
| XCTAssertEqual(blog1.posts?.count, 0) | ||
| XCTAssertEqual(blog2a.posts?.count, 2) | ||
| XCTAssertEqual(blog3b.posts?.count, 2) | ||
| } | ||
|
|
||
| func testDeduplicationMigratesLocalDrafts() { | ||
| let account = createAccount() | ||
|
|
||
| let blog1 = createBlog(id: 1, url: "blog1.example.com", account: account) | ||
|
|
||
| let blog2a = createBlog(id: 2, url: "blog2.example.com", account: account) | ||
| createDraft(title: "Post 1 in Blog 2", blog: blog2a, id: 1) | ||
| createDraft(title: "Draft 2 in Blog 2", blog: blog2a) | ||
| let blog2b = createBlog(id: 2, url: "blog2.example.com", account: account) | ||
| createDraft(title: "Post 1 in Blog 2", blog: blog2b, id: 1) | ||
| createDraft(title: "Post 3 in Blog 2", blog: blog2b, id: 3) | ||
| createDraft(title: "Draft 4 in Blog 2", blog: blog2b) | ||
|
|
||
| XCTAssertEqual(account.blogs.count, 3) | ||
| XCTAssertEqual(blog1.posts?.count, 0) | ||
| XCTAssertEqual(blog2a.posts?.count, 2) | ||
| XCTAssertEqual(blog2b.posts?.count, 3) | ||
|
|
||
| deduplicateAndSave(account) | ||
|
|
||
| XCTAssertEqual(account.blogs.count, 2) | ||
|
|
||
| XCTAssertFalse(isDeleted(blog1)) | ||
| // We don't care which one is deleted, but one of them should be | ||
| XCTAssertTrue(isDeleted(blog2a) != isDeleted(blog2b), "Exactly one copy of Blog 2 should have been deleted") | ||
|
|
||
| XCTAssertEqual(blog1.posts?.count, 0) | ||
| guard let blog2Final = account.blogs.first(where: { $0.dotComID == 2 }) else { | ||
| return XCTFail("There should be one blog with ID = 2") | ||
| } | ||
| XCTAssertTrue(findPost(title: "Post 1 in Blog 2", in: blog2Final)) | ||
| XCTAssertTrue(findPost(title: "Draft 2 in Blog 2", in: blog2Final)) | ||
| XCTAssertTrue(findPost(title: "Draft 4 in Blog 2", in: blog2Final)) | ||
| } | ||
| } | ||
|
|
||
| private extension BlogServiceDeduplicationTests { | ||
| func deduplicateAndSave(_ account: WPAccount) { | ||
| blogService.deduplicateBlogs(for: account) | ||
| contextManager.saveContextAndWait(context) | ||
| } | ||
|
|
||
| func isDeleted(_ object: NSManagedObject) -> Bool { | ||
| return object.isDeleted || object.managedObjectContext == nil | ||
| } | ||
|
|
||
| func findPost(title: String, in blog: Blog) -> Bool { | ||
| return blog.posts?.contains(where: { (post) in | ||
| post.postTitle?.contains(title) ?? false | ||
| }) ?? false | ||
| } | ||
|
|
||
| @discardableResult | ||
| func createAccount() -> WPAccount { | ||
| let accountService = AccountService(managedObjectContext: context) | ||
| return accountService.createOrUpdateAccount(withUsername: "twoface", authToken: "twotoken") | ||
| } | ||
|
|
||
| @discardableResult | ||
| func createBlog(id: Int, url: String, account: WPAccount) -> Blog { | ||
| let blog = NSEntityDescription.insertNewObject(forEntityName: "Blog", into: context) as! Blog | ||
| blog.dotComID = id as NSNumber | ||
| blog.url = url | ||
| blog.xmlrpc = url | ||
| blog.account = account | ||
| return blog | ||
| } | ||
|
|
||
| @discardableResult | ||
| func createDraft(title: String, blog: Blog, id: Int? = nil) -> AbstractPost { | ||
| let post = NSEntityDescription.insertNewObject(forEntityName: "Post", into: context) as! Post | ||
| post.postTitle = title | ||
| post.blog = blog | ||
| post.postID = id.map({ $0 as NSNumber }) | ||
| return post | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this crash in cases where a XML-RPC blog would get duplicated? They will have a
dotComIDof nil, which would get coalesced to0in the line above and then trigger thisassert.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but this is based on
Accountanyway, so they have to beRESTblogs anyway. Nevermind.