From 1630dae2dbf26b2a0eec639c69ff60643cf6f8d1 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Tue, 20 Aug 2019 09:46:57 -0300 Subject: [PATCH] FEATURE: Publish read state on group messages. (#7989) * Enable or disable read state based on group attribute * When read state needs to be published, the minimum unread count is calculated in the topic query. This way, we can know if someone reads the last post * The option can be enabled/disabled from the UI * The read indicator will live-updated using message bus * Show read indicator on every post * The read indicator now shows read count and can be expanded to see user avatars * Read count gets updated everytime someone reads a message * Simplify topic-list read indicator logic * Unsubscribe from message bus on willDestroyElement, removed unnecesarry values from post-menu, and added a comment to explain where does minimum_unread_count comes from --- .../components/scrolling-post-stream.js.es6 | 9 +- .../components/topic-list-item.js.es6 | 36 ++++++++ .../discourse/controllers/topic.js.es6 | 11 +++ .../discourse/lib/transform-post.js.es6 | 3 +- .../javascripts/discourse/models/group.js.es6 | 3 +- .../groups-form-interaction-fields.hbs | 10 +++ .../templates/list/topic-list-item.raw.hbs | 3 + .../javascripts/discourse/templates/topic.hbs | 1 + .../discourse/widgets/post-menu.js.es6 | 86 +++++++++++++++++- .../discourse/widgets/post-stream.js.es6 | 1 + .../stylesheets/common/base/_topic-list.scss | 6 ++ .../stylesheets/common/base/topic-post.scss | 3 +- .../stylesheets/desktop/topic-post.scss | 4 +- app/assets/stylesheets/mobile/topic-post.scss | 1 + app/controllers/admin/groups_controller.rb | 3 +- app/controllers/groups_controller.rb | 3 +- app/controllers/post_readers_controller.rb | 26 ++++++ app/models/group.rb | 1 + app/models/topic_list.rb | 5 +- app/models/topic_tracking_state.rb | 36 +++++++- app/serializers/basic_group_serializer.rb | 3 +- app/serializers/listable_topic_serializer.rb | 15 +++- app/serializers/post_serializer.rb | 8 ++ app/serializers/topic_view_serializer.rb | 7 +- app/serializers/web_hook_post_serializer.rb | 3 + .../web_hook_topic_view_serializer.rb | 4 + config/locales/client.en.yml | 3 + config/routes.rb | 1 + config/site_settings.yml | 3 +- ...0190807194043_groups_publish_read_state.rb | 7 ++ lib/topic_query.rb | 27 +++++- lib/topic_view.rb | 8 ++ spec/components/topic_query_spec.rb | 43 +++++++++ spec/models/topic_tracking_state_spec.rb | 72 +++++++++++++++ spec/requests/post_readers_controller_spec.rb | 90 +++++++++++++++++++ 35 files changed, 524 insertions(+), 21 deletions(-) create mode 100644 app/controllers/post_readers_controller.rb create mode 100644 db/migrate/20190807194043_groups_publish_read_state.rb create mode 100644 spec/requests/post_readers_controller_spec.rb diff --git a/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 b/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 index 4121a8b0b33..89a48126f30 100644 --- a/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 +++ b/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 @@ -40,7 +40,8 @@ export default MountWidget.extend({ "gaps", "selectedQuery", "selectedPostsCount", - "searchService" + "searchService", + "showReadIndicator" ); }, @@ -291,6 +292,12 @@ export default MountWidget.extend({ onRefresh: "refreshLikes" }); } + + if (args.refreshReaders) { + this.dirtyKeys.keyDirty(`post-menu-${args.id}`, { + onRefresh: "refreshReaders" + }); + } } else if (args.force) { this.dirtyKeys.forceAll(); } diff --git a/app/assets/javascripts/discourse/components/topic-list-item.js.es6 b/app/assets/javascripts/discourse/components/topic-list-item.js.es6 index 564938e0566..9907e0286a8 100644 --- a/app/assets/javascripts/discourse/components/topic-list-item.js.es6 +++ b/app/assets/javascripts/discourse/components/topic-list-item.js.es6 @@ -35,6 +35,42 @@ export const ListItemDefaults = { attributeBindings: ["data-topic-id"], "data-topic-id": Ember.computed.alias("topic.id"), + didInsertElement() { + this._super(...arguments); + + if (typeof this.get("topic.read_by_group_member") !== "undefined") { + this.messageBus.subscribe(this.readIndicatorChannel, data => { + const nodeClassList = document.querySelector( + `.indicator-topic-${data.topic_id}` + ).classList; + + if (data.show_indicator) { + nodeClassList.remove("unread"); + } else { + nodeClassList.add("unread"); + } + }); + } + }, + + willDestroyElement() { + this._super(...arguments); + + if (typeof this.get("topic.read_by_group_member") !== "undefined") { + this.messageBus.unsubscribe(this.readIndicatorChannel); + } + }, + + @computed("topic.id") + readIndicatorChannel(topicId) { + return `/private-messages/group-read/${topicId}`; + }, + + @computed("topic.read_by_group_member") + unreadClass(readByGroupMember) { + return readByGroupMember ? "" : "unread"; + }, + @computed newDotText() { return this.currentUser && this.currentUser.trust_level > 0 diff --git a/app/assets/javascripts/discourse/controllers/topic.js.es6 b/app/assets/javascripts/discourse/controllers/topic.js.es6 index c1983bc4c16..25a635b48bf 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js.es6 +++ b/app/assets/javascripts/discourse/controllers/topic.js.es6 @@ -1348,6 +1348,17 @@ export default Ember.Controller.extend(bufferedProperty("model"), { }) .then(() => refresh({ id: data.id, refreshLikes: true })); break; + case "read": + postStream + .triggerChangedPost(data.id, data.updated_at, { + preserveCooked: true + }) + .then(() => + refresh({ + id: data.id, + refreshReaders: topic.show_read_indicator + }) + ); case "revised": case "rebaked": { postStream diff --git a/app/assets/javascripts/discourse/lib/transform-post.js.es6 b/app/assets/javascripts/discourse/lib/transform-post.js.es6 index 7a1872b4fe5..5f882d8a933 100644 --- a/app/assets/javascripts/discourse/lib/transform-post.js.es6 +++ b/app/assets/javascripts/discourse/lib/transform-post.js.es6 @@ -71,7 +71,8 @@ export function transformBasicPost(post) { expandablePost: false, replyCount: post.reply_count, locked: post.locked, - userCustomFields: post.user_custom_fields + userCustomFields: post.user_custom_fields, + readCount: post.readers_count }; _additionalAttributes.forEach(a => (postAtts[a] = post[a])); diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index 5ea8f11616f..04898356354 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/app/assets/javascripts/discourse/models/group.js.es6 @@ -178,7 +178,8 @@ const Group = RestModel.extend({ allow_membership_requests: this.allow_membership_requests, full_name: this.full_name, default_notification_level: this.default_notification_level, - membership_request_template: this.membership_request_template + membership_request_template: this.membership_request_template, + publish_read_state: this.publish_read_state }; if (!this.id) { diff --git a/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs b/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs index b2f254ca328..959e2e12d13 100644 --- a/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs +++ b/app/assets/javascripts/discourse/templates/components/groups-form-interaction-fields.hbs @@ -52,6 +52,16 @@ class="groups-form-messageable-level"}} +
+ +
+ {{#if showEmailSettings}}
diff --git a/app/assets/javascripts/discourse/templates/list/topic-list-item.raw.hbs b/app/assets/javascripts/discourse/templates/list/topic-list-item.raw.hbs index 6b6b7b2af14..dba2384f786 100644 --- a/app/assets/javascripts/discourse/templates/list/topic-list-item.raw.hbs +++ b/app/assets/javascripts/discourse/templates/list/topic-list-item.raw.hbs @@ -23,6 +23,9 @@ {{~#if showTopicPostBadges}} {{~raw "topic-post-badges" unread=topic.unread newPosts=topic.displayNewPosts unseen=topic.unseen url=topic.lastUnreadUrl newDotText=newDotText}} {{~/if}} + + {{~d-icon "far-eye"}} +
{{#unless hideCategory}} diff --git a/app/assets/javascripts/discourse/templates/topic.hbs b/app/assets/javascripts/discourse/templates/topic.hbs index f8e5ee39034..c10ec5d1175 100644 --- a/app/assets/javascripts/discourse/templates/topic.hbs +++ b/app/assets/javascripts/discourse/templates/topic.hbs @@ -177,6 +177,7 @@ selectedPostsCount=selectedPostsCount selectedQuery=selectedQuery gaps=model.postStream.gaps + showReadIndicator=model.show_read_indicator showFlags=(action "showPostFlags") editPost=(action "editPost") showHistory=(route-action "showHistory") diff --git a/app/assets/javascripts/discourse/widgets/post-menu.js.es6 b/app/assets/javascripts/discourse/widgets/post-menu.js.es6 index 8739675e2f8..3d7a06801f2 100644 --- a/app/assets/javascripts/discourse/widgets/post-menu.js.es6 +++ b/app/assets/javascripts/discourse/widgets/post-menu.js.es6 @@ -52,6 +52,36 @@ export function buildButton(name, widget) { } } +registerButton("read-count", attrs => { + if (attrs.showReadIndicator) { + const count = attrs.readCount; + if (count > 0) { + return { + action: "toggleWhoRead", + title: "post.controls.read_indicator", + className: "button-count read-indicator", + contents: count, + iconRight: true, + addContainer: false + }; + } + } +}); + +registerButton("read", attrs => { + const disabled = attrs.readCount === 0; + if (attrs.showReadIndicator) { + return { + action: "toggleWhoRead", + title: "post.controls.read_indicator", + icon: "far-eye", + before: "read-count", + addContainer: false, + disabled + }; + } +}); + function likeCount(attrs) { const count = attrs.likeCount; @@ -341,7 +371,12 @@ export default createWidget("post-menu", { }, defaultState() { - return { collapsed: true, likedUsers: [], adminVisible: false }; + return { + collapsed: true, + likedUsers: [], + readers: [], + adminVisible: false + }; }, buildKey: attrs => `post-menu-${attrs.id}`, @@ -508,6 +543,19 @@ export default createWidget("post-menu", { ); } + if (state.readers.length) { + const remaining = state.totalReaders - state.readers.length; + contents.push( + this.attach("small-user-list", { + users: state.readers, + addSelf: false, + listClassName: "who-read", + description: "post.actions.people.read", + count: remaining + }) + ); + } + return contents; }, @@ -525,9 +573,15 @@ export default createWidget("post-menu", { showMoreActions() { this.state.collapsed = false; - if (!this.state.likedUsers.length) { - return this.getWhoLiked(); - } + const likesPromise = !this.state.likedUsers.length + ? this.getWhoLiked() + : Ember.RSVP.resolve(); + + return likesPromise.then(() => { + if (!this.state.readers.length) { + return this.getWhoRead(); + } + }); }, like() { @@ -562,6 +616,12 @@ export default createWidget("post-menu", { } }, + refreshReaders() { + if (this.state.readers.length) { + return this.getWhoRead(); + } + }, + getWhoLiked() { const { attrs, state } = this; @@ -576,6 +636,15 @@ export default createWidget("post-menu", { }); }, + getWhoRead() { + const { attrs, state } = this; + + return this.store.find("post-reader", { id: attrs.id }).then(users => { + state.readers = users.map(avatarAtts); + state.totalReaders = users.totalRows; + }); + }, + toggleWhoLiked() { const state = this.state; if (state.likedUsers.length) { @@ -583,5 +652,14 @@ export default createWidget("post-menu", { } else { return this.getWhoLiked(); } + }, + + toggleWhoRead() { + const state = this.state; + if (this.state.readers.length) { + state.readers = []; + } else { + return this.getWhoRead(); + } } }); diff --git a/app/assets/javascripts/discourse/widgets/post-stream.js.es6 b/app/assets/javascripts/discourse/widgets/post-stream.js.es6 index da059dc964f..9291d8a4e67 100644 --- a/app/assets/javascripts/discourse/widgets/post-stream.js.es6 +++ b/app/assets/javascripts/discourse/widgets/post-stream.js.es6 @@ -136,6 +136,7 @@ export default createWidget("post-stream", { this.attach("post-small-action", transformed, { model: post }) ); } else { + transformed.showReadIndicator = attrs.showReadIndicator; result.push(this.attach("post", transformed, { model: post })); } diff --git a/app/assets/stylesheets/common/base/_topic-list.scss b/app/assets/stylesheets/common/base/_topic-list.scss index 4d77eb01f18..0fd6593434d 100644 --- a/app/assets/stylesheets/common/base/_topic-list.scss +++ b/app/assets/stylesheets/common/base/_topic-list.scss @@ -133,6 +133,12 @@ .raw-topic-link > * { pointer-events: none; } + + .read-indicator { + &.unread { + display: none; + } + } } .link-bottom-line { diff --git a/app/assets/stylesheets/common/base/topic-post.scss b/app/assets/stylesheets/common/base/topic-post.scss index 8d6e880ddd1..03c00c58ed5 100644 --- a/app/assets/stylesheets/common/base/topic-post.scss +++ b/app/assets/stylesheets/common/base/topic-post.scss @@ -641,7 +641,8 @@ blockquote > *:last-child { font-size: $font-down-1; } -.who-liked { +.who-liked, +.who-read { transition: height 0.5s; a { margin: 0 0.25em 0.5em 0; diff --git a/app/assets/stylesheets/desktop/topic-post.scss b/app/assets/stylesheets/desktop/topic-post.scss index 07238d3ceaf..21b94f83761 100644 --- a/app/assets/stylesheets/desktop/topic-post.scss +++ b/app/assets/stylesheets/desktop/topic-post.scss @@ -66,6 +66,7 @@ nav.post-controls { margin-left: 0; margin-right: 0; &.my-likes, + &.read-indicator, &.regular-likes { // Like count on posts .d-icon { @@ -838,7 +839,8 @@ a.attachment:before { } } -.who-liked { +.who-liked, +.who-read { margin-top: 20px; margin-bottom: 0; width: 100%; diff --git a/app/assets/stylesheets/mobile/topic-post.scss b/app/assets/stylesheets/mobile/topic-post.scss index 726c6fd614d..30b2f6d976b 100644 --- a/app/assets/stylesheets/mobile/topic-post.scss +++ b/app/assets/stylesheets/mobile/topic-post.scss @@ -38,6 +38,7 @@ span.badge-posts { flex: 0 1 auto; button { &.like, + &.read-indicator, &.create-flag { flex: 1 1 auto; } diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 48ea9822b35..17c98b2d31e 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -153,7 +153,8 @@ class Admin::GroupsController < Admin::AdminController :default_notification_level, :membership_request_template, :owner_usernames, - :usernames + :usernames, + :publish_read_state ] custom_fields = Group.editable_group_custom_fields permitted << { custom_fields: custom_fields } unless custom_fields.blank? diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 724d9818af5..461a98074b8 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -552,7 +552,8 @@ class GroupsController < ApplicationController :name, :grant_trust_level, :automatic_membership_email_domains, - :automatic_membership_retroactive + :automatic_membership_retroactive, + :publish_read_state ]) custom_fields = Group.editable_group_custom_fields diff --git a/app/controllers/post_readers_controller.rb b/app/controllers/post_readers_controller.rb new file mode 100644 index 00000000000..76d75a70ac1 --- /dev/null +++ b/app/controllers/post_readers_controller.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class PostReadersController < ApplicationController + requires_login + + def index + post = Post.includes(topic: %i[allowed_groups]).find(params[:id]) + read_state = post.topic.allowed_groups.any? { |g| g.publish_read_state? && g.users.include?(current_user) } + raise Discourse::InvalidAccess unless read_state + + readers = User + .joins(:topic_users) + .where('topic_users.topic_id = ? AND COALESCE(topic_users.last_read_post_number, 1) >= ?', post.topic_id, post.post_number) + .where.not(id: [current_user.id, post.user_id]) + + readers = readers.map do |r| + { + id: r.id, avatar_template: r.avatar_template, + username: r.username, + username_lower: r.username_lower + } + end + + render_json_dump(post_readers: readers) + end +end diff --git a/app/models/group.rb b/app/models/group.rb index fd76e8af8ff..a04cdef63e5 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -897,6 +897,7 @@ end # visibility_level :integer default(0), not null # public_exit :boolean default(FALSE), not null # public_admission :boolean default(FALSE), not null +# publish_read_state :boolean default(FALSE), not null # membership_request_template :text # messageable_level :integer default(0) # mentionable_level :integer default(0) diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index 69cedff1346..e546df96f04 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -41,7 +41,8 @@ class TopicList :current_user, :tags, :shared_drafts, - :category + :category, + :publish_read_state ) def initialize(filter, current_user, topics, opts = nil) @@ -57,6 +58,8 @@ class TopicList if @opts[:tags] @tags = Tag.where(id: @opts[:tags]).all end + + @publish_read_state = !!@opts[:publish_read_state] end def top_tags diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 88eb8110b91..4c26cc93b31 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -128,19 +128,26 @@ class TopicTrackingState end def self.publish_read(topic_id, last_read_post_number, user_id, notification_level = nil) - highest_post_number = Topic.where(id: topic_id).pluck(:highest_post_number).first + topic = Topic.select(:highest_post_number, :archetype, :id).find_by(id: topic_id) message = { topic_id: topic_id, message_type: "read", payload: { last_read_post_number: last_read_post_number, - highest_post_number: highest_post_number, + highest_post_number: topic.highest_post_number, topic_id: topic_id, notification_level: notification_level } } + if topic.private_message? + groups = read_allowed_groups_of(topic) + post = Post.find_by(topic_id: topic.id, post_number: last_read_post_number) + trigger_post_read_count_update(post, groups) + update_topic_list_read_indicator(topic, groups, last_read_post_number, user_id, true) + end + MessageBus.publish(self.unread_channel_key(user_id), message.as_json, user_ids: [user_id]) end @@ -332,6 +339,8 @@ SQL message = { topic_id: topic.id } + groups = read_allowed_groups_of(topic) + update_topic_list_read_indicator(topic, groups, topic.highest_post_number, topic.last_post_user_id, false) channels.each do |channel, ids| MessageBus.publish( @@ -341,4 +350,27 @@ SQL ) end end + + def self.read_allowed_groups_of(topic) + topic.allowed_groups + .joins(:group_users) + .select('ARRAY_AGG(group_users.user_id) AS members', :name, :publish_read_state) + .group('groups.id') + end + + def self.update_topic_list_read_indicator(topic, groups, last_read_post_number, user_id, read_event) + groups.each do |group| + member = group.members.include?(user_id) + next unless group.publish_read_state? && last_read_post_number == topic.highest_post_number + next if (read_event && !member) || (!read_event && member) + message = { topic_id: topic.id, show_indicator: read_event }.as_json + + MessageBus.publish("/private-messages/group-read/#{topic.id}", message, user_ids: group.members) + end + end + + def self.trigger_post_read_count_update(post, groups) + return if groups.none?(&:publish_read_state?) + post.publish_change_to_clients!(:read) + end end diff --git a/app/serializers/basic_group_serializer.rb b/app/serializers/basic_group_serializer.rb index 3f1cb66dab8..d3c012194a2 100644 --- a/app/serializers/basic_group_serializer.rb +++ b/app/serializers/basic_group_serializer.rb @@ -31,7 +31,8 @@ class BasicGroupSerializer < ApplicationSerializer :is_group_user, :is_group_owner, :members_visibility_level, - :can_see_members + :can_see_members, + :publish_read_state def include_display_name? object.automatic diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb index 0d4208c5bfd..cdd00c1f67b 100644 --- a/app/serializers/listable_topic_serializer.rb +++ b/app/serializers/listable_topic_serializer.rb @@ -25,7 +25,8 @@ class ListableTopicSerializer < BasicTopicSerializer :notification_level, :bookmarked, :liked, - :unicode_title + :unicode_title, + :read_by_group_member has_one :last_poster, serializer: BasicUserSerializer, embed: :objects @@ -121,6 +122,18 @@ class ListableTopicSerializer < BasicTopicSerializer PinnedCheck.unpinned?(object, object.user_data) end + def read_by_group_member + # object#minimum_unread_count is a dynamically generated attribute. + # See TopicQuery#append_read_state for more information. + return false unless object.respond_to?(:minimum_unread_count) + + object.minimum_unread_count && object.minimum_unread_count <= 0 + end + + def include_read_by_group_member? + !!object.topic_list&.publish_read_state + end + protected def unread_helper diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 7ab24caacc5..d0bd771a2a2 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -26,6 +26,7 @@ class PostSerializer < BasicPostSerializer :quote_count, :incoming_link_count, :reads, + :readers_count, :score, :yours, :topic_id, @@ -458,6 +459,13 @@ class PostSerializer < BasicPostSerializer can_review_topic? end + def readers_count + read_count = object.reads - 1 # Exclude logged user + read_count -= 1 unless yours + + read_count < 0 ? 0 : read_count + end + private def can_review_topic? diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index abcfdb8af85..1f177645164 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -71,7 +71,8 @@ class TopicViewSerializer < ApplicationSerializer :participant_count, :destination_category_id, :pm_with_non_human_user, - :queued_posts_count + :queued_posts_count, + :show_read_indicator ) has_one :details, serializer: TopicViewDetailsSerializer, root: false, embed: :objects @@ -248,4 +249,8 @@ class TopicViewSerializer < ApplicationSerializer def include_queued_posts_count? scope.is_staff? && object.queued_posts_enabled end + + def show_read_indicator + object.show_read_indicator? + end end diff --git a/app/serializers/web_hook_post_serializer.rb b/app/serializers/web_hook_post_serializer.rb index d2b173485a2..2f78f3ca886 100644 --- a/app/serializers/web_hook_post_serializer.rb +++ b/app/serializers/web_hook_post_serializer.rb @@ -32,4 +32,7 @@ class WebHookPostSerializer < PostSerializer object.topic ? object.topic.posts_count : 0 end + def include_readers_count? + false + end end diff --git a/app/serializers/web_hook_topic_view_serializer.rb b/app/serializers/web_hook_topic_view_serializer.rb index 5c47ead356f..0d83cee5aef 100644 --- a/app/serializers/web_hook_topic_view_serializer.rb +++ b/app/serializers/web_hook_topic_view_serializer.rb @@ -28,6 +28,10 @@ class WebHookTopicViewSerializer < TopicViewSerializer end end + def include_show_read_indicator? + false + end + def created_by BasicUserSerializer.new(object.topic.user, scope: scope, root: false) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d444d498a5a..f8161e0ba2e 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2421,6 +2421,7 @@ en: reply: "begin composing a reply to this post" like: "like this post" has_liked: "you've liked this post" + read_indicator: "members who read this post" undo_like: "undo like" edit: "edit this post" edit_action: "Edit" @@ -2478,6 +2479,7 @@ en: notify_user: "sent a message" bookmark: "bookmarked this" like: "liked this" + read: "read this" like_capped: one: "and {{count}} other liked this" other: "and {{count}} others liked this" @@ -3217,6 +3219,7 @@ en: members_visibility_levels: title: "Who can see this group members?" description: "Admins can see members of all groups." + publish_read_state: "On group messages publish group read state" membership: automatic: Automatic diff --git a/config/routes.rb b/config/routes.rb index 2ef2345bb95..da8e98ed0f6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -614,6 +614,7 @@ Discourse::Application.routes.draw do get "excerpt" => "excerpt#show" resources :post_action_users + resources :post_readers, only: %i[index] resources :post_actions do collection do get "users" diff --git a/config/site_settings.yml b/config/site_settings.yml index 2ca78030f90..d20a6aa4e5d 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -191,9 +191,10 @@ basic: post_menu: client: true type: list - default: "like|share|flag|edit|bookmark|delete|admin|reply" + default: "read|like|share|flag|edit|bookmark|delete|admin|reply" allow_any: false choices: + - read - like - edit - flag diff --git a/db/migrate/20190807194043_groups_publish_read_state.rb b/db/migrate/20190807194043_groups_publish_read_state.rb new file mode 100644 index 00000000000..0849b374f2b --- /dev/null +++ b/db/migrate/20190807194043_groups_publish_read_state.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class GroupsPublishReadState < ActiveRecord::Migration[5.2] + def change + add_column :groups, :publish_read_state, :boolean, null: false, default: false + end +end diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 1b90719454f..e7ac08398cb 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -344,11 +344,13 @@ class TopicQuery def list_private_messages_group(user) list = private_messages_for(user, :group) - group_id = Group.where('name ilike ?', @options[:group_name]).pluck(:id).first + group = Group.where('name ilike ?', @options[:group_name]).select(:id, :publish_read_state).first + publish_read_state = !!group&.publish_read_state list = list.joins("LEFT JOIN group_archived_messages gm ON gm.topic_id = topics.id AND - gm.group_id = #{group_id.to_i}") + gm.group_id = #{group&.id&.to_i}") list = list.where("gm.id IS NULL") - create_list(:private_messages, {}, list) + list = append_read_state(list, group) if publish_read_state + create_list(:private_messages, { publish_read_state: publish_read_state }, list) end def list_private_messages_group_archive(user) @@ -1057,4 +1059,23 @@ class TopicQuery def sanitize_sql_array(input) ActiveRecord::Base.public_send(:sanitize_sql_array, input.join(',')) end + + def append_read_state(list, group) + group_id = group&.id + return list if group_id.nil? + + selected_values = list.select_values.empty? ? ['topics.*'] : list.select_values + selected_values << "tuig.minimum_unread_count" + + # The calculation was borrowed from lib/unread.rb + minimum_unread_count = TopicUser + .joins(:topic) + .joins("INNER JOIN group_users ON group_users.user_id = topic_users.user_id") + .where(group_users: { group_id: group_id }) + .select( + "MIN(COALESCE(topics.highest_post_number, 0) - COALESCE(highest_seen_post_number, 0)) AS minimum_unread_count, topic_id" + ).group(:topic_id).to_sql + + list.joins("LEFT OUTER JOIN (#{minimum_unread_count}) tuig ON topics.id = tuig.topic_id").select(*selected_values) + end end diff --git a/lib/topic_view.rb b/lib/topic_view.rb index c8ac58063cc..69c3ca64400 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -109,6 +109,14 @@ class TopicView @personal_message = @topic.private_message? end + def show_read_indicator? + return false unless @user || topic.private_message? + + topic.allowed_groups.any? do |group| + group.publish_read_state? && group.users.include?(@user) + end + end + def canonical_path path = relative_url.dup path << diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index 3eccdec1420..a2df1f707f4 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -1020,6 +1020,49 @@ describe TopicQuery do expect(topics).to eq([]) end + + context "Calculating minimum unread count for a topic" do + before { group.update!(publish_read_state: true) } + + let(:listed_message) do + TopicQuery.new(nil, group_name: group.name) + .list_private_messages_group(creator) + .topics.first + end + + it 'returns a positive number when noone has read the last message' do + group_message.update!(highest_post_number: 1) + TopicUser.create!(user: creator, topic: group_message) + + expect(listed_message.minimum_unread_count).to eq(1) + end + + it 'returns 0 when all posts were read' do + group_message.update!(highest_post_number: 1) + TopicUser.create!(user: creator, topic: group_message, highest_seen_post_number: 1) + + expect(listed_message.minimum_unread_count).to eq(0) + end + + it 'returns the minimum number of unread posts when there are more than one user' do + new_user = Fabricate(:user) + group.add(new_user) + group_message.update!(highest_post_number: 3) + TopicUser.create!(user: creator, topic: group_message, highest_seen_post_number: 1) + TopicUser.create!(user: new_user, topic: group_message, highest_seen_post_number: 2) + + expect(listed_message.minimum_unread_count).to eq(1) + end + + it 'returns the minimum number of unread posts when there are more than one user' do + new_user = Fabricate(:topic_allowed_user, topic: group_message).user + group_message.update!(highest_post_number: 3) + TopicUser.create!(user: creator, topic: group_message, highest_seen_post_number: 1) + TopicUser.create!(user: new_user, topic: group_message, highest_seen_post_number: 2) + + expect(listed_message.minimum_unread_count).to eq(2) + end + end end context "shared drafts" do diff --git a/spec/models/topic_tracking_state_spec.rb b/spec/models/topic_tracking_state_spec.rb index 261c3752d27..ceff1b9c95a 100644 --- a/spec/models/topic_tracking_state_spec.rb +++ b/spec/models/topic_tracking_state_spec.rb @@ -254,6 +254,78 @@ describe TopicTrackingState do end end + describe '#publish_read' do + fab!(:group) { Fabricate(:group) } + let(:read_topic_key) { "/private-messages/group-read/#{@group_message.id}" } + let(:read_post_key) { "/topic/#{@group_message.id}" } + let(:latest_post_number) { 3 } + + before do + group.add(user) + @group_message = Fabricate(:private_message_topic, + allowed_groups: [group], + topic_allowed_users: [Fabricate.build(:topic_allowed_user, user: user)], + highest_post_number: latest_post_number + ) + @post = Fabricate(:post, topic: @group_message, post_number: latest_post_number) + end + + it 'does not publish the read indicator if the option is disabled' do + messages = MessageBus.track_publish(read_topic_key) do + TopicTrackingState.publish_read(@group_message.id, latest_post_number, user.id) + end + + expect(messages).to be_empty + end + + it 'does not trigger a read count update if no allowed groups have the option enabled' do + messages = MessageBus.track_publish(read_post_key) do + TopicTrackingState.publish_read(@group_message.id, latest_post_number, user.id) + end + + expect(messages).to be_empty + end + + context 'when the read indicator is enabled' do + before { group.update!(publish_read_state: true) } + + it 'does publish the read indicator' do + message = MessageBus.track_publish(read_topic_key) do + TopicTrackingState.publish_read(@group_message.id, latest_post_number, user.id) + end.first + + expect(message.data['topic_id']).to eq @group_message.id + end + + it 'does not publish the read indicator if the message is not the last one' do + not_last_post_number = latest_post_number - 1 + Fabricate(:post, topic: @group_message, post_number: not_last_post_number) + messages = MessageBus.track_publish(read_topic_key) do + TopicTrackingState.publish_read(@group_message.id, not_last_post_number, user.id) + end + + expect(messages).to be_empty + end + + it 'does not publish the read indicator if the user is not a group member' do + allowed_user = Fabricate(:topic_allowed_user, topic: @group_message) + messages = MessageBus.track_publish(read_topic_key) do + TopicTrackingState.publish_read(@group_message.id, latest_post_number, allowed_user.user_id) + end + + expect(messages).to be_empty + end + + it 'publish a read count update to every client' do + message = MessageBus.track_publish(read_post_key) do + TopicTrackingState.publish_read(@group_message.id, latest_post_number, user.id) + end.first + + expect(message.data[:type]).to eq :read + end + end + end + it "correctly handles muted categories" do user = Fabricate(:user) diff --git a/spec/requests/post_readers_controller_spec.rb b/spec/requests/post_readers_controller_spec.rb new file mode 100644 index 00000000000..fa2cf3d8728 --- /dev/null +++ b/spec/requests/post_readers_controller_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe PostReadersController do + describe '#index' do + fab!(:admin) { Fabricate(:admin) } + fab!(:reader) { Fabricate(:user) } + + before { sign_in(admin) } + + before do + @group = Fabricate(:group) + @group_message = Fabricate(:private_message_topic, allowed_groups: [@group]) + @post = Fabricate(:post, topic: @group_message, post_number: 3) + end + + context 'When the user has access to readers data' do + before do + @group.update!(publish_read_state: true) + @group.add(admin) + @group.add(reader) + end + + it 'returns an empty list when nobody has read the topic' do + get '/post_readers.json', params: { id: @post.id } + + readers = JSON.parse(response.body)['post_readers'] + + expect(readers).to be_empty + end + + it 'returns an user who read until that post' do + TopicUser.create!(user: reader, topic: @group_message, last_read_post_number: 3) + + get '/post_readers.json', params: { id: @post.id } + reader_data = JSON.parse(response.body)['post_readers'].first + + assert_reader_is_correctly_serialized(reader_data, reader, @post) + end + + it 'returns an user who read pass that post' do + TopicUser.create!(user: reader, topic: @group_message, last_read_post_number: 4) + + get '/post_readers.json', params: { id: @post.id } + reader_data = JSON.parse(response.body)['post_readers'].first + + assert_reader_is_correctly_serialized(reader_data, reader, @post) + end + + it 'return an empty list when nodobody read unti that post' do + TopicUser.create!(user: reader, topic: @group_message, last_read_post_number: 1) + + get '/post_readers.json', params: { id: @post.id } + readers = JSON.parse(response.body)['post_readers'] + + expect(readers).to be_empty + end + + it "doesn't include current_user in the readers list" do + TopicUser.create!(user: admin, topic: @group_message, last_read_post_number: 3) + + get '/post_readers.json', params: { id: @post.id } + reader = JSON.parse(response.body)['post_readers'].detect { |r| r['username'] == admin.username } + + expect(reader).to be_nil + end + end + + def assert_reader_is_correctly_serialized(reader_data, reader, post) + expect(reader_data['avatar_template']).to eq reader.avatar_template + expect(reader_data['username']).to eq reader.username + expect(reader_data['username_lower']).to eq reader.username_lower + end + + it 'returns forbidden if no group has publish_read_state enabled' do + get '/post_readers.json', params: { id: @post.id } + + expect(response).to be_forbidden + end + + it 'returns forbidden if current_user is not a member of a group with publish_read_state enabled' do + @group.update!(publish_read_state: true) + + get '/post_readers.json', params: { id: @post.id } + + expect(response).to be_forbidden + end + end +end