Implement fetching posts/pages list in PostRepository #21814

crazytonyli · 2023-10-18T01:26:49Z

Description

This new implementation is a replacement of the syncing post function in PostService. However, it's not a direct translation.

Issues

The existing syncing post function has a few issues.

Issue 1: The syncing post function actually fetches all pages in the site, regardless what the function argument requests.

Here are two instances of the issue: #21813 and #21812.

The same scenarios will appear in other place that call the syncing post function. i.e. Site Settings, Menu. And, there is no way to disable this behaviour currently, because it's coded as an internal implementation detail.

For reference, probably not really an useful information, but #10264 is the PR that implemented this behaviour.

Issue 2: It's not clear when to use the `purgesLocalSync` argument.

When this argument is true, only the returned posts are kept in the local database and all other posts will be deleted.

A typical use case is posts/pages list. The list is implemented as displaying posts that's in Core Data database. The list changes when fetching posts API requests come back and new posts are added or updated in the database. As user scrolls to bottom of the list, the app sends new requests to load more posts. The purgesLocalSync value depending on different scenarios:

purgesLocalSync = true when loading the first page. This can happen when user enters the screen, or pulls to refresh. In this scenario, we want to delete other posts in the database, so that only the first page is displayed, and other posts can be re-fetched when user continues to scroll through the list.
purgesLocalSync = false when loading the following pages. This happens when user scrolls to the bottom of the list. In this scenario, we want to keep all loaded posts in the database so that all can be displayed on screen.

A definitely incorrect usage is #21754. The syncing post call should not set purgeLocalSync to true, because there is no need to only keep the three load pages and delete all others.

New implementation

The new implementation in PostRepository addresses the above two issues.

To address issue 2, I have created two funtions for different scenarios: paginate for loading paginated list (Posts List and Pages List) and search for finding pages without thinking about the state of the database.

For issue 1, the new implementation removes the ability to loading all pages. This of course is a reverse of the original PR #10264. But I can add a dedicated function loadAll later to be used in Pages List. And, there should be less chance that this loadAll function getting used in places where don't need to load all posts or pages.

Tests

~~I will add some unit tests in a follow up PR, because I want to spend dedicated time on writing some useful tests.~~

Update: I have added unit tests to this PR.

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)
None.
What automated tests I added (or what prevented me from doing so)
See the "Tests" section.

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

wpmobilebot · 2023-10-18T01:52:51Z

📲 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	pr21814-4dac5b7
	Version	23.4
	Bundle ID	org.wordpress.alpha
	Commit	`4dac5b7`
	App Center Build	WPiOS - One-Offs #7461

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

wpmobilebot · 2023-10-18T01:52:59Z

📲 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	pr21814-4dac5b7
	Version	23.4
	Bundle ID	com.jetpack.alpha
	Commit	`4dac5b7`
	App Center Build	jetpack-installable-builds #6490

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

kean · 2023-10-18T16:03:36Z

@crazytonyli please keep in mind that the posts/pages list and search are re-worked almost entirely in #21789 and #21744. I would suggest targeting the task/ui-modernization-posts-and-pages feature branch for your changes too (went with a feature branch because the changes are localized to a handful of classes and it would've required too much effort to try and feature-flag it).

crazytonyli · 2023-10-18T20:54:02Z

@kean Thanks for letting me know. I'll checkout the feature branch.

After a quick look at #21744, it appears there is no changes to how the PostService.syncPosts function is called. So, I think my change here would be simply additions on top of your UI modernization changes. And the new PostRepositor functions here can be used to replace all the PostService.syncPosts calls.

…sult

mokagio · 2023-10-19T02:30:12Z

WordPress/Classes/Services/PostRepository.swift

+    func statuses() -> [String]? {
+        options.statuses
+    }


Have you considered making this and the other computed vars?

I don't know if there's a recommendation somewhere. It might just be my personal preference, but I find that this kind of getters read better as properties than functions. I guess methods give me the idea that something is happening under the hood, whereas properties feel more like reading information.

Update: Never mind... This conforms to the PostServiceRemoteOptions protocol (a protocol that should be a struct by now...) and therefore they need to be func:

Yeah, Objective-C...

mokagio · 2023-10-19T02:35:09Z

WordPress/Classes/Services/PostRepository.swift

+            // There is an assertion above to ensure the app doesn't fall into this case.
+            return []


Have you considered fatalError, given this should be an unreachable code path?

Or, is return [] a safeguard for the production code?

Just don't want to duplicate the assertion code and its messaging. 😄

mokagio · 2023-10-19T02:43:45Z

WordPress/Classes/Services/PostRepository.swift

+            return updatedPosts.compactMap {
+                guard let post = $0 as? P else {
+                    fatalError("Expecting a \(postType) as \(type), but got \($0)")
+                }
+                return TaggedManagedObjectID(post)
+            }


