Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use PostRepository to fetch pages in the Menu screen #21949

Merged
merged 6 commits into from
Nov 6, 2023
Merged

Use PostRepository to fetch pages in the Menu screen #21949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fb3fad8
Use PostRepository to fetch pages in the Menu screen
crazytonyli Nov 2, 2023
0e63b7f
Add a release note
crazytonyli Nov 2, 2023
187e134
Add an unit test for cancelling fetching all pages
crazytonyli Nov 5, 2023
53d634a
Add unit tests for fetching all pages with mutli-page API responses
crazytonyli Nov 5, 2023
5339b86
Add another unit test which is backed by API stubs
crazytonyli Nov 6, 2023
ed781a5
Add an unit test for fetching pages API error
crazytonyli Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
-----
Expand Down
10 changes: 3 additions & 7 deletions WordPress/Classes/Services/MenusService.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines -75 to +77
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick. There docs in the previous method has @params. The new one doesn't. One could see this as a regression.

At the same time, I personally prefer this style of documentation, which I interpret as "only document was is not obvious."

What do you think? Is it worth using this as an example to start a team-wide conversation to define a standard?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"only document was is not obvious."

This is exactly what I was thinking. I don't think those param docs in the original function is really useful.

*
*/
- (void)generateDefaultMenuItemsForBlog:(Blog *)blog
success:(nullable void(^)(NSArray <MenuItem *> * _Nullable defaultItems))success
failure:(nullable MenusServiceFailureBlock)failure;
- (nullable MenuItem *)createItemWithPageID:(NSManagedObjectID *)pageObjectID inContext:(NSManagedObjectContext *)context;

@end

Expand Down
60 changes: 15 additions & 45 deletions WordPress/Classes/Services/MenusService.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@
#import "WordPress-Swift.h"
@import WordPressKit;

NS_ASSUME_NONNULL_BEGIN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed? I'm guessing because it's not necessary in .m files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. No benefit whatsoever in using it in .m files.


@implementation MenusService

#pragma mark - Menus availability
Expand Down Expand Up @@ -161,47 +159,21 @@ - (void)deleteMenu:(Menu *)menu
failure:failure];
}

- (void)generateDefaultMenuItemsForBlog:(Blog *)blog
success:(nullable void(^)(NSArray <MenuItem *> * _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];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered passing an error object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That didn't cross my mind. That's a good idea though: turning this Objective-C function to a throwing Swift function.

But unfortunately, after giving it go, I noticed Swift turns the function to createItem() throws -> MenuItem, but I want an optional return value: createItem() throws -> MenuItem?. I think I'll keep it as it is, because returning an non optional MenuItem is a lie.

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
Expand Down Expand Up @@ -435,5 +407,3 @@ - (RemoteMenuItem *)remoteItemFromItem:(MenuItem *)item withItems:(NSOrderedSet<
}

@end

NS_ASSUME_NONNULL_END
38 changes: 38 additions & 0 deletions WordPress/Classes/Services/PostRepository.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Blog>) -> Task<[TaggedManagedObjectID<Page>], Swift.Error> {
Task {
let pageSize = 100
var allPages = [TaggedManagedObjectID<Page>]()
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<P: AbstractPost>(
type: P.Type,
searchInput: String? = nil,
Expand Down
1 change: 1 addition & 0 deletions WordPress/Classes/System/WordPress-Bridging-Header.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#import "MeHeaderView.h"
#import "MenuItem.h"
#import "MenuItemsViewController.h"
#import "MenusService.h"
#import "MenusViewController.h"

#import "NSObject+Helpers.h"
Expand Down
5 changes: 4 additions & 1 deletion WordPress/Classes/ViewRelated/Menus/MenusViewController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
20 changes: 12 additions & 8 deletions WordPress/Classes/ViewRelated/Menus/MenusViewController.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
#import <WordPressShared/WPFontManager.h>
#import <WordPressShared/WPStyleGuide.h>



static CGFloat const ScrollViewOffsetAdjustmentPadding = 10.0;

@interface MenusViewController () <UIScrollViewDelegate, MenuHeaderViewControllerDelegate, MenuDetailsViewControllerDelegate, MenuItemsViewControllerDelegate>
Expand All @@ -32,9 +30,6 @@ @interface MenusViewController () <UIScrollViewDelegate, MenuHeaderViewControlle
@property (nonatomic, strong, readonly) UILabel *itemsLoadingLabel;
@property (nonatomic, strong, readonly) UIBarButtonItem *saveButtonItem;

@property (nonatomic, strong, readonly) Blog *blog;
@property (nonatomic, strong, readonly) MenusService *menusService;

@property (nonatomic, strong) MenuLocation *selectedMenuLocation;
@property (nonatomic, strong) Menu *updatedMenuForSaving;
@property (nonatomic, strong) Menu *initialMenuSelection;
Expand All @@ -45,6 +40,8 @@ @interface MenusViewController () <UIScrollViewDelegate, MenuHeaderViewControlle
@property (nonatomic, assign) BOOL needsSave;
@property (nonatomic, assign) BOOL isSaving;

@property (nonatomic, strong) void (^fetchAllPagesCancellationHandler)(void);

@end

@implementation MenusViewController
Expand Down Expand Up @@ -176,6 +173,15 @@ - (void)viewWillDisappear:(BOOL)animated
[[NSNotificationCenter defaultCenter] removeObserver:self];
}

- (void)viewDidDisappear:(BOOL)animated
{
[super viewDidDisappear:animated];

if (self.navigationController == nil && self.fetchAllPagesCancellationHandler != nil) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the code check for the navigationController value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure we only cancel the fetching when user taps the Back button. viewDidDisappear is also called when a new view controller is pushed into the stack. In which case, we'd like the pages fetching to continue.

self.fetchAllPagesCancellationHandler();
}
}

#pragma mark - Views setup

- (void)updateScrollViewContentSize
Expand Down Expand Up @@ -355,9 +361,7 @@ - (void)loadDefaultMenuItemsIfNeeded
void(^failureBlock)(NSError *) = ^(NSError * __unused error) {
weakSelf.itemsLoadingLabel.text = NSLocalizedString(@"An error occurred loading the menu, please check your internet connection.", @"Menus error message seen when an error occurred loading a specific menu.");
};
[self.menusService generateDefaultMenuItemsForBlog:self.blog
success:successBlock
failure:failureBlock];
self.fetchAllPagesCancellationHandler = [self fetchAllPagesWithSuccess:successBlock failure:failureBlock];
}
}

Expand Down
36 changes: 36 additions & 0 deletions WordPress/Classes/ViewRelated/Menus/MenusViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import Foundation

extension MenusViewController {

/// Fetch all pages from the site.
///
/// - Returns: A block that can be used to cancel the fetching.
@objc func fetchAllPages(success: @escaping ([MenuItem]) -> 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<Page>]
do {
allPages = try await fetchAllPagesTask.value
} catch {
failure(error)
return
}

guard let menusService = self?.menusService else { return }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest moving the check at the start of the Task to exit early. But maybe it's best to check if self is still available after the fetchAllPagesTask completed, in case the ViewController (self) was deallocated in the meantime. Is this the reason for the check being at this point?


await menusService.managedObjectContext.perform {
let items = allPages.compactMap {
menusService.createItem(withPageID: $0.objectID, in: menusService.managedObjectContext)
}
success(items)
}
}

return fetchAllPagesTask.cancel
}

}
6 changes: 6 additions & 0 deletions WordPress/WordPress.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -6895,6 +6897,7 @@
4A358DE829B5F14C00BFCEBE /* SharingButton+Lookup.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "SharingButton+Lookup.swift"; sourceTree = "<group>"; };
4A526BDD296BE9A50007B5BA /* CoreDataService.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = CoreDataService.m; sourceTree = "<group>"; };
4A526BDE296BE9A50007B5BA /* CoreDataService.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CoreDataService.h; sourceTree = "<group>"; };
4A535E132AF3368B008B87B9 /* MenusViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MenusViewController.swift; sourceTree = "<group>"; };
4A76A4BA29D4381000AABF4B /* CommentService+LikesTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "CommentService+LikesTests.swift"; sourceTree = "<group>"; };
4A76A4BC29D43BFD00AABF4B /* CommentService+MorderationTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "CommentService+MorderationTests.swift"; sourceTree = "<group>"; };
4A76A4BE29D4F0A500AABF4B /* reader-post-comments-success.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = "reader-post-comments-success.json"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -10046,6 +10049,7 @@
children = (
086E1FDE1BBB35D2002D86CA /* MenusViewController.h */,
086E1FDF1BBB35D2002D86CA /* MenusViewController.m */,
4A535E132AF3368B008B87B9 /* MenusViewController.swift */,
C3B554502965C32A00A04753 /* MenusViewController+JetpackBannerViewController.swift */,
08216FA81CDBF95100304BA7 /* MenuItemEditingViewController.h */,
08216FA91CDBF95100304BA7 /* MenuItemEditingViewController.m */,
Expand Down Expand Up @@ -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 */,
Expand Down Expand Up @@ -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 */,
Expand Down
11 changes: 11 additions & 0 deletions WordPress/WordPressTest/PostRepositoryPostsListTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading