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

Implement fetching posts/pages list in PostRepository #21814

Merged
Merged
Changes from 5 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
202 changes: 202 additions & 0 deletions WordPress/Classes/Services/PostRepository.swift
Original file line number Diff line number Diff line change
@@ -228,3 +228,205 @@ final class PostRepository {
}

}

// MARK: - Posts/Pages List

private final class PostRepositoryPostsSerivceRemoteOptions: NSObject, PostServiceRemoteOptions {
struct Options {
var statuses: [String]?
var number: Int = 100
var offset: Int = 0
var order: PostServiceResultsOrder = .descending
var orderBy: PostServiceResultsOrdering = .byDate
var authorID: NSNumber?
var search: String?
var meta: String? = "autosave"
}

var options: Options

init(options: Options) {
self.options = options
}

func statuses() -> [String]? {
options.statuses
}
Comment on lines +252 to +254
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 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:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, Objective-C...


func number() -> NSNumber {
NSNumber(value: options.number)
}

func offset() -> NSNumber {
NSNumber(value: options.offset)
}

func order() -> PostServiceResultsOrder {
options.order
}

func orderBy() -> PostServiceResultsOrdering {
options.orderBy
}

func authorID() -> NSNumber? {
options.authorID
}

func search() -> String? {
options.search
}

func meta() -> String? {
options.meta
}
}

private extension PostServiceRemote {

func getPosts(ofType type: String, options: PostRepositoryPostsSerivceRemoteOptions) async throws -> [RemotePost] {
try await withCheckedThrowingContinuation { continuation in
self.getPostsOfType(type, options: self.dictionary(with: options), success: {
continuation.resume(returning: $0 ?? [])
}, failure: {
continuation.resume(throwing: $0!)
})
}
}
}

