Arvados

Changes at arvados-workbench2|80852d13ef70331d0b5dcb7c0741956967129728 branch 18559-user-profile
Test developer-tests-workbench2: #609

• Add route and panel for /user/uuid, moving my-account to use the same code
• Add readonly fields to user service (fullName, isInvited, writableBy)
• Add setup method to user service
• Fix group breadcrumbs showing uuid on direct load
• Add uuid aliases for user profile and my account in nav actions for breadcrumbs to be clickable
• Prevent click event bubbling on renderer actions for data explorers with row click actions

Some comments as a first pass, checking usability and not looking at the code:

• When I tried accessing my non-admin user's profile page, I couldn't edit the information. It doesn't allow edits with an admin account either. (testing both with ce8i5)
  • Upon further testing, it seems that the profile page form sometimes activates the "save changes" button when one of either "Organization" and "E-mail at organization" is modified. In my tests, it changed which one flagged the form as "dirty" depending on whether the user is admin or not.
  • Either way, the other 2 fields (Role & Website) doesn't seem to set the form in "dirty mode", or at least they don't activate the "Save changes" button.
• When adding information on my non-admin user profile (both org and e-mail at org fields) and clicked "save changes", those two fields got cleared but I got a message telling me that the data was saved. It required me to click on the "Refresh" button for those fields to be filled again.
  • Related to the previous set of comments: when I added data to these 2 fields, I was able to add data also to "Role" & "Website" in a second pass (because now they activate the "save changes" button). They also suffer from the "field cleared after saving" issue.
• If name, last name, email and username aren't editable, do you think it would be better to present them as normal text? I think showing those values as part of the form may give the user the false idea that are somehow editable.
• The Groups tab lists not only "role" type groups but also "projects" type groups (and maybe others?). I think the idea is to only list "role" groups.
• Do you think it would be better to not show the "Admin" tab to those users that can't click on it? I personally prefer to not clutter the UI when it isn't necessary.

Changes at commit:arvados-worbnehc2|46b878b9773789f7a953f58f3de2cc4bf370e153 branch 18559-user-profile
Tests developer-tests-workbench2: #615

• Fix saving profile values
• Change read only fields to plain text
• Filter groups to role groups
• Hide admin tab to non admins
• Correctly setup vm in users > create user dialog
• Add tests

This is looking really good! Here're some minor observation & questions:

• Email & URL format validation might be needed for the profile fields.
• In the "My account" page ADMIN tab of an admin user, the "Log In" option is offered even though the user is already logged in as themselves. This is a small detail that I'm not sure will create issues, but if it's simple to fix (e.g.: disable the Log In button when the user is the same as the active one) I think it's a nice detail.
• In the user's listing, accessing the account page is by a simple click. This is nice & simple but I think it won't be obvious for the new users. One way of making it more obvious I think would be to change the mouse pointer like we do for example on the left hand side tree.
• Related to the above comment, maybe it would be also useful to add some menu item "Account settings" in every users' Actions button, wdyt?
• There's a warning about an unused symbol imported at src/views/user-profile-panel/user-profile-panel-root.tsx:L26
• While doing manual tests on my local arvbox, I tried deactivating a user and then "setup account" it again. The result was that the user kept the is_active: false flag. I'm not 100% sure if this is the expected behavior.
• On the new tests: The need for {force: true} on .click() is usually a sign of unneeded UI re-rendering. Is this really necessary on these new tests? Could a guard be set on a certain condition right before the clicking?

Comments:

• The "Admin" tab should show if the user is inactive, inactive but set up, or active
• "Setup user" button opening a "Setup VM" dialog is unnecessary, it should just set up the user (no dialog). The state change should be visible on the admin tab (previous bullet point), with the user going from "inactive" to "set up"
• Remove the "Activity" tab from the users list
• Add "Visit home project" button on user profile

One additional comment:

As a ce8i5 admin user, I tried to edit the profile of a 9tee4 user (both clusters in a peer federation) and got an error when clicking on the "Save" button.
For these cases, it would be convenient to set the form to read-only mode and maybe add a small note clarifying why this is not possible.

Updated comments:

Here's the discussion about the user account states

I don't want to be difficult but what if we eliminated the "Admin" tab entirely, and just made the "Set up" / "Deactivate" / "Log in as" items in the action menu?

Then it should be accessible either right-clicking on the user in the list, using the action menu of the user list, or through the same menu on the user profile page (needs to be added).

• Add action menu to user profile page
• "Setup user" button opening a "Setup VM" dialog is unnecessary, it should just set up the user (no extra dialog).
• The users list should show the state of each user ("inactive", "set up", or "active"), this should replace the "is_active" column.
• The "Profile" tab should also show if the user is "inactive", "set up", or "active"
• The user uuid should be available in both the user list and user profile, with a "copy to clipboard" button
• Remove the "Activity" tab from the users list

This looks great! I have some minor/optional suggestions but overall I think this is ready for merging:

• Profile field validations on URL & email doesn't seem to work (they just check thei length): I just wanted to mention it, but I'm not sure is needed right now as the profile may change depending on cluster config.
• The "Account Settings" context menu item shouldn't be included on the Account page's actions button, as it's superfluous.
• Related to the previous comment: The "Activate/Deactivate/Setup User" actions always appear without regard of the selected user's state (both at the account page and user's list). I think it should be possible to not include them depending on the state, as we do with writable/read-only collections.
• File cypress/integration/user-profile.spec.js: In beforeEach() it seems that the admin and active user get their profile reset. I think a better way to do it is via a user update request call. You'll save a lot of time doing it that way instead of exercising the UI every time.

Updates at arvados-workbench2|b13aaa8909cc1f8b4edf8d32fda9580a3c899418
Tests developer-tests-workbench2: #641

• Added context menu filters
• Updated cypress tests to test filters and use api to reset profiles

This LGTM, thanks!