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

Message reactions #4

Open
bates64 opened this issue Mar 14, 2018 · 6 comments
Open

Message reactions #4

bates64 opened this issue Mar 14, 2018 · 6 comments
Labels
Milestone

Comments

@bates64
Copy link
Contributor

bates64 commented Mar 14, 2018

(This is just a draft. CC @towerofnix @PullJosh @hedgehog1029 for opinions)

Reactions are entirely OPTIONAL for implementations - servers MUST respond with a NO error on all endpoints if reactions are disabled or not supported.

Responds with a list of reactions to the message under { reactions }. If there are none, return { reactions: [] }. If reactions are not supported or otherwise disabled, respond with a 'NO' error.

eg.

{
  reactions: [
    { emote: ':nonfree:', userIDs: [ userID ] },
    { emote: '👌', userIDs: [ userID, userID ] }
  ]
}
  • POST /api/messages/:id/reactions

emote in body, string
requires reactToMessages permission

Reacts to a message with emote, which may be an existing emote :shortcode: or a valid unicode emoji character (we need to specify the exact codepoints for this).

  • DELETE /api/messages/:id/reactions/:emote

emote in body, string - see above for restrictions
requires a session user

Retracts a previous reaction of emote by the requesting user. Error if there is no existing reaction by the requesting user, or just return {} anyway.

  • If an emote is deleted, servers SHOULD remove all reactions of it.

  • Two new events:

Event name Data Emit when
message/react { messageID, emote, userID } Someone reacts to a message (POST)
message/unreact " Someone unreacts (DELETE)

We should wait until 1.0.0 is stable etc. before adding this to spec - I'd say 1.1.0 would be a good contender as this isn't breaking.

@PullJosh
Copy link

In terms of permissions, I think it would be useful for each channel to have four states (relative to each user):

  • Completely private (can't be seen)
  • Read only
  • React only
  • React and comment

Does the current spec allow for this level of control?

@bates64
Copy link
Contributor Author

bates64 commented Mar 14, 2018

@PullJosh yep! channels don't have states - they have their own permission levels. You can achieve those states with the following permissions on a given role for a channel:

  • Completely private (can't be seen) - readMessages: false
  • Read only - readMessages: true, sendMessages: false
  • React only - reactToMessages: true, readMessages: true, sendMessages: false
  • React and comment - reactToMessages: true, readMessages: true, sendMessages: true

@towerofnix
Copy link
Member

This generally looks really good! Thanks for putting together the proposal.

[POST /api/messages/:id/reactions] requires reactToMessages permission

I assume that, since messages are owned by channels, you would need this permission on the message's channel? You might want to make that clear in the spec.

@bates64
Copy link
Contributor Author

bates64 commented Mar 14, 2018

@towerofnix could you explain how channel permissions actually work?

If I had the following applied to me:

  • reactToMessages: true globally
  • reactToMessages: false in #no-react
  • reactToMessages: undefined in #yes-react

I'd expect to be able to:

  • react to messages in #yes-react & other channels
  • NOT react to messages in #no-react

Would be great to clarify this, if it's correct (in that case no changes need to be made to this proposal; reactToMessages globally is enough, we should just define permissions better). I guess a good way of thinking about it is that channels simply override global permissions in their scope?

@towerofnix
Copy link
Member

@heyitsmeuralex That's accurate, yes. And a good way to think about it.

The "how permissions work" section already covers this: "Channel-specific permissions for roles of the user" is higher up in the priority list than anything server-wide. I haven't read through the spec too much though; it miiiight need to be clarified there.

@PullJosh
Copy link

I haven't read through the spec too much though; it miiiight need to be clarified there.

Top-tier semicolon usage. Been a while since I've seen one used well. 👌

@bates64 bates64 added this to the 1.1.0 milestone May 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants