DEV: Update discourse-presence plugin to use new PresenceChannel system (PR #14519)

This removes all custom controllers and redis/messagebus logic from discourse-presence, and replaces it with core’s new PresenceChannel system.

All functionality should be retained. This implementation should scale much better to large numbers of users, reduce the number of HTTP requests made by clients, and reduce the volume of messages on the MessageBus.

For more information on PresenceChannel, see 31db8352

(currently based on #14518, will be rebased after that is merged)

GitHub

  replyChannelName(topicId, isReply, isWhisper) {
    if (topicId && (isReply || isWhisper)) {
      return `/discourse-presence/reply/${topicId}`;
    }

Minor suggestion but I feel like it’ll make the function easier to read without having to lookup what id represents.

I don’t think we have a #handle_message instance method anymore so we’ll probably need to update the description here.

The description is incorrect here because we’re asserting against a private topic and not a private message.

Same as DEV: Update discourse-presence plugin to use new PresenceChannel system by davidtaylorhq · Pull Request #14519 · discourse/discourse · GitHub

Not really sure if we need to pluck the ids here when we already have access to the user records.

Do we need to publish the user id since the user will have to be a part of the staff group for whispers?

When a post has been locked, it prevents the author from editing the post so I don’t think we need to allow the author here.

I think it’ll be useful to test the whisper and edit channels as well. For example, the edit channel requires a post to be in a topic that has not been deleted.

It seems like the pattern here is to raise a ActiveRecord::NotFound error so I’m wondering if we should do just a Topic.find(post.topic_id) instead?

One thing I don’t quite understand is why this callback is called. Is it done each time once per subscription to the channel? Reason I’m asking is because I’m wondering what happens if a new group or user is added into a PM. Will the config be updated?

Might want to test with a non-staff user to confirm that whisper presence is not seen by them.

Was thinking it might be worthwhile to test that editing does not publish presence here.

I can’t remember but do we allow anon to view user presence? If so, we might be able to return earlier in this function for an anon user.

Documentation is here:

So the result is cached for up to 2 minutes, and therefore it may take up to 2 minutes for PM participant changes to be reflected in the presence data. I think that should be ok, but if it becomes a problem we could look at adding a deliberate ‘clear channel config cache’ method which could be called whenever PM participants change.

We do allow anons to view presence :+1: Will add an earlier return for them

The permissions are handled on the server, so we can’t really test this in a purely client-side test :cry: We do test this case in RSpec though.

Thanks for the detailed review @tgxworld - I think I’ve addressed all your feedback. Would you mind giving it another skim through?

Frontend tests are still failing in the legacy qunit environment, so I need to fix that. They’re working fine in the ember-cli environment, so I think this is a testing issue rather than a real problem.

Icic. I agree with what you said and live with this for awhile to see if people notices it.

Good point no worry about this then.