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

[iOS] Admin Dashboard - Users #1287

Merged
merged 26 commits into from
Oct 31, 2024
Merged

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Oct 23, 2024

Reference

This is a rewrite of #1278 with the goal of staying more focused on Users. I intend on adding password reset/changes later but for the sake of making this easier to review this is going to only tie back to users. Even that wasn't totally accomplished by this PR but it's much more focused than #1278.

Summary

Creates a list of all Users from the Jellyfin Server (Mirroring DevicesView) and a DetailView. For now the DetailView is pretty empty but I linked it to the previous work done here: #1277. This allows you to see devices owned only by a user. Additionally, this allows for deleting only the devices for a specific user when doing select all.

I have linked this UserView so it can be reached from the DeviceDetailsView by selecting the User or from the ActiveSessionsDetailView by selecting the User.

Issues/Quirks

  • My UserAdminViewModel has a loadDetails action. This is because it's possible to get to the user page with an incomplete UserDto. From DeviceDetailsView & ActiveSessionsDetailView you only have UserID and UserName so any future password resets or permission changes would not be possible. Loading Details ensures all information for that user exists in the Observer.
  • There is a parameter in getDevices to filter by UserID. I don't know if this is an SDK issue or an API issue but this filter does not work. To get around this, I create var deviceBox which filters by the UserId. I want to eventually resolve the underlying issue but if it comes down to C# that's gonna bum me out...
  • I use grayscale on user icons for users who are IsDisabled == True. Not a quirk but it could not the not-right way to go about it.

Screenshots

Updated Dashboard Simulator Screenshot - iPhone 16 Pro - 2024-10-23 at 15:51:20
User List Simulator Screenshot - iPhone 16 Pro - 2024-10-23 at 15:51:28
User Details Simulator Screenshot - iPhone 16 Pro - 2024-10-23 at 15:51:33
Filtered Devices Simulator Screenshot - iPhone 16 Pro - 2024-10-23 at 15:51:55

Future Steps

UsersView

  • Create New User (AddUserView)
  • Delete User (Discussion Required)

UserDetailsView

  • Password (Reset/Set New)
  • Profile (UserDto.Configuration)
  • Permissions (UserDto.Policy)
  • Access (UserDto.Policy)
  • Parental Controls (UserDto.Policy)

…omes an issue IF the Devices are loaded from a User who doesn't have Devices. Otherwise, there should be at least 1 Device.
@JPKribs
Copy link
Member Author

JPKribs commented Oct 25, 2024

I've adjusted this to user a viewModel instead of an Observer. I'm curious if there is a better solution to:

My UserAdminViewModel has a loadDetails action. This is because it's possible to get to the user page with an incomplete UserDto. From DeviceDetailsView & ActiveSessionsDetailView you only have UserID and UserName so any future password resets or permission changes would not be possible. Loading Details ensures all information for that user exists in the Observer.

The reason I am using this as a solution is because future functionality like Permissions/Configuration changes require the full UserDto be populated to get current vs new. For the UserDetailsView, I suppose we can just pass in the UserDto but then we also have the issue that lastSeen date/relative might not be available or could be incorrect. The example is if I come from DevicesView > DeviceDetailsView > UserDetailsView the only 'lastActivityDate' I have is the Device. So this would not be a reflection of the last User activity but that specific device.

All in all, not a huge issue. This API call is done based on a single user but I'd be interested if there was a better/cleaner way to do this.

Lastly, the getDevices(userID: userID) doesn't appear to be filtering. I still need to test the API directly to see if that is an SDK issue or an API issue.

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

Initial pass, looks great!

@JPKribs
Copy link
Member Author

JPKribs commented Oct 27, 2024

I've got most of these changes in! I have a couple more design changes I'd love input on but I will pick this back up tomorrow/later in the week. I'm going to call it for now since I'm mostly happy with the current working version!

@JPKribs
Copy link
Member Author

JPKribs commented Oct 27, 2024

I think I have all of the changes accounted for now. This PR is getting a tad bigger because I decided to move my addUser logic into this PR. I'm happy with how the UI looks but this ServerUsersView now allows:

  • Creation
  • Deletion (Bulk/Single)
  • Filtering

So if this feels too clutter I won't be offended. I've left the 2 outstanding review changes that I'm unsure about open. One for design and one for the User/Device link.

@LePips
Copy link
Member

LePips commented Oct 29, 2024

I change the buttons to be in a Menu, as I don't really want to overload with buttons, but I may be too minimalistic. I've also made the delete button a pill button to live on the bottom toolbar and will want the other delete buttons to be migrated to it.

For user creation, that should live in its own view model.

@JPKribs
Copy link
Member Author

JPKribs commented Oct 30, 2024

I change the buttons to be in a Menu, as I don't really want to overload with buttons, but I may be too minimalistic. I've also made the delete button a pill button to live on the bottom toolbar and will want the other delete buttons to be migrated to it.

