Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

UX: Update topics stats automatically (#17135) 

Updates automatically data on the stats section of the topic.

It will update automatically the following information: likes, replies and last reply (timestamp and user)
This commit is contained in:
Sérgio Saquetim 2022-06-27 18:21:05 -03:00 committed by GitHub
parent 31a0bf11f2
commit 5840fb5c62
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 438 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
app/assets/javascripts/discourse/app/controllers/topic.js
View File

@ -1700,6 +1700,30 @@ export default Controller.extend(bufferedProperty("model"), {
topic.set("message_archived", true);
break;
}
case "stats": {
let updateStream = false;
["last_posted_at", "like_count", "posts_count"].forEach(
(property) => {
const value = data[property];
if (typeof value !== "undefined") {
topic.set(property, value);
updateStream = true;
}
}
);
if (data["last_poster"]) {
topic.details.set("last_poster", data["last_poster"]);
updateStream = true;
}
if (updateStream) {
postStream
.triggerChangedTopicStats()
.then((firstPostId) => refresh({ id: firstPostId }));
}
break;
}
default: {
let callback = customPostMessageCallbacks[data.type];
if (callback) {

11
app/assets/javascripts/discourse/app/models/post-stream.js
View File

@ -872,6 +872,17 @@ export default RestModel.extend({
return resolved;
},
triggerChangedTopicStats() {
if (this.firstPostNotLoaded) {
return Promise.reject();
}
return Promise.resolve().then(() => {
const firstPost = this.posts.findBy("post_number", 1);
return firstPost.id;
});
},
postForPostNumber(postNumber) {
if (!this.hasPosts) {
return;

135
app/assets/javascripts/discourse/tests/acceptance/topic-test.js
View File

@ -3,6 +3,7 @@ import {
chromeTest,
count,
exists,
publishToMessageBus,
query,
queryAll,
selectText,
@ -12,6 +13,7 @@ import {
click,
currentURL,
fillIn,
settled,
triggerKeyEvent,
visit,
} from "@ember/test-helpers";
@ -651,3 +653,136 @@ acceptance("Navigating between topics", function (needs) {
assert.ok(currentURL().includes("/280"));
});
});
acceptance("Topic stats update automatically", function () {
test("Likes count updates automatically", async function (assert) {
await visit("/t/internationalization-localization/280");
const likesDisplay = query("#post_1 .topic-map .likes .number");
const oldLikes = likesDisplay.textContent;
const likesChangedFixture = {
id: 280,
type: "stats",
like_count: 999,
};
// simulate the topic like_count being changed
publishToMessageBus("/topic/280", likesChangedFixture);
await settled();
const newLikes = likesDisplay.textContent;
assert.notEqual(
oldLikes,
newLikes,
"it updates the like count on the topic stats"
);
assert.equal(
newLikes,
likesChangedFixture.like_count,
"it updates the like count with the expected value"
);
});
const postsChangedFixture = {
id: 280,
type: "stats",
posts_count: 999,
last_posted_at: "2022-06-20T21:01:45.844Z",
last_poster: {
id: 1,
username: "test",
name: "Mr. Tester",
avatar_template: "http://www.example.com/avatar/updated_avatar.png",
},
};
test("Replies count updates automatically", async function (assert) {
await visit("/t/internationalization-localization/280");
const repliesDisplay = query("#post_1 .topic-map .replies .number");
const oldReplies = repliesDisplay.textContent;
// simulate the topic posts_count being changed
publishToMessageBus("/topic/280", postsChangedFixture);
await settled();
const newLikes = repliesDisplay.textContent;
assert.notEqual(
oldReplies,
newLikes,
"it updates the replies count on the topic stats"
);
assert.equal(
newLikes,
postsChangedFixture.posts_count - 1, // replies = posts_count - 1
"it updates the replies count with the expected value"
);
});
test("Last replier avatar updates automatically", async function (assert) {
await visit("/t/internationalization-localization/280");
const avatarImg = query("#post_1 .topic-map .last-reply .avatar");
const oldAvatarTitle = avatarImg.title;
const oldAvatarSrc = avatarImg.src;
// simulate the topic posts_count being changed
publishToMessageBus("/topic/280", postsChangedFixture);
await settled();
const newAvatarTitle = avatarImg.title;
const newAvatarSrc = avatarImg.src;
assert.notEqual(
oldAvatarTitle,
newAvatarTitle,
"it updates the last poster avatar title on the topic stats"
);
assert.equal(
newAvatarTitle,
postsChangedFixture.last_poster.name,
"it updates the last poster avatar title with the expected value"
);
assert.notEqual(
oldAvatarSrc,
newAvatarSrc,
"it updates the last poster avatar src on the topic stats"
);
assert.equal(
newAvatarSrc,
postsChangedFixture.last_poster.avatar_template,
"it updates the last poster avatar src with the expected value"
);
});
test("Last replied at updates automatically", async function (assert) {
await visit("/t/internationalization-localization/280");
const lastRepliedAtDisplay = query(
"#post_1 .topic-map .last-reply .relative-date"
);
const oldTime = lastRepliedAtDisplay.dataset.time;
// simulate the topic posts_count being changed
publishToMessageBus("/topic/280", postsChangedFixture);
await settled();
const newTime = lastRepliedAtDisplay.dataset.time;
assert.notEqual(
oldTime,
newTime,
"it updates the last posted time on the topic stats"
);
assert.equal(
newTime,
new Date(postsChangedFixture.last_posted_at).getTime(),
"it updates the last posted time with the expected value"
);
});
});

11
app/models/post.rb
View File

@ -198,6 +198,8 @@ class Post < ActiveRecord::Base
# but message is safe to skip
return unless topic
skip_topic_stats = opts.delete(:skip_topic_stats)
message = {
id: id,
post_number: post_number,
@ -209,19 +211,14 @@ class Post < ActiveRecord::Base
}.merge(opts)
publish_message!("/topic/#{topic_id}", message)
Topic.publish_stats_to_clients!(topic.id, type) unless skip_topic_stats
end
def publish_message!(channel, message, opts = {})
return unless topic
if Topic.visible_post_types.include?(post_type)
if topic.private_message?
opts[:user_ids] = User.human_users.where("admin OR moderator").pluck(:id)
opts[:user_ids] |= topic.allowed_users.pluck(:id)
opts[:user_ids] |= topic.allowed_group_users.pluck(:id)
else
opts[:group_ids] = topic.secure_group_ids
end
opts.merge!(topic.secure_audience_publish_messages)
else
opts[:user_ids] = User.human_users
.where("admin OR moderator OR id = ?", user_id)

56
app/models/topic.rb
View File

@ -783,7 +783,7 @@ class Topic < ActiveRecord::Base
SQL
end
# If a post is deleted we have to update our highest post counters
# If a post is deleted we have to update our highest post counters and last post information
def self.reset_highest(topic_id)
archetype = Topic.where(id: topic_id).pluck_first(:archetype)
@ -818,7 +818,16 @@ class Topic < ActiveRecord::Base
deleted_at IS NULL AND
post_type <> 4
#{post_type}
)
),
last_post_user_id = COALESCE((
SELECT user_id FROM posts
WHERE topic_id = :topic_id AND
deleted_at IS NULL AND
post_type <> 4
#{post_type}
ORDER BY created_at desc
LIMIT 1
), last_post_user_id)
WHERE id = :topic_id
RETURNING highest_post_number
SQL
@ -1805,6 +1814,49 @@ class Topic < ActiveRecord::Base
self.allowed_groups.where(smtp_enabled: true).first
end
def secure_audience_publish_messages
target_audience = {}
if private_message?
target_audience[:user_ids] = User.human_users.where("admin OR moderator").pluck(:id)
target_audience[:user_ids] |= allowed_users.pluck(:id)
target_audience[:user_ids] |= allowed_group_users.pluck(:id)
else
target_audience[:group_ids] = secure_group_ids
end
target_audience
end
def self.publish_stats_to_clients!(topic_id, type, opts = {})
topic = Topic.find_by(id: topic_id)
return unless topic.present?
case type
when :liked, :unliked
stats = { like_count: topic.like_count }
when :created, :destroyed, :deleted, :recovered
stats = { posts_count: topic.posts_count,
last_posted_at: topic.last_posted_at.as_json,
last_poster: BasicUserSerializer.new(topic.last_poster, root: false).as_json }
else
stats = nil
end
if stats
secure_audience = topic.secure_audience_publish_messages
if secure_audience[:user_ids] != [] && secure_audience[:group_ids] != []
message = stats.merge({
id: topic_id,
updated_at: Time.now,
type: :stats,
})
MessageBus.publish("/topic/#{topic_id}", message, opts.merge(secure_audience))
end
end
end
private
def invite_to_private_message(invited_by, target_user, guardian)

2
lib/post_creator.rb
View File

@ -633,7 +633,7 @@ class PostCreator
def publish
return if @opts[:import_mode] || @post.post_number == 1
@post.publish_change_to_clients! :created
@post.publish_change_to_clients! :created, { skip_topic_stats: @post.post_number == 1 }
end
def extract_links

4
lib/post_destroyer.rb
View File

@ -98,6 +98,7 @@ class PostDestroyer
topic.update_column(:user_id, Discourse::SYSTEM_USER_ID) if !topic.user_id
topic.recover!(@user) if @post.is_first_post?
topic.update_statistics
Topic.publish_stats_to_clients!(topic.id, :recovered)
UserActionManager.post_created(@post)
DiscourseEvent.trigger(:post_recovered, @post, @opts, @user)
@ -135,7 +136,8 @@ class PostDestroyer
end
end
@post.publish_change_to_clients! :recovered
# skip also publishing topic stats because they weren't updated yet
@post.publish_change_to_clients! :recovered, { skip_topic_stats: true }
TopicTrackingState.publish_recover(@post.topic) if @post.topic && @post.is_first_post?
end

18
spec/lib/post_action_creator_spec.rb
View File

@ -67,12 +67,28 @@ describe PostActionCreator do
PostActionCreator.new(user, post, like_type_id).perform
end
message = messages.last.data
message = messages.find { |msg| msg.data[:type] === :liked }.data
expect(message).to be_present
expect(message[:type]).to eq(:liked)
expect(message[:likes_count]).to eq(1)
expect(message[:user_id]).to eq(user.id)
end
it 'notifies updated topic stats to subscribers' do
topic = Fabricate(:topic)
post = Fabricate(:post, topic: topic)
expect(post.reload.like_count).to eq(0)
messages = MessageBus.track_publish("/topic/#{topic.id}") do
PostActionCreator.new(user, post, like_type_id).perform
end
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
expect(stats_message.data[:like_count]).to eq(1)
end
it 'does not create an invalid post action' do
result = PostActionCreator.new(user, nil, like_type_id).perform
expect(result.failed?).to eq(true)

19
spec/lib/post_action_destroyer_spec.rb
View File

@ -25,11 +25,28 @@ describe PostActionDestroyer do
PostActionDestroyer.destroy(user, post, :like)
end
message = messages.last.data
message = messages.find { |msg| msg.data[:type] === :unliked }.data
expect(message).to be_present
expect(message[:type]).to eq(:unliked)
expect(message[:likes_count]).to eq(0)
expect(message[:user_id]).to eq(user.id)
end
it 'notifies updated topic stats to subscribers' do
topic = Fabricate(:topic)
post = Fabricate(:post, topic: topic)
PostActionCreator.new(user, post, PostActionType.types[:like]).perform
expect(post.reload.like_count).to eq(1)
messages = MessageBus.track_publish("/topic/#{topic.id}") do
PostActionDestroyer.destroy(user, post, :like)
end
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
expect(stats_message.data[:like_count]).to eq(0)
end
end
context 'post action doesn’t exist' do

26
spec/lib/post_creator_spec.rb
View File

@ -162,7 +162,7 @@ describe PostCreator do
channels = messages.map { |m| m.channel }.sort
# 2 for topic, one to notify of new topic another for tracking state
# 3 for topic, one to notify of new topic, one for topic stats and another for tracking state
expect(channels).to eq(
[
"/new",
@ -174,6 +174,7 @@ describe PostCreator do
"/latest",
"/topic/#{created_post.topic_id}",
"/topic/#{created_post.topic_id}",
"/topic/#{created_post.topic_id}",
"/user-drafts/#{admin.id}",
"/user-drafts/#{admin.id}",
"/user-drafts/#{admin.id}",
@ -208,6 +209,9 @@ describe PostCreator do
draft_count = messages.find { |m| m.channel == "/user-drafts/#{p.user_id}" }
expect(draft_count).not_to eq(nil)
topics_stats = messages.find { |m| m.channel == "/topic/#{p.topic.id}" && m.data[:type] == :stats }
expect(topics_stats).to eq(nil)
expect(messages.filter { |m| m.channel != "/distributed_hash" }.length).to eq(7)
end
@ -851,6 +855,26 @@ describe PostCreator do
expect(topic.word_count).to eq(6)
end
it "publishes updates to topic stats" do
reply_timestamp = 1.day.from_now.round
# tests if messages of type :stats are published and the relevant data is fetched from the topic
messages = MessageBus.track_publish("/topic/#{topic.id}") do
PostCreator.new(
evil_trout,
raw: 'other post in topic',
topic_id: topic.id,
created_at: reply_timestamp
).create
end
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
expect(stats_message.data[:posts_count]).to eq(2)
expect(stats_message.data[:last_posted_at]).to eq(reply_timestamp.as_json)
expect(stats_message.data[:last_poster]).to eq(BasicUserSerializer.new(evil_trout, root: false).as_json)
end
it "updates topic stats even when topic fails validation" do
topic.update_columns(title: 'below 15 chars')

63
spec/lib/post_destroyer_spec.rb
View File

@ -1103,4 +1103,67 @@ describe PostDestroyer do
expect { regular_post.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
end
describe "publishes messages to subscribers" do
# timestamps are rounded because postgres truncates the timestamp. that would cause the comparison if we compared
# these timestamps with the one read from the database
fab!(:first_post) { Fabricate(:post, created_at: 10.days.ago.round) }
fab!(:walter_white) { Fabricate(:walter_white) }
let!(:topic) { first_post.topic }
let!(:reply) { Fabricate(:post, topic: topic, created_at: 5.days.ago.round, user: coding_horror) }
let!(:expendable_reply) { Fabricate(:post, topic: topic, created_at: 2.days.ago.round, user: walter_white) }
it 'when a post is destroyed publishes updated topic stats' do
expect(topic.reload.posts_count).to eq(3)
messages = MessageBus.track_publish("/topic/#{topic.id}") do
PostDestroyer.new(moderator, expendable_reply, force_destroy: true).destroy
end
expect { expendable_reply.reload }.to raise_error(ActiveRecord::RecordNotFound)
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
expect(stats_message.data[:posts_count]).to eq(2)
expect(stats_message.data[:last_posted_at]).to eq(reply.created_at.as_json)
expect(stats_message.data[:last_poster]).to eq(BasicUserSerializer.new(reply.user, root: false).as_json)
end
it 'when a post is deleted publishes updated topic stats' do
expect(topic.reload.posts_count).to eq(3)
messages = MessageBus.track_publish("/topic/#{topic.id}") do
PostDestroyer.new(moderator, expendable_reply).destroy
end
expect(expendable_reply.reload.deleted_at).not_to eq(nil)
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
expect(stats_message.data[:posts_count]).to eq(2)
expect(stats_message.data[:last_posted_at]).to eq(reply.created_at.as_json)
expect(stats_message.data[:last_poster]).to eq(BasicUserSerializer.new(reply.user, root: false).as_json)
end
it 'when a post is recovered publishes update topic stats' do
expect(topic.reload.posts_count).to eq(3)
PostDestroyer.new(moderator, expendable_reply).destroy
expect(topic.reload.posts_count).to eq(2)
expendable_reply.reload
messages = MessageBus.track_publish("/topic/#{topic.id}") do
PostDestroyer.new(admin, expendable_reply).recover
end
expect(topic.reload.posts_count).to eq(3)
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
expect(stats_message.data[:posts_count]).to eq(3)
expect(stats_message.data[:last_posted_at]).to eq(expendable_reply.created_at.as_json)
expect(stats_message.data[:last_poster]).to eq(BasicUserSerializer.new(expendable_reply.user, root: false).as_json)
end
end
end

37
spec/models/post_spec.rb
View File

@ -1826,10 +1826,41 @@ describe Post do
version: post.version
}
MessageBus.expects(:publish).once.with("/topic/#{topic.id}", message, is_a(Hash)) do |_, _, options|
options[:user_ids].sort == [user1.id, user2.id, user3.id].sort
messages = MessageBus.track_publish("/topic/#{topic.id}") do
post.publish_change_to_clients!(:created)
end
post.publish_change_to_clients!(:created)
created_message = messages.select { |msg| msg.data[:type] == :created }.first
expect(created_message).to be_present
expect(created_message.data).to eq(message)
expect(created_message.user_ids.sort).to eq([user1.id, user2.id, user3.id].sort)
stats_message = messages.select { |msg| msg.data[:type] == :created }.first
expect(stats_message).to be_present
expect(stats_message.user_ids.sort).to eq([user1.id, user2.id, user3.id].sort)
end
it 'also publishes topic stats' do
messages = MessageBus.track_publish("/topic/#{topic.id}") do
post.publish_change_to_clients!(:created)
end
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
end
it 'skips publishing topic stats when requested' do
messages = MessageBus.track_publish("/topic/#{topic.id}") do
post.publish_change_to_clients!(:anything, { skip_topic_stats: true })
end
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_blank
# ensure that :skip_topic_stats did not get merged with the message
other_message = messages.select { |msg| msg.data[:type] == :anything }.first
expect(other_message).to be_present
expect(other_message.data.key?(:skip_topic_stats)).to be_falsey
end
end

49
spec/models/topic_spec.rb
View File

@ -3051,4 +3051,53 @@ describe Topic do
expect(topic.reload.cannot_permanently_delete_reason(Fabricate(:admin))).to eq(nil)
end
end
describe "#publish_stats_to_clients!" do
fab!(:user1) { Fabricate(:user) }
fab!(:user2) { Fabricate(:user) }
fab!(:topic) { Fabricate(:topic, user: user1) }
fab!(:post1) { Fabricate(:post, topic: topic, user: user1) }
fab!(:post2) { Fabricate(:post, topic: topic, user: user2) }
fab!(:like1) { Fabricate(:like, post: post1, user: user2) }
it "it is triggered when a post publishes a message of type :liked or :unliked" do
[:liked, :unliked].each do |action|
messages = MessageBus.track_publish("/topic/#{topic.id}") do
post1.publish_change_to_clients!(action)
end
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
expect(stats_message.data[:like_count]).to eq(topic.like_count)
end
end
it "it is triggered when a post publishes a message of type :created, :destroyed, :deleted, :recovered" do
freeze_time Date.today
[:created, :destroyed, :deleted, :recovered].each do |action|
messages = MessageBus.track_publish("/topic/#{topic.id}") do
post1.publish_change_to_clients!(action)
end
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_present
expect(stats_message.data[:posts_count]).to eq(topic.posts_count)
expect(stats_message.data[:last_posted_at]).to eq(topic.last_posted_at.as_json)
expect(stats_message.data[:last_poster]).to eq(BasicUserSerializer.new(topic.last_poster, root: false).as_json)
end
end
it "it is not triggered when a post publishes an unhandled kind of message" do
[:unhandled, :unknown, :dont_care].each do |action|
messages = MessageBus.track_publish("/topic/#{topic.id}") do
post1.publish_change_to_clients!(action)
end
stats_message = messages.select { |msg| msg.data[:type] == :stats }.first
expect(stats_message).to be_blank
end
end
end
end