From 869f9b20a2dc62fcaa3c596b383f80a0b6b20c84 Mon Sep 17 00:00:00 2001 From: Kane York Date: Thu, 14 May 2020 15:42:00 -0700 Subject: [PATCH] PERF: Dematerialize topic_reply_count (#9769) * PERF: Dematerialize topic_reply_count It's only ever used for trust level promotions that run daily, or compared to 0. We don't need to track it on every post creation. * UX: Add symbol in TL3 report if topic reply count is capped * DEV: Drop user_stats.topic_reply_count column --- .../admin/models/tl3-requirements.js | 5 +++ .../admin/templates/user-tl3-requirements.hbs | 2 +- .../components/notification-consent-banner.js | 8 ++--- .../discourse/app/controllers/composer.js | 7 +--- app/jobs/scheduled/tl3_promotions.rb | 1 - app/models/trust_level3_requirements.rb | 6 +--- app/models/user_stat.rb | 34 +++++++++++++++---- app/serializers/current_user_serializer.rb | 11 ++---- .../20200513185052_drop_topic_reply_count.rb | 19 +++++++++++ lib/post_creator.rb | 5 --- lib/post_destroyer.rb | 5 --- lib/post_revisor.rb | 2 -- lib/promotion.rb | 2 +- lib/tasks/import.rake | 18 +--------- script/import_scripts/base.rb | 27 --------------- spec/components/post_creator_spec.rb | 22 ------------ spec/components/promotion_spec.rb | 13 +++++-- spec/jobs/tl3_promotions_spec.rb | 1 - spec/services/post_owner_changer_spec.rb | 4 --- 19 files changed, 73 insertions(+), 119 deletions(-) create mode 100644 db/post_migrate/20200513185052_drop_topic_reply_count.rb diff --git a/app/assets/javascripts/admin/models/tl3-requirements.js b/app/assets/javascripts/admin/models/tl3-requirements.js index 424aea4f58d..6331333de7c 100644 --- a/app/assets/javascripts/admin/models/tl3-requirements.js +++ b/app/assets/javascripts/admin/models/tl3-requirements.js @@ -12,6 +12,11 @@ export default EmberObject.extend({ return Math.round((minDaysVisited * 100) / timePeriod); }, + @discourseComputed("num_topics_replied_to", "min_topics_replied_to") + capped_topics_replied_to(numReplied, minReplied) { + return numReplied > minReplied; + }, + @discourseComputed( "days_visited", "min_days_visited", diff --git a/app/assets/javascripts/admin/templates/user-tl3-requirements.hbs b/app/assets/javascripts/admin/templates/user-tl3-requirements.hbs index 6a76b438a14..de71c0bfb9d 100644 --- a/app/assets/javascripts/admin/templates/user-tl3-requirements.hbs +++ b/app/assets/javascripts/admin/templates/user-tl3-requirements.hbs @@ -33,7 +33,7 @@ {{i18n "admin.user.tl3_requirements.topics_replied_to"}} {{check-icon model.tl3Requirements.met.topics_replied_to}} - {{model.tl3Requirements.num_topics_replied_to}} + {{#if model.tl3Requirements.capped_topics_replied_to}}≥ {{/if}}{{model.tl3Requirements.num_topics_replied_to}} {{model.tl3Requirements.min_topics_replied_to}} diff --git a/app/assets/javascripts/discourse/app/components/notification-consent-banner.js b/app/assets/javascripts/discourse/app/components/notification-consent-banner.js index 97ee9b0aa7d..0ef2aca78aa 100644 --- a/app/assets/javascripts/discourse/app/components/notification-consent-banner.js +++ b/app/assets/javascripts/discourse/app/components/notification-consent-banner.js @@ -20,21 +20,19 @@ export default DesktopNotificationConfig.extend({ "isNotSupported", "isEnabled", "bannerDismissed", - "currentUser.reply_count", - "currentUser.topic_count" + "currentUser.any_posts" ) showNotificationPromptBanner( isNotSupported, isEnabled, bannerDismissed, - replyCount, - topicCount + anyPosts ) { return ( this.siteSettings.push_notifications_prompt && !isNotSupported && this.currentUser && - replyCount + topicCount > 0 && + anyPosts && Notification.permission !== "denied" && Notification.permission !== "granted" && !isEnabled && diff --git a/app/assets/javascripts/discourse/app/controllers/composer.js b/app/assets/javascripts/discourse/app/controllers/composer.js index cd3d3576bbc..4890db311eb 100644 --- a/app/assets/javascripts/discourse/app/controllers/composer.js +++ b/app/assets/javascripts/discourse/app/controllers/composer.js @@ -732,12 +732,7 @@ export default Controller.extend({ this.close(); - const currentUser = this.currentUser; - if (composer.creatingTopic) { - currentUser.set("topic_count", currentUser.topic_count + 1); - } else { - currentUser.set("reply_count", currentUser.reply_count + 1); - } + this.currentUser.set("any_posts", true); const post = result.target; if (post && !staged) { diff --git a/app/jobs/scheduled/tl3_promotions.rb b/app/jobs/scheduled/tl3_promotions.rb index 7dcce77a05e..af1d6350ff4 100644 --- a/app/jobs/scheduled/tl3_promotions.rb +++ b/app/jobs/scheduled/tl3_promotions.rb @@ -34,7 +34,6 @@ module Jobs ).where.not(id: demoted_user_ids) .joins(:user_stat) .where("user_stats.days_visited >= ?", SiteSetting.tl3_requires_days_visited) - .where("user_stats.topic_reply_count >= ?", SiteSetting.tl3_requires_topics_replied_to) .where("user_stats.topics_entered >= ?", SiteSetting.tl3_requires_topics_viewed_all_time) .where("user_stats.posts_read_count >= ?", SiteSetting.tl3_requires_posts_read_all_time) .where("user_stats.likes_given >= ?", SiteSetting.tl3_requires_likes_given) diff --git a/app/models/trust_level3_requirements.rb b/app/models/trust_level3_requirements.rb index bf511a06476..3e35497d197 100644 --- a/app/models/trust_level3_requirements.rb +++ b/app/models/trust_level3_requirements.rb @@ -143,11 +143,7 @@ class TrustLevel3Requirements end def num_topics_replied_to - @user.posts - .public_posts - .where("posts.created_at > ? AND posts.post_number > 1", time_period.days.ago) - .select("distinct topic_id") - .count + @user.user_stat.calc_topic_reply_count!(min_topics_replied_to + 1, time_period.days.ago) end def min_topics_replied_to diff --git a/app/models/user_stat.rb b/app/models/user_stat.rb index cda2c1513ab..1eb168bc9be 100644 --- a/app/models/user_stat.rb +++ b/app/models/user_stat.rb @@ -4,6 +4,9 @@ class UserStat < ActiveRecord::Base belongs_to :user after_save :trigger_badges + # TODO(2021-05-13): Remove + self.ignored_columns = ["topic_reply_count"] + def self.ensure_consistency!(last_seen = 1.hour.ago) reset_bounce_scores update_distinct_badge_count @@ -151,12 +154,30 @@ class UserStat < ActiveRecord::Base end # topic_reply_count is a count of posts in other users' topics - def update_topic_reply_count - self.topic_reply_count = Topic - .joins("INNER JOIN posts ON topics.id = posts.topic_id AND topics.user_id <> posts.user_id") - .where("posts.deleted_at IS NULL AND posts.user_id = ?", self.user_id) - .distinct - .count + def calc_topic_reply_count!(max, start_time = nil) + sql = <<~SQL + SELECT COUNT(*) count + FROM ( + SELECT DISTINCT posts.topic_id + FROM posts + INNER JOIN topics ON topics.id = posts.topic_id + WHERE posts.user_id = ? + AND topics.user_id <> posts.user_id + AND posts.deleted_at IS NULL AND topics.deleted_at IS NULL + AND topics.archetype <> 'private_message' + #{start_time.nil? ? '' : 'AND posts.created_at > ?'} + LIMIT ? + ) as user_topic_replies + SQL + if start_time.nil? + DB.query_single(sql, self.user_id, max).first + else + DB.query_single(sql, self.user_id, start_time, max).first + end + end + + def any_posts + user.posts.exists? end MAX_TIME_READ_DIFF = 100 @@ -212,7 +233,6 @@ end # posts_read_count :integer default(0), not null # likes_given :integer default(0), not null # likes_received :integer default(0), not null -# topic_reply_count :integer default(0), not null # new_since :datetime not null # read_faq :datetime # first_post_created_at :datetime diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 9e238375cc4..290569a3b17 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -12,8 +12,7 @@ class CurrentUserSerializer < BasicUserSerializer :moderator?, :staff?, :title, - :reply_count, - :topic_count, + :any_posts, :enable_quoting, :enable_defer, :external_links_in_new_tab, @@ -66,12 +65,8 @@ class CurrentUserSerializer < BasicUserSerializer object.user_stat.read_faq? end - def topic_count - object.user_stat.topic_count - end - - def reply_count - object.user_stat.topic_reply_count + def any_posts + object.user_stat.any_posts end def hide_profile_and_presence diff --git a/db/post_migrate/20200513185052_drop_topic_reply_count.rb b/db/post_migrate/20200513185052_drop_topic_reply_count.rb new file mode 100644 index 00000000000..5bae3435c6b --- /dev/null +++ b/db/post_migrate/20200513185052_drop_topic_reply_count.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class DropTopicReplyCount < ActiveRecord::Migration[6.0] + DROPPED_COLUMNS ||= { + user_stats: %i{ + topic_reply_count + } + } + + def up + DROPPED_COLUMNS.each do |table, columns| + Migration::ColumnDropper.execute_drop(table, columns) + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 40856682ab8..8a37e4c9c64 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -539,11 +539,6 @@ class PostCreator @user.user_stat.topic_count += 1 if @post.is_first_post? end - # We don't count replies to your own topics - if !@opts[:import_mode] && @user.id != @topic.user_id - @user.user_stat.update_topic_reply_count - end - @user.user_stat.save! if !@topic.private_message? && @post.post_type != Post.types[:whisper] diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 578722543fb..9a5aece702f 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -350,11 +350,6 @@ class PostDestroyer author.user_stat.topic_count -= 1 if @post.is_first_post? end - # We don't count replies to your own topics in topic_reply_count - if @topic && author.id != @topic.user_id - author.user_stat.update_topic_reply_count - end - author.user_stat.save! if @post.created_at == author.last_posted_at diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index 97d516b0ed3..3c9ec277cdb 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -394,7 +394,6 @@ class PostRevisor prev_owner_user_stat.topic_count -= 1 if @post.is_first_post? prev_owner_user_stat.likes_received -= likes end - prev_owner_user_stat.update_topic_reply_count if @post.created_at == prev_owner.user_stat.first_post_created_at prev_owner_user_stat.first_post_created_at = prev_owner.posts.order('created_at ASC').first.try(:created_at) @@ -408,7 +407,6 @@ class PostRevisor new_owner_user_stat.topic_count += 1 if @post.is_first_post? new_owner_user_stat.likes_received += likes end - new_owner_user_stat.update_topic_reply_count new_owner_user_stat.save! end end diff --git a/lib/promotion.rb b/lib/promotion.rb index 3734ab8faae..d96b304d5ab 100644 --- a/lib/promotion.rb +++ b/lib/promotion.rb @@ -91,7 +91,7 @@ class Promotion return false if stat.days_visited < SiteSetting.tl2_requires_days_visited return false if stat.likes_received < SiteSetting.tl2_requires_likes_received return false if stat.likes_given < SiteSetting.tl2_requires_likes_given - return false if stat.topic_reply_count < SiteSetting.tl2_requires_topic_reply_count + return false if stat.calc_topic_reply_count!(SiteSetting.tl2_requires_topic_reply_count) < SiteSetting.tl2_requires_topic_reply_count true end diff --git a/lib/tasks/import.rake b/lib/tasks/import.rake index 14212bfb956..9084a6492a6 100644 --- a/lib/tasks/import.rake +++ b/lib/tasks/import.rake @@ -227,7 +227,6 @@ def update_user_stats # TODO: topic_count is counting all topics you replied in as if you started the topic. # TODO: post_count is counting first posts. - # TODO: topic_reply_count is never used, and is counting PMs here. DB.exec <<-SQL WITH X AS ( SELECT p.user_id @@ -235,20 +234,6 @@ def update_user_stats , COUNT(DISTINCT p.topic_id) topics , MIN(p.created_at) min_created_at , COALESCE(COUNT(DISTINCT DATE(p.created_at)), 0) days - , COALESCE(( - SELECT COUNT(*) - FROM topics - WHERE id IN ( - SELECT topic_id - FROM posts p2 - JOIN topics t2 ON t2.id = p2.topic_id - WHERE p2.deleted_at IS NULL - AND p2.post_type = 1 - AND NOT COALESCE(p2.hidden, 't') - AND p2.user_id <> t2.user_id - AND p2.user_id = p.user_id - ) - ), 0) topic_replies FROM posts p JOIN topics t ON t.id = p.topic_id WHERE p.deleted_at IS NULL @@ -268,7 +253,6 @@ def update_user_stats , topics_entered = X.topics , first_post_created_at = X.min_created_at , days_visited = X.days - , topic_reply_count = X.topic_replies FROM X WHERE user_stats.user_id = X.user_id AND (post_count <> X.posts @@ -278,7 +262,7 @@ def update_user_stats OR topics_entered <> X.topics OR COALESCE(first_post_created_at, '1970-01-01') <> X.min_created_at OR days_visited <> X.days - OR topic_reply_count <> X.topic_replies) + ) SQL end diff --git a/script/import_scripts/base.rb b/script/import_scripts/base.rb index 48bd67e516f..035c8a3e572 100644 --- a/script/import_scripts/base.rb +++ b/script/import_scripts/base.rb @@ -58,7 +58,6 @@ class ImportScripts::Base update_post_timings update_feature_topic_users update_category_featured_topics - update_topic_count_replies reset_topic_counters end @@ -690,19 +689,6 @@ class ImportScripts::Base end def update_user_stats - puts "", "Updating topic reply counts..." - - count = 0 - total = User.real.count - - User.real.find_each do |u| - u.create_user_stat if u.user_stat.nil? - us = u.user_stat - us.update_topic_reply_count - us.save - print_status(count += 1, total, get_start_time("user_stats")) - end - puts "", "Updating first_post_created_at..." DB.exec <<~SQL @@ -807,19 +793,6 @@ class ImportScripts::Base end end - def update_topic_count_replies - puts "", "Updating user topic reply counts" - - count = 0 - total = User.real.count - - User.real.find_each do |u| - u.user_stat.update_topic_reply_count - u.user_stat.save! - print_status(count += 1, total, get_start_time("topic_count_replies")) - end - end - def update_tl0 puts "", "Setting users with no posts to trust level 0" diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 7882e11c4b7..59369598049 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -253,28 +253,6 @@ describe PostCreator do creator_with_image_sizes.create end - it 'increases topic response counts' do - first_post = creator.create - - # ensure topic user is correct - topic_user = first_post.user.topic_users.find_by(topic_id: first_post.topic_id) - expect(topic_user).to be_present - expect(topic_user).to be_posted - expect(topic_user.last_read_post_number).to eq(first_post.post_number) - expect(topic_user.highest_seen_post_number).to eq(first_post.post_number) - - user2 = Fabricate(:coding_horror) - expect(user2.user_stat.topic_reply_count).to eq(0) - - expect(first_post.user.user_stat.reload.topic_reply_count).to eq(0) - - PostCreator.new(user2, topic_id: first_post.topic_id, raw: "this is my test post 123").create - - expect(first_post.user.user_stat.reload.topic_reply_count).to eq(0) - - expect(user2.user_stat.reload.topic_reply_count).to eq(1) - end - it 'sets topic excerpt if first post, but not second post' do first_post = creator.create topic = first_post.topic.reload diff --git a/spec/components/promotion_spec.rb b/spec/components/promotion_spec.rb index 8f29ef2725b..232715f9fe0 100644 --- a/spec/components/promotion_spec.rb +++ b/spec/components/promotion_spec.rb @@ -124,6 +124,8 @@ describe Promotion do context "that has done the requisite things" do before do + SiteSetting.tl2_requires_topic_reply_count = 3 + stat = user.user_stat stat.topics_entered = SiteSetting.tl2_requires_topics_entered stat.posts_read_count = SiteSetting.tl2_requires_read_posts @@ -131,7 +133,10 @@ describe Promotion do stat.days_visited = SiteSetting.tl2_requires_days_visited * 60 stat.likes_received = SiteSetting.tl2_requires_likes_received stat.likes_given = SiteSetting.tl2_requires_likes_given - stat.topic_reply_count = SiteSetting.tl2_requires_topic_reply_count + SiteSetting.tl2_requires_topic_reply_count.times do |_| + topic = Fabricate(:topic) + reply = Fabricate(:post, topic: topic, user: user, post_number: 2) + end @result = promotion.review end @@ -148,6 +153,7 @@ describe Promotion do context "when the account hasn't existed long enough" do it "does not promote the user" do user.created_at = 1.minute.ago + SiteSetting.tl2_requires_topic_reply_count = 3 stat = user.user_stat stat.topics_entered = SiteSetting.tl2_requires_topics_entered @@ -156,7 +162,10 @@ describe Promotion do stat.days_visited = SiteSetting.tl2_requires_days_visited * 60 stat.likes_received = SiteSetting.tl2_requires_likes_received stat.likes_given = SiteSetting.tl2_requires_likes_given - stat.topic_reply_count = SiteSetting.tl2_requires_topic_reply_count + SiteSetting.tl2_requires_topic_reply_count.times do |_| + topic = Fabricate(:topic) + reply = Fabricate(:post, topic: topic, user: user, post_number: 2) + end result = promotion.review expect(result).to eq(false) diff --git a/spec/jobs/tl3_promotions_spec.rb b/spec/jobs/tl3_promotions_spec.rb index f76007df55c..6bb23fca83a 100644 --- a/spec/jobs/tl3_promotions_spec.rb +++ b/spec/jobs/tl3_promotions_spec.rb @@ -8,7 +8,6 @@ describe Jobs::Tl3Promotions do user.create_user_stat if user.user_stat.nil? user.user_stat.update!( days_visited: 1000, - topic_reply_count: 1000, topics_entered: 1000, posts_read_count: 1000, likes_given: 1000, diff --git a/spec/services/post_owner_changer_spec.rb b/spec/services/post_owner_changer_spec.rb index 1cee8eeae94..d2cf53b6add 100644 --- a/spec/services/post_owner_changer_spec.rb +++ b/spec/services/post_owner_changer_spec.rb @@ -111,14 +111,12 @@ describe PostOwnerChanger do topic_count: 1, post_count: 1, first_post_created_at: p1.created_at, - topic_reply_count: 0 ) p2user.user_stat.update!( topic_count: 0, post_count: 1, first_post_created_at: p2.created_at, - topic_reply_count: 1 ) UserAction.create!(action_type: UserAction::NEW_TOPIC, user_id: p1user.id, acting_user_id: p1user.id, @@ -155,13 +153,11 @@ describe PostOwnerChanger do p1_user_stat = p1user.user_stat expect(p1_user_stat.first_post_created_at).to eq(nil) - expect(p1_user_stat.topic_reply_count).to eq(0) expect(p1_user_stat.likes_received).to eq(0) p2_user_stat = p2user.user_stat expect(p2_user_stat.first_post_created_at).to eq(nil) - expect(p2_user_stat.topic_reply_count).to eq(0) user_a_stat = user_a.user_stat