From 5b844f53206961b8eaeeb2e6dbef0c3538b32180 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 23 Dec 2014 12:46:10 +1100 Subject: [PATCH] FEATURE: more than 1 site customization can be enabled at once FIX: more robust site customizations Rewrote site customization to use distributed cache and a much cleaner css delivery mechanism --- .../admin/models/site_customization.js | 2 +- .../site_customizations_controller.rb | 9 + app/models/site_customization.rb | 174 ++++++--------- config/routes.rb | 1 + ...20141222230707_amend_site_customization.rb | 5 + .../site_customizations_controller_spec.rb | 45 ++++ spec/models/site_customization_spec.rb | 209 +++++------------- 7 files changed, 183 insertions(+), 262 deletions(-) create mode 100644 app/controllers/site_customizations_controller.rb create mode 100644 db/migrate/20141222230707_amend_site_customization.rb create mode 100644 spec/controllers/site_customizations_controller_spec.rb diff --git a/app/assets/javascripts/admin/models/site_customization.js b/app/assets/javascripts/admin/models/site_customization.js index bc41b4936..94a056784 100644 --- a/app/assets/javascripts/admin/models/site_customization.js +++ b/app/assets/javascripts/admin/models/site_customization.js @@ -11,7 +11,7 @@ Discourse.SiteCustomization = Discourse.Model.extend({ description: function() { return "" + this.name + (this.enabled ? ' (*)' : ''); - }.property('selected', 'name'), + }.property('selected', 'name', 'enabled'), changed: function() { var self = this; diff --git a/app/controllers/site_customizations_controller.rb b/app/controllers/site_customizations_controller.rb new file mode 100644 index 000000000..248a4bacc --- /dev/null +++ b/app/controllers/site_customizations_controller.rb @@ -0,0 +1,9 @@ +class SiteCustomizationsController < ApplicationController + skip_before_filter :check_xhr, :redirect_to_login_if_required + + def show + expires_in 1.year, public: true + render text: SiteCustomization.stylesheet_contents(params[:key], params[:target] == "mobile" ? :mobile : :desktop), + content_type: "text/css" + end +end diff --git a/app/models/site_customization.rb b/app/models/site_customization.rb index 43f7752fc..2f6c4667a 100644 --- a/app/models/site_customization.rb +++ b/app/models/site_customization.rb @@ -1,14 +1,12 @@ require_dependency 'sass/discourse_sass_compiler' require_dependency 'sass/discourse_stylesheets' +require_dependency 'distributed_cache' class SiteCustomization < ActiveRecord::Base ENABLED_KEY = '7e202ef2-56d7-47d5-98d8-a9c8d15e57dd' - # placing this in uploads to ease deployment rules - CACHE_PATH = 'uploads/stylesheet-cache' - @lock = Mutex.new + @cache = DistributedCache.new('site_customization') before_create do - self.position ||= (SiteCustomization.maximum(:position) || 0) + 1 self.enabled ||= false self.key ||= SecureRandom.uuid true @@ -34,13 +32,9 @@ class SiteCustomization < ActiveRecord::Base end after_save do - File.delete(stylesheet_fullpath) if File.exists?(stylesheet_fullpath) && stylesheet_changed? - File.delete(stylesheet_fullpath(:mobile)) if File.exists?(stylesheet_fullpath(:mobile)) && mobile_stylesheet_changed? remove_from_cache! if stylesheet_changed? || mobile_stylesheet_changed? - ensure_stylesheets_on_disk! - # TODO: this is broken now because there's mobile stuff too - MessageBus.publish "/file-change/#{key}", stylesheet_hash + MessageBus.publish "/file-change/#{key}", SecureRandom.hex end MessageBus.publish "/header-change/#{key}", header if header_changed? MessageBus.publish "/footer-change/#{key}", footer if footer_changed? @@ -48,93 +42,83 @@ class SiteCustomization < ActiveRecord::Base end after_destroy do - File.delete(stylesheet_fullpath) if File.exists?(stylesheet_fullpath) - File.delete(stylesheet_fullpath(:mobile)) if File.exists?(stylesheet_fullpath(:mobile)) - self.remove_from_cache! + remove_from_cache! end def self.enabled_key ENABLED_KEY.dup << RailsMultisite::ConnectionManagement.current_db end - def self.enabled_style_key - @cache ||= {} - preview_style = @cache[enabled_key] - return if preview_style == :none - return preview_style if preview_style - - @lock.synchronize do - style = find_by(enabled: true) - if style - @cache[enabled_key] = style.key - else - @cache[enabled_key] = :none - nil - end - end + def self.enabled_stylesheet_contents(target=:desktop) + @cache["enabled_stylesheet_#{target}"] ||= where(enabled: true) + .order(:name) + .pluck(target == :desktop ? :stylesheet_baked : :mobile_stylesheet_baked) + .compact + .join("\n") end - def self.custom_stylesheet(preview_style, target=:desktop) - preview_style ||= enabled_style_key - style = lookup_style(preview_style) - style.stylesheet_link_tag(target).html_safe if style - end - - def self.custom_header(preview_style, target=:desktop) - preview_style ||= enabled_style_key - style = lookup_style(preview_style) - if style && ((target != :mobile && style.header) || (target == :mobile && style.mobile_header)) - target == :mobile ? style.mobile_header.html_safe : style.header.html_safe + def self.stylesheet_contents(key, target) + if key == ENABLED_KEY + enabled_stylesheet_contents(target) else - "" + where(key: key) + .pluck(target == :mobile ? :mobile_stylesheet_baked : :stylesheet_baked) + .first end end - def self.custom_footer(preview_style, target=:dekstop) - preview_style ||= enabled_style_key - style = lookup_style(preview_style) - if style && ((target != :mobile && style.footer) || (target == :mobile && style.mobile_footer)) - target == :mobile ? style.mobile_footer.html_safe : style.footer.html_safe + def self.custom_stylesheet(preview_style=nil, target=:desktop) + preview_style ||= ENABLED_KEY + if preview_style == ENABLED_KEY + stylesheet_link_tag(ENABLED_KEY, target, enabled_stylesheet_contents(target)) else - "" + lookup_field(preview_style, target, :stylesheet_link_tag) end end - def self.lookup_style(key) + def self.custom_header(preview_style=nil, target=:desktop) + preview_style ||= ENABLED_KEY + lookup_field(preview_style, target, :header) + end + + def self.custom_footer(preview_style=nil, target=:dekstop) + preview_style ||= ENABLED_KEY + lookup_field(preview_style,target,:footer) + end + + def self.lookup_field(key, target, field) return if key.blank? - # cache is cross site resiliant cause key is secure random - @cache ||= {} - ensure_cache_listener - style = @cache[key] - return style if style + cache_key = key + target.to_s + field.to_s; - @lock.synchronize do - style = find_by(key: key) - style.ensure_stylesheets_on_disk! if style - @cache[key] = style - end - end + lookup = @cache[cache_key] + return lookup.html_safe if lookup - def self.ensure_cache_listener - unless @subscribed - klass = self - MessageBus.subscribe("/site_customization") do |msg| - message = msg.data - klass.remove_from_cache!(message["key"], false) + styles = + if key == ENABLED_KEY + order(:name).where(enabled:true).to_a + else + [find_by(key: key)].compact end - @subscribed = true - end + val = + if styles.present? + styles.map do |style| + lookup = target == :mobile ? "mobile_#{field}" : field + style.send(lookup) + end.compact.join("\n") + end + + (@cache[cache_key] = val || "").html_safe end def self.remove_from_cache!(key, broadcast = true) MessageBus.publish('/site_customization', key: key) if broadcast - if @cache - @lock.synchronize do - @cache[key] = nil - end - end + clear_cache! + end + + def self.clear_cache! + @cache.clear end def remove_from_cache! @@ -142,53 +126,25 @@ class SiteCustomization < ActiveRecord::Base self.class.remove_from_cache!(key) end - def stylesheet_hash(target=:desktop) - Digest::MD5.hexdigest( target == :mobile ? mobile_stylesheet : stylesheet ) - end - - def cache_fullpath - "#{Rails.root}/public/#{CACHE_PATH}" - end - - def ensure_stylesheets_on_disk! - [[:desktop, 'stylesheet_baked'], [:mobile, 'mobile_stylesheet_baked']].each do |target, baked_attr| - path = stylesheet_fullpath(target) - dir = cache_fullpath - FileUtils.mkdir_p(dir) - unless File.exists?(path) - File.open(path, "w") do |f| - f.puts self.send(baked_attr) - end - end - end - end - - def stylesheet_filename(target=:desktop) - target == :desktop ? "/#{self.key}.css" : "/#{target}_#{self.key}.css" - end - - def stylesheet_fullpath(target=:desktop) - "#{cache_fullpath}#{stylesheet_filename(target)}" + def mobile_stylesheet_link_tag + stylesheet_link_tag(:mobile) end def stylesheet_link_tag(target=:desktop) - return mobile_stylesheet_link_tag if target == :mobile - return "" unless stylesheet.present? - return @stylesheet_link_tag if @stylesheet_link_tag - ensure_stylesheets_on_disk! - @stylesheet_link_tag = link_css_tag "/#{CACHE_PATH}#{stylesheet_filename}?#{stylesheet_hash}" + content = target == :mobile ? mobile_stylesheet : stylesheet + SiteCustomization.stylesheet_link_tag(key, target, content) end - def mobile_stylesheet_link_tag - return "" unless mobile_stylesheet.present? - return @mobile_stylesheet_link_tag if @mobile_stylesheet_link_tag - ensure_stylesheets_on_disk! - @mobile_stylesheet_link_tag = link_css_tag "/#{CACHE_PATH}#{stylesheet_filename(:mobile)}?#{stylesheet_hash(:mobile)}" + def self.stylesheet_link_tag(key, target, content) + return "" unless content.present? + + hash = Digest::MD5.hexdigest(content) + link_css_tag "/site_customizations/#{key}.css?target=#{target}&v=#{hash}" end - def link_css_tag(href) + def self.link_css_tag(href) href = (GlobalSetting.cdn_url || "") + href - %Q{} + %Q{}.html_safe end end diff --git a/config/routes.rb b/config/routes.rb index f8daf518c..308bbf3cf 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -26,6 +26,7 @@ Discourse::Application.routes.draw do resources :about get "site" => "site#index" + get "site_customizations/:key" => "site_customizations#show" resources :forums get "srv/status" => "forums#status" diff --git a/db/migrate/20141222230707_amend_site_customization.rb b/db/migrate/20141222230707_amend_site_customization.rb new file mode 100644 index 000000000..70b3067f6 --- /dev/null +++ b/db/migrate/20141222230707_amend_site_customization.rb @@ -0,0 +1,5 @@ +class AmendSiteCustomization < ActiveRecord::Migration + def change + remove_column :site_customizations, :position + end +end diff --git a/spec/controllers/site_customizations_controller_spec.rb b/spec/controllers/site_customizations_controller_spec.rb new file mode 100644 index 000000000..3097ed0ca --- /dev/null +++ b/spec/controllers/site_customizations_controller_spec.rb @@ -0,0 +1,45 @@ +require 'spec_helper' + +describe SiteCustomizationsController do + + before do + SiteCustomization.clear_cache! + end + + it 'can deliver enabled css' do + SiteCustomization.create!(name: '1', + user_id: -1, + enabled: true, + mobile_stylesheet: '.a1{margin: 1px;}', + stylesheet: '.b1{margin: 1px;}' + ) + + SiteCustomization.create!(name: '2', + user_id: -1, + enabled: true, + mobile_stylesheet: '.a2{margin: 1px;}', + stylesheet: '.b2{margin: 1px;}' + ) + + get :show, key: SiteCustomization::ENABLED_KEY, format: :css, target: 'mobile' + response.body.should =~ /\.a1.*\.a2/m + + get :show, key: SiteCustomization::ENABLED_KEY, format: :css + response.body.should =~ /\.b1.*\.b2/m + end + + it 'can deliver specific css' do + c = SiteCustomization.create!(name: '1', + user_id: -1, + enabled: true, + mobile_stylesheet: '.a1{margin: 1px;}', + stylesheet: '.b1{margin: 1px;}' + ) + + get :show, key: c.key, format: :css, target: 'mobile' + response.body.should =~ /\.a1/ + + get :show, key: c.key, format: :css + response.body.should =~ /\.b1/ + end +end diff --git a/spec/models/site_customization_spec.rb b/spec/models/site_customization_spec.rb index ce967487f..af709d7e5 100644 --- a/spec/models/site_customization_spec.rb +++ b/spec/models/site_customization_spec.rb @@ -2,6 +2,10 @@ require 'spec_helper' describe SiteCustomization do + before do + SiteCustomization.clear_cache! + end + let :user do Fabricate(:user) end @@ -23,168 +27,69 @@ describe SiteCustomization do s.key.should_not == nil end - context 'caching' do + it 'can enable more than one style at once' do + c1 = SiteCustomization.create!(name: '2', user_id: user.id, header: 'World', + enabled: true, mobile_header: 'hi', footer: 'footer', + stylesheet: '.hello{.world {color: blue;}}') - context 'enabled style' do - before do - @customization = customization - end + SiteCustomization.create!(name: '1', user_id: user.id, header: 'Hello', + enabled: true, mobile_footer: 'mfooter', + mobile_stylesheet: '.hello{margin: 1px;}', + stylesheet: 'p{width: 1px;}' + ) - it 'finds no style when none enabled' do - SiteCustomization.enabled_style_key.should == nil - end + SiteCustomization.custom_header.should == "Hello\nWorld" + SiteCustomization.custom_header(nil, :mobile).should == "hi" + SiteCustomization.custom_footer(nil, :mobile).should == "mfooter" + SiteCustomization.custom_footer.should == "footer" + desktop_css = SiteCustomization.custom_stylesheet + desktop_css.should =~ Regexp.new("#{SiteCustomization::ENABLED_KEY}.css\\?target=desktop") - it 'finds the enabled style' do - @customization.enabled = true - @customization.save - SiteCustomization.enabled_style_key.should == @customization.key - end + mobile_css = SiteCustomization.custom_stylesheet(nil, :mobile) + mobile_css.should =~ Regexp.new("#{SiteCustomization::ENABLED_KEY}.css\\?target=mobile") - it 'finds no enabled style on other sites' do - @customization.enabled = true - @customization.save + SiteCustomization.enabled_stylesheet_contents.should =~ /\.hello \.world/ - RailsMultisite::ConnectionManagement.expects(:current_db).returns("foo").twice - # the mocking is tricky, lets remove the record so we can properly pretend we are on another db - # this bypasses the before / after stuff - SiteCustomization.exec_sql('delete from site_customizations') + # cache expiry + c1.enabled = false + c1.save - SiteCustomization.enabled_style_key.should == nil - end - end + SiteCustomization.custom_stylesheet.should_not == desktop_css + SiteCustomization.enabled_stylesheet_contents.should_not =~ /\.hello \.world/ + end - it 'ensure stylesheet is on disk on first fetch' do - c = customization - c.remove_from_cache! - File.delete(c.stylesheet_fullpath) - File.delete(c.stylesheet_fullpath(:mobile)) + it 'should be able to look up stylesheets by key' do + c = SiteCustomization.create!(name: '2', user_id: user.id, + enabled: true, + stylesheet: '.hello{.world {color: blue;}}', + mobile_stylesheet: '.world{.hello{color: black;}}') - SiteCustomization.custom_stylesheet(c.key) - File.exists?(c.stylesheet_fullpath).should == true - File.exists?(c.stylesheet_fullpath(:mobile)).should == true - end - - context '#custom_stylesheet' do - it 'should allow me to lookup a filename containing my preview stylesheet' do - SiteCustomization.custom_stylesheet(customization.key).should == - "" - end - - it "should return blank link tag for mobile if mobile_stylesheet is blank" do - SiteCustomization.custom_stylesheet(customization.key, :mobile).should == "" - end - - it "should return link tag for mobile custom stylesheet" do - SiteCustomization.custom_stylesheet(customization_with_mobile.key, :mobile).should == - "" - end - end - - context '#custom_header' do - it "returns empty string when there is no custom header" do - c = SiteCustomization.create!(customization_params.merge(header: '')) - SiteCustomization.custom_header(c.key).should == '' - end - - it "can return the custom header html" do - SiteCustomization.custom_header(customization.key).should == customization_params[:header] - end - - it "returns empty string for mobile header when there's no custom mobile header" do - SiteCustomization.custom_header(customization.key, :mobile).should == '' - end - - it "can return the custom mobile header html" do - SiteCustomization.custom_header(customization_with_mobile.key, :mobile).should == customization_with_mobile.mobile_header - end - end - - it 'should fix stylesheet files after changing the stylesheet' do - old_file = customization.stylesheet_fullpath - original = SiteCustomization.custom_stylesheet(customization.key) - - File.exists?(old_file).should == true - customization.stylesheet = "div { clear:both; }" - customization.save - - SiteCustomization.custom_stylesheet(customization.key).should_not == original - end - - it 'should fix mobile stylesheet files after changing the mobile_stylesheet' do - old_file = customization_with_mobile.stylesheet_fullpath(:mobile) - original = SiteCustomization.custom_stylesheet(customization_with_mobile.key, :mobile) - - File.exists?(old_file).should == true - customization_with_mobile.mobile_stylesheet = "div { clear:both; }" - customization_with_mobile.save - - SiteCustomization.custom_stylesheet(customization_with_mobile.key).should_not == original - end - - it 'should delete old stylesheet files after deleting' do - old_file = customization.stylesheet_fullpath - customization.ensure_stylesheets_on_disk! - customization.destroy - File.exists?(old_file).should == false - end - - it 'should delete old mobile stylesheet files after deleting' do - old_file = customization_with_mobile.stylesheet_fullpath(:mobile) - customization_with_mobile.ensure_stylesheets_on_disk! - customization_with_mobile.destroy - File.exists?(old_file).should == false - end - - it 'should nuke old revs out of the cache' do - old_style = SiteCustomization.custom_stylesheet(customization.key) - - customization.stylesheet = "hello worldz" - customization.save - SiteCustomization.custom_stylesheet(customization.key).should_not == old_style - end - - it 'should nuke old revs out of the cache for mobile too' do - old_style = SiteCustomization.custom_stylesheet(customization_with_mobile.key) - - customization_with_mobile.mobile_stylesheet = "hello worldz" - customization_with_mobile.save - SiteCustomization.custom_stylesheet(customization.key, :mobile).should_not == old_style - end - - - it 'should compile scss' do - c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: '$black: #000; #a { color: $black; }', header: '') - s = c.stylesheet_baked.gsub(' ', '').gsub("\n", '') - (s.include?("#a{color:#000;}") || s.include?("#a{color:black;}")).should == true - end - - it 'should compile mobile scss' do - c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: '', header: '', mobile_stylesheet: '$black: #000; #a { color: $black; }', mobile_header: '') - s = c.mobile_stylesheet_baked.gsub(' ', '').gsub("\n", '') - (s.include?("#a{color:#000;}") || s.include?("#a{color:black;}")).should == true - end - - it 'should allow including discourse styles' do - c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: '@import "desktop";', mobile_stylesheet: '@import "mobile";') - c.stylesheet_baked.should_not =~ /Syntax error/ - c.stylesheet_baked.length.should > 1000 - c.mobile_stylesheet_baked.should_not =~ /Syntax error/ - c.mobile_stylesheet_baked.length.should > 1000 - end - - it 'should provide an awesome error on failure' do - c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: "$black: #000; #a { color: $black; }\n\n\nboom", header: '') - c.stylesheet_baked.should =~ /Syntax error/ - c.mobile_stylesheet_baked.should_not be_present - end - - it 'should provide an awesome error on failure for mobile too' do - c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: '', header: '', mobile_stylesheet: "$black: #000; #a { color: $black; }\n\n\nboom", mobile_header: '') - c.mobile_stylesheet_baked.should =~ /Syntax error/ - c.stylesheet_baked.should_not be_present - end + SiteCustomization.custom_stylesheet(c.key, :mobile).should =~ Regexp.new("#{c.key}.css\\?target=mobile") + SiteCustomization.custom_stylesheet(c.key).should =~ Regexp.new("#{c.key}.css\\?target=desktop") end + + it 'should allow including discourse styles' do + c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: '@import "desktop";', mobile_stylesheet: '@import "mobile";') + c.stylesheet_baked.should_not =~ /Syntax error/ + c.stylesheet_baked.length.should be > 1000 + c.mobile_stylesheet_baked.should_not =~ /Syntax error/ + c.mobile_stylesheet_baked.length.should be > 1000 + end + + it 'should provide an awesome error on failure' do + c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: "$black: #000; #a { color: $black; }\n\n\nboom", header: '') + c.stylesheet_baked.should =~ /Syntax error/ + c.mobile_stylesheet_baked.should_not be_present + end + + it 'should provide an awesome error on failure for mobile too' do + c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: '', header: '', mobile_stylesheet: "$black: #000; #a { color: $black; }\n\n\nboom", mobile_header: '') + c.mobile_stylesheet_baked.should =~ /Syntax error/ + c.stylesheet_baked.should_not be_present + end + + end