extension PostRepository {

/// Fetch posts or pages from the given site page by page. All fetched posts are saved to the local database.
///
/// - Parameters:
/// - type: `Post.self` and `Page.self` are the only acceptable types.
/// - statuses: Filter posts or pages with given status.
/// - authorUserID: Filter posts or pages that are authored by given user.
/// - offset: The position of the paginated request. Pass 0 for the first page and count of already fetched results for following pages.
/// - number: Number of posts or pages should be fetched.
/// - blogID: The blog from which to fetch posts or pages
/// - Returns: Object identifiers of the fetched posts.
/// - SeeAlso: https://developer.wordpress.com/docs/api/1.1/get/sites/%24site/posts/
func paginate<P: AbstractPost>(
type: P.Type = P.self,
statuses: [BasePost.Status],
authorUserID: NSNumber? = nil,
offset: Int,
number: Int,
in blogID: TaggedManagedObjectID<Blog>
) async throws -> [TaggedManagedObjectID<P>] {
try await fetch(
type: type,
statuses: statuses,
authorUserID: authorUserID,
range: offset..<(offset + max(number, 0)),
orderBy: .byDate,
descending: true,
// Only delete other local posts if the current call is the first pagination request.
deleteOtherLocalPosts: offset == 0,
in: blogID
)
}

/// Search posts or pages in the given site. All fetched posts are saved to the local database.
///
/// - Parameters:
/// - type: `Post.self` and `Page.self` are the only acceptable types.
/// - input: The text input from user. Or `nil` for searching all posts or pages.
/// - statuses: Filter posts or pages with given status.
/// - authorUserID: Filter posts or pages that are authored by given user.
/// - limit: Number of posts or pages should be fetched.
/// - orderBy: The property by which to sort posts or pages.
/// - descending: Whether to sort the results in descending order.
/// - blogID: The blog from which to search posts or pages
/// - Returns: Object identifiers of the search result.
/// - SeeAlso: https://developer.wordpress.com/docs/api/1.1/get/sites/%24site/posts/
func search<P: AbstractPost>(
type: P.Type = P.self,
input: String?,
statuses: [BasePost.Status],
authorUserID: NSNumber? = nil,
limit: Int,
orderBy: PostServiceResultsOrdering,
descending: Bool,
in blogID: TaggedManagedObjectID<Blog>
) async throws -> [TaggedManagedObjectID<P>] {
try await fetch(
type: type,
searchInput: input,
statuses: statuses,
authorUserID: authorUserID,
range: 0..<max(limit, 0),
orderBy: orderBy,
descending: descending,
deleteOtherLocalPosts: false,
in: blogID
)
}

private func fetch<P: AbstractPost>(
type: P.Type,
searchInput: String? = nil,
statuses: [BasePost.Status]?,
authorUserID: NSNumber?,
range: Range<Int>,
orderBy: PostServiceResultsOrdering = .byDate,
descending: Bool = true,
deleteOtherLocalPosts: Bool,
in blogID: TaggedManagedObjectID<Blog>
) async throws -> [TaggedManagedObjectID<P>] {
assert(type == Post.self || type == Page.self, "Only support fetching Post or Page")
assert(range.lowerBound >= 0)

let postType: String
if type == Post.self {
postType = "post"
} else if type == Page.self {
postType = "page"
} else {
// There is an assertion above to ensure the app doesn't fall into this case.
return []
Comment on lines +388 to +389
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 fatalError, given this should be an unreachable code path?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

let remote = try await coreDataStack.performQuery { [remoteFactory] context in
let blog = try context.existingObject(with: blogID)
return remoteFactory.forBlog(blog)
}
guard let remote else {
throw PostRepository.Error.remoteAPIUnavailable
}

let options = PostRepositoryPostsSerivceRemoteOptions(options: .init(
statuses: statuses?.strings,
number: range.count,
offset: range.lowerBound,
order: descending ? .descending : .ascending,
orderBy: orderBy,
authorID: authorUserID,
search: searchInput
))
let remotePosts = try await remote.getPosts(ofType: postType, options: options)

let updatedPosts = try await coreDataStack.performAndSave { context in
let updatedPosts = PostHelper.merge(
remotePosts,
ofType: postType,
withStatuses: statuses?.strings,
byAuthor: authorUserID,
for: try context.existingObject(with: blogID),
purgeExisting: deleteOtherLocalPosts,
in: context
)
return updatedPosts.compactMap {
guard let post = $0 as? P else {
fatalError("Expecting a \(postType) as \(type), but got \($0)")
}
return TaggedManagedObjectID(post)
}
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 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Updated in 4dac5b7

}

return updatedPosts
}

}
4 changes: 4 additions & 0 deletions WordPress/WordPress.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
@@ -1267,6 +1267,7 @@
4AA33F022999D11A005B6E23 /* ReaderSiteTopic+Lookup.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4AA33F002999D11A005B6E23 /* ReaderSiteTopic+Lookup.swift */; };
4AA33F04299A1F93005B6E23 /* ReaderTagTopic+Lookup.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4AA33F03299A1F93005B6E23 /* ReaderTagTopic+Lookup.swift */; };
4AA33F05299A1F93005B6E23 /* ReaderTagTopic+Lookup.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4AA33F03299A1F93005B6E23 /* ReaderTagTopic+Lookup.swift */; };
4AA7EE0F2ADF7367007D261D /* PostRepositoryPostsListTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4AA7EE0E2ADF7367007D261D /* PostRepositoryPostsListTests.swift */; };
4AAD69082A6F68A5007FE77E /* MediaRepositoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4AAD69072A6F68A5007FE77E /* MediaRepositoryTests.swift */; };
4AD5656C28E3D0670054C676 /* ReaderPost+Helper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4AD5656B28E3D0670054C676 /* ReaderPost+Helper.swift */; };
4AD5656D28E3D0670054C676 /* ReaderPost+Helper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4AD5656B28E3D0670054C676 /* ReaderPost+Helper.swift */; };
@@ -6869,6 +6870,7 @@
4AA33EFA2999AE3B005B6E23 /* ReaderListTopic+Creation.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "ReaderListTopic+Creation.swift"; sourceTree = "<group>"; };
4AA33F002999D11A005B6E23 /* ReaderSiteTopic+Lookup.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "ReaderSiteTopic+Lookup.swift"; sourceTree = "<group>"; };
4AA33F03299A1F93005B6E23 /* ReaderTagTopic+Lookup.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "ReaderTagTopic+Lookup.swift"; sourceTree = "<group>"; };
4AA7EE0E2ADF7367007D261D /* PostRepositoryPostsListTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PostRepositoryPostsListTests.swift; sourceTree = "<group>"; };
4AAD69072A6F68A5007FE77E /* MediaRepositoryTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MediaRepositoryTests.swift; sourceTree = "<group>"; };
4AD5656B28E3D0670054C676 /* ReaderPost+Helper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "ReaderPost+Helper.swift"; sourceTree = "<group>"; };
4AD5656E28E413160054C676 /* Blog+History.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Blog+History.swift"; sourceTree = "<group>"; };
@@ -15478,6 +15480,7 @@
570B037622F1FFF6009D8411 /* PostCoordinatorFailedPostsFetcherTests.swift */,
57569CF1230485680052EE14 /* PostAutoUploadInteractorTests.swift */,
4A2C73F62A9585B000ACE79E /* PostRepositoryTests.swift */,
4AA7EE0E2ADF7367007D261D /* PostRepositoryPostsListTests.swift */,
57D66B9C234BB78B005A2D74 /* PostServiceWPComTests.swift */,
57240223234E5BE200227067 /* PostServiceSelfHostedTests.swift */,
575802122357C41200E4C63C /* MediaCoordinatorTests.swift */,
@@ -23563,6 +23566,7 @@
3FFE3C0828FE00D10021BB96 /* StatsSegmentedControlDataTests.swift in Sources */,
015BA4EB29A788A300920F4B /* StatsTotalInsightsCellTests.swift in Sources */,
D81C2F5820F86CEA002AE1F1 /* NetworkStatus.swift in Sources */,
4AA7EE0F2ADF7367007D261D /* PostRepositoryPostsListTests.swift in Sources */,
E1C545801C6C79BB001CEB0E /* MediaSettingsTests.swift in Sources */,
806BA11C2A492B0F00052422 /* BlazeCampaignDetailsWebViewModelTests.swift in Sources */,
7E987F5A2108122A00CAFB88 /* NotificationUtility.swift in Sources */,
Loading