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

group preference actions #180

Merged
merged 14 commits into from
Feb 21, 2024
Merged

group preference actions #180

merged 14 commits into from
Feb 21, 2024

Conversation

tuddman
Copy link
Contributor

@tuddman tuddman commented Feb 16, 2024

Summary

With xmtp/proto#146 in, this patch adds initial support for recognizing group consents: AllowGroup and DenyGroup.

Bumps org.xmtp:proto-kotlin to 3.43.2

Fixes some other kotlin warnings

@tuddman tuddman requested a review from a team as a code owner February 16, 2024 18:30
Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great! Thanks for doing this. A couple of notes
Can you update this location to correctly reflect the groups consent state https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/Conversation.kt#L103

Can you add in the implicit consent like we do for DMs on newGroup https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/Conversations.kt#L94-L116

We also do implicit consent when sending a message to a conversation with a state of unknown so we should probably also do that for groups. https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/Group.kt#L40

library/src/main/java/org/xmtp/android/library/Contacts.kt Outdated Show resolved Hide resolved
Comment on lines +355 to +357
var result = boClient.contacts.isGroupAllowed(group.id)

assert(!result)
Copy link
Contributor

@nplasterer nplasterer Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that this test should fail now since the default is allow on newGroup? Possibly do you need to call refreshConsentList() to get the latest results.

Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great. Approving to unblock.
Would like to see some additional testing around the implicit consent stuff

@tuddman tuddman merged commit e1eec1d into main Feb 21, 2024
5 of 6 checks passed
@tuddman tuddman deleted the st/group-preference-actions branch February 21, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants