From 95969eb9732f426fd626427a1d0ac45a6d4051c4 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 8 Nov 2021 12:50:08 -0500 Subject: [PATCH] Caching sum privileged reaction scores (#15299) * Caching sum privileged reaction scores This relates to work on improving the feed. Namely that in the current proposal in PR #15240 I'm not accounting for how privileged users have given feedback on an article. With this change, I will now have a cached value on the articles table that can be a proxy for how the privileged users have reacted. In addition there's a small refactor that moves a constant to the correct scoping object. Dependent on #15283. * Fixing method name to `exists?` Prior to this commit, I was using `exist?` which doesn't work for ActiveRecord::Relation objects. --- .../admin/privileged_reactions_controller.rb | 4 +--- app/models/article.rb | 1 + app/models/reaction.rb | 7 +++++++ ...each_articles_privileged_user_point_count.rb | 17 +++++++++++++++++ ...articles_privileged_user_point_count_spec.rb | 17 +++++++++++++++++ spec/models/article_spec.rb | 8 ++++++++ 6 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 lib/data_update_scripts/20211104200856_update_each_articles_privileged_user_point_count.rb create mode 100644 spec/lib/data_update_scripts/update_each_articles_privileged_user_point_count_spec.rb diff --git a/app/controllers/admin/privileged_reactions_controller.rb b/app/controllers/admin/privileged_reactions_controller.rb index b388942e5..1b7864db6 100644 --- a/app/controllers/admin/privileged_reactions_controller.rb +++ b/app/controllers/admin/privileged_reactions_controller.rb @@ -2,12 +2,10 @@ module Admin class PrivilegedReactionsController < Admin::ApplicationController layout "admin" - PRIVILEGED_REACTION_CATEGORIES = %i[thumbsup thumbsdown vomit].freeze - def index @q = Reaction .includes(:user, :reactable) - .where(category: PRIVILEGED_REACTION_CATEGORIES) + .privileged_category .order(created_at: :desc) .ransack(params[:q]) @privileged_reactions = @q.result.page(params[:page] || 1).per(25) diff --git a/app/models/article.rb b/app/models/article.rb index 1c1d8718f..7c43ed016 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -460,6 +460,7 @@ class Article < ApplicationRecord def update_score self.score = reactions.sum(:points) + Reaction.where(reactable_id: user_id, reactable_type: "User").sum(:points) update_columns(score: score, + privileged_users_reaction_points_sum: reactions.privileged_category.sum(:points), comment_score: comments.sum(:score), hotness_score: BlackBox.article_hotness_score(self), spaminess_rating: BlackBox.calculate_spaminess(self)) diff --git a/app/models/reaction.rb b/app/models/reaction.rb index 7113a1682..ad0fbd0eb 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -5,8 +5,14 @@ class Reaction < ApplicationRecord "thumbsdown" => -10.0 }.freeze + # The union of public and privileged categories CATEGORIES = %w[like readinglist unicorn thinking hands thumbsup thumbsdown vomit].freeze + + # These are the general category of reactions that anyone can choose PUBLIC_CATEGORIES = %w[like readinglist unicorn thinking hands].freeze + + # These are categories of reactions that administrators can select + PRIVILEGED_CATEGORIES = %w[thumbsup thumbsdown vomit].freeze REACTABLE_TYPES = %w[Comment Article User].freeze STATUSES = %w[valid invalid confirmed archived].freeze @@ -31,6 +37,7 @@ class Reaction < ApplicationRecord .or(comment_vomits.where(reactable_id: user.comment_ids)) .or(user_vomits.where(user_id: user.id)) } + scope :privileged_category, -> { where(category: PRIVILEGED_CATEGORIES) } validates :category, inclusion: { in: CATEGORIES } validates :reactable_type, inclusion: { in: REACTABLE_TYPES } diff --git a/lib/data_update_scripts/20211104200856_update_each_articles_privileged_user_point_count.rb b/lib/data_update_scripts/20211104200856_update_each_articles_privileged_user_point_count.rb new file mode 100644 index 000000000..11ca87292 --- /dev/null +++ b/lib/data_update_scripts/20211104200856_update_each_articles_privileged_user_point_count.rb @@ -0,0 +1,17 @@ +module DataUpdateScripts + class UpdateEachArticlesPrivilegedUserPointCount + def run + # This code is "idempotent" in that it's calculating and caching a value stored elsewhere. + Article.find_in_batches do |articles| + articles.each do |article| + # Given that we have a default of 0, don't update if we + # don't have any reactions. + next unless article.reactions.privileged_category.exists? + + article.update_column(:privileged_users_reaction_points_sum, + article.reactions.privileged_category.sum(:points)) + end + end + end + end +end diff --git a/spec/lib/data_update_scripts/update_each_articles_privileged_user_point_count_spec.rb b/spec/lib/data_update_scripts/update_each_articles_privileged_user_point_count_spec.rb new file mode 100644 index 000000000..377d23a39 --- /dev/null +++ b/spec/lib/data_update_scripts/update_each_articles_privileged_user_point_count_spec.rb @@ -0,0 +1,17 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20211104200856_update_each_articles_privileged_user_point_count.rb", +) + +describe DataUpdateScripts::UpdateEachArticlesPrivilegedUserPointCount do + it "updates articles scores" do + article = create(:article) + user = create(:user, :trusted) + reaction = create(:thumbsdown_reaction, user: user, reactable: article) + + # Short-circuiting callbacks + reaction.update_column(:points, 1_000_000) + + expect { described_class.new.run }.to change { article.reload.privileged_users_reaction_points_sum }.to(1_000_000) + end +end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 562259a00..b6c1dfeee 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -1331,6 +1331,14 @@ RSpec.describe Article, type: :model do article.update_score expect { article.update_score }.not_to change { article.reload.hotness_score } end + + it "caches the privileged score values" do + user = create(:user, :trusted) + + create(:thumbsdown_reaction, reactable: article, user: user) + + expect { article.update_score }.to change { article.reload.privileged_users_reaction_points_sum } + end end describe "#feed_source_url and canonical_url must be unique for published articles" do