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

DM updates - default to not displaying dm groups #1046

Merged
merged 11 commits into from
Sep 21, 2024

Conversation

cameronvoell
Copy link
Contributor

@cameronvoell cameronvoell commented Sep 10, 2024

See #1074

As a pre-requisite for dual sending 1 to 1 messages, we will be creating DM groups that we do not want to show up for users until we switch to V3 only clients. This PR filters DM groups from list and stream conversations functions that are used in bindings and downstream SDKs

@cameronvoell cameronvoell changed the base branch from mls-dm-release to cv/validate-initial-dm-group September 19, 2024 05:19
@cameronvoell cameronvoell changed the base branch from cv/validate-initial-dm-group to mls-dm-release September 19, 2024 05:19
@cameronvoell cameronvoell mentioned this pull request Sep 19, 2024
4 tasks
@cameronvoell cameronvoell marked this pull request as ready for review September 19, 2024 06:37
@cameronvoell cameronvoell requested a review from a team as a code owner September 19, 2024 06:37
@cameronvoell cameronvoell changed the base branch from mls-dm-release to cv/validate-initial-dm-group September 19, 2024 06:38
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.

Maybe you covered this somewhere but is there a way to override the filter out functionality? So for these pure V3 clients that they can still get conversations?

Base automatically changed from cv/validate-initial-dm-group to mls-dm-release September 19, 2024 18:08
bindings_ffi/src/mls.rs Outdated Show resolved Hide resolved
convo_callback(convo)
let provider = client.context.mls_provider()?;
// Don't execute callback for dms unless include_dm is true
if include_dm || convo.metadata(provider)?.conversation_type != ConversationType::Dm
Copy link
Contributor

@insipx insipx Sep 20, 2024

Choose a reason for hiding this comment

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

it would be better to do this filter in stream_conversations itself rather than the callback method -- the callback methods should retain the same behavior as the Non-callback variants, but just accept a function instead. this makes the logic for both easier to follow

Here is a place you could insert a check/filter for DM Group (i.e another filter/filter map call just above that, or even just save the iterator and if the dm_group flag is on, filter the iterator again)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call 👌 , fixed here fc02806

cameronvoell and others added 2 commits September 20, 2024 16:13
@cameronvoell
Copy link
Contributor Author

Maybe you covered this somewhere but is there a way to override the filter out functionality? So for these pure V3 clients that they can still get conversations?

@nplasterer For streams to include Dm groups, we just need to pass in true to stream_conversations_with_callback here:

For list to include Dm groups, we need to pass in true for include_dm_groups in FindGroupParams struct here:

include_dm_groups: false,

@cameronvoell cameronvoell merged commit 76a8d46 into mls-dm-release Sep 21, 2024
9 checks passed
@cameronvoell cameronvoell deleted the cv/dm-dual-sending branch September 21, 2024 06:39
nplasterer added a commit that referenced this pull request Oct 1, 2024
* Adds functions for creating a DM group (#901)

* update bindings cargo locks

* Added dm group functionality

* dm members can update all metadata

* fix tests

* fix indentation

* fix test imports

* gen protos back to main

---------

Co-authored-by: cameronvoell <cameronvoell@users.noreply.github.com>

* Private Preferences DB (#946)

* create the database migration for the private preference work

* update the table to be focused on consent

* first pass at database storage structure

* update the get method for consent records

* fix up the set method

* add a test

* fix up the test

* fix up the clippy error with consent record

* fix up the clippy error with consent record

* fix up all clippy issues

* cargo fmt

* Validate dm group metadata + permissions from welcome (#1075)

* validate dm group before creating from welcome

* lint fix

* lint fix

---------

Co-authored-by: cameronvoell <cameronvoell@users.noreply.github.com>

* fixes after merge

* DM updates - default to not displaying dm groups (#1046)

* bindings create_dm function

* find groups by default does not include dm groups

* fmt fix

* dont execute callbacks when dm group welcomes are streamed

* Update bindings_ffi/src/mls.rs

Co-authored-by: Andrew Plaza <github.tech@liquidthink.net>

* fixed bad merge

* filter dms in stream_conversations

* surface include_dm_groups in bindings list function more clearly

---------

Co-authored-by: cameronvoell <cameronvoell@users.noreply.github.com>
Co-authored-by: Andrew Plaza <github.tech@liquidthink.net>

* fix merge conflicts

* cargo clippy

* Remove tracing from test

* Fix test

* try and fix the tests

* fix up the test

---------

Co-authored-by: cameronvoell <cameronvoell@users.noreply.github.com>
Co-authored-by: Naomi Plasterer <naomi@xmtp.com>
Co-authored-by: Andrew Plaza <github.tech@liquidthink.net>
Co-authored-by: Ry Racherbaumer <ry@xmtp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants