adds sso_identifier to /admin/users#6491
adds sso_identifier to /admin/users#6491Katzenkralle wants to merge 8 commits intodani-garcia:mainfrom
Conversation
|
I had added it only to But it would probably make sense to return the same json object in all of |
|
Sure, all paths now return the same object |
|
Looking at this right now. Further, I also do not like that I think it's more idiomatic and less error prone to create a new function, something like What do you think @stefan0xC? |
|
I am not really the person to ask when it comes to idiomatic Rust as that is not my area of expertise but I agree that this can probably be improved and simplified. With regard to the change of the response key format I think it would also be fine to adapt the templates. Maybe we could mention it in the release notes that the templates have changed and/or that they will break for users that have customized them? |
src/db/models/user.rs
Outdated
| pub async fn find_by_uuid(uuid: &UserId, conn: &DbConn) -> Option<(User, Option<Self>)> { | ||
| db_run! { conn: { | ||
| users::table | ||
| .left_join(sso_users::table) | ||
| .select(<(User, Option<Self>)>::as_select()) | ||
| .filter(users::uuid.eq(uuid)) | ||
| .first::<(User, Option<Self>)>(conn) | ||
| .ok() | ||
| }} | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this can now be simplified to just return the SsoUser entry?
There was a problem hiding this comment.
Sure, I originally had it keep returning User for all other find_by_XXX calls of SsoUser returned it, so I thought it might be beneficial for future use.
src/api/admin.rs
Outdated
| async fn get_sso_user(user_id: &UserId, conn: &DbConn) -> Option<SsoUser> { | ||
| SsoUser::find_by_uuid(user_id, conn).await.and_then(|user_and_sso| user_and_sso.1) | ||
| } | ||
|
|
There was a problem hiding this comment.
This function would become obsolete if SsoUser::find_by_uuid were to return an Option<SsoUser>.
src/api/admin.rs
Outdated
| usr["lastActive"] = match u.last_active(conn).await { | ||
| Some(dt) => json!(format_naive_datetime_local(&dt, DT_FMT)), | ||
| None => json!(None::<String>), | ||
| None => json!("Never"), |
There was a problem hiding this comment.
I think that it would probably be cleaner to return nothing here and have the template render Never if that is possible. That way the Admin template can also be translated.
src/api/admin.rs
Outdated
| #[get("/users")] | ||
| async fn get_users_json(_token: AdminToken, conn: DbConn) -> Json<Value> { | ||
| let users = User::get_all(&conn).await; | ||
| async fn get_users_property(users: Vec<(User, Option<SsoUser>)>, conn: &DbConn) -> Vec<Value> { |
There was a problem hiding this comment.
Just as an idea but maybe we could rename the function so it's clearer what it does? E.g. enrich_users_json or something like that.
4 participants
Includes
sso_identifierin JSON return of/admin/usersFixes #6413