From 0f5cfa9791f9176d2163e172a0883683ff9aa626 Mon Sep 17 00:00:00 2001 From: Momo Ozawa Date: Wed, 12 Aug 2020 08:14:43 +0900 Subject: [PATCH 1/5] Compare sites fetched from CoreData against sites fetched from remote - Keep track of sites fetched from remote - Compare them against sites fetched from CoreData - If a site fetched from CoreData isn't in the the list of sites fetched from remote, mark that site as being unfollowed --- WordPress/Classes/Services/ReaderTopicService.m | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/WordPress/Classes/Services/ReaderTopicService.m b/WordPress/Classes/Services/ReaderTopicService.m index 77afbcaa5626..6667db225408 100644 --- a/WordPress/Classes/Services/ReaderTopicService.m +++ b/WordPress/Classes/Services/ReaderTopicService.m @@ -51,9 +51,22 @@ - (void)fetchFollowedSitesWithSuccess:(void(^)(void))success failure:(void(^)(NS { ReaderTopicServiceRemote *service = [[ReaderTopicServiceRemote alloc] initWithWordPressComRestApi:[self apiForRequest]]; [service fetchFollowedSitesWithSuccess:^(NSArray *sites) { + NSArray *currentSiteTopics = [self allSiteTopics]; + NSMutableArray *remoteFeedIds = [NSMutableArray array]; + for (RemoteReaderSiteInfo *siteInfo in sites) { + [remoteFeedIds addObject:siteInfo.feedID]; [self siteTopicForRemoteSiteInfo:siteInfo]; } + + for (ReaderSiteTopic *siteTopic in currentSiteTopics) { + // If a site fetched from Core Data isn't included in the list of sites + // fetched from remote, that means it's no longer being followed. + if (![remoteFeedIds containsObject:siteTopic.feedID]) { + siteTopic.following = NO; + } + } + [[ContextManager sharedInstance] saveContext:self.managedObjectContext withCompletionBlock:^{ if (success) { success(); From 8552752cebae6ad3f7ba679ce76f995c3dae64a8 Mon Sep 17 00:00:00 2001 From: Momo Ozawa Date: Thu, 13 Aug 2020 09:20:25 +0900 Subject: [PATCH 2/5] Move out merge followed sites logic into its own method --- .../Classes/Services/ReaderTopicService.h | 1 + .../Classes/Services/ReaderTopicService.m | 56 ++++++++++++------- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/WordPress/Classes/Services/ReaderTopicService.h b/WordPress/Classes/Services/ReaderTopicService.h index 4c6bcf69df14..4da69970d967 100644 --- a/WordPress/Classes/Services/ReaderTopicService.h +++ b/WordPress/Classes/Services/ReaderTopicService.h @@ -241,6 +241,7 @@ extern NSString * const ReaderTopicFreshlyPressedPathCommponent; @end @interface ReaderTopicService (Tests) +- (void)mergeFollowedSites:(NSArray *)sites withSuccess:(void (^)(void))success; - (void)mergeMenuTopics:(NSArray *)topics withSuccess:(void (^)(void))success; - (NSString *)formatTitle:(NSString *)str; @end diff --git a/WordPress/Classes/Services/ReaderTopicService.m b/WordPress/Classes/Services/ReaderTopicService.m index 6667db225408..bdab6759c88a 100644 --- a/WordPress/Classes/Services/ReaderTopicService.m +++ b/WordPress/Classes/Services/ReaderTopicService.m @@ -51,27 +51,7 @@ - (void)fetchFollowedSitesWithSuccess:(void(^)(void))success failure:(void(^)(NS { ReaderTopicServiceRemote *service = [[ReaderTopicServiceRemote alloc] initWithWordPressComRestApi:[self apiForRequest]]; [service fetchFollowedSitesWithSuccess:^(NSArray *sites) { - NSArray *currentSiteTopics = [self allSiteTopics]; - NSMutableArray *remoteFeedIds = [NSMutableArray array]; - - for (RemoteReaderSiteInfo *siteInfo in sites) { - [remoteFeedIds addObject:siteInfo.feedID]; - [self siteTopicForRemoteSiteInfo:siteInfo]; - } - - for (ReaderSiteTopic *siteTopic in currentSiteTopics) { - // If a site fetched from Core Data isn't included in the list of sites - // fetched from remote, that means it's no longer being followed. - if (![remoteFeedIds containsObject:siteTopic.feedID]) { - siteTopic.following = NO; - } - } - - [[ContextManager sharedInstance] saveContext:self.managedObjectContext withCompletionBlock:^{ - if (success) { - success(); - } - }]; + [self mergeFollowedSites:sites withSuccess:success]; } failure:^(NSError *error) { if (failure) { failure(error); @@ -938,6 +918,40 @@ - (NSString *)formatTitle:(NSString *)str return [title capitalizedStringWithLocale:[NSLocale currentLocale]]; } + +/** +Saves the specified `ReaderSiteTopics`. Any `ReaderSiteTopics` not included in the passed +array are marked as being unfollowed in Core Data. + +@param topics An array of `ReaderSiteTopics` to save. +*/ +- (void)mergeFollowedSites:(NSArray *)sites withSuccess:(void (^)(void))success +{ + [self.managedObjectContext performBlock:^{ + NSArray *currentSiteTopics = [self allSiteTopics]; + NSMutableArray *remoteFeedIds = [NSMutableArray array]; + + for (RemoteReaderSiteInfo *siteInfo in sites) { + [remoteFeedIds addObject:siteInfo.feedID]; + [self siteTopicForRemoteSiteInfo:siteInfo]; + } + + for (ReaderSiteTopic *siteTopic in currentSiteTopics) { + // If a site fetched from Core Data isn't included in the list of sites + // fetched from remote, that means it's no longer being followed. + if (![remoteFeedIds containsObject:siteTopic.feedID]) { + siteTopic.following = NO; + } + } + + [[ContextManager sharedInstance] saveContext:self.managedObjectContext withCompletionBlock:^{ + if (success) { + success(); + } + }]; + }]; +} + /** Saves the specified `ReaderAbstractTopics`. Any `ReaderAbstractTopics` not included in the passed array are removed from Core Data. From 469f693458bf540e5fbff64e34baaed847710d2e Mon Sep 17 00:00:00 2001 From: Momo Ozawa Date: Thu, 13 Aug 2020 09:20:39 +0900 Subject: [PATCH 3/5] Add test --- .../ReaderTopicServiceTest.swift | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/WordPress/WordPressTest/ReaderTopicServiceTest.swift b/WordPress/WordPressTest/ReaderTopicServiceTest.swift index 11a9ff54b43c..ae7b9d01ceb0 100644 --- a/WordPress/WordPressTest/ReaderTopicServiceTest.swift +++ b/WordPress/WordPressTest/ReaderTopicServiceTest.swift @@ -95,6 +95,25 @@ final class ReaderTopicSwiftTest: XCTestCase { } } + func remoteSiteInfoForTests() -> [RemoteReaderSiteInfo] { + let foo = RemoteReaderSiteInfo() + foo.feedID = 1 + foo.isFollowing = true + foo.postsEndpoint = "/sites/foo" + + let bar = RemoteReaderSiteInfo() + bar.feedID = 2 + bar.isFollowing = true + bar.postsEndpoint = "/sites/bar" + + let baz = RemoteReaderSiteInfo() + baz.feedID = 3 + baz.isFollowing = true + baz.postsEndpoint = "/sites/baz" + + return [foo, bar, baz] + } + func remoteTopicsForTests() -> [RemoteReaderTopic] { let foo = RemoteReaderTopic() foo.topicID = 1 @@ -138,6 +157,51 @@ final class ReaderTopicSwiftTest: XCTestCase { // MARK: Tests + /** + Ensure that followed sites a user unfollows from are set to unfollowed in core data when merging + results from the REST API. + */ + func testUnfollowedSiteIsUnfollowedDuringSync() { + guard let context = context else { + XCTFail("Context is nil") + return + } + let remoteSites = remoteSiteInfoForTests() + + // Setup + var expect = expectation(description: "sites saved expectation") + let service = ReaderTopicService(managedObjectContext: context) + service.mergeFollowedSites(remoteSites, withSuccess: { () -> Void in + expect.fulfill() + }) + waitForExpectations(timeout: expectationTimeout, handler: nil) + + // Sites exist in the context + let request = NSFetchRequest(entityName: ReaderSiteTopic.classNameWithoutNamespaces()) + request.predicate = NSPredicate(format: "following = YES") + var count = try! context.count(for: request) + XCTAssertEqual(count, remoteSites.count, "Number of sites in context did not match expectations") + + // Merge new set of sites + expect = expectation(description: "sites saved expectation") + let foo = remoteSites.first as RemoteReaderSiteInfo? + service.mergeFollowedSites([foo!], withSuccess: { () -> Void in + expect.fulfill() + }) + waitForExpectations(timeout: expectationTimeout, handler: nil) + + // Make sure the unfollowed sites were unfollowed when merged + count = try! context.count(for: request) + XCTAssertEqual(count, 1, "Number of sites in context did not match expectations") + do { + let results = try context.fetch(request) + let site = results.first as! ReaderSiteTopic + XCTAssertEqual(site.feedID, foo?.feedID, "The site returned was not the one expected.") + } catch let error as NSError { + XCTAssertNil(error, "Error executing fetch request.") + } + } + /** Ensure that topics a user unsubscribes from are removed from core data when merging results from the REST API. From 62bff800da873753feea993ba16c7d8b09c16c6f Mon Sep 17 00:00:00 2001 From: Momo Ozawa Date: Thu, 13 Aug 2020 09:27:23 +0900 Subject: [PATCH 4/5] Fix whitespace --- WordPress/Classes/Services/ReaderTopicService.m | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/Services/ReaderTopicService.m b/WordPress/Classes/Services/ReaderTopicService.m index bdab6759c88a..8f68abb99fba 100644 --- a/WordPress/Classes/Services/ReaderTopicService.m +++ b/WordPress/Classes/Services/ReaderTopicService.m @@ -918,7 +918,6 @@ - (NSString *)formatTitle:(NSString *)str return [title capitalizedStringWithLocale:[NSLocale currentLocale]]; } - /** Saves the specified `ReaderSiteTopics`. Any `ReaderSiteTopics` not included in the passed array are marked as being unfollowed in Core Data. @@ -930,7 +929,7 @@ - (void)mergeFollowedSites:(NSArray *)sites withSuccess:(void (^)(void))success [self.managedObjectContext performBlock:^{ NSArray *currentSiteTopics = [self allSiteTopics]; NSMutableArray *remoteFeedIds = [NSMutableArray array]; - + for (RemoteReaderSiteInfo *siteInfo in sites) { [remoteFeedIds addObject:siteInfo.feedID]; [self siteTopicForRemoteSiteInfo:siteInfo]; @@ -943,7 +942,7 @@ - (void)mergeFollowedSites:(NSArray *)sites withSuccess:(void (^)(void))success siteTopic.following = NO; } } - + [[ContextManager sharedInstance] saveContext:self.managedObjectContext withCompletionBlock:^{ if (success) { success(); From 6d4135c9c3ed1c3d9d83fd6058e3fb1cc5e70fba Mon Sep 17 00:00:00 2001 From: Momo Ozawa Date: Sun, 16 Aug 2020 13:50:27 +0900 Subject: [PATCH 5/5] Organize test using Arrange/Act/Assert --- .../ReaderTopicServiceTest.swift | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/WordPress/WordPressTest/ReaderTopicServiceTest.swift b/WordPress/WordPressTest/ReaderTopicServiceTest.swift index ae7b9d01ceb0..0c0a3af611bc 100644 --- a/WordPress/WordPressTest/ReaderTopicServiceTest.swift +++ b/WordPress/WordPressTest/ReaderTopicServiceTest.swift @@ -162,39 +162,36 @@ final class ReaderTopicSwiftTest: XCTestCase { results from the REST API. */ func testUnfollowedSiteIsUnfollowedDuringSync() { - guard let context = context else { - XCTFail("Context is nil") - return - } + // Arrange: Setup let remoteSites = remoteSiteInfoForTests() + let service = ReaderTopicService(managedObjectContext: context!) + let foo = remoteSites.first as RemoteReaderSiteInfo? - // Setup + // Act: Save sites var expect = expectation(description: "sites saved expectation") - let service = ReaderTopicService(managedObjectContext: context) service.mergeFollowedSites(remoteSites, withSuccess: { () -> Void in expect.fulfill() }) waitForExpectations(timeout: expectationTimeout, handler: nil) - // Sites exist in the context + // Assert: Sites exist in the context let request = NSFetchRequest(entityName: ReaderSiteTopic.classNameWithoutNamespaces()) request.predicate = NSPredicate(format: "following = YES") - var count = try! context.count(for: request) + var count = try! context!.count(for: request) XCTAssertEqual(count, remoteSites.count, "Number of sites in context did not match expectations") - // Merge new set of sites + // Act: Merge new set of sites expect = expectation(description: "sites saved expectation") - let foo = remoteSites.first as RemoteReaderSiteInfo? service.mergeFollowedSites([foo!], withSuccess: { () -> Void in expect.fulfill() }) waitForExpectations(timeout: expectationTimeout, handler: nil) - // Make sure the unfollowed sites were unfollowed when merged - count = try! context.count(for: request) + // Assert: Unfollowed sites were unfollowed when merged + count = try! context!.count(for: request) XCTAssertEqual(count, 1, "Number of sites in context did not match expectations") do { - let results = try context.fetch(request) + let results = try context!.fetch(request) let site = results.first as! ReaderSiteTopic XCTAssertEqual(site.feedID, foo?.feedID, "The site returned was not the one expected.") } catch let error as NSError {