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 users when creating a conversation #1569

Merged
merged 5 commits into from
Jun 3, 2021

Conversation

mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Jun 3, 2021

This adds remote users when creating a conversation. Note: several checks are still missing, as noted in a related funuction addMembers:

  • A call must be made to the remote backend informing it that this user is now part of that conversation. Use 'updateConversationMemberships'.
    -- that call should probably be made after inserting the conversation membership happens in this backend.
  • Events should support remote / qualified users, too.

This PR supersedes PR #1528.

- It allows to access a Galley instance in a monad implementing the class.
- Checks if remote users exist
- Adds remote users to the database
- Adds integration tests for qualified users
- Updates golden tests related to NewConv
@mdimjasevic mdimjasevic force-pushed the mdimjasevic/fed-new-conv-adt-and-tests branch from 132e453 to 176bfc3 Compare June 3, 2021 09:51
Copy link
Contributor

@pcapriotti pcapriotti 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, but it's missing some "fromJSON" golden tests to check that the schema for NewConv is backwards compatible, i.e. it can still parse NewConv requests that only have unqualified user ids.

I can add that myself later, if that's ok.

@mdimjasevic
Copy link
Contributor Author

Looks good, but it's missing some "fromJSON" golden tests to check that the schema for NewConv is backwards compatible, i.e. it can still parse NewConv requests that only have unqualified user ids.

I can add that myself later, if that's ok.

Hm, I thought there is a test that does that. Initially I changed that test, but then someone (Joe?) told me it should still parse even when the new field is not given in JSON, so I reverted the change and the test still passed, i.e., it should be backwards compatible.

At any rate, if you think such a test case is missing, feel free to add it.

This is to make sure the `qualified_users` is indeed optional in the
JSON being parsed.
Comment on lines +34 to +36
testCase "NewConv" $
testFromJSONObjects
[(testObject_NewConvUnmanaged_user_1, "testObject_NewConvUnmanaged_user_1.json")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this!

@mdimjasevic mdimjasevic merged commit ed7e93f into develop Jun 3, 2021
@mdimjasevic mdimjasevic deleted the mdimjasevic/fed-new-conv-adt-and-tests branch June 3, 2021 12:29
@pcapriotti
Copy link
Contributor

🎉

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