Move Application Passwords entry to user details #23834

crazytonyli · 2024-11-20T06:37:48Z

This is an alternative to #23819.

In #23819, application passwords are displayed in the user details view, like wp-admin. But I think @kean 's suggestion in #23819 (comment) makes more sense.

Regression Notes

Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)

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.

Testing checklist:

dangermattic · 2024-11-20T06:38:18Z

	1 Warning
⚠️	This PR is assigned to the milestone 25.6. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

wpmobilebot · 2024-11-20T06:47:18Z

📲 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	pr23834-10c98ee
	Version	25.4.2
	Bundle ID	org.wordpress.alpha
	Commit	`10c98ee`
	App Center Build	WPiOS - One-Offs #11131

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

wpmobilebot · 2024-11-20T06:57:01Z

📲 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	pr23834-10c98ee
	Version	25.4.2
	Bundle ID	com.jetpack.alpha
	Commit	`10c98ee`
	App Center Build	jetpack-installable-builds #10171

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

kean · 2024-11-20T13:15:02Z

WordPress/Classes/ApplicationToken/ApplicationTokenListView.swift

-                List(viewModel.applicationTokens) { token in
-                    ApplicationTokenListItemView(item: token)
+        ZStack {
+            Color(.systemGroupedBackground)


(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.

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

kean · 2024-11-20T13:15:14Z

WordPress/Classes/ApplicationToken/ApplicationTokenListView.swift

+            Color(.systemGroupedBackground)
+                .ignoresSafeArea()
+
+            Group {


(nit) Group seems redundant.

Removed in 1b0daa0

kean · 2024-11-20T13:15:49Z

WordPress/Classes/ApplicationToken/ApplicationTokenListView.swift

+                        List(viewModel.applicationTokens) { token in
+                            ApplicationTokenListItemView(item: token)
+                        }
+                        .listStyle(.insetGrouped)


(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.

kean · 2024-11-20T13:36:29Z

WordPress/WordPressTest/ApplicationPasswords/ApplicationPasswordsViewModelTests.swift

@@ -3,6 +3,7 @@ import XCTest

 @testable import WordPress

+@MainActor


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

kean · 2024-11-20T14:02:33Z

WordPress/Classes/ApplicationToken/ApplicationTokenListView.swift

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

-    private let dataProvider: ApplicationTokenListDataProvider!
+    let dataProvider: ApplicationTokenListDataProvider!


Does it need a force unwrap?

Good catch! Addressed in db61f33

jkmassel · 2024-11-25T19:37:17Z

If the site doesn't have a valid Application Token, every request just fails – there's no feedback or alerting.

We should address that before this ships.

I also noticed that User Management is still behind a feature flag – with this change, we should tie user management and application passwords together, because otherwise it's impossible to get at your application password list.

jkmassel · 2024-11-25T19:38:34Z

Should admin users be able to see others' application passwords?

crazytonyli · 2024-11-26T01:00:25Z

If the site doesn't have a valid Application Token, every request just fails – there's no feedback or alerting.

This is an existing issue with account passwords too. There is no re-authentication flow, when account password is changed.

Should admin users be able to see others' application passwords?

Not sure about if they should, but they can 😄 . On web, admin users can view, add, and revoke others application passwords.

jkmassel · 2024-11-27T22:27:17Z

WordPress/Classes/Users/Views/UserDetailsView.swift

                Section(Strings.accountManagementSectionTitle) {
-                    Button(Strings.setNewPasswordActionTitle) {
-                        presentPasswordAlert = true
+                    if isCurrentUser {


I think this is why I couldn't see Application Passwords for other users?

Can we make that possible?

Doesn't have to be in this PR

Oh sorry, I didn't realize your question was about the current implementation in the app.

That's in my TODO list. It's not available now because the "Application Passwords" entry was implemented as for current user only.

crazytonyli added the Self-hosted label Nov 20, 2024

crazytonyli added this to the 25.6 milestone Nov 20, 2024

crazytonyli requested a review from kean November 20, 2024 06:37

crazytonyli mentioned this pull request Nov 20, 2024

Clean up viewing Application Passwords code #23819

Closed

14 tasks

crazytonyli mentioned this pull request Nov 20, 2024

Extract delete user confirmation sheet into its own view #23835

Merged

14 tasks

kean reviewed Nov 20, 2024

View reviewed changes

crazytonyli requested a review from jkmassel November 24, 2024 20:53

crazytonyli mentioned this pull request Nov 26, 2024

Put the current user at the top of the list #23855

Merged

14 tasks

crazytonyli added 6 commits November 27, 2024 13:30

Move Application Passwords entry to user details

ea57e2b

Call fetchOtherUsers when taps "Delete User"

6bebb44

Fix unit test compiling issues

27855d2

Change the Application Password list view background

9d67d18

Remove an unnecessary IUO

dbb1690

Remove a reduandant Group

10c98ee

jkmassel force-pushed the move-application-passwords-list-entry branch from 1b0daa0 to 10c98ee Compare November 27, 2024 20:30

jkmassel approved these changes Nov 27, 2024

View reviewed changes

crazytonyli added this pull request to the merge queue Nov 27, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 27, 2024

crazytonyli added this pull request to the merge queue Nov 27, 2024

Merged via the queue into trunk with commit 729854a Nov 28, 2024
24 checks passed

crazytonyli deleted the move-application-passwords-list-entry branch November 28, 2024 00:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Application Passwords entry to user details #23834

Move Application Passwords entry to user details #23834

crazytonyli commented Nov 20, 2024

dangermattic commented Nov 20, 2024

wpmobilebot commented Nov 20, 2024 •

edited

Loading

wpmobilebot commented Nov 20, 2024 •

edited

Loading

kean Nov 20, 2024

crazytonyli Nov 21, 2024

kean Nov 20, 2024

crazytonyli Nov 21, 2024

kean Nov 20, 2024

kean Nov 20, 2024

kean Nov 20, 2024

crazytonyli Nov 21, 2024

jkmassel commented Nov 25, 2024

jkmassel commented Nov 25, 2024

crazytonyli commented Nov 26, 2024

jkmassel Nov 27, 2024

crazytonyli Nov 27, 2024

		@@ -3,6 +3,7 @@ import XCTest

		@testable import WordPress

		@MainActor

Move Application Passwords entry to user details #23834

Move Application Passwords entry to user details #23834

Conversation

crazytonyli commented Nov 20, 2024

Regression Notes

dangermattic commented Nov 20, 2024

wpmobilebot commented Nov 20, 2024 • edited Loading

wpmobilebot commented Nov 20, 2024 • edited Loading

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

jkmassel commented Nov 25, 2024

jkmassel commented Nov 25, 2024

crazytonyli commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Nov 20, 2024 •

edited

Loading

wpmobilebot commented Nov 20, 2024 •

edited

Loading