From 301a0fa54eefb97f891b03c95de4bf4fb7f5cb4a Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Wed, 29 Apr 2020 12:48:55 +0800 Subject: [PATCH] FEATURE: Redesign discourse-presence to track state on the client side. (#9487) Before this commit, the presence state of users were stored on the server side and any updates to the state meant we had to publish the entire state to the clients. Also, the way the state of users were stored on the server side meant we didn't have a way to differentiate between replying users and whispering users. In this redesign, we decided to move the tracking of users state to the client side and have the server publish client events instead. As a result of this change, we're able to remove the number of opened connections needed to track presence and also reduce the payload that is sent for each event. At the same time, we've also improved on the restrictions when publishing message_bus messages. Users that do not have permission to see certain events will not receive messages for those events. --- .../composer-presence-display.js.es6 | 151 ++--- .../components/topic-presence-display.js.es6 | 54 +- .../discourse/lib/presence-manager.js.es6 | 201 ++++++ .../components/composer-presence-display.hbs | 4 +- .../connectors/composer-fields/presence.hbs | 1 + .../topic-above-footer-buttons/presence.hbs | 2 +- .../initializers/discourse-presence.js.es6 | 42 ++ .../discourse-presence/config/settings.yml | 1 + plugins/discourse-presence/plugin.rb | 248 ++++---- .../spec/presence_manager_spec.rb | 108 ---- .../spec/requests/presence_controller_spec.rb | 590 +++++++++++++----- 11 files changed, 859 insertions(+), 543 deletions(-) create mode 100644 plugins/discourse-presence/assets/javascripts/discourse/lib/presence-manager.js.es6 create mode 100644 plugins/discourse-presence/assets/javascripts/initializers/discourse-presence.js.es6 delete mode 100644 plugins/discourse-presence/spec/presence_manager_spec.rb diff --git a/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6 index 1e782973760..e6a366087b9 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6 +++ b/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6 @@ -1,12 +1,11 @@ -import { cancel, debounce, once } from "@ember/runloop"; import Component from "@ember/component"; -import { equal, gt } from "@ember/object/computed"; -import { Promise } from "rsvp"; -import { ajax } from "discourse/lib/ajax"; -import computed, { observes, on } from "discourse-common/utils/decorators"; - -export const keepAliveDuration = 10000; -export const bufferTime = 3000; +import { cancel } from "@ember/runloop"; +import { equal, gt, readOnly } from "@ember/object/computed"; +import discourseComputed, { + observes, + on +} from "discourse-common/utils/decorators"; +import { REPLYING, CLOSED, EDITING } from "../lib/presence-manager"; export default Component.extend({ // Passed in variables @@ -15,115 +14,67 @@ export default Component.extend({ topic: null, reply: null, title: null, + isWhispering: null, - // Internal variables - previousState: null, - currentState: null, - presenceUsers: null, - channel: null, - + presenceManager: readOnly("topic.presenceManager"), + users: readOnly("presenceManager.users"), + editingUsers: readOnly("presenceManager.editingUsers"), isReply: equal("action", "reply"), - shouldDisplay: gt("users.length", 0), @on("didInsertElement") - composerOpened() { - this._lastPublish = new Date(); - once(this, "updateState"); + subscribe() { + this.presenceManager && this.presenceManager.subscribe(); }, - @observes("action", "post.id", "topic.id") - composerStateChanged() { - once(this, "updateState"); + @discourseComputed( + "post.id", + "editingUsers.@each.last_seen", + "users.@each.last_seen" + ) + presenceUsers(postId, editingUsers, users) { + if (postId) { + return editingUsers.filterBy("post_id", postId); + } else { + return users; + } }, + shouldDisplay: gt("presenceUsers.length", 0), + @observes("reply", "title") typing() { - if (new Date() - this._lastPublish > keepAliveDuration) { - this.publish({ current: this.currentState }); + if (this.presenceManager) { + const postId = this.get("post.id"); + + this._throttle = this.presenceManager.throttlePublish( + postId ? EDITING : REPLYING, + this.whisper, + postId + ); + } + }, + + @observes("whisper") + cancelThrottle() { + this._cancelThrottle(); + }, + + @observes("post.id") + stopEditing() { + if (this.presenceManager && !this.get("post.id")) { + this.presenceManager.publish(CLOSED, this.whisper); } }, @on("willDestroyElement") composerClosing() { - this.publish({ previous: this.currentState }); - cancel(this._pingTimer); - cancel(this._clearTimer); - }, - - updateState() { - let state = null; - const action = this.action; - - if (action === "reply" || action === "edit") { - state = { action }; - if (action === "reply") state.topic_id = this.get("topic.id"); - if (action === "edit") state.post_id = this.get("post.id"); + if (this.presenceManager) { + this._cancelThrottle(); + this.presenceManager.publish(CLOSED, this.whisper); } - - this.set("previousState", this.currentState); - this.set("currentState", state); }, - @observes("currentState") - currentStateChanged() { - if (this.channel) { - this.messageBus.unsubscribe(this.channel); - this.set("channel", null); - } - - this.clear(); - - if (!["reply", "edit"].includes(this.action)) { - return; - } - - this.publish({ - response_needed: true, - previous: this.previousState, - current: this.currentState - }).then(r => { - if (this.isDestroyed) { - return; - } - this.set("presenceUsers", r.users); - this.set("channel", r.messagebus_channel); - - if (!r.messagebus_channel) { - return; - } - - this.messageBus.subscribe( - r.messagebus_channel, - message => { - if (!this.isDestroyed) this.set("presenceUsers", message.users); - this._clearTimer = debounce( - this, - "clear", - keepAliveDuration + bufferTime - ); - }, - r.messagebus_id - ); - }); - }, - - clear() { - if (!this.isDestroyed) this.set("presenceUsers", []); - }, - - publish(data) { - this._lastPublish = new Date(); - - // Don't publish presence if disabled - if (this.currentUser.hide_profile_and_presence) { - return Promise.resolve(); - } - - return ajax("/presence/publish", { type: "POST", data }); - }, - - @computed("presenceUsers", "currentUser.id") - users(users, currentUserId) { - return (users || []).filter(user => user.id !== currentUserId); + _cancelThrottle() { + cancel(this._throttle); } }); diff --git a/plugins/discourse-presence/assets/javascripts/discourse/components/topic-presence-display.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/components/topic-presence-display.js.es6 index 1089246fb65..9cfb46f7ba1 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/components/topic-presence-display.js.es6 +++ b/plugins/discourse-presence/assets/javascripts/discourse/components/topic-presence-display.js.es6 @@ -1,59 +1,21 @@ -import { cancel, debounce } from "@ember/runloop"; import Component from "@ember/component"; -import { gt } from "@ember/object/computed"; -import computed, { on } from "discourse-common/utils/decorators"; -import { - keepAliveDuration, - bufferTime -} from "discourse/plugins/discourse-presence/discourse/components/composer-presence-display"; - -const MB_GET_LAST_MESSAGE = -2; +import { gt, readOnly } from "@ember/object/computed"; +import { on } from "discourse-common/utils/decorators"; export default Component.extend({ - topicId: null, - presenceUsers: null, + topic: null, + presenceManager: readOnly("topic.presenceManager"), + users: readOnly("presenceManager.users"), shouldDisplay: gt("users.length", 0), - clear() { - if (!this.isDestroyed) this.set("presenceUsers", []); - }, - @on("didInsertElement") - _inserted() { - this.clear(); - - this.messageBus.subscribe( - this.channel, - message => { - if (!this.isDestroyed) this.set("presenceUsers", message.users); - this._clearTimer = debounce( - this, - "clear", - keepAliveDuration + bufferTime - ); - }, - MB_GET_LAST_MESSAGE - ); + subscribe() { + this.get("presenceManager").subscribe(); }, @on("willDestroyElement") _destroyed() { - cancel(this._clearTimer); - this.messageBus.unsubscribe(this.channel); - }, - - @computed("topicId") - channel(topicId) { - return `/presence/topic/${topicId}`; - }, - - @computed("presenceUsers", "currentUser.{id,ignored_users}") - users(users, currentUser) { - const ignoredUsers = currentUser.ignored_users || []; - return (users || []).filter( - user => - user.id !== currentUser.id && !ignoredUsers.includes(user.username) - ); + this.get("presenceManager").unsubscribe(); } }); diff --git a/plugins/discourse-presence/assets/javascripts/discourse/lib/presence-manager.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/lib/presence-manager.js.es6 new file mode 100644 index 00000000000..fe5c1bd6b62 --- /dev/null +++ b/plugins/discourse-presence/assets/javascripts/discourse/lib/presence-manager.js.es6 @@ -0,0 +1,201 @@ +import EmberObject from "@ember/object"; +import { cancel, later, throttle } from "@ember/runloop"; +import { ajax } from "discourse/lib/ajax"; +import discourseComputed from "discourse-common/utils/decorators"; + +// The durations chosen here determines the accuracy of the presence feature and +// is tied closely with the server side implementation. Decreasing the duration +// to increase the accuracy will come at the expense of having to more network +// calls to publish the client's state. +// +// Logic walk through of our heuristic implementation: +// - When client A is typing, a message is published every KEEP_ALIVE_DURATION_SECONDS. +// - Client B receives the message and stores each user in an array and marks +// the user with a client-side timestamp of when the user was seen. +// - If client A continues to type, client B will continue to receive messages to +// update the client-side timestamp of when client A was last seen. +// - If client A disconnects or becomes inactive, the state of client A will be +// cleaned up on client B by a scheduler that runs every TIMER_INTERVAL_MILLISECONDS +const KEEP_ALIVE_DURATION_SECONDS = 10; +const BUFFER_DURATION_SECONDS = KEEP_ALIVE_DURATION_SECONDS + 2; + +const MESSAGE_BUS_LAST_ID = 0; +const TIMER_INTERVAL_MILLISECONDS = 2000; + +export const REPLYING = "replying"; +export const EDITING = "editing"; +export const CLOSED = "closed"; + +const PresenceManager = EmberObject.extend({ + users: null, + editingUsers: null, + subscribed: null, + topic: null, + currentUser: null, + messageBus: null, + siteSettings: null, + + init() { + this._super(...arguments); + + this.setProperties({ + users: [], + editingUsers: [], + subscribed: false + }); + }, + + subscribe() { + if (this.subscribed) return; + + this.messageBus.subscribe( + this.channel, + message => { + const { user, state } = message; + if (this.get("currentUser.id") === user.id) return; + + switch (state) { + case REPLYING: + this._appendUser(this.users, user); + break; + case EDITING: + this._appendUser(this.editingUsers, user, { + post_id: parseInt(message.post_id, 10) + }); + break; + case CLOSED: + this._removeUser(user); + break; + } + }, + MESSAGE_BUS_LAST_ID + ); + + this.set("subscribed", true); + }, + + unsubscribe() { + this.messageBus.unsubscribe(this.channel); + this._stopTimer(); + this.set("subscribed", false); + }, + + @discourseComputed("topic.id") + channel(topicId) { + return `/presence/${topicId}`; + }, + + throttlePublish(state, whisper, postId) { + return throttle( + this, + this.publish, + state, + whisper, + postId, + KEEP_ALIVE_DURATION_SECONDS * 1000 + ); + }, + + publish(state, whisper, postId) { + const data = { + state, + topic_id: this.get("topic.id") + }; + + if (whisper) { + data.is_whisper = 1; + } + + if (postId) { + data.post_id = postId; + } + + return ajax("/presence/publish", { + type: "POST", + data + }); + }, + + _removeUser(user) { + [this.users, this.editingUsers].forEach(users => { + const existingUser = users.findBy("id", user.id); + if (existingUser) users.removeObject(existingUser); + }); + }, + + _cleanUpUsers() { + [this.users, this.editingUsers].forEach(users => { + const staleUsers = []; + + users.forEach(user => { + if (user.last_seen <= Date.now() - BUFFER_DURATION_SECONDS * 1000) { + staleUsers.push(user); + } + }); + + users.removeObjects(staleUsers); + }); + + return this.users.length === 0 && this.editingUsers.length === 0; + }, + + _appendUser(users, user, attrs) { + let existingUser; + let usersLength = 0; + + users.forEach(u => { + if (u.id === user.id) { + existingUser = u; + } + + if (attrs && attrs.post_id) { + if (u.post_id === attrs.post_id) usersLength++; + } else { + usersLength++; + } + }); + + const props = attrs || {}; + props.last_seen = Date.now(); + + if (existingUser) { + existingUser.setProperties(props); + } else { + const limit = this.get("siteSettings.presence_max_users_shown"); + + if (usersLength < limit) { + users.pushObject(EmberObject.create(Object.assign(user, props))); + } + } + + this._startTimer(() => { + this._cleanUpUsers(); + }); + }, + + _scheduleTimer(callback) { + return later( + this, + () => { + const stop = callback(); + + if (!stop) { + this.set("_timer", this._scheduleTimer(callback)); + } + }, + TIMER_INTERVAL_MILLISECONDS + ); + }, + + _stopTimer() { + cancel(this._timer); + }, + + _startTimer(callback) { + if (!this._timer) { + this.set("_timer", this._scheduleTimer(callback)); + } + } +}); + +export default PresenceManager; diff --git a/plugins/discourse-presence/assets/javascripts/discourse/templates/components/composer-presence-display.hbs b/plugins/discourse-presence/assets/javascripts/discourse/templates/components/composer-presence-display.hbs index 97936135d0f..453b13d07a7 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/templates/components/composer-presence-display.hbs +++ b/plugins/discourse-presence/assets/javascripts/discourse/templates/components/composer-presence-display.hbs @@ -1,7 +1,7 @@ {{#if shouldDisplay}}
- {{#each users as |user|}} + {{#each presenceUsers as |user|}} {{avatar user avatarTemplatePath="avatar_template" usernamePath="username" imageSize="small"}} {{/each}}
@@ -16,4 +16,4 @@ --}}...
-{{/if}} \ No newline at end of file +{{/if}} diff --git a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs index af4de7c592b..8fa1fb3862c 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs +++ b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs @@ -4,4 +4,5 @@ topic=model.topic reply=model.reply title=model.title + whisper=model.whisper }} diff --git a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/topic-above-footer-buttons/presence.hbs b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/topic-above-footer-buttons/presence.hbs index 0ee449cf1da..c8514c7edcb 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/topic-above-footer-buttons/presence.hbs +++ b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/topic-above-footer-buttons/presence.hbs @@ -1 +1 @@ -{{topic-presence-display topicId=model.id}} +{{topic-presence-display topic=model}} diff --git a/plugins/discourse-presence/assets/javascripts/initializers/discourse-presence.js.es6 b/plugins/discourse-presence/assets/javascripts/initializers/discourse-presence.js.es6 new file mode 100644 index 00000000000..1b591ce4d13 --- /dev/null +++ b/plugins/discourse-presence/assets/javascripts/initializers/discourse-presence.js.es6 @@ -0,0 +1,42 @@ +import { withPluginApi } from "discourse/lib/plugin-api"; +import PresenceManager from "../discourse/lib/presence-manager"; + +function initializeDiscoursePresence(api) { + const currentUser = api.getCurrentUser(); + const siteSettings = api.container.lookup("site-settings:main"); + + if (currentUser && !currentUser.hide_profile_and_presence) { + api.modifyClass("model:topic", { + presenceManager: null + }); + + api.modifyClass("route:topic-from-params", { + setupController() { + this._super(...arguments); + + this.modelFor("topic").set( + "presenceManager", + PresenceManager.create({ + topic: this.modelFor("topic"), + currentUser, + messageBus: api.container.lookup("message-bus:main"), + siteSettings + }) + ); + } + }); + } +} + +export default { + name: "discourse-presence", + after: "message-bus", + + initialize(container) { + const siteSettings = container.lookup("site-settings:main"); + + if (siteSettings.presence_enabled) { + withPluginApi("0.8.40", initializeDiscoursePresence); + } + } +}; diff --git a/plugins/discourse-presence/config/settings.yml b/plugins/discourse-presence/config/settings.yml index 07c96c3d2b8..21fbddf5ff0 100644 --- a/plugins/discourse-presence/config/settings.yml +++ b/plugins/discourse-presence/config/settings.yml @@ -4,5 +4,6 @@ plugins: client: true presence_max_users_shown: default: 5 + client: true min: 1 max: 50 diff --git a/plugins/discourse-presence/plugin.rb b/plugins/discourse-presence/plugin.rb index 54d694b069e..ce91b1ad2e5 100644 --- a/plugins/discourse-presence/plugin.rb +++ b/plugins/discourse-presence/plugin.rb @@ -2,8 +2,8 @@ # name: discourse-presence # about: Show which users are writing a reply to a topic -# version: 1.0 -# authors: André Pereira, David Taylor +# version: 2.0 +# authors: André Pereira, David Taylor, tgxworld # url: https://github.com/discourse/discourse/tree/master/plugins/discourse-presence enabled_site_setting :presence_enabled @@ -15,161 +15,155 @@ PLUGIN_NAME ||= -"discourse-presence" after_initialize do + MessageBus.register_client_message_filter('/presence/') do |message| + published_at = message.data["published_at"] + + if published_at + (Time.zone.now.to_i - published_at) <= ::Presence::MAX_BACKLOG_AGE_SECONDS + else + false + end + end + module ::Presence + MAX_BACKLOG_AGE_SECONDS = 10 + class Engine < ::Rails::Engine engine_name PLUGIN_NAME isolate_namespace Presence end end - module ::Presence::PresenceManager - MAX_BACKLOG_AGE ||= 60 - - def self.get_redis_key(type, id) - "presence:#{type}:#{id}" - end - - def self.get_messagebus_channel(type, id) - "/presence/#{type}/#{id}" - end - - # return true if a key was added - def self.add(type, id, user_id) - key = get_redis_key(type, id) - result = Discourse.redis.hset(key, user_id, Time.zone.now) - Discourse.redis.expire(key, MAX_BACKLOG_AGE) - result - end - - # return true if a key was deleted - def self.remove(type, id, user_id) - key = get_redis_key(type, id) - Discourse.redis.expire(key, MAX_BACKLOG_AGE) - Discourse.redis.hdel(key, user_id) > 0 - end - - def self.get_users(type, id) - user_ids = Discourse.redis.hkeys(get_redis_key(type, id)).map(&:to_i) - User.where(id: user_ids) - end - - def self.publish(type, id) - users = get_users(type, id) - serialized_users = users.map { |u| BasicUserSerializer.new(u, root: false) } - message = { users: serialized_users, time: Time.now.to_i } - messagebus_channel = get_messagebus_channel(type, id) - - topic = type == 'post' ? Post.find_by(id: id).topic : Topic.find_by(id: id) - - if topic.private_message? - user_ids = User.where('admin OR moderator').pluck(:id) + topic.allowed_users.pluck(:id) - group_ids = topic.allowed_groups.pluck(:id) - - MessageBus.publish( - messagebus_channel, - message.as_json, - user_ids: user_ids, - group_ids: group_ids, - max_backlog_age: MAX_BACKLOG_AGE - ) - else - MessageBus.publish( - messagebus_channel, - message.as_json, - group_ids: topic.secure_group_ids, - max_backlog_age: MAX_BACKLOG_AGE - ) - end - - users - end - - def self.cleanup(type, id) - has_changed = false - - # Delete entries older than 20 seconds - hash = Discourse.redis.hgetall(get_redis_key(type, id)) - hash.each do |user_id, time| - if Time.zone.now - Time.parse(time) >= 20 - has_changed |= remove(type, id, user_id) - end - end - - has_changed - end - - end - require_dependency "application_controller" class Presence::PresencesController < ::ApplicationController requires_plugin PLUGIN_NAME before_action :ensure_logged_in + before_action :ensure_presence_enabled - ACTIONS ||= [-"edit", -"reply"].freeze + EDITING_STATE = 'editing' + REPLYING_STATE = 'replying' + CLOSED_STATE = 'closed' - def publish - raise Discourse::NotFound if current_user.blank? || current_user.user_option.hide_profile_and_presence? - - data = params.permit( - :response_needed, - current: [:action, :topic_id, :post_id], - previous: [:action, :topic_id, :post_id] - ) - - payload = {} - - if data[:previous] && data[:previous][:action].in?(ACTIONS) - type = data[:previous][:post_id] ? 'post' : 'topic' - id = data[:previous][:post_id] ? data[:previous][:post_id] : data[:previous][:topic_id] - - topic = type == 'post' ? Post.find_by(id: id)&.topic : Topic.find_by(id: id) - - if topic - guardian.ensure_can_see!(topic) - - Presence::PresenceManager.remove(type, id, current_user.id) - Presence::PresenceManager.cleanup(type, id) - Presence::PresenceManager.publish(type, id) - end + def handle_message + [:state, :topic_id].each do |key| + raise ActionController::ParameterMissing.new(key) unless params.key?(key) end - if data[:current] && data[:current][:action].in?(ACTIONS) - type = data[:current][:post_id] ? 'post' : 'topic' - id = data[:current][:post_id] ? data[:current][:post_id] : data[:current][:topic_id] + topic_id = permitted_params[:topic_id] + topic = Topic.find_by(id: topic_id) - topic = type == 'post' ? Post.find_by(id: id)&.topic : Topic.find_by(id: id) + raise Discourse::InvalidParameters.new(:topic_id) unless topic + guardian.ensure_can_see!(topic) - if topic - guardian.ensure_can_see!(topic) + post = nil - Presence::PresenceManager.add(type, id, current_user.id) - Presence::PresenceManager.cleanup(type, id) - users = Presence::PresenceManager.publish(type, id) + if (permitted_params[:post_id]) + if (permitted_params[:state] != EDITING_STATE) + raise Discourse::InvalidParameters.new(:state) + end - if data[:response_needed] - messagebus_channel = Presence::PresenceManager.get_messagebus_channel(type, id) - users ||= Presence::PresenceManager.get_users(type, id) - payload = json_payload(messagebus_channel, users) + post = Post.find_by(id: permitted_params[:post_id]) + raise Discourse::InvalidParameters.new(:topic_id) unless post + + guardian.ensure_can_edit!(post) + end + + opts = { + max_backlog_age: Presence::MAX_BACKLOG_AGE_SECONDS + } + + case permitted_params[:state] + when EDITING_STATE + opts[:group_ids] = [Group::AUTO_GROUPS[:staff]] + + if !post.locked? && !permitted_params[:is_whisper] + opts[:user_ids] = [post.user_id] + + if topic.private_message? + if post.wiki + opts[:user_ids] = opts[:user_ids].concat( + topic.allowed_users.where( + "trust_level >= ? AND NOT admin OR moderator", + SiteSetting.min_trust_to_edit_wiki_post + ).pluck(:id) + ) + + opts[:user_ids].uniq! + + # Ignore trust level and just publish to all allowed groups since + # trying to figure out which users in the allowed groups have + # the necessary trust levels can lead to a large array of user ids + # if the groups are big. + opts[:group_ids] = opts[:group_ids].concat( + topic.allowed_groups.pluck(:id) + ) + end + else + if post.wiki + opts[:group_ids] << Group::AUTO_GROUPS[:"trust_level_#{SiteSetting.min_trust_to_edit_wiki_post}"] + elsif SiteSetting.trusted_users_can_edit_others? + opts[:group_ids] << Group::AUTO_GROUPS[:trust_level_4] + end end end + when REPLYING_STATE + if permitted_params[:is_whisper] + opts[:group_ids] = [Group::AUTO_GROUPS[:staff]] + elsif topic.private_message? + opts[:user_ids] = topic.allowed_users.pluck(:id) + + opts[:group_ids] = [Group::AUTO_GROUPS[:staff]].concat( + topic.allowed_groups.pluck(:id) + ) + else + opts[:group_ids] = topic.secure_group_ids + end + when CLOSED_STATE + if topic.private_message? + opts[:user_ids] = topic.allowed_users.pluck(:id) + + opts[:group_ids] = [Group::AUTO_GROUPS[:staff]].concat( + topic.allowed_groups.pluck(:id) + ) + else + opts[:group_ids] = topic.secure_group_ids + end end - render json: payload - end - - def json_payload(channel, users) - { - messagebus_channel: channel, - messagebus_id: MessageBus.last_id(channel), - users: users.limit(SiteSetting.presence_max_users_shown).map { |u| BasicUserSerializer.new(u, root: false) } + payload = { + user: BasicUserSerializer.new(current_user, root: false).as_json, + state: permitted_params[:state], + is_whisper: permitted_params[:is_whisper].present?, + published_at: Time.zone.now.to_i } + + if (post_id = permitted_params[:post_id]).present? + payload[:post_id] = post_id + end + + MessageBus.publish("/presence/#{topic_id}", payload, opts) + + render json: success_json end + private + + def ensure_presence_enabled + if !SiteSetting.presence_enabled || + current_user.user_option.hide_profile_and_presence? + + raise Discourse::NotFound + end + end + + def permitted_params + params.permit(:state, :topic_id, :post_id, :is_whisper) + end end Presence::Engine.routes.draw do - post '/publish' => 'presences#publish' + post '/publish' => 'presences#handle_message' end Discourse::Application.routes.append do diff --git a/plugins/discourse-presence/spec/presence_manager_spec.rb b/plugins/discourse-presence/spec/presence_manager_spec.rb deleted file mode 100644 index b6cf1155ce4..00000000000 --- a/plugins/discourse-presence/spec/presence_manager_spec.rb +++ /dev/null @@ -1,108 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe ::Presence::PresenceManager do - - let(:user1) { Fabricate(:user) } - let(:user2) { Fabricate(:user) } - let(:user3) { Fabricate(:user) } - let(:manager) { ::Presence::PresenceManager } - - let(:post1) { Fabricate(:post) } - let(:post2) { Fabricate(:post) } - - after(:each) do - Discourse.redis.del("presence:topic:#{post1.topic.id}") - Discourse.redis.del("presence:topic:#{post2.topic.id}") - Discourse.redis.del("presence:post:#{post1.id}") - Discourse.redis.del("presence:post:#{post2.id}") - end - - it 'adds, removes and lists users correctly' do - expect(manager.get_users('post', post1.id).count).to eq(0) - - expect(manager.add('post', post1.id, user1.id)).to be true - expect(manager.add('post', post1.id, user2.id)).to be true - expect(manager.add('post', post2.id, user3.id)).to be true - - expect(manager.get_users('post', post1.id).count).to eq(2) - expect(manager.get_users('post', post2.id).count).to eq(1) - - expect(manager.get_users('post', post1.id)).to contain_exactly(user1, user2) - expect(manager.get_users('post', post2.id)).to contain_exactly(user3) - - expect(manager.remove('post', post1.id, user1.id)).to be true - expect(manager.get_users('post', post1.id).count).to eq(1) - expect(manager.get_users('post', post1.id)).to contain_exactly(user2) - end - - it 'publishes correctly' do - expect(manager.get_users('post', post1.id).count).to eq(0) - - manager.add('post', post1.id, user1.id) - manager.add('post', post1.id, user2.id) - - messages = MessageBus.track_publish do - manager.publish('post', post1.id) - end - - expect(messages.count).to eq (1) - message = messages.first - - expect(message.channel).to eq("/presence/post/#{post1.id}") - - expect(message.data["users"].map { |u| u[:id] }).to contain_exactly(user1.id, user2.id) - end - - it 'publishes private message securely' do - private_post = Fabricate(:private_message_post) - manager.add('post', private_post.id, user2.id) - - messages = MessageBus.track_publish do - manager.publish('post', private_post.id) - end - - expect(messages.count).to eq (1) - message = messages.first - - expect(message.channel).to eq("/presence/post/#{private_post.id}") - - user_ids = User.where('admin or moderator').pluck(:id) - user_ids += private_post.topic.allowed_users.pluck(:id) - expect(message.user_ids).to contain_exactly(*user_ids) - end - - it 'publishes private category securely' do - group = Fabricate(:group) - category = Fabricate(:private_category, group: group) - private_topic = Fabricate(:topic, category: category) - - manager.add('topic', private_topic.id, user2.id) - - messages = MessageBus.track_publish do - manager.publish('topic', private_topic.id) - end - - expect(messages.count).to eq (1) - message = messages.first - - expect(message.channel).to eq("/presence/topic/#{private_topic.id}") - - expect(message.group_ids).to contain_exactly(*private_topic.secure_group_ids) - end - - it 'cleans up correctly' do - freeze_time Time.zone.now do - expect(manager.add('post', post1.id, user1.id)).to be true - expect(manager.cleanup('post', post1.id)).to be false # Nothing to cleanup - expect(manager.get_users('post', post1.id).count).to eq(1) - end - - # Anything older than 20 seconds should be cleaned up - freeze_time 30.seconds.from_now do - expect(manager.cleanup('post', post1.id)).to be true - expect(manager.get_users('post', post1.id).count).to eq(0) - end - end -end diff --git a/plugins/discourse-presence/spec/requests/presence_controller_spec.rb b/plugins/discourse-presence/spec/requests/presence_controller_spec.rb index 80547019a73..c13e04fa6c3 100644 --- a/plugins/discourse-presence/spec/requests/presence_controller_spec.rb +++ b/plugins/discourse-presence/spec/requests/presence_controller_spec.rb @@ -3,170 +3,442 @@ require 'rails_helper' describe ::Presence::PresencesController do - before do - SiteSetting.presence_enabled = true - end + describe '#handle_message' do + context 'when not logged in' do + it 'should raise the right error' do + post '/presence/publish.json' - let(:user1) { Fabricate(:user) } - let(:user2) { Fabricate(:user) } - let(:user3) { Fabricate(:user) } + expect(response.status).to eq(403) + end + end - let(:post1) { Fabricate(:post) } - let(:post2) { Fabricate(:post) } + context 'when logged in' do + fab!(:user) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + fab!(:admin) { Fabricate(:admin) } - let(:manager) { ::Presence::PresenceManager } + fab!(:group) do + group = Fabricate(:group) + group.add(user) + group + end - after do - Discourse.redis.del("presence:topic:#{post1.topic.id}") - Discourse.redis.del("presence:topic:#{post2.topic.id}") - Discourse.redis.del("presence:post:#{post1.id}") - Discourse.redis.del("presence:post:#{post2.id}") - end + fab!(:category) { Fabricate(:private_category, group: group) } + fab!(:private_topic) { Fabricate(:topic, category: category) } + fab!(:public_topic) { Fabricate(:topic, first_post: Fabricate(:post)) } - context 'when not logged in' do - it 'should raise the right error' do - post '/presence/publish.json' - expect(response.status).to eq(403) + fab!(:private_message) do + Fabricate(:private_message_topic, + allowed_groups: [group] + ) + end + + before do + sign_in(user) + end + + it 'returns the right response when user disables the presence feature' do + user.user_option.update_column(:hide_profile_and_presence, true) + + post '/presence/publish.json' + + expect(response.status).to eq(404) + end + + it 'returns the right response when the presence site settings is disabled' do + SiteSetting.presence_enabled = false + + post '/presence/publish.json' + + expect(response.status).to eq(404) + end + + it 'returns the right response if required params are missing' do + post '/presence/publish.json' + + expect(response.status).to eq(400) + end + + it 'returns the right response if topic_id is invalid' do + post '/presence/publish.json', params: { topic_id: -999, state: 'replying' } + + expect(response.status).to eq(400) + end + + it 'returns the right response when user does not have access to the topic' do + group.remove(user) + + post '/presence/publish.json', params: { topic_id: private_topic.id, state: 'replying' } + + expect(response.status).to eq(403) + end + + it 'returns the right response when an invalid state is provided with a post_id' do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + post_id: public_topic.first_post.id, + state: 'some state' + } + + expect(response.status).to eq(400) + end + + it 'returns the right response when user can not edit a post' do + Fabricate(:post, topic: private_topic, user: private_topic.user) + + post '/presence/publish.json', params: { + topic_id: private_topic.id, + post_id: private_topic.first_post.id, + state: 'editing' + } + + expect(response.status).to eq(403) + end + + it 'returns the right response when an invalid post_id is given' do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + post_id: -9, + state: 'editing' + } + + expect(response.status).to eq(400) + end + + it 'publishes the right message for a public topic' do + freeze_time + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { topic_id: public_topic.id, state: 'replying' } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.channel).to eq("/presence/#{public_topic.id}") + expect(message.data.dig(:user, :id)).to eq(user.id) + expect(message.data[:published_at]).to eq(Time.zone.now.to_i) + expect(message.group_ids).to eq(nil) + expect(message.user_ids).to eq(nil) + end + + it 'publishes the right message for a restricted topic' do + freeze_time + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: private_topic.id, + state: 'replying' + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.channel).to eq("/presence/#{private_topic.id}") + expect(message.data.dig(:user, :id)).to eq(user.id) + expect(message.data[:published_at]).to eq(Time.zone.now.to_i) + expect(message.group_ids).to contain_exactly(group.id) + expect(message.user_ids).to eq(nil) + end + + it 'publishes the right message for a private message' do + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: private_message.id, + state: 'replying' + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly( + group.id, + Group::AUTO_GROUPS[:staff] + ) + + expect(message.user_ids).to contain_exactly( + *private_message.topic_allowed_users.pluck(:user_id) + ) + end + + it 'publishes the message to staff group when user is whispering' do + SiteSetting.enable_whispers = true + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + state: 'replying', + is_whisper: true + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff]) + expect(message.user_ids).to eq(nil) + end + + it 'publishes the message to staff group when a staff is editing a whisper' do + SiteSetting.enable_whispers = true + sign_in(admin) + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + post_id: public_topic.first_post.id, + state: 'editing', + is_whisper: true + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff]) + expect(message.user_ids).to eq(nil) + end + + it 'publishes the message to staff group when a staff is editing a locked post' do + SiteSetting.enable_whispers = true + sign_in(admin) + locked_post = Fabricate(:post, topic: public_topic, locked_by_id: admin.id) + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + post_id: locked_post.id, + state: 'editing', + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff]) + expect(message.user_ids).to eq(nil) + end + + it 'publishes the message to author, staff group and TL4 group when editing a public post' do + post = Fabricate(:post, topic: public_topic, user: user) + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + post_id: post.id, + state: 'editing', + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly( + Group::AUTO_GROUPS[:trust_level_4], + Group::AUTO_GROUPS[:staff] + ) + + expect(message.user_ids).to contain_exactly(user.id) + end + + it 'publishes the message to author and staff group when editing a public post ' \ + 'if SiteSettings.trusted_users_can_edit_others is set to false' do + + post = Fabricate(:post, topic: public_topic, user: user) + SiteSetting.trusted_users_can_edit_others = false + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + post_id: post.id, + state: 'editing', + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff]) + expect(message.user_ids).to contain_exactly(user.id) + end + + it 'publishes the message to SiteSetting.min_trust_to_edit_wiki_post group ' \ + 'and staff group when editing a wiki in a public topic' do + + post = Fabricate(:post, topic: public_topic, user: user, wiki: true) + SiteSetting.min_trust_to_edit_wiki_post = TrustLevel.levels[:basic] + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + post_id: post.id, + state: 'editing', + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly( + Group::AUTO_GROUPS[:trust_level_1], + Group::AUTO_GROUPS[:staff] + ) + + expect(message.user_ids).to contain_exactly(user.id) + end + + it 'publishes the message to author and staff group when editing a private message' do + post = Fabricate(:post, topic: private_message, user: user) + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: private_message.id, + post_id: post.id, + state: 'editing', + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly( + Group::AUTO_GROUPS[:staff], + ) + + expect(message.user_ids).to contain_exactly(user.id) + end + + it 'publishes the message to users with trust levels of SiteSetting.min_trust_to_edit_wiki_post ' \ + 'and staff group when editing a wiki in a private message' do + + post = Fabricate(:post, + topic: private_message, + user: private_message.user, + wiki: true + ) + + user2.update!(trust_level: TrustLevel.levels[:newuser]) + group.add(user2) + + SiteSetting.min_trust_to_edit_wiki_post = TrustLevel.levels[:basic] + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: private_message.id, + post_id: post.id, + state: 'editing', + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly( + Group::AUTO_GROUPS[:staff], + group.id + ) + + expect(message.user_ids).to contain_exactly( + *private_message.allowed_users.pluck(:id) + ) + end + + it 'publises the right message when closing composer in public topic' do + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: public_topic.id, + state: described_class::CLOSED_STATE, + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to eq(nil) + expect(message.user_ids).to eq(nil) + end + + it 'publises the right message when closing composer in private topic' do + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: private_topic.id, + state: described_class::CLOSED_STATE, + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly(group.id) + expect(message.user_ids).to eq(nil) + end + + it 'publises the right message when closing composer in private message' do + post = Fabricate(:post, topic: private_message, user: user) + + messages = MessageBus.track_publish do + post '/presence/publish.json', params: { + topic_id: private_message.id, + state: described_class::CLOSED_STATE, + } + + expect(response.status).to eq(200) + end + + expect(messages.length).to eq(1) + + message = messages.first + + expect(message.group_ids).to contain_exactly( + Group::AUTO_GROUPS[:staff], + group.id + ) + + expect(message.user_ids).to contain_exactly( + *private_message.allowed_users.pluck(:id) + ) + end end end - - context 'when logged in' do - before do - sign_in(user1) - end - - it "doesn't produce an error" do - expect { post '/presence/publish.json' }.not_to raise_error - end - - it "does not publish for users with disabled presence features" do - user1.user_option.update_column(:hide_profile_and_presence, true) - post '/presence/publish.json' - expect(response.code).to eq("404") - end - - it "uses guardian to secure endpoint" do - private_post = Fabricate(:private_message_post) - - post '/presence/publish.json', params: { - current: { action: 'edit', post_id: private_post.id } - } - - expect(response.code.to_i).to eq(403) - - group = Fabricate(:group) - category = Fabricate(:private_category, group: group) - private_topic = Fabricate(:topic, category: category) - - post '/presence/publish.json', params: { - current: { action: 'edit', topic_id: private_topic.id } - } - - expect(response.code.to_i).to eq(403) - end - - it "returns a response when requested" do - messages = MessageBus.track_publish do - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'edit', post_id: post1.id }, response_needed: true - } - end - - expect(messages.count).to eq(1) - - data = JSON.parse(response.body) - - expect(data['messagebus_channel']).to eq("/presence/post/#{post1.id}") - expect(data['messagebus_id']).to eq(MessageBus.last_id("/presence/post/#{post1.id}")) - expect(data['users'][0]["id"]).to eq(user1.id) - end - - it "doesn't return a response when not requested" do - messages = MessageBus.track_publish do - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'edit', post_id: post1.id } - } - end - - expect(messages.count).to eq(1) - - data = JSON.parse(response.body) - expect(data).to eq({}) - end - - it "does send duplicate messagebus messages" do - messages = MessageBus.track_publish do - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'edit', post_id: post1.id } - } - end - - expect(messages.count).to eq(1) - - messages = MessageBus.track_publish do - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'edit', post_id: post1.id } - } - end - - # we do this cause we also publish time - expect(messages.count).to eq(1) - end - - it "clears 'previous' state when supplied" do - messages = MessageBus.track_publish do - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'edit', post_id: post1.id } - } - - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'edit', post_id: post2.id }, - previous: { compose_state: 'open', action: 'edit', post_id: post1.id } - } - end - - expect(messages.count).to eq(3) - end - - it 'cleans up old users when requested' do - freeze_time Time.zone.now do - manager.add('topic', post1.topic.id, user2.id) - end - - # Anything older than 20 seconds should be cleaned up - freeze_time 30.seconds.from_now do - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'reply', topic_id: post1.topic.id }, response_needed: true - } - - data = JSON.parse(response.body) - - expect(data['users'].length).to eq(1) - end - - end - - describe 'when post has been deleted' do - it 'should return an empty response' do - post1.destroy! - - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'edit', post_id: post1.id } - } - - expect(response.status).to eq(200) - expect(JSON.parse(response.body)).to eq({}) - - post '/presence/publish.json', params: { - current: { compose_state: 'open', action: 'edit', post_id: post2.id }, - previous: { compose_state: 'open', action: 'edit', post_id: post1.id } - } - - expect(response.status).to eq(200) - expect(JSON.parse(response.body)).to eq({}) - end - end - - end - end