For user creation, that should live in its own view model.

I can take a look at this tomorrow! Thanks for the second set of eyes!

@JPKribs
Copy link
Member Author

JPKribs commented Oct 31, 2024

I change the buttons to be in a Menu, as I don't really want to overload with buttons, but I may be too minimalistic.

I had a chance to look this over. I'm actually a big fan! I think that might be a better look that some of the existing Edit/Add/Delete buttons I'm made for other parts of the Admin Dashboard. Do you want me to use this style for the other Edit/Add/Delete buttons in place of, for example, the add button so the ... is used there instead? Or should I leave views with only a single button as those single buttons?

I updated the Devices page to have the same Delete button.

I've updated everything related to users and I've tested that it's still all working as expected. I did see you commented out some of the backgroundStates.remove() calls in the ViewModels. I was guessing that's related to the warning they give since I'm 'not using' the results of the call. I'm not sure if there is a better way to do it but for now I have that re-enabled. Let me know if you have want to change this back.

Otherwise, I think this is ready to go from my end! Let me know if you want anything else adjusted!

@LePips
Copy link
Member

LePips commented Oct 31, 2024

I did my cleanup and some refactoring for the device details since I noticed that was still in a box design and made its own view model.

For the add user view, that is now a modal since information creation/editing should now be in modals. Other design cleanup as well.

@JPKribs
Copy link
Member Author

JPKribs commented Oct 31, 2024

I did my cleanup and some refactoring for the device details since I noticed that was still in a box design and made its own view model.

For the add user view, that is now a modal since information creation/editing should now be in modals. Other design cleanup as well.

Thank you for your help cleaning this up! I do see 2 things I can address from my end but wanted your opinion:

Filtering users looks like this if there are no users (see below). I don't think this is bad but would we prefer this none is centered? Does the None need to have the separator removed? I'd guess this is the same style for the Devices as well but I don't think that matters since there will never be a case where there are no devices. Since, the device you are on will have to exist in the devices list.

Empty User List Simulator Screenshot - iPhone 16 Pro - 2024-10-31 at 13 40 01

EDIT: Nevermind, Devices looks like this:

            if viewModel.devices.isEmpty {
                Text(L10n.none)
                    .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .center)
                    .listRowSeparator(.hidden)
                    .listRowInsets(.zero)
            }

If I update users to have the same, this now looks like this (see below) do we prefer this?

Empty User List (New) Simulator Screenshot - iPhone 16 Pro - 2024-10-31 at 13 40 01

Secondly, when creating a user, I was having the viewModel for ServerUsersView always .getUsers onAppear to account for user creation. I don't think that was the best way to do this but the current version doesn't update the user list when a user is created. I see you have a comment for

TODO: somehow route to new user details

on the addUser that kind of resolves this since the need to update the user list isn't there. That being said, for the time being do we feel fine using the onAppear() call for the ServerUserList for this purpose or is there a better route to add the new user to the ViewModel from another ViewModel?

@LePips
Copy link
Member

LePips commented Oct 31, 2024

You can create a new notification in SwiftfinNotifications called didAddServerUser and use onNotification to listen when that is fired, passing in the new user object in the notification. From that, I would want us to push from the user list view to the user detail view so that the user could be further customized.

…ch should never get used...) with the same logic instead of 2 spacer()s. Finally, remove all Save New User Button on the AddServerUserView. TODO: The ServerUserView doesn't update when a new user is created
@JPKribs
Copy link
Member Author

JPKribs commented Oct 31, 2024

You can create a new notification in SwiftfinNotifications called didAddServerUser and use onNotification to listen when that is fired, passing in the new user object in the notification. From that, I would want us to push from the user list view to the user detail view so that the user could be further customized.

This was way easier than I thought it was going to be! I have something that is working how we expect it. All the existing Notification logic was really easy to follow and implement. I only have one quirk that works but might not be the best route. I put in a 0.1 second delay. Otherwise, the addServerUser router.dismissCoordinator() dismisses the new serverUserDetailsView that pops up. So, without the delay it all runs then immediately kicks me out of details. With this 0.1 second delay it triggers appropriately and at the end of the AddUser process, it leaves the user on the ServerUserDetailsView for the newly created UserDto.

Sorry to bother you further on this one. Let me know if there is anything I am missing or can improve on for this!

@LePips
Copy link
Member

LePips commented Oct 31, 2024

At least for now with Stinsen, we can have an action fire after the coordinator is dismissed.

@JPKribs
Copy link
Member Author

JPKribs commented Oct 31, 2024

At least for now with Stinsen, we can have an action fire after the coordinator is dismissed.

Very cool! Thank you for your help on this!

@LePips LePips merged commit e0990e3 into jellyfin:main Oct 31, 2024
4 checks passed
@JPKribs JPKribs deleted the adminDashboardUsers branch November 12, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants