diff --git a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m index 0dd5027b8ca4..c869009d64cc 100644 --- a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m +++ b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m @@ -149,4 +149,81 @@ - (void)testSyncFailureShouldBeCalledOnce [self waitForExpectations:@[completion] timeout:1]; } +/// Regression: when the save context can't resolve the blog, `success` must not +/// also fire. Previously the completion called `success(nil)` alongside `failure` +/// (a double callback that passed nil into the non-null `PostCategory` block). +/// The blog is intentionally left unsaved (see `setUp`), so its temporary +/// objectID doesn't resolve in the background save context — the "no blog" path. +- (void)testThatCreateCategoryDoesNotAlsoCallSuccessWhenBlogIsMissing +{ + TaxonomyServiceRemoteREST *remote = self.service.remoteForStubbing; + + RemotePostCategory *received = [RemotePostCategory new]; + received.categoryID = @123; + received.name = @"category name"; + received.parentID = @0; + + OCMStub([remote createCategory:[OCMArg any] + success:([OCMArg invokeBlockWithArgs:received, nil]) + failure:[OCMArg any]]); + + XCTestExpectation *failed = [self expectationWithDescription:@"failure is called"]; + XCTestExpectation *successNotCalled = [self expectationWithDescription:@"success is not called"]; + successNotCalled.inverted = YES; + + [self.service createCategoryWithName:@"category name" + parentCategoryObjectID:nil + forBlogObjectID:self.blog.objectID + success:^(PostCategory * _Nonnull __unused category) { + [successNotCalled fulfill]; + } failure:^(NSError * _Nonnull __unused error) { + // Failure is delivered from the main-queue completion, not the background save context. + XCTAssertTrue([NSThread isMainThread]); + [failed fulfill]; + }]; + + [self waitForExpectations:@[failed, successNotCalled] timeout:1]; +} + +/// Happy path: when the blog resolves in the save context, the created category +/// is looked up and handed to `success` exactly once (on the main queue) and +/// `failure` is not called. The blog is saved first — unlike the no-blog test +/// above — so its permanent objectID resolves in the background save context. +- (void)testThatCreateCategoryCallsSuccessWithTheCreatedCategory +{ + [self.manager saveContextAndWait:self.manager.mainContext]; + + TaxonomyServiceRemoteREST *remote = self.service.remoteForStubbing; + + RemotePostCategory *received = [RemotePostCategory new]; + received.categoryID = @123; + received.name = @"category name"; + received.parentID = @0; + + OCMStub([remote createCategory:[OCMArg any] + success:([OCMArg invokeBlockWithArgs:received, nil]) + failure:[OCMArg any]]); + + XCTestExpectation *succeeded = [self expectationWithDescription:@"success is called with the created category"]; + XCTestExpectation *failureNotCalled = [self expectationWithDescription:@"failure is not called"]; + failureNotCalled.inverted = YES; + + [self.service createCategoryWithName:@"category name" + parentCategoryObjectID:nil + forBlogObjectID:self.blog.objectID + success:^(PostCategory * _Nonnull category) { + // Success is delivered from the main-queue completion. + XCTAssertTrue([NSThread isMainThread]); + XCTAssertNotNil(category); + XCTAssertEqualObjects(category.categoryID, @123); + XCTAssertEqualObjects(category.categoryName, @"category name"); + XCTAssertEqualObjects(category.parentID, @0); + [succeeded fulfill]; + } failure:^(NSError * _Nonnull __unused error) { + [failureNotCalled fulfill]; + }]; + + [self waitForExpectations:@[succeeded, failureNotCalled] timeout:1]; +} + @end diff --git a/WordPress/Classes/Services/PostCategoryService.h b/WordPress/Classes/Services/PostCategoryService.h index d07b1d3381a3..cad835b9eb3c 100644 --- a/WordPress/Classes/Services/PostCategoryService.h +++ b/WordPress/Classes/Services/PostCategoryService.h @@ -8,7 +8,8 @@ NS_ASSUME_NONNULL_BEGIN @protocol CoreDataStack; typedef NS_ENUM(NSInteger, PostCategoryServiceErrors) { - PostCategoryServiceErrorsBlogNotFound + PostCategoryServiceErrorsBlogNotFound, + PostCategoryServiceErrorsCategoryNotFound }; @interface PostCategoryService : NSObject @@ -39,8 +40,8 @@ typedef NS_ENUM(NSInteger, PostCategoryServiceErrors) { - (void)createCategoryWithName:(NSString *)name parentCategoryObjectID:(nullable NSManagedObjectID *)parentCategoryObjectID forBlogObjectID:(NSManagedObjectID *)blogObjectID - success:(nullable void (^)(PostCategory *category))success - failure:(nullable void (^)(NSError *error))failure; + success:(void (^)(PostCategory *category))success + failure:(void (^)(NSError *error))failure; @end NS_ASSUME_NONNULL_END diff --git a/WordPress/Classes/Services/PostCategoryService.m b/WordPress/Classes/Services/PostCategoryService.m index 8fa51d3f946a..104103a39d90 100644 --- a/WordPress/Classes/Services/PostCategoryService.m +++ b/WordPress/Classes/Services/PostCategoryService.m @@ -22,6 +22,13 @@ - (NSError *)serviceErrorNoBlog userInfo:nil]; } +- (NSError *)serviceErrorCategoryNotFound +{ + return [NSError errorWithDomain:NSStringFromClass([self class]) + code:PostCategoryServiceErrorsCategoryNotFound + userInfo:nil]; +} + - (void)syncCategoriesForBlog:(Blog *)blog success:(nullable void (^)(void))success failure:(nullable void (^)(NSError *error))failure @@ -88,8 +95,8 @@ - (void)syncCategoriesForBlog:(Blog *)blog - (void)createCategoryWithName:(NSString *)name parentCategoryObjectID:(nullable NSManagedObjectID *)parentCategoryObjectID forBlogObjectID:(NSManagedObjectID *)blogObjectID - success:(nullable void (^)(PostCategory *category))success - failure:(nullable void (^)(NSError *error))failure + success:(void (^)(PostCategory *category))success + failure:(void (^)(NSError *error))failure { NSParameterAssert(name != nil); Blog * __block blog = nil; @@ -108,12 +115,11 @@ - (void)createCategoryWithName:(NSString *)name id remote = [self remoteForBlog:blog]; [remote createCategory:remoteCategory success:^(RemotePostCategory *receivedCategory) { + NSError * __block error = nil; [self.coreDataStack performAndSaveUsingBlock:^(NSManagedObjectContext *context) { Blog *blog = [context existingObjectWithID:blogObjectID error:nil]; if (!blog) { - if (failure) { - failure([self serviceErrorNoBlog]); - } + error = [self serviceErrorNoBlog]; return; } PostCategory *newCategory = [PostCategory createWithBlogObjectID:blogObjectID inContext:context]; @@ -129,11 +135,23 @@ - (void)createCategoryWithName:(NSString *)name newCategory.parentID = @0; } } completion:^{ - if (success) { + // Deliver exactly one callback. `failure` is invoked from the + // main-queue completion (not the background save block) so the + // no-blog path matches the threading of the success path. + if (error) { + failure(error); + } else { PostCategory *newCategory = [PostCategory lookupWithBlogObjectID:blogObjectID categoryID:receivedCategory.categoryID inContext:[self.coreDataStack mainContext]]; - success(newCategory); + if (newCategory) { + success(newCategory); + } else { + // The category was created remotely but couldn't be + // resolved locally (e.g. the response had no usable ID). + // Report failure rather than dropping the callback. + failure([self serviceErrorCategoryNotFound]); + } } if ([remote isKindOfClass:[TaxonomyServiceRemoteXMLRPC class]]) { // XML-RPC only returns ID, let's fetch the new category as