Skip to content

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Sep 10, 2021

This PR implements the ability to update status flags (hidden/muted/archived) for remote conversations.

This adds fields to the cassandra table containing remote conversation membership information for local users, namely otr_muted_status, otr_muted_ref, otr_archived, otr_archived_ref, hidden, hidden_ref. Note that the conversation role is not part of this table, because it is stored in the same backend as the conversation itself.

The qualified endpoint stub for setting self member status has now been fully implemented.

The PR also contains a few refactorings, which were useful because of the split between self member status fields and conversation role:

  • make the user id unqualified in the get-conversations RPC (SQCORE-917)
  • extract metadata fields out of the Conversation type
  • extract member status fields out of the LocalMember type and remove the type variable
  • make the get-conversations RPC return a (new type) RemoteConversation,
    which does not contain any self member status fields, only a conversation role.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If a cassandra schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
  • changelog.d contains the following bits of information:
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@pcapriotti pcapriotti force-pushed the pcapriotti/remote-conv-status branch 5 times, most recently from f5e00f5 to 6f87eb8 Compare September 14, 2021 09:12
@pcapriotti pcapriotti marked this pull request as ready for review September 14, 2021 09:30
@pcapriotti pcapriotti force-pushed the pcapriotti/remote-conv-status branch 3 times, most recently from cc9a235 to ce68561 Compare September 14, 2021 12:49
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

This looks great!

I left some comments inlined. I wasn't sure what's going on there so it'd be great if you could clarify that.

pcapriotti and others added 14 commits September 15, 2021 08:42
This factors out metadata fields from the `Conversation` type so that
they can be reused elsewhere without duplicating code.

At the moment they are only used in the internal `GET
i/conversations/:cnv/meta` endpoint, but later on this extraction will
be useful to improve the `get-conversations` federation RPC.
This is a refactoring of `LocalMember` to extract the member status
fields (hidden/muted/archived) to their own type, so that they can later
be reused to transform a remote conversation to a local one.

Also, the `InternalMember` type constructor has been removed, since the
only used instantiation of it was the `LocalMember` type synomyn.
The user domain has to be the same as the origin domain, so omitting it
does not lose any information and saves us a check (which we weren't
doing anyway).
This removes the hack of returning a fake self member in the
get-conversations RPC, by introducing a different type for remote
conversations.

A `RemoteConversation` object contains the same
information as a normal `Conversation`, except the self member is
replaced by simply a `RoleName`, which is the only bit of information
for remote users that is actually stored locally.

The functions in `Galley.API.Mapping` have been rewritten to fit the new
convention.

Notes:

 - the Mapping tests have been commented out; they probably should be
   rewritten and become unit tests;

 - remote conversation status flags are still always set to their
   default values.
Since one2one conversations with remote users are not implemented yet,
the test has been simplified, and it now only creates a group
conversation.
Attempting to set hidden flag would incorrectly set the archived flag.
This is now fixed.
Co-authored-by: Marko Dimjašević <[email protected]>
@pcapriotti pcapriotti force-pushed the pcapriotti/remote-conv-status branch from 681b892 to 6d867cd Compare September 15, 2021 06:46
Moved the schema definition of `ConversationMetadata` closer to the type
definition.
Replaced the hardcoded admin role for local members to a randomly
generated role.
The user alice was incorrectly created as a local user, only to be then
regarded as a remote one. This didn't impact the functionality of the
test, but it created an unnecessary local user, and made the logic of
the test confusing.
@pcapriotti pcapriotti merged commit 909a72b into develop Sep 15, 2021
@pcapriotti pcapriotti deleted the pcapriotti/remote-conv-status branch September 15, 2021 13:05
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