-
Notifications
You must be signed in to change notification settings - Fork 4
WIP Settings Module #6
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
base: main
Are you sure you want to change the base?
Conversation
|
||
// It's not real it's magic | ||
pub fn get_tenant_id() -> Uuid { | ||
Uuid::parse_str("2eda70a4-e7ef-4c1d-8e2c-d1f050f1cf9e").unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm, do we really need it?
instance: &str, | ||
) -> ProblemResponse { | ||
let problem = Problem::new(status, title, detail) | ||
.with_type(format!("https://errors.example.com/{}", code)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we keep it?
use modkit::{Problem, ProblemResponse}; | ||
|
||
/// Helper to create a ProblemResponse with less boilerplate | ||
pub fn from_parts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it be a library function with good human-readable (= LLM readable) function description ?
use crate::domain::error::DomainError; | ||
|
||
/// Map domain error to RFC9457 ProblemResponse | ||
pub fn map_domain_error(e: &DomainError, instance: &str) -> ProblemResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename instance
to something else? Or it's a standard term for this operation?
.await | ||
{ | ||
Ok(settings) => Ok(Json(SettingsDTO { | ||
user_id: settings.user_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to have a 'mapper' function always, not to make it inline, because I can assume we'll end up with multiple similar cases like this in the future. For example what if get_setting, get_settings, update_setting, post_setting, etc. return the same SettingsDTO
.description("Retrieve user's settings") | ||
.tag("settings") | ||
.handler(handlers::get_settings) | ||
.json_response_with_schema::<dto::SettingsDTO>(openapi, 200, "User's settings") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there enums/constants for that? Just for easier source code grep
in the future
impl Default for Settings { | ||
fn default() -> Self { | ||
Self { | ||
user_id: auth::get_user_id(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing, I'd have a mapper function...
user_id: Uuid, | ||
tenant_id: Uuid, | ||
) -> Result<Settings, DomainError> { | ||
debug!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this debug() be a service method that would always automatically print tenant id, user id?
I see it would be a common problem/pattern for many services
|
||
/// Data for updating an existing user entity | ||
pub struct UpdateSettingsEntity { | ||
pub theme: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean we are going to copy and copy all the model fields in NewXXX, UpdateXXX, Model, DTO, and such?
Isn't it too verbose?
|
||
/// Convert a database entity to a contract model (owned version) | ||
impl From<SettingsEntity> for Settings { | ||
fn from(rhs: SettingsEntity) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rhs
? just curious
pub async fn get_settings( | ||
&self, | ||
user_id: Uuid, | ||
tenant_id: Uuid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a context object (with user_id and tenant_id and request_id, etc), not separate values?
|
||
#[derive(Clone, Debug, PartialEq, DeriveEntityModel)] | ||
#[sea_orm(table_name = "settings")] | ||
pub struct Model { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have a convention for the Model
name here? Like, what if have several such 'models' in single file?
} | ||
|
||
/// Update user's settings | ||
pub async fn update<TX>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we have a generic upsert (save) method that handles DB timeout/retries, and such
0175334
to
582f8a5
Compare
No description provided.