-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat(group): add message expiration to group metadata #1477
Conversation
xmtp_mls/src/configuration.rs
Outdated
@@ -55,6 +55,7 @@ pub const DEFAULT_GROUP_NAME: &str = ""; | |||
pub const DEFAULT_GROUP_DESCRIPTION: &str = ""; | |||
pub const DEFAULT_GROUP_IMAGE_URL_SQUARE: &str = ""; | |||
pub const DEFAULT_GROUP_PINNED_FRAME_URL: &str = ""; | |||
pub const DEFAULT_MESSAGE_EXPIRATION_MS: i64 = 0; |
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.
Still thinking of setting this to null instead of 0, if you have another idea, lmk.
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 would. And just wrap the insert logic down below with a
if let Some(message_expiration_ms) = opts.message_expiration_ms {
// insert...
}
Putting in a zero just seems like we're emulating null-state when we don't have to.
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.
@cameronvoell might have thoughts on this.
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.
Since its a value in the map, instead of keeping a default value I decided to not insert it at all.
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.
We had issues in the past of if two people are using two different versions of the SDK in the same group chat. We need to make sure that the person on the old version of the sdk does not break. I believe there was some sort of migration that needs to happen but again @cameronvoell would remember better than me how we decided to go about this for new metadata.
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.
No migration needed here, existing groups will not have message expiration set, but updating the expiration set will be possible with default permission of admin_only (for groups created before this change goes in) =>
libxmtp/xmtp_mls/src/groups/group_permissions.rs
Lines 1041 to 1049 in ab9bb05
// Policy is not found for metadata change, let's check if the new field contains the super_admin prefix | |
// and evaluate accordingly | |
let policy_for_unrecognized_field = | |
if change.field_name.starts_with(SUPER_ADMIN_METADATA_PREFIX) { | |
MetadataPolicies::allow_if_actor_super_admin() | |
} else { | |
// Otherwise we default to admin only for fields with missing policies | |
MetadataPolicies::allow_if_actor_admin() | |
}; |
That path of adding new metadata to existing groups was tested with this change here, which we can reference for adding expiration in this pr.
@@ -161,12 +161,13 @@ pub struct PermissionPolicySet { | |||
pub update_group_description_policy: PermissionPolicy, | |||
pub update_group_image_url_square_policy: PermissionPolicy, | |||
pub update_group_pinned_frame_url_policy: PermissionPolicy, | |||
pub update_message_expiration_ms_policy: PermissionPolicy, |
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 Added only one policy for both message_expiration_ms
and message_expiration_from_ms
. wdyt?
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.
yea I think that makes sense, doesnt make sense for someone to be able to change one without the other 👍
This pull request adds support for message expiration policies in group settings. The key updates include:
• A new message_expiration_ms field added to group creation options: holds how long after the message was received gets deleted
• A new message_expiration_from_ms field added to group creation options: holds from when the messages get deleted
• Permission updates to allow managing message expiration settings.
#1449