From 7c741fa0d673f3c808cf3f77127e44e823ea40f8 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Tue, 27 Aug 2019 09:09:00 -0300 Subject: [PATCH] FEATURE: Publish read state on group messages. (Originally introduced in #7989) (#8025) * Revert "Revert "FEATURE: Publish read state on group messages. (#7989) [Undo revert] (#8024)"" This reverts commit 36425eb9f04cfac7201632d648050cb43a035bc1. * Fix: Show who read only if the attribute is enabled * PERF: Precalculate the last post readed by a group member * Use book-reader icon instear of far-eye * FIX: update topic groups correctly * DEV: Tidy up read indicator update on write --- .../components/scrolling-post-stream.js.es6 | 9 +- .../components/topic-list-item.js.es6 | 41 +++++++++ .../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 | 5 ++ .../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/post_timing.rb | 1 + app/models/topic_group.rb | 71 +++++++++++++++ app/models/topic_list.rb | 5 +- app/models/topic_tracking_state.rb | 56 +++++++++++- 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 ++ ...20190820192341_create_topic_group_table.rb | 14 +++ lib/post_jobs_enqueuer.rb | 1 + lib/svg_sprite/svg_sprite.rb | 1 + lib/topic_query.rb | 20 ++++- lib/topic_view.rb | 8 ++ spec/components/topic_query_spec.rb | 18 ++++ spec/models/topic_group_spec.rb | 85 ++++++++++++++++++ spec/models/topic_tracking_state_spec.rb | 64 +++++++++++++ spec/requests/post_readers_controller_spec.rb | 90 +++++++++++++++++++ vendor/assets/svg-icons/fontawesome/solid.svg | 4 + 42 files changed, 688 insertions(+), 21 deletions(-) create mode 100644 app/controllers/post_readers_controller.rb create mode 100644 app/models/topic_group.rb create mode 100644 db/migrate/20190807194043_groups_publish_read_state.rb create mode 100644 db/migrate/20190820192341_create_topic_group_table.rb create mode 100644 spec/models/topic_group_spec.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..e7ed6186b34 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,47 @@ export const ListItemDefaults = { attributeBindings: ["data-topic-id"], "data-topic-id": Ember.computed.alias("topic.id"), + didInsertElement() { + this._super(...arguments); + + if (this.includeReadIndicator) { + 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 (this.includeReadIndicator) { + this.messageBus.unsubscribe(this.readIndicatorChannel); + } + }, + + @computed("topic.id") + readIndicatorChannel(topicId) { + return `/private-messages/read-indicator/${topicId}`; + }, + + @computed("topic.read_by_group_member") + unreadClass(readByGroupMember) { + return readByGroupMember ? "" : "unread"; + }, + + @computed("topic.read_by_group_member") + includeReadIndicator(readByGroupMember) { + return typeof readByGroupMember !== "undefined"; + }, + @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..f6d2ddfc382 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,11 @@ {{~#if showTopicPostBadges}} {{~raw "topic-post-badges" unread=topic.unread newPosts=topic.displayNewPosts unseen=topic.unseen url=topic.lastUnreadUrl newDotText=newDotText}} {{~/if}} + {{~#if includeReadIndicator}} + + {{~d-icon "book-reader"}} + + {{~/if}}
{{#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..db2ac3ba5c2 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: "book-reader", + 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 && this.attrs.showReadIndicator) { + 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/post_timing.rb b/app/models/post_timing.rb index e3d94baf2a4..9d7b2f1d30f 100644 --- a/app/models/post_timing.rb +++ b/app/models/post_timing.rb @@ -176,6 +176,7 @@ SQL topic_time = max_time_per_post if topic_time > max_time_per_post TopicUser.update_last_read(current_user, topic_id, highest_seen, new_posts_read, topic_time, opts) + TopicGroup.update_last_read(current_user, topic_id, highest_seen) if total_changed > 0 current_user.reload diff --git a/app/models/topic_group.rb b/app/models/topic_group.rb new file mode 100644 index 00000000000..b69016a94e0 --- /dev/null +++ b/app/models/topic_group.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +class TopicGroup < ActiveRecord::Base + belongs_to :group + belongs_to :topic + + def self.update_last_read(user, topic_id, post_number) + updated_groups = update_read_count(user, topic_id, post_number) + create_topic_group(user, topic_id, post_number, updated_groups.map(&:group_id)) + TopicTrackingState.publish_read_indicator_on_read(topic_id, post_number, user.id) + end + + def self.new_message_update(user, topic_id, post_number) + updated_groups = update_read_count(user, topic_id, post_number) + create_topic_group(user, topic_id, post_number, updated_groups.map(&:group_id)) + TopicTrackingState.publish_read_indicator_on_write(topic_id, post_number, user.id) + end + + def self.update_read_count(user, topic_id, post_number) + update_query = <<~SQL + UPDATE topic_groups tg + SET + last_read_post_number = GREATEST(:post_number, tg.last_read_post_number), + updated_at = :now + FROM topic_allowed_groups tag + INNER JOIN group_users gu ON gu.group_id = tag.group_id + WHERE gu.user_id = :user_id + AND tag.topic_id = :topic_id + AND tg.topic_id = :topic_id + RETURNING + tg.group_id + SQL + + updated_groups = DB.query( + update_query, + user_id: user.id, topic_id: topic_id, post_number: post_number, now: DateTime.now + ) + end + + def self.create_topic_group(user, topic_id, post_number, updated_group_ids) + query = <<~SQL + INSERT INTO topic_groups (topic_id, group_id, last_read_post_number, created_at, updated_at) + SELECT tag.topic_id, tag.group_id, :post_number, :now, :now + FROM topic_allowed_groups tag + INNER JOIN group_users gu ON gu.group_id = tag.group_id + WHERE gu.user_id = :user_id + AND tag.topic_id = :topic_id + SQL + + query += 'AND NOT(tag.group_id IN (:already_updated_groups))' unless updated_group_ids.length.zero? + + DB.exec( + query, + user_id: user.id, topic_id: topic_id, post_number: post_number, now: DateTime.now, already_updated_groups: updated_group_ids + ) + end +end + +# == Schema Information +# +# Table name: topic_groups +# +# id :integer not null, primary key +# group_id :integer not null +# topic_id :integer not null +# last_read_post_number :integer default(0), not null +# +# Indexes +# +# index_topic_allowed_groups_on_group_id_and_topic_id (group_id,topic_id) UNIQUE +# 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..6f054921da7 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -128,14 +128,14 @@ 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 } @@ -341,4 +341,56 @@ SQL ) end end + + def self.publish_read_indicator_on_write(topic_id, last_read_post_number, user_id) + topic = Topic.includes(:allowed_groups).select(:highest_post_number, :archetype, :id).find_by(id: topic_id) + + if topic.private_message? + groups = read_allowed_groups_of(topic) + update_topic_list_read_indicator(topic, groups, topic.highest_post_number, user_id, false) + end + end + + def self.publish_read_indicator_on_read(topic_id, last_read_post_number, user_id) + topic = Topic.includes(:allowed_groups).select(:highest_post_number, :archetype, :id).find_by(id: topic_id) + + 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 + end + + def self.read_allowed_groups_of(topic) + topic.allowed_groups + .joins(:group_users) + .where(publish_read_state: true) + .select('ARRAY_AGG(group_users.user_id) AS members', :name, :id) + .group('groups.id') + end + + def self.update_topic_list_read_indicator(topic, groups, last_read_post_number, user_id, read_event) + return unless last_read_post_number == topic.highest_post_number + message = { topic_id: topic.id, show_indicator: read_event }.as_json + groups_to_update = [] + + groups.each do |group| + member = group.members.include?(user_id) + + member_writing = (!read_event && member) + non_member_reading = (read_event && !member) + next if non_member_reading || member_writing + + groups_to_update << group + end + + return if groups_to_update.empty? + MessageBus.publish("/private-messages/read-indicator/#{topic.id}", message, user_ids: groups_to_update.flat_map(&:members)) + end + + def self.trigger_post_read_count_update(post, groups) + return if groups.empty? + 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..be762bed44d 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#last_read_post_number is an attribute selected from a joined table. + # See TopicQuery#append_read_state for more information. + return false unless object.respond_to?(:last_read_post_number) + + object.last_read_post_number >= object.highest_post_number + 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 9a8cd888943..befb4d1b78f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -605,6 +605,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 e4aca2b7b29..bd25b6f28af 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/db/migrate/20190820192341_create_topic_group_table.rb b/db/migrate/20190820192341_create_topic_group_table.rb new file mode 100644 index 00000000000..76c2ab6acc6 --- /dev/null +++ b/db/migrate/20190820192341_create_topic_group_table.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreateTopicGroupTable < ActiveRecord::Migration[5.2] + def change + create_table :topic_groups do |t| + t.integer :group_id, null: false + t.integer :topic_id, null: false + t.integer :last_read_post_number, null: false, default: 0 + t.timestamps null: false + end + + add_index :topic_groups, %i[group_id topic_id], unique: true + end +end diff --git a/lib/post_jobs_enqueuer.rb b/lib/post_jobs_enqueuer.rb index bc302eda3ec..8659cba7d1d 100644 --- a/lib/post_jobs_enqueuer.rb +++ b/lib/post_jobs_enqueuer.rb @@ -22,6 +22,7 @@ class PostJobsEnqueuer if @topic.private_message? TopicTrackingState.publish_private_message(@topic, post: @post) + TopicGroup.new_message_update(@topic.last_poster, @topic.id, @post.post_number) end end diff --git a/lib/svg_sprite/svg_sprite.rb b/lib/svg_sprite/svg_sprite.rb index 11814d4342e..237cefd4179 100644 --- a/lib/svg_sprite/svg_sprite.rb +++ b/lib/svg_sprite/svg_sprite.rb @@ -28,6 +28,7 @@ module SvgSprite "bell-slash", "bold", "book", + "book-reader", "bookmark", "briefcase", "calendar-alt", diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 1b90719454f..cbb76449828 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,16 @@ 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 << "COALESCE(tg.last_read_post_number, 0) AS last_read_post_number" + + list + .joins("LEFT OUTER JOIN topic_groups tg ON topics.id = tg.topic_id AND tg.group_id = #{group_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..e3e2ab2724e 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -1020,6 +1020,24 @@ 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 the last read post number' do + topic_group = TopicGroup.create!( + topic: group_message, group: group, last_read_post_number: 10 + ) + + expect(listed_message.last_read_post_number).to eq(topic_group.last_read_post_number) + end + end end context "shared drafts" do diff --git a/spec/models/topic_group_spec.rb b/spec/models/topic_group_spec.rb new file mode 100644 index 00000000000..4eb5264595d --- /dev/null +++ b/spec/models/topic_group_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe TopicGroup do + describe '#update_last_read' do + fab!(:group) { Fabricate(:group) } + fab!(:user) { Fabricate(:user) } + + before do + @topic = Fabricate(:private_message_topic, allowed_groups: [group]) + group.add(user) + end + + it 'does nothing if the user is not a member of an allowed group' do + another_user = Fabricate(:user) + + described_class.update_last_read(another_user, @topic.id, @topic.highest_post_number) + created_topic_group = described_class.where(topic: @topic, group: group).exists? + + expect(created_topic_group).to eq(false) + end + + it 'creates a new record if the user is a member of an allowed group' do + described_class.update_last_read(user, @topic.id, @topic.highest_post_number) + created_topic_group = described_class.find_by(topic: @topic, group: group) + + expect(created_topic_group.last_read_post_number).to eq @topic.highest_post_number + end + + it 'does nothing if the topic does not have allowed groups' do + @topic.update!(allowed_groups: []) + + described_class.update_last_read(user, @topic.id, @topic.highest_post_number) + created_topic_group = described_class.where(topic: @topic, group: group).exists? + + expect(created_topic_group).to eq(false) + end + + it 'updates an existing record with a higher post number' do + described_class.create!(topic: @topic, group: group, last_read_post_number: @topic.highest_post_number - 1) + + described_class.update_last_read(user, @topic.id, @topic.highest_post_number) + created_topic_group = described_class.find_by(topic: @topic, group: group) + + expect(created_topic_group.last_read_post_number).to eq @topic.highest_post_number + end + + it 'does nothing if the user read post number is lower than the current one' do + highest_read_number = @topic.highest_post_number + 1 + described_class.create!(topic: @topic, group: group, last_read_post_number: highest_read_number) + + described_class.update_last_read(user, @topic.id, @topic.highest_post_number) + created_topic_group = described_class.find_by(topic: @topic, group: group) + + expect(created_topic_group.last_read_post_number).to eq highest_read_number + end + + it 'creates a new record if the list of allowed groups has changed' do + another_allowed_group = Fabricate(:group) + another_allowed_group.add(user) + @topic.allowed_groups << another_allowed_group + described_class.create!(topic: @topic, group: group, last_read_post_number: @topic.highest_post_number) + + described_class.update_last_read(user, @topic.id, @topic.highest_post_number) + created_topic_group = described_class.find_by(topic: @topic, group: another_allowed_group) + + expect(created_topic_group.last_read_post_number).to eq @topic.highest_post_number + end + + it 'Only updates the record that shares the same topic_id' do + new_post_number = 100 + topic2 = Fabricate(:private_message_topic, allowed_groups: [group], topic_allowed_users: []) + described_class.create!(topic: @topic, group: group, last_read_post_number: @topic.highest_post_number) + described_class.create!(topic: topic2, group: group, last_read_post_number: topic2.highest_post_number) + + described_class.update_last_read(user, @topic.id, new_post_number) + created_topic_group = described_class.find_by(topic: @topic, group: group) + created_topic_group2 = described_class.find_by(topic: topic2, group: group) + + expect(created_topic_group.last_read_post_number).to eq new_post_number + expect(created_topic_group2.last_read_post_number).to eq topic2.highest_post_number + end + end +end diff --git a/spec/models/topic_tracking_state_spec.rb b/spec/models/topic_tracking_state_spec.rb index 261c3752d27..7a8c3c601eb 100644 --- a/spec/models/topic_tracking_state_spec.rb +++ b/spec/models/topic_tracking_state_spec.rb @@ -254,6 +254,70 @@ describe TopicTrackingState do end end + describe '#publish_read_private_message' do + fab!(:group) { Fabricate(:group) } + let(:read_topic_key) { "/private-messages/read-indicator/#{@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 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_indicator_on_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_indicator_on_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_indicator_on_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_indicator_on_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_indicator_on_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 diff --git a/vendor/assets/svg-icons/fontawesome/solid.svg b/vendor/assets/svg-icons/fontawesome/solid.svg index aa652f5d0c2..1a9ad774d9a 100644 --- a/vendor/assets/svg-icons/fontawesome/solid.svg +++ b/vendor/assets/svg-icons/fontawesome/solid.svg @@ -3744,4 +3744,8 @@ Yin Yang + + Book Reader + +