From 5f9b6ea96b2437542b70f6b221fbdb558d9b5296 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:55:57 -0600 Subject: [PATCH 1/5] Fix double-callback and nil category in PostCategoryService create --- WordPress/Classes/Services/PostCategoryService.m | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/WordPress/Classes/Services/PostCategoryService.m b/WordPress/Classes/Services/PostCategoryService.m index 8fa51d3f946a..27784ba98f0b 100644 --- a/WordPress/Classes/Services/PostCategoryService.m +++ b/WordPress/Classes/Services/PostCategoryService.m @@ -133,7 +133,9 @@ - (void)createCategoryWithName:(NSString *)name PostCategory *newCategory = [PostCategory lookupWithBlogObjectID:blogObjectID categoryID:receivedCategory.categoryID inContext:[self.coreDataStack mainContext]]; - success(newCategory); + if (newCategory) { + success(newCategory); + } } if ([remote isKindOfClass:[TaxonomyServiceRemoteXMLRPC class]]) { // XML-RPC only returns ID, let's fetch the new category as From 0bd488d72b0fffc1ba21ecc7f4dab72c0a8f3f7d Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:42:55 -0600 Subject: [PATCH 2/5] Add regression test for the category-create double callback --- .../Tests/Services/PostCategoryServiceTests.m | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m index 0dd5027b8ca4..8c5bbb6984f9 100644 --- a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m +++ b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m @@ -149,4 +149,38 @@ - (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) { + [failed fulfill]; + }]; + + [self waitForExpectations:@[failed, successNotCalled] timeout:1]; +} + @end From f6ed996ef86af2d110f866c21243effa3f0c0aa0 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:36:30 -0600 Subject: [PATCH 3/5] Guarantee exactly one callback in PostCategoryService create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior fix guarded `success(newCategory)` behind `if (newCategory)`, which stopped the `success(nil)` crash but left a path where neither `success` nor `failure` fires: the blog resolves and the category saves, but the main-context lookup returns nil (e.g. a response that carried no usable category ID). The only caller, `PostCategoryCreateView`, clears `isSaving` solely in its failure block, so a dropped callback hangs the save spinner indefinitely. Adopt the `__block NSError *error` pattern already used by `syncCategoriesForBlog:`: the no-blog branch records the error and the main-queue completion delivers exactly one callback — `failure(error)`, `success(category)`, or `failure(serviceErrorCategoryNotFound)` when the created category can't be resolved locally. This also moves the no-blog failure onto the main queue, matching the success path; the regression test now asserts that delivery is on the main thread. --- .../Tests/Services/PostCategoryServiceTests.m | 2 ++ .../Classes/Services/PostCategoryService.h | 3 +- .../Classes/Services/PostCategoryService.m | 30 +++++++++++++++---- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m index 8c5bbb6984f9..0cffd998266a 100644 --- a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m +++ b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m @@ -177,6 +177,8 @@ - (void)testThatCreateCategoryDoesNotAlsoCallSuccessWhenBlogIsMissing 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]; }]; diff --git a/WordPress/Classes/Services/PostCategoryService.h b/WordPress/Classes/Services/PostCategoryService.h index d07b1d3381a3..307f00d0d6e2 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 diff --git a/WordPress/Classes/Services/PostCategoryService.m b/WordPress/Classes/Services/PostCategoryService.m index 27784ba98f0b..99063f9c94e1 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 @@ -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,12 +135,26 @@ - (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) { + if (failure) { + failure(error); + } + } else { PostCategory *newCategory = [PostCategory lookupWithBlogObjectID:blogObjectID categoryID:receivedCategory.categoryID inContext:[self.coreDataStack mainContext]]; if (newCategory) { - success(newCategory); + if (success) { + success(newCategory); + } + } else if (failure) { + // 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]]) { From 90891a391de228007f5328dd26ecfb3560187ab7 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 1 Sep 2026 23:06:33 -0600 Subject: [PATCH 4/5] Test the category-create success path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PostCategoryServiceTests` only exercised the no-blog error path; the happy path — blog resolves, the category is created and handed back through `success` — was never executed by any test. Save the blog so its objectID resolves in the background save context, then assert `success` fires once on the main queue with the created category's ID, name, and parent, and that `failure` does not fire. --- .../Tests/Services/PostCategoryServiceTests.m | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m index 0cffd998266a..c869009d64cc 100644 --- a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m +++ b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m @@ -185,4 +185,45 @@ - (void)testThatCreateCategoryDoesNotAlsoCallSuccessWhenBlogIsMissing [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 From 8eec585940ef0c2d78ef78fd2686213e1728912d Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 2 Sep 2026 07:55:25 -0600 Subject: [PATCH 5/5] Require the category-create success/failure callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `createCategoryWithName:…:success:failure:` has a single caller — `PostCategoryCreateView` (Swift) — which always provides both blocks, so the `nullable` annotations bought nothing but three `if (success)` / `if (failure)` guards. Mark both blocks non-null: Swift enforces `_Nonnull` at compile time, so the caller cannot pass nil, and the completion drops the guards while making "exactly one callback" a hard invariant. The sibling `syncCategoriesForBlog:` methods stay `nullable` — they are genuinely called with `success:nil failure:nil` (the XML-RPC post-create re-sync). --- WordPress/Classes/Services/PostCategoryService.h | 4 ++-- WordPress/Classes/Services/PostCategoryService.m | 14 +++++--------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/WordPress/Classes/Services/PostCategoryService.h b/WordPress/Classes/Services/PostCategoryService.h index 307f00d0d6e2..cad835b9eb3c 100644 --- a/WordPress/Classes/Services/PostCategoryService.h +++ b/WordPress/Classes/Services/PostCategoryService.h @@ -40,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 99063f9c94e1..104103a39d90 100644 --- a/WordPress/Classes/Services/PostCategoryService.m +++ b/WordPress/Classes/Services/PostCategoryService.m @@ -95,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; @@ -139,18 +139,14 @@ - (void)createCategoryWithName:(NSString *)name // main-queue completion (not the background save block) so the // no-blog path matches the threading of the success path. if (error) { - if (failure) { - failure(error); - } + failure(error); } else { PostCategory *newCategory = [PostCategory lookupWithBlogObjectID:blogObjectID categoryID:receivedCategory.categoryID inContext:[self.coreDataStack mainContext]]; if (newCategory) { - if (success) { - success(newCategory); - } - } else if (failure) { + 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.