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 remote members to conversations #1529

Merged
merged 14 commits into from
May 27, 2021

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented May 26, 2021

Some preliminary work to make the endpoint for adding remote users to conversations functional. This implements point (1) in the comment of addMembers, i.e. it checks if remote users exist before adding them to a conversation. If some remote users do not exist, a new error with status code 400 is returned.

I also added some tests using the mock federator exposed by #1524.

The rest of the functionality required by addMembers will be implemented in follow-up PRs.

TODO

Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Looks good.

I suppose an end2end test would be nice to catch possible misconfigurations here now that real RPCs are made from/to galley. (in this or a follow-up PR)

You can also remove point (1) from that futurework comment now 😸

Comment on lines 919 to 920
[mkProfile remoteCharlie (Name "charlie")]
(postQualifiedMembers' g alice (remoteBob :| []) convId)
Copy link
Member

Choose a reason for hiding this comment

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

Do I read this code correctly as "dear Mock Federator, please give back a profile for Charlie when asking for the userId of bob" ? If so, this is an interesting test case that is good to cover but implausible to happen in well-behaved backends. Could you also extend this test to include the (likely more common) case of the domain being correct but that user not existing (should give 404 in the client-server API, not sure what the RPC will give) or that user having been deleted (deleted flag in the profile being set to true)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think what I meant to do here is add both Bob and Charlie to the conversation, where the remote only knows about Charlie. But I forgot to include Charlie in the request.

I will add the other tests you suggested, though, they do sound like good scenarios to check.

Test scenario where both Bob and Charlie are added to a conversation,
but only Charlie exists on the remote backend.
@pcapriotti pcapriotti force-pushed the pcapriotti/add-members-to-conversation branch from 3004acf to d0d9ad3 Compare May 27, 2021 09:43
Mark first point of the TODO list as DONE, but leave it there for future
reference.
@pcapriotti pcapriotti merged commit 7db6eba into develop May 27, 2021
@pcapriotti pcapriotti deleted the pcapriotti/add-members-to-conversation branch May 27, 2021 13:55
@fisx fisx mentioned this pull request May 31, 2021
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