Use PostRepository to fetch pages in the Menu screen #21949

crazytonyli · 2023-11-02T06:20:43Z

Fixes #21947

There are a few issues in the "sync posts" function in PostServices, which I have described in #21814. In this PR, I have replace one usage of the "sync posts" function with the new implementation in PostRepository.

To test: verify #21947 is fixed.

Regression Notes

Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
Verify Menus screen enters an infinite loop of loading site's pages #21947
What automated tests I added (or what prevented me from doing so)
Unit tests for successful API responses and cancellation.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist: N/A

Fixes #21947

wpmobilebot · 2023-11-02T06:40:47Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21949-ed781a5
	Version	23.6
	Bundle ID	org.wordpress.alpha
	Commit	`ed781a5`
	App Center Build	WPiOS - One-Offs #7709

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-11-02T06:44:49Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21949-ed781a5
	Version	23.6
	Bundle ID	com.jetpack.alpha
	Commit	`ed781a5`
	App Center Build	jetpack-installable-builds #6724

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

mokagio

I'm approving to unblock but I haven't had the time to manually test.

To be clear, that's on me. I'll try to come back to this, but wanted to unblock just in case.

mokagio · 2023-11-06T01:19:18Z

WordPress/Classes/Services/MenusService.h

- *  @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.


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?

"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.

mokagio · 2023-11-06T01:20:24Z

WordPress/Classes/Services/MenusService.m

@@ -9,8 +9,6 @@
 #import "WordPress-Swift.h"
 @import WordPressKit;

-NS_ASSUME_NONNULL_BEGIN


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

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

mokagio · 2023-11-06T01:20:57Z

WordPress/Classes/Services/MenusService.m

-                         }
-                         failure:failure];
+- (MenuItem *)createItemWithPageID:(NSManagedObjectID *)pageObjectID inContext:(NSManagedObjectContext *)context {
+    Page *page = [context existingObjectWithID:pageObjectID error:nil];


Have you considered passing an error object?

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.

mokagio · 2023-11-06T01:24:54Z

WordPress/Classes/ViewRelated/Menus/MenusViewController.m

+{
+    [super viewDidDisappear:animated];
+
+    if (self.navigationController == nil && self.fetchAllPagesCancellationHandler != nil) {


Why does the code check for the navigationController value?

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.

mokagio · 2023-11-06T01:28:36Z

WordPress/Classes/ViewRelated/Menus/MenusViewController.swift

+                return
+            }
+
+            guard let menusService = self?.menusService else { return }


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?

mokagio · 2023-11-06T01:30:47Z

WordPress/WordPressTest/PostRepositoryTests.swift

+            cancelled.fulfill()
+        }
+
+        await fulfillment(of: [cancelled], timeout: 0.3)


This is a neat test 🤩

mokagio · 2023-11-06T01:34:48Z

WordPress/WordPressTest/PostRepositoryTests.swift

+
+        let pages = try await repository.fetchAllPages(statuses: [.publish], in: blogID).value
+        XCTAssertEqual(pages.count, 110)
+    }


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.

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.

WordPress-iOS/WordPress/WordPressTest/PostRepositoryTests.swift

Lines 350 to 354 in 5339b86

override func getPostsOfType(_ postType: String!, options: [AnyHashable: Any]! = [:], success: (([RemotePost]?) -> Void)!, failure: ((Error?) -> Void)!) {

guard !remotePostsToReturnOnSyncPostsOfType.isEmpty else {

failure(testError())

return

}

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.

Use PostRepository to fetch pages in the Menu screen

fb3fad8

Fixes #21947

crazytonyli added the Menus label Nov 2, 2023

crazytonyli added this to the 23.7 milestone Nov 2, 2023

crazytonyli requested a review from mokagio November 2, 2023 06:20

crazytonyli self-assigned this Nov 2, 2023

Add a release note

0e63b7f

crazytonyli added 2 commits November 6, 2023 10:55

Add an unit test for cancelling fetching all pages

187e134

Add unit tests for fetching all pages with mutli-page API responses

53d634a

mokagio approved these changes Nov 6, 2023

View reviewed changes

crazytonyli added 2 commits November 6, 2023 15:11

Add another unit test which is backed by API stubs

5339b86

Add an unit test for fetching pages API error

ed781a5

crazytonyli enabled auto-merge November 6, 2023 02:53

crazytonyli merged commit ae33467 into trunk Nov 6, 2023
23 checks passed

crazytonyli deleted the tonyli-use-post-repository-search-in-menu branch November 6, 2023 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PostRepository to fetch pages in the Menu screen #21949

Use PostRepository to fetch pages in the Menu screen #21949

crazytonyli commented Nov 2, 2023 •

edited

Loading

wpmobilebot commented Nov 2, 2023 •

edited

Loading

wpmobilebot commented Nov 2, 2023 •

edited

Loading

mokagio left a comment

mokagio Nov 6, 2023

crazytonyli Nov 6, 2023

mokagio Nov 6, 2023

crazytonyli Nov 6, 2023

mokagio Nov 6, 2023

crazytonyli Nov 6, 2023

mokagio Nov 6, 2023

crazytonyli Nov 6, 2023

mokagio Nov 6, 2023

mokagio Nov 6, 2023

mokagio Nov 6, 2023

crazytonyli Nov 6, 2023

crazytonyli Nov 6, 2023

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

Use PostRepository to fetch pages in the Menu screen #21949

Use PostRepository to fetch pages in the Menu screen #21949

Conversation

crazytonyli commented Nov 2, 2023 • edited Loading

Regression Notes

wpmobilebot commented Nov 2, 2023 • edited Loading

wpmobilebot commented Nov 2, 2023 • edited Loading

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazytonyli commented Nov 2, 2023 •

edited

Loading

wpmobilebot commented Nov 2, 2023 •

edited

Loading

wpmobilebot commented Nov 2, 2023 •

edited

Loading