Self-hosted site user management #23768
Conversation
This code is taken from PR #23572, with minimal changes.
Generated by 🚫 Danger |
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23768-f656ec1 | |
| Version | 25.4.2 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | f656ec1 | |
| App Center Build | WPiOS - One-Offs #10986 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23768-f656ec1 | |
| Version | 25.4.2 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | f656ec1 | |
| App Center Build | jetpack-installable-builds #10026 |
|
|
Yep. I have a TODO list for those issues. As I mentioned in the PR description, they'll be addressed in future PRs.
🤣 I applaud your adventurous spirit! I saw the button but didn't have the guts to tap it. I'm surprised REST API would allow you to do that... |
| } | ||
|
|
||
| /// Objects conforming to `StringRankedSearchable` can be searched by calling `search(query:)` on a collection of them | ||
| public protocol StringRankedSearchable { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
(nit) I think Collection should have Element, so no need to reach for the Iterator
| @ScaledMetric(relativeTo: .headline) | ||
| var height: CGFloat = 48 | ||
|
|
||
| @Environment(\.sizeCategory) |
There was a problem hiding this comment.
(nit) it is deprecated https://developer.apple.com/documentation/swiftui/environmentvalues/sizecategory and the replacement is iOS 15+
| private let userProvider: UserDataProvider | ||
| private let actionDispatcher: UserManagementActionDispatcher | ||
|
|
||
| init(user: DisplayUser, userProvider: UserDataProvider, actionDispatcher: UserManagementActionDispatcher) { |
There was a problem hiding this comment.
(nit) I usually take advantage of the auto-generated init for structs by making the required parameters internal.
| /// to perform user management actions. | ||
| /// | ||
| /// The default implementation is set up for testing with SwiftUI Previews | ||
| open class UserManagementActionDispatcher: ObservableObject { |
There was a problem hiding this comment.
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
}There was a problem hiding this comment.
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.
| var body: some View { | ||
| Form { | ||
| Section(Strings.nameSectionTitle) { | ||
| LabeledContent(Strings.roleFieldTitle, value: user.role) |
There was a problem hiding this comment.
TIL didn't know about LabeledContent, nice.
| } label: { | ||
| HStack { | ||
| Spacer() | ||
| Text("Delete User") |
There was a problem hiding this comment.
Missing localization (in the whole file).
| } | ||
|
|
||
| NavigationLink { | ||
| // Pass this view's dismiss action, because if we delete a user, we want that screen *and* this one gone |
There was a problem hiding this comment.
Both of these actions – "Set New Password" and "Delete User" should be shown as modal screens.
"Delete User" should be destructive.
| } | ||
|
|
||
| Section(Strings.contactInfoSectionTitle) { | ||
| LabeledContent(Strings.emailAddressFieldTitle, value: user.emailAddress) |
There was a problem hiding this comment.
Probably not the best way to display it. I'd suggest a vertical stack.
| }, label: { | ||
| HStack { | ||
| Spacer() | ||
| Text("Save Changes") |
|
@kean Thanks for the review! I have addressed most of your comments in their own commits at #23777. A couple of things that I didn't address because I plan to refactor them later
|
The majority of code in this PR is taken from #23572. See the first comment.
I added some minor changes in the following two commits: Removing a couple of global static variables and adding a feature flag for the new User Management feature.
I will make more changes to fine-tune both the UI and the implementation.
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:
RELEASE-NOTES.txtif necessary.Testing checklist: