Skip to content
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
2 changes: 1 addition & 1 deletion Modules/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ let package = Package(
.target(name: "WordPressFlux"),
.target(name: "WordPressSharedObjC", resources: [.process("Resources")]),
.target(name: "WordPressShared", dependencies: [.target(name: "WordPressSharedObjC")], resources: [.process("Resources")]),
.target(name: "WordPressUI", resources: [.process("Resources")]),
.target(name: "WordPressUI", dependencies: [.target(name: "WordPressShared")], resources: [.process("Resources")]),
.testTarget(name: "JetpackStatsWidgetsCoreTests", dependencies: [.target(name: "JetpackStatsWidgetsCore")]),
.testTarget(name: "DesignSystemTests", dependencies: [.target(name: "DesignSystem")]),
.testTarget(name: "WordPressFluxTests", dependencies: ["WordPressFlux"]),
Expand Down
11 changes: 11 additions & 0 deletions Modules/Sources/WordPressShared/Utility/StringRankedSearch.swift
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,14 @@ extension StringRankedSearch {
.map(\.0)
}
}

/// Objects conforming to `StringRankedSearchable` can be searched by calling `search(query:)` on a collection of them
public protocol StringRankedSearchable {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest removing the protocol and sticking with closures. The same type can have different search criteria depending on the context, so passing a closure is more flexible. This is similar to the way filter(:) uses closures and not protocols.

It may be worth adding a generic to Collection or something like this, but I'm not sure it's worth it.

extension Collection {
    func search(searchTerm: query, _ closure: (Element) -> String)
}

var searchString: String { get }
}

public extension Collection where Iterator.Element: StringRankedSearchable {
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) I think Collection should have Element, so no need to reach for the Iterator

func search(query: String, minScore: Double = 0.7) -> [Iterator.Element] {
StringRankedSearch(searchTerm: query).search(in: self, minScore: minScore) { $0.searchString }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import SwiftUI

public extension EdgeInsets {
static let zero = EdgeInsets(top: 0, leading: 0, bottom: 0, trailing: 0)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import SwiftUI

struct UserListItem: View {

@ScaledMetric(relativeTo: .headline)
var height: CGFloat = 48

@Environment(\.sizeCategory)
Copy link
Contributor

Choose a reason for hiding this comment

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

var sizeCategory

private let user: DisplayUser
private let userProvider: UserDataProvider
private let actionDispatcher: UserManagementActionDispatcher

init(user: DisplayUser, userProvider: UserDataProvider, actionDispatcher: UserManagementActionDispatcher) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) I usually take advantage of the auto-generated init for structs by making the required parameters internal.

self.user = user
self.userProvider = userProvider
self.actionDispatcher = actionDispatcher
}

var body: some View {
NavigationLink {
UserDetailView(user: user, userProvider: userProvider, actionDispatcher: actionDispatcher)
} label: {
HStack(alignment: .top) {
if !sizeCategory.isAccessibilityCategory {
if let profilePhotoUrl = user.profilePhotoUrl {
UserProfileImage(size: CGSize(width: height, height: height), url: profilePhotoUrl)
}
}
VStack(alignment: .leading) {
Text(user.displayName).font(.headline)
Text(user.handle).font(.body).foregroundStyle(.secondary)
}
}
}
}
}

#Preview {
UserListItem(user: DisplayUser.MockUser, userProvider: MockUserProvider(), actionDispatcher: UserManagementActionDispatcher())
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import SwiftUI

struct UserProfileImage: View {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use AvatarView and avoid AsyncImage as it uses URLSession.shared and basically has no caching. AvatarView is in the app target, so User* types will likely need to be moved to the app target.

The app has CachedAsyncImage that uses app's ImageDownloader with a memory cache.


private let size: CGSize

private let url: URL

init(size: CGSize, email: String) {
self.size = size
self.url = URL(string: "https://gravatar.com/avatar/58fc51586c9a1f9895ac70e3ca60886e?size=256")!
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) mock?

}

init(size: CGSize, url: URL) {
self.size = size
self.url = url
}

var body: some View {
AsyncImage(
url: self.url,
content: { image in
image.resizable()
.frame(width: size.height, height: size.height)
.aspectRatio(contentMode: .fit)
.clipShape(.rect(cornerRadius: 4.0))
},
placeholder: {
ProgressView().frame(width: size.height, height: size.height)
}
)
}
}

#Preview {
UserProfileImage(size: CGSize(width: 64, height: 64), email: "[email protected]")
}
45 changes: 45 additions & 0 deletions Modules/Sources/WordPressUI/Views/Users/UserProvider.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import Foundation

public protocol UserDataProvider {

typealias CachedUserListCallback = ([WordPressUI.DisplayUser]) async -> Void

func fetchCurrentUserCan(_ capability: String) async throws -> Bool
func fetchUsers(cachedResults: CachedUserListCallback?) async throws -> [WordPressUI.DisplayUser]

func invalidateCaches() async throws
}

/// Subclass this and register it with the SwiftUI `.environmentObject` method
/// to perform user management actions.
///
/// The default implementation is set up for testing with SwiftUI Previews
open class UserManagementActionDispatcher: ObservableObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

The app doesn't use this type of naming elsewhere ("Dispatcher"). I'd suggest sticking with the current naming conventions. I would also suggest avoiding subclassing by adding a protocol like this:

protocol UserManagementServiceProtocol {
    // Changed from "fetchCurrentUserCan" and made non-async (we aren't going to perform a fetch for every individual capability?)
    func hasCapability(_ capability: String) -> Bool

    // Or something else other that a separate CachedUserListCallback closure
    func getCachedUsers() -> [User]

    func fetchUsers() async throws -> [WordPressUI.DisplayUser]
    
    // Why was it async throws? Noting that it seems to be unused.
    func invalidateCaches() 

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

    func deleteUser(id: Int32, reassigningPostsTo userId: Int32) async throws
}

Copy link
Contributor

@kean kean Nov 6, 2024

Choose a reason for hiding this comment

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

It's also not clear what WordPressUI.DisplayUser is and what UserDataProvider belongs to.

If it's a model layer, it should returns users as is: User, SiteUser?. Perhaps there needs to be a namespace for the types associated with the WordPress sites to disambiguate. The name DisplayUser doesn't make much sense.

If it's a ViewModel layer, it should be UserView(Cell)Model.

public init() {}

open func setNewPassword(id: Int32, newPassword: String) async throws {
try await Task.sleep(for: .seconds(2))
}

open func deleteUser(id: Int32, reassigningPostsTo userId: Int32) async throws {
try await Task.sleep(for: .seconds(2))
}
}

package struct MockUserProvider: UserDataProvider {

let dummyDataUrl = URL(string: "https://my.api.mockaroo.com/users.json?key=067c9730")!

package func fetchUsers(cachedResults: CachedUserListCallback? = nil) async throws -> [WordPressUI.DisplayUser] {
let response = try await URLSession.shared.data(from: dummyDataUrl)
return try JSONDecoder().decode([DisplayUser].self, from: response.0)
}

package func fetchCurrentUserCan(_ capability: String) async throws -> Bool {
true
}

package func invalidateCaches() async throws {
// Do nothing
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import Foundation
import WordPressShared

public struct DisplayUser: Identifiable, Codable {
public let id: Int32
public let handle: String
public let username: String
public let firstName: String
public let lastName: String
public let displayName: String
public let profilePhotoUrl: URL?
public let role: String

public let emailAddress: String
public let websiteUrl: String?

public let biography: String?

public init(
id: Int32,
handle: String,
username: String,
firstName: String,
lastName: String,
displayName: String,
profilePhotoUrl: URL?,
role: String,
emailAddress: String,
websiteUrl: String?,
biography: String?
) {
self.id = id
self.handle = handle
self.username = username
self.firstName = firstName
self.lastName = lastName
self.displayName = displayName
self.profilePhotoUrl = profilePhotoUrl
self.role = role
self.emailAddress = emailAddress
self.websiteUrl = websiteUrl
self.biography = biography
}

static package let MockUser = DisplayUser(
id: 16,
handle: "@person",
username: "example",
firstName: "John",
lastName: "Smith",
displayName: "John Smith",
profilePhotoUrl: URL(string: "https://gravatar.com/avatar/58fc51586c9a1f9895ac70e3ca60886e?size=256"),
role: "administrator",
emailAddress: "[email protected]",
websiteUrl: "",
biography: ""
)
}

extension DisplayUser: StringRankedSearchable {
public var searchString: String {

// These are in ranked order – the higher something is in the list, the more heavily it's weighted
[
handle,
Copy link
Contributor

Choose a reason for hiding this comment

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

The main things that should be on this list are username and displayName, which is what you see on the "Users" list. It may be worth also adding a emailAddress.

emailAddress appears twice.

username,
firstName,
lastName,
emailAddress,
displayName,
emailAddress,
]
.compactMap { $0 }
.joined(separator: " ")
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import SwiftUI

public class UserChangePasswordViewModel: ObservableObject {

public enum Errors: LocalizedError {
case passwordMustNotBeEmpty

public var errorDescription: String? {
switch self {
case .passwordMustNotBeEmpty: NSLocalizedString(
"userchangepassword.error.empty",
value: "Password must not be empty",
comment: "An error message that appears when an empty password has been entered"
)
}
}
}

@Published
var password: String = ""

@Published
var isChangingPassword: Bool = false
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Add private(set) where it applies.


@Published
var error: Error? = nil

@Environment(\.dismiss)
var dismissAction

private let actionDispatcher: UserManagementActionDispatcher
let user: DisplayUser

init(user: DisplayUser, actionDispatcher: UserManagementActionDispatcher) {
self.user = user
self.actionDispatcher = actionDispatcher
}

func didTapChangePassword(callback: @escaping () -> Void) {

if password.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
withAnimation {
error = Errors.passwordMustNotBeEmpty
}
return
}

withAnimation {
error = nil
}

Task {
await MainActor.run {
Copy link
Contributor

Choose a reason for hiding this comment

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

You typically don't want to use await MainActor.run imperatively. The better option is to declare that Task runs is isolated to the main actor:

Task { @MainActor in
   // remove await MainActor.run
}

I'd also consider isolating the entire ViewModel to the main actor to ensure none of its properties are updated in the background.

withAnimation {
self.isChangingPassword = true
}
}

do {
try await actionDispatcher.setNewPassword(id: user.id, newPassword: password)
} catch {
self.error = error
}

await MainActor.run {
withAnimation {
self.isChangingPassword = false
}
}

await MainActor.run {
callback()
Copy link
Contributor

Choose a reason for hiding this comment

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

The callback isn't needed since the ViewModel already has dismissAction and can dismiss itself.

}
}
}
}
Loading