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.

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
Changes from 4 commits
Commits
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
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
-----
10 changes: 3 additions & 7 deletions WordPress/Classes/Services/MenusService.h
Original file line number Diff line number Diff line change
@@ -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

60 changes: 15 additions & 45 deletions WordPress/Classes/Services/MenusService.m
Original file line number Diff line number Diff line change
@@ -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
@@ -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
@@ -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
Original file line number Diff line number Diff line change
@@ -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,
1 change: 1 addition & 0 deletions WordPress/Classes/System/WordPress-Bridging-Header.h
Original file line number Diff line number Diff line change
@@ -34,6 +34,7 @@
#import "MeHeaderView.h"
#import "MenuItem.h"
#import "MenuItemsViewController.h"
#import "MenusService.h"
#import "MenusViewController.h"

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



static CGFloat const ScrollViewOffsetAdjustmentPadding = 10.0;

@interface MenusViewController () <UIScrollViewDelegate, MenuHeaderViewControllerDelegate, MenuDetailsViewControllerDelegate, MenuItemsViewControllerDelegate>
@@ -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;
@@ -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
@@ -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
@@ -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];
}
}

36 changes: 36 additions & 0 deletions WordPress/Classes/ViewRelated/Menus/MenusViewController.swift
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
Original file line number Diff line number Diff line change
@@ -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 = "<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>"; };
@@ -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 */,
71 changes: 69 additions & 2 deletions WordPress/WordPressTest/PostRepositoryTests.swift
Original file line number Diff line number Diff line change
@@ -241,6 +241,65 @@ class PostRepositoryTests: CoreDataTestCase {
}
}

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)
}
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 a test to explicitly check that no more requests are made after an empty or < page size response has been received?

🤔 I guess the fact that await triggers sort of guarantees this, but I don't know... I feel like it would be "nice" to test it explicitly.

Not a must. If I had some time, I'd see if I could change the source code to make these tests pass with more requests, but I don't think I'll get to it any time soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

explicitly check that no more requests are made after an empty or < page size response has been received?

I think that's already covered here. If there are more requests fired, then the fake PostServiceRemote API response would make sure either

  • an extra 100 or 10 page instances are returned to the PostRepository fucntion. Or,
  • a failure is returned.

That means the final total pages should be more than 110 and breaks the assertion or throw an error. Given the test passes, we should(?) be fairly certain that there no unnecessary requests are fired.

I guess that's not entirely true. Because if the fetchAllPages is implemented in a way that removes duplication or swallows error, then the test would still pass when there are unnecessary requests.

I guess we could to add a XCTFail to the error case to be really really sure.

override func getPostsOfType(_ postType: String!, options: [AnyHashable: Any]! = [:], success: (([RemotePost]?) -> Void)!, failure: ((Error?) -> Void)!) {
guard !remotePostsToReturnOnSyncPostsOfType.isEmpty else {
failure(testError())
return
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added another test in ed781a5 to ensure when there is an HTTP API error, the fetchAllPages should throw, which means the error is not swallowed and at the same time ensure no extra requests are fired after an empty or < page size response.


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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a neat test 🤩

}

}

// These mock classes are copied from PostServiceWPComTests. We can't simply remove the `private` in the original class
@@ -265,7 +324,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 +348,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)!) {