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.

Sign up for GitHub

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

Jump to bottom

Move Application Passwords entry to user details #23834

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
+65 −64
Open

Move Application Passwords entry to user details #23834

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
133bfc8
Move Application Passwords entry to user details
crazytonyli Nov 20, 2024
af1180d
Call `fetchOtherUsers` when taps "Delete User"
crazytonyli Nov 20, 2024
3b81fd1
Fix unit test compiling issues
crazytonyli Nov 20, 2024
5430f25
Change the Application Password list view background
crazytonyli Nov 20, 2024
db61f33
Remove an unnecessary IUO
crazytonyli Nov 21, 2024
1b0daa0
Remove a reduandant `Group`
crazytonyli Nov 21, 2024
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
39 changes: 23 additions & 16 deletions WordPress/Classes/ApplicationToken/ApplicationTokenListView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,34 +5,41 @@ import WordPressAPI

struct ApplicationTokenListView: View {

@ObservedObject
@StateObject
private var viewModel: ApplicationTokenListViewModel

fileprivate init(tokens: [ApplicationTokenItem]) {
let dataProvider = StaticTokenProvider(tokens: .success(tokens))
self.init(viewModel: ApplicationTokenListViewModel(dataProvider: dataProvider))
self.init(dataProvider: dataProvider)
}

fileprivate init(error: Error) {
let dataProvider = StaticTokenProvider(tokens: .failure(error))
self.init(viewModel: ApplicationTokenListViewModel(dataProvider: dataProvider))
self.init(dataProvider: dataProvider)
}

init(viewModel: ApplicationTokenListViewModel) {
self.viewModel = viewModel
init(dataProvider: ApplicationTokenListDataProvider) {
_viewModel = .init(wrappedValue: ApplicationTokenListViewModel(dataProvider: dataProvider))
}

var body: some View {
VStack {
if viewModel.isLoadingData {
ProgressView()
} else if let error = viewModel.errorMessage {
EmptyStateView(Self.errorTitle, systemImage: "exclamationmark.triangle", description: error)
} else {
List(viewModel.applicationTokens) { token in
ApplicationTokenListItemView(item: token)
ZStack {
Color(.systemGroupedBackground)
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) this should not be needed as long as you are displaying the List at the bottom. The ProgressView could be shown as an .overlay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't get it to work. Do you mind showing me an example?

.ignoresSafeArea()

Group {
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Group seems redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 1b0daa0

VStack {
if viewModel.isLoadingData {
ProgressView()
} else if let error = viewModel.errorMessage {
EmptyStateView(Self.errorTitle, systemImage: "exclamationmark.triangle", description: error)
} else {
List(viewModel.applicationTokens) { token in
ApplicationTokenListItemView(item: token)
}
.listStyle(.insetGrouped)
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'd suggest using a .plain list for plain lists. The same applies to the list of users. In the list of users, I'd display the current account and the top and add some sort of marker that it is.

}
}
.listStyle(.plain)
}
}
.navigationTitle(Self.title)
Expand All @@ -47,6 +54,7 @@ struct ApplicationTokenListView: View {
}
}

@MainActor
class ApplicationTokenListViewModel: ObservableObject {

@Published
Expand All @@ -58,14 +66,13 @@ class ApplicationTokenListViewModel: ObservableObject {
@Published
private(set) var applicationTokens: [ApplicationTokenItem]

private let dataProvider: ApplicationTokenListDataProvider!
let dataProvider: ApplicationTokenListDataProvider!
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need a force unwrap?

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! Addressed in db61f33


init(dataProvider: ApplicationTokenListDataProvider) {
self.dataProvider = dataProvider
self.applicationTokens = []
}

@MainActor
func fetchTokens() async {
isLoadingData = true
defer {
Expand Down
4 changes: 0 additions & 4 deletions WordPress/Classes/Services/UserService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,6 @@ actor UserService: UserServiceProtocol {
await currentUser?.capabilities.keys.contains(capability) == true
}

func isCurrentUser(_ user: DisplayUser) async -> Bool {
await currentUser?.id == user.id
}

func deleteUser(id: Int32, reassigningPostsTo newUserId: Int32) async throws {
let result = try await client.api.users.delete(
userId: id,
Expand Down
5 changes: 3 additions & 2 deletions WordPress/Classes/Users/Components/UserListItem.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ struct UserListItem: View {
var dynamicTypeSize

let user: DisplayUser
let isCurrentUser: Bool
let userService: UserServiceProtocol
let applicationTokenListDataProvider: ApplicationTokenListDataProvider

var body: some View {
NavigationLink {
UserDetailsView(user: user, userService: userService, applicationTokenListDataProvider: applicationTokenListDataProvider)
UserDetailsView(user: user, isCurrentUser: isCurrentUser, userService: userService, applicationTokenListDataProvider: applicationTokenListDataProvider)
} label: {
HStack(alignment: .top) {
if !dynamicTypeSize.isAccessibilitySize {
Expand All @@ -30,5 +31,5 @@ struct UserListItem: View {
}

#Preview {
UserListItem(user: DisplayUser.MockUser, userService: MockUserProvider(), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
UserListItem(user: DisplayUser.MockUser, isCurrentUser: true, userService: MockUserProvider(), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
}
6 changes: 0 additions & 6 deletions WordPress/Classes/Users/UserProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ public protocol UserServiceProtocol: Actor {

func fetchUsers() async throws -> [DisplayUser]

func isCurrentUser(_ user: DisplayUser) async -> Bool

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

func setNewPassword(id: Int32, newPassword: String) async throws
Expand Down Expand Up @@ -59,10 +57,6 @@ actor MockUserProvider: UserServiceProtocol {
}
}

func isCurrentUser(_ user: DisplayUser) async -> Bool {
true
}

func isCurrentUserCapableOf(_ capability: String) async -> Bool {
true
}
Expand Down
62 changes: 32 additions & 30 deletions WordPress/Classes/Users/Views/UserDetailsView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ struct UserDetailsView: View {

fileprivate let userService: UserServiceProtocol
let user: DisplayUser
let isCurrentUser: Bool
let applicationTokenListDataProvider: ApplicationTokenListDataProvider

@State private var presentPasswordAlert: Bool = false {
didSet {
Expand All @@ -20,19 +22,19 @@ struct UserDetailsView: View {

@StateObject
fileprivate var viewModel: UserDetailViewModel
@StateObject
fileprivate var applicationTokenListViewModel: ApplicationTokenListViewModel

@StateObject
fileprivate var deleteUserViewModel: UserDeleteViewModel

@Environment(\.dismiss)
var dismissAction: DismissAction

init(user: DisplayUser, userService: UserServiceProtocol, applicationTokenListDataProvider: ApplicationTokenListDataProvider) {
init(user: DisplayUser, isCurrentUser: Bool, userService: UserServiceProtocol, applicationTokenListDataProvider: ApplicationTokenListDataProvider) {
self.user = user
self.isCurrentUser = isCurrentUser
self.userService = userService
self.applicationTokenListDataProvider = applicationTokenListDataProvider
_viewModel = StateObject(wrappedValue: UserDetailViewModel(userService: userService))
_applicationTokenListViewModel = StateObject(wrappedValue: ApplicationTokenListViewModel(dataProvider: applicationTokenListDataProvider))
_deleteUserViewModel = StateObject(wrappedValue: UserDeleteViewModel(user: user, userService: userService))
}

Expand Down Expand Up @@ -61,29 +63,34 @@ struct UserDetailsView: View {
}
}

if !applicationTokenListViewModel.applicationTokens.isEmpty {
Section(ApplicationTokenListView.title) {
ForEach(applicationTokenListViewModel.applicationTokens) { token in
ApplicationTokenListItemView(item: token)
}
}
}

if viewModel.currentUserCanModifyUsers {
if isCurrentUser || viewModel.currentUserCanModifyUsers {
Section(Strings.accountManagementSectionTitle) {
Button(Strings.setNewPasswordActionTitle) {
presentPasswordAlert = true
if isCurrentUser {
NavigationLink(ApplicationTokenListView.title) {
ApplicationTokenListView(dataProvider: applicationTokenListDataProvider)
}
}
Button(role: .destructive) {
presentUserPicker = true
} label: {
Text(
deleteUserViewModel.isDeletingUser ?
Strings.deletingUserActionTitle
: Strings.deleteUserActionTitle
)

if viewModel.currentUserCanModifyUsers {
Button(Strings.setNewPasswordActionTitle) {
presentPasswordAlert = true
}
Button(role: .destructive) {
presentUserPicker = true
Task {
if deleteUserViewModel.otherUsers.isEmpty {
await deleteUserViewModel.fetchOtherUsers()
}
}
} label: {
Text(
deleteUserViewModel.isDeletingUser ?
Strings.deletingUserActionTitle
: Strings.deleteUserActionTitle
)
}
.disabled(deleteUserViewModel.isDeletingUser)
}
.disabled(deleteUserViewModel.isDeletingUser)
}
}
}
Expand Down Expand Up @@ -114,11 +121,6 @@ struct UserDetailsView: View {
.onAppear() {
Task {
await viewModel.loadCurrentUserRole()
await deleteUserViewModel.fetchOtherUsers()

if await userService.isCurrentUser(user) {
await applicationTokenListViewModel.fetchTokens()
}
}
}
}
Expand Down Expand Up @@ -397,6 +399,6 @@ private extension String {

#Preview {
NavigationStack {
UserDetailsView(user: DisplayUser.MockUser, userService: MockUserProvider(), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
UserDetailsView(user: DisplayUser.MockUser, isCurrentUser: true, userService: MockUserProvider(), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
}
}
12 changes: 7 additions & 5 deletions WordPress/Classes/Users/Views/UserListView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ public struct UserListView: View {

@StateObject
private var viewModel: UserListViewModel
private let currentUserId: Int32
private let userService: UserServiceProtocol
private let applicationTokenListDataProvider: ApplicationTokenListDataProvider

public init(userService: UserServiceProtocol, applicationTokenListDataProvider: ApplicationTokenListDataProvider) {
public init(currentUserId: Int32, userService: UserServiceProtocol, applicationTokenListDataProvider: ApplicationTokenListDataProvider) {
self.currentUserId = currentUserId
self.userService = userService
self.applicationTokenListDataProvider = applicationTokenListDataProvider
_viewModel = StateObject(wrappedValue: UserListViewModel(userService: userService))
Expand All @@ -34,7 +36,7 @@ public struct UserListView: View {
.listRowBackground(Color.clear)
} else {
ForEach(section.users) { user in
UserListItem(user: user, userService: userService, applicationTokenListDataProvider: applicationTokenListDataProvider)
UserListItem(user: user, isCurrentUser: user.id == currentUserId, userService: userService, applicationTokenListDataProvider: applicationTokenListDataProvider)
}
}
}
Expand Down Expand Up @@ -73,18 +75,18 @@ public struct UserListView: View {

#Preview("Loading") {
NavigationView {
UserListView(userService: MockUserProvider(), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
UserListView(currentUserId: 0, userService: MockUserProvider(), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
}
}

#Preview("Error") {
NavigationView {
UserListView(userService: MockUserProvider(scenario: .error), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
UserListView(currentUserId: 0, userService: MockUserProvider(scenario: .error), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
}
}

#Preview("List") {
NavigationView {
UserListView(userService: MockUserProvider(scenario: .dummyData), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
UserListView(currentUserId: 0, userService: MockUserProvider(scenario: .dummyData), applicationTokenListDataProvider: StaticTokenProvider(tokens: .success(.testTokens)))
}
}
2 changes: 1 addition & 1 deletion ...ress/Classes/ViewRelated/Blog/Blog Details/BlogDetailsViewController+SectionHelpers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ extension BlogDetailsViewController {
let rootView = ApplicationPasswordRequiredView(blog: self.blog, localizedFeatureName: feature) { client in
let service = UserService(client: client)
let applicationPasswordService = ApplicationPasswordService(api: client, currentUserId: userId)
return UserListView(userService: service, applicationTokenListDataProvider: applicationPasswordService)
return UserListView(currentUserId: Int32(userId), userService: service, applicationTokenListDataProvider: applicationPasswordService)
}
presentationDelegate.presentBlogDetailsViewController(UIHostingController(rootView: rootView))
}
Expand Down
1 change: 1 addition & 0 deletions WordPress/WordPressTest/ApplicationPasswords/ApplicationPasswordsViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import XCTest

@testable import WordPress

@MainActor
Copy link
Contributor

Choose a reason for hiding this comment

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

Actors support in XCTest has been a bit shaky. I'd suggest writing all new tests in swift-testing. It's pretty similar to XCTest https://developer.apple.com/documentation/testing/migratingfromxctest

class ApplicationPasswordsViewModelTests: XCTestCase {

func testOrder() async throws {
Expand Down