I was surprised by the compactMap given TaggedManagedObjectID.init is not of the Optional kind.

Then I thought it might be because of the PostHelper merge... comes from the Objective-C layer, and doesn't have nullability annotation. But I saw it has NS_ASSUME_NONNULL_BEGIN and _END.

That's all to say, maybe map would do here?

Good catch! Updated in 4dac5b7

mokagio · 2023-10-19T02:52:12Z

WordPress/WordPressTest/PostRepositoryPostsListTests.swift

+    var repository: PostRepository!
+    var blogID: TaggedManagedObjectID<Blog>!
+
+    override func setUp() async throws {
+        repository = PostRepository(coreDataStack: contextManager)
+
+        let loggedIn = try await signIn()
+        blogID = try await contextManager.performAndSave {
+            let blog = try BlogBuilder($0)
+                .with(dotComID: 42)
+                .withAccount(id: loggedIn)
+                .build()
+            return TaggedManagedObjectID(blog)
+        }
+    }


How do you feel about making these into methods that each test can call as part of it's arrange step?

We'd have to code a bit more, but it would prevent the test case from holding repository and blogID in memory after it runs. (I haven't verified the behavior in Xcode 15, but it's a community documented quirk).

To be specific:

// We could alternatively read coreDataStack from contextManager in the implementation // to keep the method as requiring no arguments. func makeRepository(coreDataStack: CoreDataStack) -> PostRepository { PostRepository(coreDataStack: coreDataStack) } func makeBlogID(coreDataStack: CoreDataStack) async throws -> TaggedManagedObjectID<Blog> { // ... } func testPagination() async throws { let repository = makeRepository(...) let blogID = try await makeBlogID(...) // Given there are 15 published posts on the site try await preparePostsList(type: "post", total: 15) // ...

I'm aware of this weird XCTest behaviour. But, I don't think we should avoid using instance variables just because this weird behaviour exists? Of course, making everything transient and deallocated after each test run would reduce chances of potential issues. But that feels like a lot afford, in terms of coding, especially if we know those instance variables—even though being "leaked"–should not introduce problems.

But that feels like a lot afford, in terms of coding

Is it extra effort, though?

Most of the coding has to be done regardless. One version has it in make... functions, the other in the setup.

There's an additional advantage: each test method is self contained and sets up all and only what it needs.

I've seen test (not sure if in this codebase, but I can look it up) where steps taken in the setUp had to be partially undone in individual tests because of different state expectations. Of course, one could argue that if a test has to do that much setup, maybe the problem is in the software design...

Anyways, I think this is a conversation broader than the scope of this PR 😄 Thank you for sharing your point of view. I'll P2 at some point.

Is it extra effort, though?

Maybe not too bad in this particular test case where there is only two properties. But for other test cases that have more properties, we'll have to repeat the instantiation statements in all test functions, which feels like a bit too much...

My point is this setting up test environment code belongs to the setUp function, and it'd be much nicer to keep them there. If we have to make a convention, an alternative to moving those instantiation code out of setUp and into the test functions is, setting those properties to nil in tearDown. We kind of get benefits from both world: setting up test environment code is consolidated in the setUp function and they won't be kept in memory forever due to how XCTest runs tests.

mokagio · 2023-10-19T02:55:35Z

WordPress/WordPressTest/PostRepositoryPostsListTests.swift

+
+}
+
+// MARK: - Tests that ensure the `preparePostsList` works as expected


Grammar nitpick

Suggested change

// MARK: - Tests that ensure the `preparePostsList` works as expected

// MARK: - Tests that ensures the `preparePostsList` works as expected

Suggested change

// MARK: - Tests that ensure the `preparePostsList` works as expected

// MARK: - Tests to ensure the `preparePostsList` works as expected

mokagio · 2023-10-19T02:58:59Z

WordPress/WordPressTest/PostRepositoryPostsListTests.swift

+
+}
+
+private extension PostRepositoryPostsListTests {


Nitpick: Reading the test top to bottom, I was surprised by the Stub in the tests names above. I think if I'd seen the method definition before the tests, the logic would have been clearer.

Alternatively, the method could live in a dedicated test helper object, with dedicated test file. But I wouldn't recommend going to that extent unless we were to reuse it elsewhere.

mokagio · 2023-10-19T02:59:56Z

WordPress/WordPressTest/PostRepositoryPostsListTests.swift

+        XCTAssertEqual(total, 15)
+    }
+
+}


Have you considered adding tests to ensure the orderBy and descending parameters are respected?

