From c05aced0943c60d5ccee442ab3f0f46ab243ecd9 Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Tue, 11 Aug 2020 16:28:59 -0400 Subject: [PATCH] FIX: Invalidate cache when updating color scheme colors (#10417) --- app/controllers/stylesheets_controller.rb | 17 +++++++---- app/models/color_scheme.rb | 2 ++ lib/stylesheet/manager.rb | 28 ++++++++++++------ spec/components/stylesheet/manager_spec.rb | 34 +++++++++++++++++++--- 4 files changed, 62 insertions(+), 19 deletions(-) diff --git a/app/controllers/stylesheets_controller.rb b/app/controllers/stylesheets_controller.rb index b986de1e3c0..3391456cda7 100644 --- a/app/controllers/stylesheets_controller.rb +++ b/app/controllers/stylesheets_controller.rb @@ -29,14 +29,19 @@ class StylesheetsController < ApplicationController # TODO add theme # calling this method ensures we have a cache for said target # we hold of re-compilation till someone asks for asset - if target.include?("theme") - split_target, theme_id = target.split(/_(-?[0-9]+)/) - theme = Theme.find_by(id: theme_id) if theme_id.present? - else + if target.include?("color_definitions") split_target, color_scheme_id = target.split(/_(-?[0-9]+)/) - theme = Theme.find_by(color_scheme_id: color_scheme_id) + Stylesheet::Manager.color_scheme_stylesheet_link_tag(color_scheme_id) + else + if target.include?("theme") + split_target, theme_id = target.split(/_(-?[0-9]+)/) + theme = Theme.find_by(id: theme_id) if theme_id.present? + else + split_target, color_scheme_id = target.split(/_(-?[0-9]+)/) + theme = Theme.find_by(color_scheme_id: color_scheme_id) + end + Stylesheet::Manager.stylesheet_link_tag(split_target, nil, theme&.id) end - Stylesheet::Manager.stylesheet_link_tag(split_target, nil, theme&.id) end cache_time = request.env["HTTP_IF_MODIFIED_SINCE"] diff --git a/app/models/color_scheme.rb b/app/models/color_scheme.rb index 9c50aa716e9..eb09f1cddbf 100644 --- a/app/models/color_scheme.rb +++ b/app/models/color_scheme.rb @@ -271,6 +271,8 @@ class ColorScheme < ActiveRecord::Base def publish_discourse_stylesheet if self.id + Stylesheet::Manager.color_scheme_cache_clear(self) + theme_ids = Theme.where(color_scheme_id: self.id).pluck(:id) if theme_ids.present? Stylesheet::Manager.cache.clear diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb index 7f7ab020dcd..5416e381d69 100644 --- a/lib/stylesheet/manager.rb +++ b/lib/stylesheet/manager.rb @@ -106,19 +106,18 @@ class Stylesheet::Manager target = COLOR_SCHEME_STYLESHEET.to_sym current_hostname = Discourse.current_hostname - color_scheme_name = Slug.for(color_scheme.name) + color_scheme&.id.to_s - array_cache_key = "color_scheme_stylesheet_#{color_scheme_name}_#{current_hostname}" - stylesheets = cache[array_cache_key] + cache_key = color_scheme_cache_key(color_scheme) + stylesheets = cache[cache_key] return stylesheets if stylesheets.present? - stylesheet = { color_scheme_name: color_scheme_name } + stylesheet = { color_scheme_id: color_scheme&.id } builder = self.new(target, nil, color_scheme) - builder.compile unless File.exists?(builder.stylesheet_fullpath) + href = builder.stylesheet_path(current_hostname) stylesheet[:new_href] = href - cache[array_cache_key] = stylesheet.freeze + cache[cache_key] = stylesheet.freeze stylesheet end @@ -130,6 +129,16 @@ class Stylesheet::Manager %[].html_safe end + def self.color_scheme_cache_key(color_scheme) + color_scheme_name = Slug.for(color_scheme.name) + color_scheme&.id.to_s + "#{COLOR_SCHEME_STYLESHEET}_#{color_scheme_name}_#{Discourse.current_hostname}" + end + + def self.color_scheme_cache_clear(color_scheme) + cache_key = color_scheme_cache_key(color_scheme) + cache[cache_key] = nil + end + def self.precompile_css themes = Theme.where('user_selectable OR id = ?', SiteSetting.default_theme_id).pluck(:id, :name) themes << nil @@ -149,11 +158,11 @@ class Stylesheet::Manager cs_ids = Theme.where('user_selectable OR id = ?', SiteSetting.default_theme_id).pluck(:color_scheme_id) ColorScheme.where(id: cs_ids).each do |cs| target = COLOR_SCHEME_STYLESHEET - cache_key = "#{target}_#{cs.id}" STDERR.puts "precompile target: #{target} #{cs.name}" builder = self.new(target, nil, cs) builder.compile(force: true) + cache_key = color_scheme_cache_key(cs) cache[cache_key] = nil end @@ -300,7 +309,7 @@ class Stylesheet::Manager if is_theme? "#{@target}_#{theme.id}" elsif @color_scheme - "#{@target}_#{Slug.for(@color_scheme.name) + @color_scheme&.id.to_s}" + "#{@target}_#{Slug.for(@color_scheme.name)}_#{@color_scheme&.id.to_s}" else scheme_string = theme && theme.color_scheme ? "_#{theme.color_scheme.id}" : "" "#{@target}#{scheme_string}" @@ -384,7 +393,8 @@ class Stylesheet::Manager def color_scheme_digest - cs = theme&.color_scheme + cs = @color_scheme || theme&.color_scheme + category_updated = Category.where("uploaded_background_id IS NOT NULL").pluck(:updated_at).map(&:to_i).sum if cs || category_updated > 0 diff --git a/spec/components/stylesheet/manager_spec.rb b/spec/components/stylesheet/manager_spec.rb index c74dce1d988..0aa0a6c8208 100644 --- a/spec/components/stylesheet/manager_spec.rb +++ b/spec/components/stylesheet/manager_spec.rb @@ -163,6 +163,18 @@ describe Stylesheet::Manager do expect(digest3).to_not eq(digest2) expect(digest3).to_not eq(digest1) end + + it "updates digest when updating a color scheme" do + scheme = ColorScheme.create_from_base(name: "Neutral", base_scheme_id: "Neutral") + manager = Stylesheet::Manager.new(:color_definitions, nil, scheme) + digest1 = manager.color_scheme_digest + + ColorSchemeRevisor.revise(scheme, colors: [{ name: "primary", hex: "CC0000" }]) + + digest2 = manager.color_scheme_digest + + expect(digest1).to_not eq(digest2) + end end describe 'color_scheme_stylesheets' do @@ -193,12 +205,12 @@ describe Stylesheet::Manager do SiteSetting.default_theme_id = theme.id link = Stylesheet::Manager.color_scheme_stylesheet_link_tag() - expect(link).to include("/stylesheets/color_definitions_funky#{cs.id}_") + expect(link).to include("/stylesheets/color_definitions_funky_#{cs.id}_") end it "uses the correct scheme when colors are passed" do link = Stylesheet::Manager.color_scheme_stylesheet_link_tag(ColorScheme.first.id) - slug = Slug.for(ColorScheme.first.name) + ColorScheme.first.id.to_s + slug = Slug.for(ColorScheme.first.name) + "_" + ColorScheme.first.id.to_s expect(link).to include("/stylesheets/color_definitions_#{slug}_") end @@ -208,7 +220,21 @@ describe Stylesheet::Manager do SiteSetting.default_theme_id = theme.id link = Stylesheet::Manager.color_scheme_stylesheet_link_tag() - expect(link).to include("/stylesheets/color_definitions_funky-bunch#{cs.id}_") + expect(link).to include("/stylesheets/color_definitions_funky-bunch_#{cs.id}_") + end + + it "updates outputted colors when updating a color scheme" do + scheme = ColorScheme.create_from_base(name: "Neutral", base_scheme_id: "Neutral") + manager = Stylesheet::Manager.new(:color_definitions, nil, scheme) + stylesheet = manager.compile + + ColorSchemeRevisor.revise(scheme, colors: [{ name: "primary", hex: "CC0000" }]) + + manager2 = Stylesheet::Manager.new(:color_definitions, nil, scheme) + stylesheet2 = manager2.compile + + expect(stylesheet).not_to eq(stylesheet2) + expect(stylesheet2).to include("--primary: #c00;") end end @@ -237,7 +263,7 @@ describe Stylesheet::Manager do scheme2 = ColorScheme.create!(name: "scheme2") core_targets = [:desktop, :mobile, :desktop_rtl, :mobile_rtl, :admin] theme_targets = [:desktop_theme, :mobile_theme] - color_scheme_targets = ["color_definitions_scheme1", "color_definitions_scheme2"] + color_scheme_targets = ["color_definitions_scheme1_#{scheme1.id}", "color_definitions_scheme2_#{scheme2.id}"] Theme.update_all(user_selectable: false) user_theme = Fabricate(:theme, user_selectable: true, color_scheme: scheme1)