diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 1c9101dd5ffe..56f0bb28a843 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -1,6 +1,7 @@ 23.7 ----- * [*] Bug fix: Reader now scrolls to the top when tapping the status bar. [#21914] +* [*] Fix an issue in Menu screen where it fails to create default menu items. [#21949] 23.6 ----- diff --git a/WordPress/Classes/Services/MenusService.h b/WordPress/Classes/Services/MenusService.h index c1b78825d356..f9522ade7f90 100644 --- a/WordPress/Classes/Services/MenusService.h +++ b/WordPress/Classes/Services/MenusService.h @@ -72,16 +72,12 @@ typedef void(^MenusServiceFailureBlock)(NSError *error); failure:(nullable MenusServiceFailureBlock)failure; /** - * @brief Generate a list MenuItems from the blog's top-level pages. + * @brief Create a list MenuItems from the given page. * - * @param blog The blog to use for pages. Cannot be nil. - * @param success The success handler. Can be nil. - * @param failure The failure handler. Can be nil. + * @return A MenuItem instance for the page if it's a top-level page. Otherwise, nil. * */ -- (void)generateDefaultMenuItemsForBlog:(Blog *)blog - success:(nullable void(^)(NSArray * _Nullable defaultItems))success - failure:(nullable MenusServiceFailureBlock)failure; +- (nullable MenuItem *)createItemWithPageID:(NSManagedObjectID *)pageObjectID inContext:(NSManagedObjectContext *)context; @end diff --git a/WordPress/Classes/Services/MenusService.m b/WordPress/Classes/Services/MenusService.m index ce37c91fd105..838af168bcf7 100644 --- a/WordPress/Classes/Services/MenusService.m +++ b/WordPress/Classes/Services/MenusService.m @@ -9,8 +9,6 @@ #import "WordPress-Swift.h" @import WordPressKit; -NS_ASSUME_NONNULL_BEGIN - @implementation MenusService #pragma mark - Menus availability @@ -161,47 +159,21 @@ - (void)deleteMenu:(Menu *)menu failure:failure]; } -- (void)generateDefaultMenuItemsForBlog:(Blog *)blog - success:(nullable void(^)(NSArray * _Nullable defaultItems))success - failure:(nullable MenusServiceFailureBlock)failure -{ - // Get the latest list of Pages available to the site. - PostServiceSyncOptions *options = [[PostServiceSyncOptions alloc] init]; - options.statuses = @[PostStatusPublish]; - options.order = PostServiceResultsOrderAscending; - options.orderBy = PostServiceResultsOrderingByTitle; - PostService *postService = [[PostService alloc] initWithManagedObjectContext:self.managedObjectContext]; - [postService syncPostsOfType:PostServiceTypePage - withOptions:options - forBlog:blog - success:^(NSArray *pages) { - [self.managedObjectContext performBlock:^{ - - if (!pages.count) { - success(nil); - return; - } - NSMutableArray *items = [NSMutableArray arrayWithCapacity:pages.count]; - // Create menu items for the top parent pages. - for (Page *page in pages) { - if ([page.parentID integerValue] > 0) { - continue; - } - MenuItem *pageItem = [NSEntityDescription insertNewObjectForEntityForName:[MenuItem entityName] inManagedObjectContext:self.managedObjectContext]; - pageItem.contentID = page.postID; - pageItem.name = page.titleForDisplay; - pageItem.type = MenuItemTypePage; - [items addObject:pageItem]; - } - - [[ContextManager sharedInstance] saveContext:self.managedObjectContext withCompletionBlock:^{ - if (success) { - success(items); - } - } onQueue:dispatch_get_main_queue()]; - }]; - } - failure:failure]; +- (MenuItem *)createItemWithPageID:(NSManagedObjectID *)pageObjectID inContext:(NSManagedObjectContext *)context { + Page *page = [context existingObjectWithID:pageObjectID error:nil]; + if (page == nil) { + return nil; + } + + if ([page.parentID integerValue] > 0) { + return nil; + } + + MenuItem *pageItem = [NSEntityDescription insertNewObjectForEntityForName:[MenuItem entityName] inManagedObjectContext:self.managedObjectContext]; + pageItem.contentID = page.postID; + pageItem.name = page.titleForDisplay; + pageItem.type = MenuItemTypePage; + return pageItem; } #pragma mark - private @@ -435,5 +407,3 @@ - (RemoteMenuItem *)remoteItemFromItem:(MenuItem *)item withItems:(NSOrderedSet< } @end - -NS_ASSUME_NONNULL_END diff --git a/WordPress/Classes/Services/PostRepository.swift b/WordPress/Classes/Services/PostRepository.swift index 7f66031aace2..34d389859c90 100644 --- a/WordPress/Classes/Services/PostRepository.swift +++ b/WordPress/Classes/Services/PostRepository.swift @@ -373,6 +373,44 @@ extension PostRepository { ) } + /// Fetch all pages of the given site. + /// + /// It's higly recommended to cancel the returned task at an appropriate timing. + /// + /// - Warning: As the function name suggests, calling this function makes many API requests to fetch the site's + /// _all pages_. Fetching all pages may be handy in some use cases, but also can be wasteful when user aborts + /// in the middle of fetching all pages, if the fetching is not cancelled. + /// + /// - Parameters: + /// - statuses: Only fetch pages whose status is included in the given statues. + /// - blogID: Object ID of the site. + /// - Returns: A `Task` instance representing the fetching. The fetch pages API requests will stop if the task is cancelled. + func fetchAllPages(statuses: [BasePost.Status], in blogID: TaggedManagedObjectID) -> Task<[TaggedManagedObjectID], Swift.Error> { + Task { + let pageSize = 100 + var allPages = [TaggedManagedObjectID]() + while true { + try Task.checkCancellation() + + let pageRange = allPages.count..<(allPages.count + pageSize) + let current = try await fetch( + type: Page.self, + statuses: statuses, + authorUserID: nil, + range: pageRange, + deleteOtherLocalPosts: false, + in: blogID + ) + allPages.append(contentsOf: current) + + if current.isEmpty || current.count < pageSize { + break + } + } + return allPages + } + } + private func fetch( type: P.Type, searchInput: String? = nil, diff --git a/WordPress/Classes/System/WordPress-Bridging-Header.h b/WordPress/Classes/System/WordPress-Bridging-Header.h index b88bb353230f..d2347d4c6f0d 100644 --- a/WordPress/Classes/System/WordPress-Bridging-Header.h +++ b/WordPress/Classes/System/WordPress-Bridging-Header.h @@ -34,6 +34,7 @@ #import "MeHeaderView.h" #import "MenuItem.h" #import "MenuItemsViewController.h" +#import "MenusService.h" #import "MenusViewController.h" #import "NSObject+Helpers.h" diff --git a/WordPress/Classes/ViewRelated/Menus/MenusViewController.h b/WordPress/Classes/ViewRelated/Menus/MenusViewController.h index 37f2e35f93f8..c2778e3ad0b9 100644 --- a/WordPress/Classes/ViewRelated/Menus/MenusViewController.h +++ b/WordPress/Classes/ViewRelated/Menus/MenusViewController.h @@ -2,12 +2,15 @@ NS_ASSUME_NONNULL_BEGIN -@class Blog; +@class Blog, MenusService; @interface MenusViewController : UIViewController + (MenusViewController *)controllerWithBlog:(Blog *)blog; +@property (nonatomic, strong, readonly) Blog *blog; +@property (nonatomic, strong, readonly) MenusService *menusService; + @end NS_ASSUME_NONNULL_END diff --git a/WordPress/Classes/ViewRelated/Menus/MenusViewController.m b/WordPress/Classes/ViewRelated/Menus/MenusViewController.m index d90a3c7ad4d3..d0ea15f24916 100644 --- a/WordPress/Classes/ViewRelated/Menus/MenusViewController.m +++ b/WordPress/Classes/ViewRelated/Menus/MenusViewController.m @@ -15,8 +15,6 @@ #import #import - - static CGFloat const ScrollViewOffsetAdjustmentPadding = 10.0; @interface MenusViewController () @@ -32,9 +30,6 @@ @interface MenusViewController () Void, failure: @escaping (Error) -> Void) -> () -> Void { + let coreDataStack = ContextManager.shared + let repository = PostRepository(coreDataStack: coreDataStack) + let fetchAllPagesTask = repository.fetchAllPages(statuses: [.publish], in: TaggedManagedObjectID(blog)) + + // Wait for the fetching all pages task to complete and pass its result to the success or failure block. + Task { [weak self] in + let allPages: [TaggedManagedObjectID] + do { + allPages = try await fetchAllPagesTask.value + } catch { + failure(error) + return + } + + guard let menusService = self?.menusService else { return } + + await menusService.managedObjectContext.perform { + let items = allPages.compactMap { + menusService.createItem(withPageID: $0.objectID, in: menusService.managedObjectContext) + } + success(items) + } + } + + return fetchAllPagesTask.cancel + } + +} diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index b1489a9b2b5e..9a4eb7e639c0 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -1270,6 +1270,8 @@ 4A358DEA29B5F14C00BFCEBE /* SharingButton+Lookup.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A358DE829B5F14C00BFCEBE /* SharingButton+Lookup.swift */; }; 4A526BDF296BE9A50007B5BA /* CoreDataService.m in Sources */ = {isa = PBXBuildFile; fileRef = 4A526BDD296BE9A50007B5BA /* CoreDataService.m */; }; 4A526BE0296BE9A50007B5BA /* CoreDataService.m in Sources */ = {isa = PBXBuildFile; fileRef = 4A526BDD296BE9A50007B5BA /* CoreDataService.m */; }; + 4A535E142AF3368B008B87B9 /* MenusViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A535E132AF3368B008B87B9 /* MenusViewController.swift */; }; + 4A535E152AF3368B008B87B9 /* MenusViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A535E132AF3368B008B87B9 /* MenusViewController.swift */; }; 4A76A4BB29D4381100AABF4B /* CommentService+LikesTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A76A4BA29D4381000AABF4B /* CommentService+LikesTests.swift */; }; 4A76A4BD29D43BFD00AABF4B /* CommentService+MorderationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A76A4BC29D43BFD00AABF4B /* CommentService+MorderationTests.swift */; }; 4A76A4BF29D4F0A500AABF4B /* reader-post-comments-success.json in Resources */ = {isa = PBXBuildFile; fileRef = 4A76A4BE29D4F0A500AABF4B /* reader-post-comments-success.json */; }; @@ -6895,6 +6897,7 @@ 4A358DE829B5F14C00BFCEBE /* SharingButton+Lookup.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "SharingButton+Lookup.swift"; sourceTree = ""; }; 4A526BDD296BE9A50007B5BA /* CoreDataService.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = CoreDataService.m; sourceTree = ""; }; 4A526BDE296BE9A50007B5BA /* CoreDataService.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CoreDataService.h; sourceTree = ""; }; + 4A535E132AF3368B008B87B9 /* MenusViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MenusViewController.swift; sourceTree = ""; }; 4A76A4BA29D4381000AABF4B /* CommentService+LikesTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "CommentService+LikesTests.swift"; sourceTree = ""; }; 4A76A4BC29D43BFD00AABF4B /* CommentService+MorderationTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "CommentService+MorderationTests.swift"; sourceTree = ""; }; 4A76A4BE29D4F0A500AABF4B /* reader-post-comments-success.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = "reader-post-comments-success.json"; sourceTree = ""; }; @@ -10046,6 +10049,7 @@ children = ( 086E1FDE1BBB35D2002D86CA /* MenusViewController.h */, 086E1FDF1BBB35D2002D86CA /* MenusViewController.m */, + 4A535E132AF3368B008B87B9 /* MenusViewController.swift */, C3B554502965C32A00A04753 /* MenusViewController+JetpackBannerViewController.swift */, 08216FA81CDBF95100304BA7 /* MenuItemEditingViewController.h */, 08216FA91CDBF95100304BA7 /* MenuItemEditingViewController.m */, @@ -22147,6 +22151,7 @@ F1DB8D292288C14400906E2F /* Uploader.swift in Sources */, FA3A281B2A39C8FF00206D74 /* BlazeCampaignSingleStatView.swift in Sources */, 3FB1929026C6109F000F5AA3 /* TimeSelectionView.swift in Sources */, + 4A535E142AF3368B008B87B9 /* MenusViewController.swift in Sources */, FAB8AB8B25AFFE7500F9F8A0 /* JetpackRestoreService.swift in Sources */, 821738091FE04A9E00BEC94C /* DateAndTimeFormatSettingsViewController.swift in Sources */, 5D6C4B081B603E03005E3C43 /* WPContentSyncHelper.swift in Sources */, @@ -23802,6 +23807,7 @@ FABB20CD2602FC2C00C8785C /* PostUploadOperation.swift in Sources */, FABB20CE2602FC2C00C8785C /* VerticallyStackedButton.m in Sources */, FABB20CF2602FC2C00C8785C /* PageListViewController.swift in Sources */, + 4A535E152AF3368B008B87B9 /* MenusViewController.swift in Sources */, FABB20D02602FC2C00C8785C /* CoreDataHelper.swift in Sources */, FABB20D12602FC2C00C8785C /* LocalCoreDataService.m in Sources */, FE29EFCE29A91160007CE034 /* WPAdminConvertibleRouter.swift in Sources */, diff --git a/WordPress/WordPressTest/PostRepositoryPostsListTests.swift b/WordPress/WordPressTest/PostRepositoryPostsListTests.swift index 504a5c6197c5..b385a1a3284f 100644 --- a/WordPress/WordPressTest/PostRepositoryPostsListTests.swift +++ b/WordPress/WordPressTest/PostRepositoryPostsListTests.swift @@ -62,6 +62,17 @@ class PostRepositoryPostsListTests: CoreDataTestCase { XCTAssertEqual(total, 15) } + func testFetchingAllPages() async throws { + try await preparePostsList(type: "page", total: 1_120) + + let pages = try await repository.fetchAllPages(statuses: [], in: blogID).value + XCTAssertEqual(pages.count, 1_120) + + // There should still be 15 posts after the search: no local posts should be deleted + let total = await contextManager.performQuery { $0.countObjects(ofType: Page.self) } + XCTAssertEqual(total, 1_120) + } + } // MARK: - Tests that ensure the `preparePostsList` works as expected diff --git a/WordPress/WordPressTest/PostRepositoryTests.swift b/WordPress/WordPressTest/PostRepositoryTests.swift index 765a99acf347..059e6ff8f47e 100644 --- a/WordPress/WordPressTest/PostRepositoryTests.swift +++ b/WordPress/WordPressTest/PostRepositoryTests.swift @@ -241,6 +241,77 @@ class PostRepositoryTests: CoreDataTestCase { } } + func testFetchAllPagesAPIError() async throws { + // Use an empty array to simulate an HTTP API error + remoteMock.remotePostsToReturnOnSyncPostsOfType = [] + + do { + let _ = try await repository.fetchAllPages(statuses: [], in: blogID).value + XCTFail("The above call should throw") + } catch { + // Do nothing. + } + } + + func testFetchAllPagesStopsOnEmptyAPIResponse() async throws { + // Given two pages of API result: first page returns 100 page instances, and the second page returns an empty result. + remoteMock.remotePostsToReturnOnSyncPostsOfType = [ + try (1...100).map { + let post = try XCTUnwrap(RemotePost(siteID: NSNumber(value: $0), status: "publish", title: "Post: Test", content: "This is a test post")) + post.type = "page" + return post + }, + [] + ] + + let pages = try await repository.fetchAllPages(statuses: [.publish], in: blogID).value + XCTAssertEqual(pages.count, 100) + } + + func testFetchAllPagesStopsOnNonFullPageAPIResponse() async throws { + // Given two pages of API result: first page returns 100 page instances, and the second page returns 10 (any amount that's less than 100) page instances. + remoteMock.remotePostsToReturnOnSyncPostsOfType = [ + try (1...100).map { + let post = try XCTUnwrap(RemotePost(siteID: NSNumber(value: $0), status: "publish", title: "Post: Test", content: "This is a test post")) + post.type = "page" + return post + }, + try (1...10).map { + let post = try XCTUnwrap(RemotePost(siteID: NSNumber(value: $0), status: "publish", title: "Post: Test", content: "This is a test post")) + post.type = "page" + return post + }, + ] + + let pages = try await repository.fetchAllPages(statuses: [.publish], in: blogID).value + XCTAssertEqual(pages.count, 110) + } + + func testCancelFetchAllPages() async throws { + remoteMock.remotePostsToReturnOnSyncPostsOfType = try (1...10).map { pageNo in + try (1...100).map { + let post = try XCTUnwrap(RemotePost(siteID: NSNumber(value: pageNo * 100 + $0), status: "publish", title: "Post: Test", content: "This is a test post")) + post.type = "page" + return post + } + } + + let cancelled = expectation(description: "Fetching task returns cancellation error") + let task = repository.fetchAllPages(statuses: [.publish], in: blogID) + + DispatchQueue.global().asyncAfter(deadline: .now() + .microseconds(100)) { + task.cancel() + } + + do { + let _ = try await task.value + } catch is CancellationError { + cancelled.fulfill() + } + + await fulfillment(of: [cancelled], timeout: 0.3) + } + } // These mock classes are copied from PostServiceWPComTests. We can't simply remove the `private` in the original class @@ -265,7 +336,7 @@ private class PostServiceRESTMock: PostServiceRemoteREST { } var remotePostToReturnOnGetPostWithID: RemotePost? - var remotePostsToReturnOnSyncPostsOfType = [RemotePost]() + var remotePostsToReturnOnSyncPostsOfType = [[RemotePost]]() // Each element contains an array of RemotePost for one API request. var remotePostToReturnOnUpdatePost: RemotePost? var remotePostToReturnOnCreatePost: RemotePost? @@ -289,7 +360,15 @@ private class PostServiceRESTMock: PostServiceRemoteREST { } override func getPostsOfType(_ postType: String!, options: [AnyHashable: Any]! = [:], success: (([RemotePost]?) -> Void)!, failure: ((Error?) -> Void)!) { - success(self.remotePostsToReturnOnSyncPostsOfType) + guard !remotePostsToReturnOnSyncPostsOfType.isEmpty else { + failure(testError()) + return + } + + let result = remotePostsToReturnOnSyncPostsOfType.removeFirst() + DispatchQueue.main.asyncAfter(deadline: .now() + .microseconds(50)) { + success(result) + } } override func update(_ post: RemotePost!, success: ((RemotePost?) -> Void)!, failure: ((Error?) -> Void)!) {