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

Add/remove groups #250

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Add/remove groups #250

merged 1 commit into from
Feb 9, 2024

Conversation

alexrisch
Copy link
Contributor

Added functionality for add remove groups

Screenshot 2024-02-09 at 9 43 11 AM

Added functionality for add remove groups
@alexrisch alexrisch requested a review from a team as a code owner February 9, 2024 16:48
Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Good job on the fast turnaround!

}
val group = findGroup(clientAddress, id)

runBlocking { group?.addMembers(peerAddresses) }
Copy link
Contributor

Choose a reason for hiding this comment

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

In the long-term it might be good to return an error if the group is not found, here and in the other methods, but doesn't need to block this PR

// throw new Error(
// 'num groups for bob should be 0, but it is ' + bobGroups.length
// )
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that when this is uncommented that the test fails because the size of bobClient.conversations.listGroups() is still 1 after he is removed.

@richardhuaaa Is this expected that groups you have been removed from still are returned from listGroups()? If so, what is the proper way for someone to detect that they have been removed from a group?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @cameronvoell, yes with the current code the group and message history will remain in your database, but you should stop receiving updates. @neekolas is working on a way to notify the platform/app layer that a user has been removed from a group, but it is not ready yet.

Is this expected that groups you have been removed from still are returned from listGroups()?

Alex was also confused about this, lmk if this seems unintuitive from the perspective of API design or if there's a better way to expose this. I don't think we've necessarily optimized around developer experience so far

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now it's expected, a user will see past messages but not future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richardhuaaa I think from a UX perspective it's a bit confusing
From a developer perspective it makes sense if the purpose is to allow the developer freedom to clear the messages in some way.
Meaning if I as an integrating developer want to "remove" the user from the chat and clear messages I can, or if I want them to just stop receiving updates I have the options to do either

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey cameronvoell, yes with the current code the group and message history will remain in your database, but you should stop receiving updates. neekolas is working on a way to notify the platform/app layer that a user has been removed from a group, but it is not ready yet.

Alex was also confused about this, lmk if this seems unintuitive from the perspective of API design or if there's a better way to expose this. I don't think we've necessarily optimized around developer experience so far

@richardhuaaa Sounds like it's just unintuitive because that one piece is not finished yet. Think the only real missing piece is a recommended way for someone to be notified that theyve been blocked, once we have that we should be able to handle the rest on the SDK side. Thanks for confirming!

Copy link
Contributor

@neekolas neekolas Feb 9, 2024

Choose a reason for hiding this comment

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

Agreed. I don't think it's our job to decide what to do after the user has been removed. Some apps may want to continue showing the conversation and history (just with the ability to interact removed), some won't. We just need to surface that information to developers. I'll work on adding that today.

@alexrisch alexrisch merged commit d12fbd9 into beta Feb 9, 2024
4 of 5 checks passed
@alexrisch alexrisch deleted the ar/groups-add-remove branch February 9, 2024 17:54
Copy link
Contributor

github-actions bot commented Feb 9, 2024

🎉 This PR is included in version 1.27.0-beta.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

🎉 This PR is included in version 1.28.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

🎉 This PR is included in version 1.30.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

🎉 This PR is included in version 1.31.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants