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

Align with fractal-server 2.6.0 and 2.6.1 (re: user settings) #563

Closed
tcompa opened this issue Sep 20, 2024 · 7 comments · Fixed by #564
Closed

Align with fractal-server 2.6.0 and 2.6.1 (re: user settings) #563

tcompa opened this issue Sep 20, 2024 · 7 comments · Fixed by #564

Comments

@tcompa
Copy link
Collaborator

tcompa commented Sep 20, 2024

In fractal-server 2.6, we will extract several attributes from the user table into a dedicated user-settings table, and we will also start including more attributes (which were currently global variables).

The main breaking changes in the API are in the `/auth/users/ routes.

  1. Three properties (slurm_user, slurm_accounts, cache_dir) are not part of request/response bodies for GET/PATCH requests to /auth/users/{id}/ and /auth/current-user/ any more.
  2. New GET/PATCH endpoints are available at /auth/users/{id}/settings/ (for the admin) or /auth/current-users/settings/ (for the user).
  3. It's still TBD (but likely) whether some endpoints (e.g. GET /auth/users/ or GET /auth/users/{id}/) will automatically include a property user.settings, or whether this will have to go through a second API call.
  4. The response of GET/PATCH /auth/current-user/ will not include a settings property, but it will be required to make a second API call to /auth/current-user/settings/
  5. Overall, the scope of PATCH /auth/current-user/ will decrease, or it may even be removed - TBD.

Most internal changes concern the SLURM-related job execution and task collection, which are likely not tested in fractal-web CI.


In the webclient, we need to make changes in two parts:

Admin area

In the page where the admin can edit a single user, they should see two different forms: one to edit the user properties, and one to edit the user_settings one. Each form has a different "save" button.

User area

We should split the "profile" page into two pages: profile page (which is essentially read-only) and settings page (where the user can modify some values).

There is a decision to be taken about which settings to expose in the PATCH-related form. The database user-setting table has several columns, but only a subset of them are relevant. Which subset is relevant depends on a backend variable (FRACTAL_RUNNER_BACKEND). If we duplicate this env variable in fractal-web, then we can show a different form based on its value. Other ideas that do not rely on duplicating this variable are welcome.

@zonia3000
Copy link
Collaborator

zonia3000 commented Sep 23, 2024

In the page where the admin can edit a single user, they should see two different forms: one to edit the user properties, and one to edit the user_settings one. Each form has a different "save" button.

What about the "create new user" page? I assume that we should display only the first form. Once the user is created what should the page do? I see 2 possible alternatives:

  1. Redirect back to the users table (current behavior);
  2. Display the second form, so that one can edit the user settings just after the creation of a new user (this corresponds to redirecting to the user edit page).

@tcompa
Copy link
Collaborator Author

tcompa commented Sep 23, 2024

Good point.

I assume that we should display only the first form

Yes, because (at least for the moment) the creation of a user always creates empty settings.


I'd say your option 2 is better. After creating the user, the admin sees the second form where they can edit the user settings.
I have no strong opinion on whether the two forms are shown in the same "Create user" page (with the settings form only enabled after the user has been created) or in two different pages.

@zonia3000
Copy link
Collaborator

A note for the backend: it seems that the /auth/users/{user_id}/settings endpoint accepts empty strings in the SLURM accounts array: slurm_accounts: [""]

@zonia3000
Copy link
Collaborator

I see that the endpoint /api/auth/users is now returning null slurm_user fields (I imagine that they will be removed). Those fields are used to populate the Owner dropdown in the admin Tasks page. If the username field is null then the slurm_user field is used. How should we handle this case? I want to avoid having to perform one call to /auth/users/{user_id}/settings for each user without the username field.

@tcompa
Copy link
Collaborator Author

tcompa commented Sep 23, 2024

There is a decision to be taken about which settings to expose in the PATCH-related form. The database user-setting table has several columns, but only a subset of them are relevant. Which subset is relevant depends on a backend variable (FRACTAL_RUNNER_BACKEND). If we duplicate this env variable in fractal-web, then we can show a different form based on its value. Other ideas that do not rely on duplicating this variable are welcome.

Let's start like this:

  • New FRACTAL_RUNNER_BACKEND env variable (e.g. FRACTAL_RUNNER_BACKEND: Literal['local', 'local_experimental', 'slurm', 'slurm_ssh'] = 'local').
    (in the future we may expose this info from an endpoint)

If local or local_experimental: no property shown.

If slurm:
editable properties: slurm_accounts and cache_dir
non-editable properties: slurm_user

if slurm_ssh:
editable properties: slurm_accounts
non-editable properties: ssh_username

@tcompa
Copy link
Collaborator Author

tcompa commented Sep 23, 2024

I see that the endpoint /api/auth/users is now returning null slurm_user fields (I imagine that they will be removed). Those fields are used to populate the Owner dropdown in the admin Tasks page. If the username field is null then the slurm_user field is used. How should we handle this case? I want to avoid having to perform one call to /auth/users/{user_id}/settings for each user without the username field.

For the moment, let's disable the per-owner query - since we are in the process of dropping that attribute

@tcompa tcompa changed the title [placeholder] Align with fractal-server 2.6.0 and 2.6.1 (re: user settings) Align with fractal-server 2.6.0 and 2.6.1 (re: user settings) Sep 24, 2024
@zonia3000 zonia3000 mentioned this issue Sep 25, 2024
1 task
@tcompa
Copy link
Collaborator Author

tcompa commented Sep 25, 2024

#563 (comment) was for the user profile, but we should also describe the admin-area side of this.

Here is the full description

admin-area / local or local_experimental

no attributes

user's profile / local or local_experimental

no attributes

admin-area / slurm

editable: slurm_user, cache_dir, slurm_accounts
locked: none

user's profile / slurm

editable: cache_dir, slurm_accounts
locked: slurm_user

admin-area / slurm_ssh

editable: ssh_host, ssh_username, ssh_private_key_path, ssh_tasks_dir, ssh_jobs_dir, slurm_accounts
locked: none

user's profile / slurm_ssh

editable: slurm_accounts
locked: ssh_username,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

2 participants