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

Qualify users and conversations in Event #1547

Merged
merged 8 commits into from
Jun 4, 2021

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented May 28, 2021

This makes the conversation ID and originating user ID of Event qualified, as well as the new member IDs in the case of MemberJoin event.

The change is pretty big, but there is nothing really going on except adjusting all the places where events are used to make everything type-check. Some functions gained a new Domain argument which is assumed to be the local domain, in order to be able to qualify IDs.

As more and more things become qualified, the need for these ad-hoc qualifications should hopefully reduce.

@pcapriotti pcapriotti changed the title [WIP] Make qualify users and conversations in Event [skip ci] [WIP] Qualify users and conversations in Event [skip ci] May 31, 2021
@pcapriotti pcapriotti changed the title [WIP] Qualify users and conversations in Event [skip ci] [WIP] Qualify users and conversations in Event May 31, 2021
Also update examples and golden tests.

Consumers of the generated JSON should not be affected, because the
fields containing unqualified data are still present in the output.
- Add default value for `conversation_role` field in the schema for `SimpleMember`
- Add test to check that the default value above is used
- Inline an object schema definition for `SimpleMembers`
@pcapriotti pcapriotti force-pushed the pcapriotti/qualified-events branch from e1f14cb to 669ad09 Compare May 31, 2021 11:44
@pcapriotti pcapriotti changed the title [WIP] Qualify users and conversations in Event Qualify users and conversations in Event May 31, 2021
@pcapriotti pcapriotti marked this pull request as ready for review May 31, 2021 13:10
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.

It's a big diff to review indeed. I admit I only looked at the Haskell changes and did not review the changes to the 125 json files, but glancing at it it appears backwards compatible.
Great work!

@@ -56,12 +55,12 @@ updateConversationMemberships cmu = do
when (not (null localUsers)) $ do
Data.addLocalMembersToRemoteConv localUserIds (cmuConvId cmu)
-- FUTUREWORK: the resulting event should have qualified users and conversations
Copy link
Member

Choose a reason for hiding this comment

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

that futurework is done I suppose (feel free to remove this comment only in a follow-up PR though so you don't need to wait for CI again)

@pcapriotti pcapriotti merged commit 8521e6f into develop Jun 4, 2021
@pcapriotti pcapriotti deleted the pcapriotti/qualified-events branch June 4, 2021 06:17
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