From d462abb5b5c624138ebf7a033221bd238cec4ee6 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Fri, 1 Apr 2022 10:54:55 -0400 Subject: [PATCH] Ensuring article's cache updates on org image update (#17052) There are three major things occurring in this pull request: 1. Renaming `Article#update_cached_user` to `Article#set_cached_entities`. 2. Reducing an organization's direct knowledge of which of the org's attributes an article caches. 3. Removing duplicate calls to update the article associated with the organization. For renaming to `Article#set_cached_entities`, the prior method implied we were updating the persistence layer. However, we were not making any save nor update calls. This rename should clarify intention. For reducing knowledge, the comments for `Article::ATTRIBUTES_CACHED_FOR_RELATED_ENTITY` should explain the details. And last, removing the duplicate calls; we had three methods that were attempting to build and update the `Article#cached_organization`'s value. Closes forem/forem#17041 --- app/models/article.rb | 18 ++++++++++++++-- app/models/organization.rb | 19 ++++------------- spec/models/organization_spec.rb | 35 ++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/app/models/article.rb b/app/models/article.rb index 7099a75b0..7a30dbf4c 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -11,6 +11,20 @@ class Article < ApplicationRecord include StringAttributeCleaner.nullify_blanks_for(:canonical_url, on: :before_save) DEFAULT_FEED_PAGINATION_WINDOW_SIZE = 50 + # When we cache an entity, either {User} or {Organization}, these are the names of the attributes + # we cache. + # + # @note I would prefer that this constant were in the {Article::CachedEntity} namespace, but it + # didn't work out well. Further, since Organization doesn't really know about + # Articles::CachedEntity, I'd rather it not "peek" into a class for which it has no + # knowledge. + # + # @note [@jeremyf] I have added the profile_image attribute, even though that's not one of the + # Articles::CachedEntity attributes. This is necessary to detect the change. + # + # @see Articles::CachedEntity caching strategy for entity attributes + ATTRIBUTES_CACHED_FOR_RELATED_ENTITY = %i[name profile_image profile_image_url slug username].freeze + attr_accessor :publish_under_org attr_writer :series @@ -149,7 +163,7 @@ class Article < ApplicationRecord before_validation :evaluate_markdown, :create_slug before_validation :remove_prohibited_unicode_characters before_validation :normalize_title - before_save :update_cached_user + before_save :set_cached_entities before_save :set_all_dates before_save :calculate_base_scores @@ -825,7 +839,7 @@ class Article < ApplicationRecord self.password = SecureRandom.hex(60) end - def update_cached_user + def set_cached_entities self.cached_organization = organization ? Articles::CachedEntity.from_object(organization) : nil self.cached_user = user ? Articles::CachedEntity.from_object(user) : nil end diff --git a/app/models/organization.rb b/app/models/organization.rb index ca4895632..f0f5b0432 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -13,17 +13,12 @@ class Organization < ApplicationRecord before_validation :check_for_slug_change before_validation :evaluate_markdown - # TODO: [@rhymes] revisit this callback and `update_articles_cached_organization` - # when we remove Elasticsearch - before_save :update_articles before_save :remove_at_from_usernames before_save :generate_secret after_save :bust_cache - # This callback will eventually invoke Article.update_cached_user to update the organization.name - # only when it has been changed, thus invoking the trigger on Article.reading_list_document - after_update_commit :update_articles_cached_organization + after_update_commit :conditionally_update_articles after_destroy_commit :bust_cache has_many :articles, dependent: :nullify @@ -141,16 +136,10 @@ class Organization < ApplicationRecord self.slug = slug&.downcase end - def update_articles - return unless saved_change_to_slug || saved_change_to_name || saved_change_to_profile_image + def conditionally_update_articles + return unless Article::ATTRIBUTES_CACHED_FOR_RELATED_ENTITY.detect { |attr| saved_change_to_attribute?(attr) } - articles.update(cached_organization: Articles::CachedEntity.from_object(self)) - end - - def update_articles_cached_organization - return unless saved_change_to_attribute?(:name) - - articles.update(cached_organization: Articles::CachedEntity.from_object(self)) + articles.each(&:save) end def bust_cache diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 96dd85dbe..e105b3d42 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -249,6 +249,41 @@ RSpec.describe Organization, type: :model do expect(article.path).to include(new_slug) end + it "updates article's cached_organization profile image", :aggregate_failures do + # What is going on with this spec? Follow the comments. + # + # tl;dr - verifying fix for https://github.com/forem/forem/issues/17041 + + # Create an organization and article, verify the article has cached the initial profile + # image of the organization. + original_org = create(:organization, profile_image: File.open(Rails.root.join("app/assets/images/1.png"))) + article = create(:article, organization: original_org) + expect(article.cached_organization.profile_image_url).to eq(original_org.profile_image_url) + + # For good measure let's have two of these articles + create(:article, organization: original_org) + + # A foible of CarrierWave is that I can't re-upload for the same model in memory. I need to + # refind the record. #reload does not work either. (I lost sleep on this portion of the + # test) + organization = described_class.find(original_org.id) + + # Verify that the organization's image changes. See the above CarrierWave foible + expect do + organization.profile_image = File.open(Rails.root.join("app/assets/images/2.png")) + organization.save! + end.to change { organization.reload.profile_image_url } + + # I want to collect reloaded versions of the organization's articles so I can see their + # cached organization profile image + articles_profile_image_urls = organization.articles + .map { |art| art.reload.cached_organization.profile_image_url }.uniq + + # Because of the change `{ organization.reload... }` the organization's profile_image_url is + # the most recent change. + expect(articles_profile_image_urls).to eq([organization.profile_image_url]) + end + it "updates article cached_organizations" do new_slug = "slug_#{rand(10_000)}"