From cd54bece92b78b89c27204f9961e4135cd6044dd Mon Sep 17 00:00:00 2001 From: Mac Siri Date: Wed, 12 Dec 2018 16:41:31 -0500 Subject: [PATCH] Extract background jobs from Reaction (#1312) * Refactor Reaction * Update reaction_spec * Extract update_reactable to job class * Create ApplicationJob * Rename Engagement::UpdateReactableJob to ReactionCascadeJob * Rename ReactionCascadeJob to Reaction::UpdateRecordsJob * Refactor Reaction::UpdateRecordsJob * Remove unused method * Change module to class --- app/jobs/application_job.rb | 7 ++ app/jobs/reaction/update_records_job.rb | 46 ++++++++++ app/models/reaction.rb | 91 ++++++------------- spec/jobs/reaction/update_records_job_spec.rb | 5 + spec/models/reaction_spec.rb | 27 ++---- 5 files changed, 97 insertions(+), 79 deletions(-) create mode 100644 app/jobs/application_job.rb create mode 100644 app/jobs/reaction/update_records_job.rb create mode 100644 spec/jobs/reaction/update_records_job_spec.rb diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb new file mode 100644 index 000000000..d394c3d10 --- /dev/null +++ b/app/jobs/application_job.rb @@ -0,0 +1,7 @@ +class ApplicationJob < ActiveJob::Base + # Automatically retry jobs that encountered a deadlock + # retry_on ActiveRecord::Deadlocked + + # Most jobs are safe to ignore if the underlying records are no longer available + # discard_on ActiveJob::DeserializationError +end diff --git a/app/jobs/reaction/update_records_job.rb b/app/jobs/reaction/update_records_job.rb new file mode 100644 index 000000000..01d2e96c5 --- /dev/null +++ b/app/jobs/reaction/update_records_job.rb @@ -0,0 +1,46 @@ +class Reaction + class UpdateRecordsJob < ApplicationJob + queue_as :default + + attr_reader :reactable, :user + + def perform(reactable, user) + @reactable = reactable + @user = user + + reactable_type = reactable.class.name + + if reactable_type == "Article" + update_article + elsif reactable_type == "Comment" + update_comment + end + user.touch + occasionally_sync_reaction_counts + end + + private + + def update_article + cache_buster = CacheBuster.new + reactable.async_score_calc + reactable.index! + cache_buster.bust "/reactions?article_id=#{reactable.id}" + cache_buster.bust user.path + end + + def update_comment + cache_buster = CacheBuster.new + reactable.save + cache_buster.bust "/reactions?commentable_id=#{reactable.commentable_id}&commentable_type=#{reactable.commentable_type}" + cache_buster.bust user.path + end + + def occasionally_sync_reaction_counts + # Fixes any out-of-sync positive_reactions_count + if rand(6) == 1 || reactable.positive_reactions_count.negative? + reactable.update_column(:positive_reactions_count, reactable.reactions.where("points > ?", 0).size) + end + end + end +end diff --git a/app/models/reaction.rb b/app/models/reaction.rb index fdc81ed9f..97892d145 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -1,84 +1,60 @@ class Reaction < ApplicationRecord + CATEGORIES = %w(like readinglist unicorn thinking hands thumbsdown vomit).freeze + belongs_to :reactable, polymorphic: true + belongs_to :user + counter_culture :reactable, column_name: proc { |model| model.points.positive? ? "positive_reactions_count" : "reactions_count" } counter_culture :user - belongs_to :user - validates :category, inclusion: { in: %w(like thinking hands unicorn thumbsdown vomit readinglist) } + validates :category, inclusion: { in: CATEGORIES } validates :reactable_type, inclusion: { in: %w(Comment Article) } validates :status, inclusion: { in: %w(valid invalid confirmed) } validates :user_id, uniqueness: { scope: %i[reactable_id reactable_type category] } validate :permissions before_save :assign_points - after_save :update_reactable - after_save :touch_user - after_save :async_bust - before_destroy :update_reactable_without_delay - before_destroy :clean_up_before_destroy + after_save :update_reactable, :async_bust + before_destroy :update_reactable, :clean_up_before_destroy - scope :for_article, ->(id) { where(reactable_id: id, reactable_type: "Article") } - - def self.count_for_article(id) - Rails.cache.fetch("count_for_reactable-Article-#{id}", expires_in: 1.hour) do - reactions = Reaction.for_article(id) - - ["like", "readinglist", "unicorn"].map do |type| - { category: type, count: reactions.where(category: type).size } + class << self + def count_for_article(id) + Rails.cache.fetch("count_for_reactable-Article-#{id}", expires_in: 1.hour) do + reactions = Reaction.where(reactable_id: id, reactable_type: "Article") + %w(like readinglist unicorn).map do |type| + { category: type, count: reactions.where(category: type).size } + end end end - end - def self.for_display(user) - includes(:reactable). - where(reactable_type: "Article", user_id: user.id). - where("created_at > ?", 5.days.ago). - select("distinct on (reactable_id) *"). - take(15) - end + def for_display(user) + includes(:reactable). + where(reactable_type: "Article", user: user). + where("created_at > ?", 5.days.ago). + select("distinct on (reactable_id) *"). + take(15) + end - def self.cached_any_reactions_for?(reactable, user, category) - Rails.cache.fetch("any_reactions_for-#{reactable.class.name}-#{reactable.id}-#{user.updated_at}-#{category}", expires_in: 24.hours) do - Reaction. - where(reactable_id: reactable.id, reactable_type: reactable.class.name, user_id: user.id, category: category). - any? + def cached_any_reactions_for?(reactable, user, category) + cache_name = "any_reactions_for-#{reactable.class.name}-#{reactable.id}-#{user.updated_at}-#{category}" + Rails.cache.fetch(cache_name, expires_in: 24.hours) do + Reaction.where(reactable: reactable, user: user, category: category).any? + end end end private def update_reactable - if reactable_type == "Article" - update_article - elsif reactable_type == "Comment" && reactable - update_comment + if destroyed? + Reaction::UpdateRecordsJob.perform_now(reactable, user) + else + Reaction::UpdateRecordsJob.perform_later(reactable, user) end - occasionally_sync_reaction_counts end - handle_asynchronously :update_reactable - - def update_article - cache_buster = CacheBuster.new - reactable.async_score_calc - reactable.index! - cache_buster.bust "/reactions?article_id=#{reactable_id}" - cache_buster.bust user.path - end - - def update_comment - cache_buster = CacheBuster.new - reactable.save unless destroyed_by_association - cache_buster.bust "/reactions?commentable_id=#{reactable.commentable_id}&commentable_type=#{reactable.commentable_type}" - cache_buster.bust user.path - end - - def touch_user - user.touch - end - handle_asynchronously :touch_user def async_bust featured_articles = Article.where(featured: true).order("hotness_score DESC").limit(3).pluck(:id) @@ -119,13 +95,6 @@ class Reaction < ApplicationRecord end end - def occasionally_sync_reaction_counts - # Fixes any out-of-sync positive_reactions_count - if rand(6) == 1 || reactable.positive_reactions_count.negative? - reactable.update_column(:positive_reactions_count, reactable.reactions.where("points > ?", 0).size) - end - end - def negative_reaction_from_untrusted_user? negative? && !user.trusted end diff --git a/spec/jobs/reaction/update_records_job_spec.rb b/spec/jobs/reaction/update_records_job_spec.rb new file mode 100644 index 000000000..b1f1160a6 --- /dev/null +++ b/spec/jobs/reaction/update_records_job_spec.rb @@ -0,0 +1,5 @@ +require "rails_helper" + +RSpec.describe Reaction::UpdateRecordsJob, type: :job do + pending "add some examples to (or delete) #{__FILE__}" +end diff --git a/spec/models/reaction_spec.rb b/spec/models/reaction_spec.rb index 350046c5c..0b2411cc9 100644 --- a/spec/models/reaction_spec.rb +++ b/spec/models/reaction_spec.rb @@ -2,15 +2,10 @@ require "rails_helper" RSpec.describe Reaction, type: :model do - let(:user) { create(:user) } - let(:author) { create(:user) } - let(:article) { create(:article, user_id: author.id, featured: true) } - let(:comment) do - create(:comment, user_id: user.id, commentable_id: article.id, commentable_type: "Article") - end - let(:reaction) do - build(:reaction, reactable: comment, reactable_type: "Comment") - end + let(:user) { create(:user) } + let(:article) { create(:article, featured: true) } + let(:comment) { create(:comment, user: user, commentable: article) } + let(:reaction) { build(:reaction, reactable: comment) } describe "actual validation" do subject { Reaction.new(reactable: article, reactable_type: "Article", user: user) } @@ -18,7 +13,7 @@ RSpec.describe Reaction, type: :model do before { user.add_role(:trusted) } it { is_expected.to belong_to(:user) } - it { is_expected.to validate_inclusion_of(:category).in_array(%w(like thinking hands unicorn thumbsdown vomit readinglist)) } + it { is_expected.to validate_inclusion_of(:category).in_array(Reaction::CATEGORIES) } it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(%i[reactable_id reactable_type category]) } # Thumbsdown and Vomits test needed @@ -47,14 +42,10 @@ RSpec.describe Reaction, type: :model do end it "does not allow reaction on unpublished article" do - reaction = build( - :reaction, user_id: user.id, reactable_id: article.id, reactable_type: "Article" - ) + reaction = build(:reaction, user: user, reactable: article) expect(reaction).to be_valid article.update_column(:published, false) - reaction = build( - :reaction, user_id: user.id, reactable_id: article.id, reactable_type: "Article" - ) + reaction = build(:reaction, user: user, reactable: article) expect(reaction).not_to be_valid end @@ -88,8 +79,8 @@ RSpec.describe Reaction, type: :model do it "runs async jobs effectively" do u2 = create(:user) c2 = create(:comment, commentable_id: article.id) - create(:reaction, user_id: u2.id, reactable_id: c2.id, reactable_type: "Comment") - create(:reaction, user_id: u2.id, reactable_id: article.id, reactable_type: "Article") + create(:reaction, user: u2, reactable: c2) + create(:reaction, user: u2, reactable: article) expect(reaction).to be_valid end end