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

Extra muted conversation field #469

Merged
merged 8 commits into from
Sep 21, 2018
Merged

Conversation

tiago-loureiro
Copy link
Contributor

@tiago-loureiro tiago-loureiro commented Sep 16, 2018

Currently, clients can only set 2 values related to muting a conversation: on/off and a ref which is a reference of when a conversation was muted (a timestamp).

This PR introduces a new field (otr_muted_status, implemented as an int32) which allows clients to set a more granular value, per conversation (e.g., 0 may mean mute certain message types and 1 would mean mute other messages or calls). The meaning of each value must be agreed upon between different clients and the backend will make no validation of this value (other than being an int32).

Some TODO's related to muting conversations and its consequences on fallback notifications were added inline as comments.

@fisx fisx self-requested a review September 17, 2018 08:07
@@ -867,6 +879,7 @@ instance FromJSON Member where
Member <$> o .: "id"
<*> o .:? "service"
<*> o .:? "otr_muted" .!= False
<*> o .:? "otr_muted_status"
Copy link
Contributor

Choose a reason for hiding this comment

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

[unrelated to this PR, but perhaps easy to fix]

what's the difference between MemberUpdate and MemberUpdateData? why do we need ToJSON instances for these? can you explain this in haddocks near the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MemberUpdateData is used for events (sent over the websocket, etc.) while MemberUpdate is what is expected by galley on the endpoint.

That being said, thanks for the question since it brought up an incomplete implementation on my end!

data Member = Member
{ memId :: !UserId
, memService :: !(Maybe ServiceRef)
, memOtrMuted :: !Bool
, memOtrMuted :: !Bool -- ^ DEPRECATED, remove it once enough clients use `memOtrMutedStatus`
, memOtrMutedStatus :: !(Maybe MutedStatus)
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the Maybe? If not, please explain difference between Nothing and Just 0 in comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Maybe is here by design; if it's a nothing, then clients have not yet set this value which is the reason why we don't want to use a default; Just 0 could mean all message types muted or it could also mean no message types muted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I believe you, just trying to understand: int in cassandra can be NULL, which means that all bits have their default, which we don't want to know. Yes, that makes sense, thanks!

@@ -392,6 +392,7 @@ data Connect = Connect

data MemberUpdateData = MemberUpdateData
{ misOtrMuted :: !(Maybe Bool)
, misOtrMutedStatus :: !(Maybe MutedStatus)
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably would have caught that first-try had there been a comment discussing the difference between this and MemberUpdate on both types. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

;)

@tiago-loureiro tiago-loureiro merged commit 9d85861 into develop Sep 21, 2018
@tiago-loureiro tiago-loureiro deleted the extra-muted-conversation branch September 21, 2018 09:57
@neongreen neongreen mentioned this pull request Oct 4, 2018
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