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)}"