As mentioned in the preparePostsList API doc, these two parameters are ignored intentionally. I didn't explain there why though. It's because I don't think implementing sorting would mean much to the test case. Asserting the returned posts are in expected order doesn't test any app code. The app code doesn't really care about the order: it blindly trusts the API would return posts in the correct order.

Oh, right right. I got confused, my bad.

I feel like if the parameters are given, we should somehow test they are passed along correctly... 🤔

Then again, as far as business logic goes, that's quite straightforward. The cost of debugging if a mistake is made should be low enough.

crazytonyli · 2023-10-19T07:50:57Z

There is one UI test failure in this PR, but I doubt it's caused by this PR's changes: None of this PR's new code is executed by the app.

crazytonyli added Posts + Pages Pages List labels Oct 18, 2023

crazytonyli added this to the 23.6 milestone Oct 18, 2023

crazytonyli requested a review from mokagio October 18, 2023 01:26

crazytonyli self-assigned this Oct 18, 2023

crazytonyli marked this pull request as draft October 18, 2023 20:54

crazytonyli added 2 commits October 19, 2023 09:58

Implement fetching posts/pages list in PostRepository

a0c6388

Add API docs

4f8178a

crazytonyli changed the base branch from trunk to task/ui-modernization-posts-and-pages October 18, 2023 22:04

crazytonyli force-pushed the tonyli-simplify-post-search branch from a3c12d9 to c9c3473 Compare October 18, 2023 22:04

Allow fetching post in empty range which means returning unlimited re…

fa641ec

…sult

crazytonyli force-pushed the tonyli-simplify-post-search branch from c9c3473 to fa641ec Compare October 18, 2023 22:05

crazytonyli marked this pull request as ready for review October 18, 2023 22:08

crazytonyli requested a review from kean October 18, 2023 22:08

crazytonyli added 2 commits October 19, 2023 12:30

Update the doc to fix incorrect REST API behaviour

ed47e2b

Add unit tests for fetching posts list

c980a30

mokagio reviewed Oct 19, 2023

View reviewed changes

Use map instead of compactMap

4dac5b7

mokagio approved these changes Oct 19, 2023

View reviewed changes

crazytonyli merged commit 2fdca5f into task/ui-modernization-posts-and-pages Oct 19, 2023

crazytonyli deleted the tonyli-simplify-post-search branch October 19, 2023 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement fetching posts/pages list in PostRepository #21814

Implement fetching posts/pages list in PostRepository #21814

crazytonyli commented Oct 18, 2023 •

edited

Loading

wpmobilebot commented Oct 18, 2023 •

edited

Loading

wpmobilebot commented Oct 18, 2023 •

edited

Loading

kean commented Oct 18, 2023 •

edited

Loading

crazytonyli commented Oct 18, 2023

mokagio Oct 19, 2023

crazytonyli Oct 19, 2023

mokagio Oct 19, 2023

crazytonyli Oct 19, 2023

mokagio Oct 19, 2023

crazytonyli Oct 19, 2023

mokagio Oct 19, 2023

crazytonyli Oct 19, 2023

mokagio Oct 19, 2023

crazytonyli Oct 19, 2023

mokagio Oct 19, 2023

mokagio Oct 19, 2023

mokagio Oct 19, 2023

crazytonyli Oct 19, 2023

mokagio Oct 19, 2023

crazytonyli commented Oct 19, 2023

		// There is an assertion above to ensure the app doesn't fall into this case.
		return []


		}

		// MARK: - Tests that ensure the `preparePostsList` works as expected

	// MARK: - Tests that ensure the `preparePostsList` works as expected
	// MARK: - Tests that ensures the `preparePostsList` works as expected

	// MARK: - Tests that ensure the `preparePostsList` works as expected
	// MARK: - Tests to ensure the `preparePostsList` works as expected

Implement fetching posts/pages list in PostRepository #21814

Implement fetching posts/pages list in PostRepository #21814

Conversation

crazytonyli commented Oct 18, 2023 • edited Loading

Description

Issues

Issue 1: The syncing post function actually fetches all pages in the site, regardless what the function argument requests.

Issue 2: It's not clear when to use the purgesLocalSync argument.

New implementation

Tests

Regression Notes

wpmobilebot commented Oct 18, 2023 • edited Loading

wpmobilebot commented Oct 18, 2023 • edited Loading

kean commented Oct 18, 2023 • edited Loading

crazytonyli commented Oct 18, 2023

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

Choose a reason for hiding this comment

crazytonyli commented Oct 19, 2023

crazytonyli commented Oct 18, 2023 •

edited

Loading

Issue 2: It's not clear when to use the `purgesLocalSync` argument.

wpmobilebot commented Oct 18, 2023 •

edited

Loading

wpmobilebot commented Oct 18, 2023 •

edited

Loading

kean commented Oct 18, 2023 •

edited

Loading