-
Notifications
You must be signed in to change notification settings - Fork 10
Add require_2fa_by, extra_domains to UpdateOrg. Add-legacy_user_id to… #75
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
Add require_2fa_by, extra_domains to UpdateOrg. Add-legacy_user_id to… #75
Conversation
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.
Thanks for putting this together! Left some comments
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 believe we don't actually need Deserialize
on UpdateOrgRequest
src/models/update_org_request.rs
Outdated
// #[serde(rename = "require_2fa_by", skip_serializing_if = "Option::is_none")] | ||
// pub require_2fa_by: Option<chrono::DateTime<chrono::Utc>>, | ||
#[serde(rename = "require_2fa_by", skip_serializing_if = "Option::is_none")] | ||
pub require_2fa_by: Option<chrono::DateTime<chrono::Utc>>, |
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.
Looks like we'll need a custom serializer for this. Something like
pub fn serialize_optional_datetime_to_string<S>(x: &Option<chrono::DateTime<chrono::UTC>>, s: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
match x {
Some(datetime) => s.serialize_str(&datetime.format("%Y-%m-%dT%H:%M:%s").to_string()),
None => s.serialize_none(),
}
}
Or something along those lines. From the docs, it seems this approach with chrono
will require us to add a chrono
feature to the Cargo.toml
. Alternatively, we can use another DateTime type. But a raw String
will put the onus of correct formatting on the caller. Either way, we should smoke test it to make sure it plays nice with the parser on the BE.
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.
(Forgot to say) to then use the custom serializer
#[serde(rename = "require_2fa_by", serialize_with = "serialize_optional_datetime_to_string")]
pub require_2fa_by: Option<chrono::DateTime<chrono::Utc>>,
@@ -46,7 +47,8 @@ impl UpdateOrgRequest { | |||
autojoin_by_domain: None, | |||
restrict_to_domain: None, | |||
legacy_org_id: None, | |||
// require_2fa_by: None, | |||
require_2fa_by: None, | |||
extra_domains: None, |
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.
Actually, a correction on this one...
we should instead define extra_domains
as
#[serde(rename = "extra_domains", skip_serializing_if = "Option::is_none")]
pub extra_domains: Option<Vec<String>>,
to distinguish between the cases where there's no change vs setting to an empty array. Then this line stays
extra_domains: None,
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.
lgtm
… UpdateUser