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

Encapsulate UserDataStore in UserService #23866

Merged
merged 3 commits into from
Dec 1, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
12 changes: 5 additions & 7 deletions WordPress/Classes/Services/UserService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,11 @@ import WordPressUI

/// UserService is responsible for fetching user acounts via the .org REST API – it's the replacement for `UsersService` (the XMLRPC-based approach)
///
actor UserService: UserServiceProtocol {
actor UserService: UserServiceProtocol, UserDataStoreProvider {
private let client: WordPressClient

private let _dataStore: InMemoryUserDataStore = .init()
var dataStore: any UserDataStore {
_dataStore
}
var userDataStore: any UserDataStore { _dataStore }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the extra indirection here? Could userDataStore just be the canonical 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.

When declaring it as var userDataStore: InMemoryUserDataStore, the compiler complains that the signature does not match the one in UserServiceProtocol.


private var _currentUser: UserWithEditContext?
private var currentUser: UserWithEditContext? {
Expand All @@ -32,10 +30,10 @@ actor UserService: UserServiceProtocol {
var started = false
for try await users in sequence {
if !started {
try await dataStore.delete(query: .all)
try await _dataStore.delete(query: .all)
}

try await dataStore.store(users.compactMap { DisplayUser(user: $0) })
try await _dataStore.store(users.compactMap { DisplayUser(user: $0) })

started = true
}
Expand All @@ -53,7 +51,7 @@ actor UserService: UserServiceProtocol {

// Remove the deleted user from the cached users list.
if result.deleted {
try await dataStore.delete(query: .id([id]))
try await _dataStore.delete(query: .id([id]))
}
}

Expand Down
36 changes: 28 additions & 8 deletions WordPress/Classes/Users/UserProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,40 @@ public enum UserDataStoreQuery: Equatable {
}

public protocol UserServiceProtocol: Actor {
var dataStore: any UserDataStore { get }

func fetchUsers() async throws

func isCurrentUserCapableOf(_ capability: String) async -> Bool

func setNewPassword(id: Int32, newPassword: String) async throws

func deleteUser(id: Int32, reassigningPostsTo newUserId: Int32) async throws

func allUsers() async throws -> [DisplayUser]

func streamSearchResult(input: String) async -> AsyncStream<Result<[DisplayUser], Error>>

func streamAll() async -> AsyncStream<Result<[DisplayUser], Error>>
}

protocol UserDataStoreProvider: Actor {
var userDataStore: any UserDataStore { get }
}

extension UserServiceProtocol where Self: UserDataStoreProvider {
func allUsers() async throws -> [DisplayUser] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I bet this could be generic, but we can do that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mind elaborating on this?

try await userDataStore.list(query: .all)
}

func streamSearchResult(input: String) async -> AsyncStream<Result<[DisplayUser], Error>> {
await userDataStore.listStream(query: .search(input))
}

func streamAll() async -> AsyncStream<Result<[DisplayUser], Error>> {
await userDataStore.listStream(query: .all)
}
}

actor MockUserProvider: UserServiceProtocol {
actor MockUserProvider: UserServiceProtocol, UserDataStoreProvider {

enum Scenario {
case infinitLoading
Expand All @@ -33,9 +55,7 @@ actor MockUserProvider: UserServiceProtocol {
var scenario: Scenario

private let _dataStore: InMemoryUserDataStore = .init()
var dataStore: any UserDataStore {
_dataStore
}
var userDataStore: any UserDataStore { _dataStore }

nonisolated let usersUpdates: AsyncStream<[DisplayUser]>
private let usersUpdatesContinuation: AsyncStream<[DisplayUser]>.Continuation
Expand All @@ -62,8 +82,8 @@ actor MockUserProvider: UserServiceProtocol {
let dummyDataUrl = URL(string: "https://my.api.mockaroo.com/users.json?key=067c9730")!
let response = try await URLSession.shared.data(from: dummyDataUrl)
let users = try JSONDecoder().decode([DisplayUser].self, from: response.0)
try await _dataStore.delete(query: .all)
try await _dataStore.store(users)
try await userDataStore.delete(query: .all)
try await userDataStore.store(users)
case .error:
throw URLError(.timedOut)
}
Expand Down
3 changes: 1 addition & 2 deletions WordPress/Classes/Users/ViewModel/UserDeleteViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ public class UserDeleteViewModel: ObservableObject {

func fetchOtherUsers() async {
do {
let users = try await userService.dataStore.list(query: .all)
self.otherUsers = users
self.otherUsers = try await userService.allUsers()
.filter { $0.id != self.user.id } // Don't allow re-assigning to yourself
.sorted(using: KeyPathComparator(\.username))
} catch {
Expand Down
19 changes: 16 additions & 3 deletions WordPress/Classes/Users/ViewModel/UserListViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ import WordPressShared
@MainActor
class UserListViewModel: ObservableObject {

enum Mode: Equatable {
case allUsers
case search(String)
}

enum RoleSection: Hashable, Comparable {
case me
case role(String)
Expand Down Expand Up @@ -46,7 +51,7 @@ class UserListViewModel: ObservableObject {
private var initialLoad = false

@Published
private(set) var query: UserDataStoreQuery = .all
private(set) var mode: Mode = .allUsers

@Published
private(set) var sortedUsers: [Section] = []
Expand All @@ -57,7 +62,8 @@ class UserListViewModel: ObservableObject {
@Published
var searchTerm: String = "" {
didSet {
self.query = .search(searchTerm)
let keyword = searchTerm.trimmingCharacters(in: .whitespacesAndNewlines)
self.mode = keyword.isEmpty ? .allUsers : .search(keyword)
}
}

Expand All @@ -82,7 +88,14 @@ class UserListViewModel: ObservableObject {
}

func performQuery() async {
let usersUpdates = await userService.dataStore.listStream(query: query)
let usersUpdates: AsyncStream<Result<[DisplayUser], Error>>
switch mode {
case .allUsers:
usersUpdates = await userService.streamAll()
case let .search(keyword):
usersUpdates = await userService.streamSearchResult(input: keyword)
}

for await users in usersUpdates {
switch users {
case let .success(users):
Expand Down
2 changes: 1 addition & 1 deletion WordPress/Classes/Users/Views/UserListView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public struct UserListView: View {
}
}
}
.task(id: viewModel.query) {
.task(id: viewModel.mode) {
await viewModel.performQuery()
}
.task { await viewModel.onAppear() }
Expand Down
28 changes: 21 additions & 7 deletions WordPress/WordPressTest/UserListViewModelTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class UserListViewModelTests: XCTestCase {

let expectation = XCTestExpectation(description: "Updated after fetch")
let task = Task.detached {
for await _ in await self.service.dataStore.listStream(query: .all) {
for await _ in await self.service.streamAll() {
expectation.fulfill()
}
}
Expand All @@ -53,7 +53,7 @@ class UserListViewModelTests: XCTestCase {
let expectation = XCTestExpectation(description: "Updated after fetch")
expectation.expectedFulfillmentCount = 5
let task = Task.detached {
for await _ in await self.service.dataStore.listStream(query: .all) {
for await _ in await self.service.streamAll() {
expectation.fulfill()
}
}
Expand All @@ -72,7 +72,7 @@ class UserListViewModelTests: XCTestCase {

let termination = XCTestExpectation(description: "Stream has finished")
let task = Task.detached { [self] in
for await _ in await self.service.dataStore.listStream(query: .all) {
for await _ in await self.service.streamAll() {
// Do nothing
}
termination.fulfill()
Expand All @@ -96,12 +96,26 @@ class UserListViewModelTests: XCTestCase {
stubDeleteUser(id: 34)

_ = await viewModel.refreshItems()
let userFetched = try await service.dataStore.list(query: .id([34])).isEmpty == false
XCTAssertTrue(userFetched)

let userExisted = expectation(description: "User 34 exists")
let userDeleted = expectation(description: "User 34 is deleted")

let subscription = Task.detached {
for await result in await self.service.streamAll() {
let users = try result.get()
if users.contains(where: { $0.id == 34 }) {
userExisted.fulfill()
} else {
userDeleted.fulfill()
}
}
}

try await service.deleteUser(id: 34, reassigningPostsTo: 1)
let userDeleted = try await service.dataStore.list(query: .id([34])).isEmpty
XCTAssertTrue(userDeleted)

await fulfillment(of: [userExisted, userDeleted], timeout: 0.3, enforceOrder: true)

subscription.cancel()
}

private func stubSuccessfullUsersFetch() {
Expand Down