From 946f25098f910e29b6d611425c1a9b1a9105f238 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 2 May 2017 16:01:01 -0400 Subject: [PATCH] Refactor theme fields so they support custom theme defined vars This paves the way to allowing themes to specify uploads and so on. --- app/controllers/admin/themes_controller.rb | 2 +- app/models/remote_theme.rb | 7 ++-- app/models/theme.rb | 15 ++++---- app/models/theme_field.rb | 33 ++++++++++++++---- app/serializers/theme_serializer.rb | 4 +-- ...501191912_add_upload_id_to_theme_fields.rb | 21 ++++++++++++ lib/stylesheet/importer.rb | 10 +++++- spec/components/stylesheet/manager_spec.rb | 18 +++++----- .../staff_action_logs_controller_spec.rb | 6 ++-- .../admin/themes_controller_spec.rb | 6 ++-- spec/models/remote_theme_spec.rb | 4 +-- spec/models/theme_field_spec.rb | 4 +-- spec/models/theme_spec.rb | 34 +++++++++++++------ spec/services/staff_action_logger_spec.rb | 8 ++--- 14 files changed, 120 insertions(+), 52 deletions(-) create mode 100644 db/migrate/20170501191912_add_upload_id_to_theme_fields.rb diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index 700df88b654..9d761948a22 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -194,7 +194,7 @@ class Admin::ThemesController < Admin::AdminController return unless fields = theme_params[:theme_fields] fields.each do |field| - @theme.set_field(field[:target], field[:name], field[:value]) + @theme.set_field(target: field[:target], name: field[:name], value: field[:value], type_id: field[:type_id]) end end diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb index d32a7705af0..a80ebf7a2ac 100644 --- a/app/models/remote_theme.rb +++ b/app/models/remote_theme.rb @@ -1,6 +1,9 @@ require_dependency 'git_importer' class RemoteTheme < ActiveRecord::Base + + ALLOWED_FIELDS = %w{scss embedded_scss head_tag header after_header body_tag footer} + has_one :theme def self.import_theme(url, user=Discourse.system_user) @@ -44,7 +47,7 @@ class RemoteTheme < ActiveRecord::Base end Theme.targets.keys.each do |target| - Theme::ALLOWED_FIELDS.each do |field| + ALLOWED_FIELDS.each do |field| lookup = if field == "scss" "#{target}.scss" @@ -55,7 +58,7 @@ class RemoteTheme < ActiveRecord::Base end value = importer["#{target}/#{lookup}"] - theme.set_field(target.to_sym, field, value) + theme.set_field(target: target.to_sym, name: field, value: value) end end diff --git a/app/models/theme.rb b/app/models/theme.rb index 1ea01b2f1ef..e41bbfab537 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -4,8 +4,6 @@ require_dependency 'stylesheet/manager' class Theme < ActiveRecord::Base - ALLOWED_FIELDS = %w{scss embedded_scss head_tag header after_header body_tag footer} - @cache = DistributedCache.new('theme') belongs_to :color_scheme @@ -206,13 +204,13 @@ class Theme < ActiveRecord::Base target = target.to_sym theme_ids = [self.id] + (included_themes.map(&:id) || []) - fields = ThemeField.where(target: [Theme.targets[target], Theme.targets[:common]]) + fields = ThemeField.where(target_id: [Theme.targets[target], Theme.targets[:common]]) .where(name: name.to_s) .includes(:theme) .joins("JOIN ( SELECT #{theme_ids.map.with_index{|id,idx| "#{id} AS theme_id, #{idx} AS sort_column"}.join(" UNION ALL SELECT ")} ) as X ON X.theme_id = theme_fields.theme_id") - .order('sort_column, target') + .order('sort_column, target_id') fields.each(&:ensure_baked!) fields end @@ -229,13 +227,16 @@ class Theme < ActiveRecord::Base @changed_colors ||= [] end - def set_field(target, name, value) + def set_field(target:, name:, value:, type: nil, type_id: nil) name = name.to_s target_id = Theme.targets[target.to_sym] raise "Unknown target #{target} passed to set field" unless target_id - field = theme_fields.find{|f| f.name==name && f.target == target_id} + type_id ||= type ? ThemeField.types[type.to_sym] : ThemeField.guess_type(name) + raise "Unknown type #{type} passed to set field" unless type_id + + field = theme_fields.find{|f| f.name==name && f.target_id == target_id && f.type_id == type_id} if field if value.blank? theme_fields.delete field.destroy @@ -246,7 +247,7 @@ class Theme < ActiveRecord::Base end end else - theme_fields.build(target: target_id, value: value, name: name) if value.present? + theme_fields.build(target_id: target_id, value: value, name: name, type_id: type_id) if value.present? end end diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 65b89db5143..f2e09d0b164 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -1,5 +1,17 @@ class ThemeField < ActiveRecord::Base + def self.types + @types ||= Enum.new(html: 0, + scss: 1, + theme_upload_var: 2, + theme_color_var: 3, + theme_var: 4) + end + + def self.theme_var_type_ids + @theme_var_type_ids ||= [2,3,4] + end + COMPILER_VERSION = 5 belongs_to :theme @@ -60,13 +72,20 @@ COMPILED [doc.to_s, errors&.join("\n")] end + def self.guess_type(name) + if html_fields.include?(name.to_s) + types[:html] + elsif scss_fields.include?(name.to_s) + types[:scss] + end + end def self.html_fields - %w(body_tag head_tag header footer after_header) + @html_fields ||= %w(body_tag head_tag header footer after_header) end def self.scss_fields - %w(scss embedded_scss) + @scss_fields ||= %w(scss embedded_scss) end @@ -105,7 +124,7 @@ COMPILED end def target_name - Theme.targets.invert[target].to_s + Theme.targets.invert[target_id].to_s end before_save do @@ -132,16 +151,18 @@ end # # id :integer not null, primary key # theme_id :integer not null -# target :integer not null -# name :string not null +# target_id :integer not null +# name :string(30) not null # value :text not null # value_baked :text # created_at :datetime # updated_at :datetime # compiler_version :integer default(0), not null # error :string +# upload_id :integer +# type_id :integer default(0), not null # # Indexes # -# index_theme_fields_on_theme_id_and_target_and_name (theme_id,target,name) UNIQUE +# theme_field_unique_index (theme_id,target_id,type_id,name) UNIQUE # diff --git a/app/serializers/theme_serializer.rb b/app/serializers/theme_serializer.rb index ab9af520a06..c66fab33b83 100644 --- a/app/serializers/theme_serializer.rb +++ b/app/serializers/theme_serializer.rb @@ -1,8 +1,8 @@ class ThemeFieldSerializer < ApplicationSerializer - attributes :name, :target, :value, :error + attributes :name, :target, :value, :error, :type_id def target - case object.target + case object.target_id when 0 then "common" when 1 then "desktop" when 2 then "mobile" diff --git a/db/migrate/20170501191912_add_upload_id_to_theme_fields.rb b/db/migrate/20170501191912_add_upload_id_to_theme_fields.rb new file mode 100644 index 00000000000..c519c0e7920 --- /dev/null +++ b/db/migrate/20170501191912_add_upload_id_to_theme_fields.rb @@ -0,0 +1,21 @@ +class AddUploadIdToThemeFields < ActiveRecord::Migration + def up + remove_index :theme_fields, [:theme_id, :target, :name] + rename_column :theme_fields, :target, :target_id + change_column :theme_fields, :name, :string, null: false, limit: 30 + + add_column :theme_fields, :upload_id, :integer + add_column :theme_fields, :type_id, :integer, null: false, default: 0 + + add_index :theme_fields, [:theme_id, :target_id, :type_id, :name], unique: true, name: 'theme_field_unique_index' + execute "UPDATE theme_fields SET type_id = 1 WHERE name IN ('scss', 'embedded_scss')" + end + + def down + execute 'drop index theme_field_unique_index' + rename_column :theme_fields, :target_id, :target + remove_column :theme_fields, :upload_id + remove_column :theme_fields, :type_id + add_index :theme_fields, [:theme_id, :target, :name], unique: true + end +end diff --git a/lib/stylesheet/importer.rb b/lib/stylesheet/importer.rb index fe4eebda749..72f679c047a 100644 --- a/lib/stylesheet/importer.rb +++ b/lib/stylesheet/importer.rb @@ -39,6 +39,11 @@ module Stylesheet colors.each do |n, hex| contents << "$#{n}: ##{hex} !default;\n" end + theme&.theme_fields&.where(type_id: ThemeField.theme_var_type_ids)&.each do |field| + escaped = field.value.gsub('"', "\\22") + escaped.gsub!("\n", "\\A") + contents << "$#{field.name}: unquote(\"#{escaped}\");\n" + end Import.new("theme_variable.scss", source: contents) end @@ -105,7 +110,10 @@ COMMENT end def theme - @theme ||= Theme.find(@theme_id) + unless @theme + @theme = (@theme_id && Theme.find(@theme_id)) || :nil + end + @theme == :nil ? nil : @theme end def apply_cdn(url) diff --git a/spec/components/stylesheet/manager_spec.rb b/spec/components/stylesheet/manager_spec.rb index 8634216cc3e..8cee6f289d9 100644 --- a/spec/components/stylesheet/manager_spec.rb +++ b/spec/components/stylesheet/manager_spec.rb @@ -20,10 +20,10 @@ describe Stylesheet::Manager do user_id: -1 ) - theme.set_field(:common, "scss", ".common{.scss{color: red;}}") - theme.set_field(:desktop, "scss", ".desktop{.scss{color: red;}}") - theme.set_field(:mobile, "scss", ".mobile{.scss{color: red;}}") - theme.set_field(:common, "embedded_scss", ".embedded{.scss{color: red;}}") + theme.set_field(target: :common, name: "scss", value: ".common{.scss{color: red;}}") + theme.set_field(target: :desktop, name: "scss", value: ".desktop{.scss{color: red;}}") + theme.set_field(target: :mobile, name: "scss", value: ".mobile{.scss{color: red;}}") + theme.set_field(target: :common, name: "embedded_scss", value: ".embedded{.scss{color: red;}}") theme.save! @@ -33,10 +33,10 @@ describe Stylesheet::Manager do user_id: -1, ) - child_theme.set_field(:common, "scss", ".child_common{.scss{color: red;}}") - child_theme.set_field(:desktop, "scss", ".child_desktop{.scss{color: red;}}") - child_theme.set_field(:mobile, "scss", ".child_mobile{.scss{color: red;}}") - child_theme.set_field(:common, "embedded_scss", ".child_embedded{.scss{color: red;}}") + child_theme.set_field(target: :common, name: "scss", value: ".child_common{.scss{color: red;}}") + child_theme.set_field(target: :desktop, name: "scss", value: ".child_desktop{.scss{color: red;}}") + child_theme.set_field(target: :mobile, name: "scss", value: ".child_mobile{.scss{color: red;}}") + child_theme.set_field(target: :common, name: "embedded_scss", value: ".child_embedded{.scss{color: red;}}") child_theme.save! theme.add_child_theme!(child_theme) @@ -55,7 +55,7 @@ describe Stylesheet::Manager do expect(css).to match(/\.desktop/) - child_theme.set_field(:desktop, :scss, ".nothing{color: green;}") + child_theme.set_field(target: :desktop, name: :scss, value: ".nothing{color: green;}") child_theme.save! new_link = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.key) diff --git a/spec/controllers/admin/staff_action_logs_controller_spec.rb b/spec/controllers/admin/staff_action_logs_controller_spec.rb index 6f8b64feca8..697a58ab4e6 100644 --- a/spec/controllers/admin/staff_action_logs_controller_spec.rb +++ b/spec/controllers/admin/staff_action_logs_controller_spec.rb @@ -19,12 +19,12 @@ describe Admin::StaffActionLogsController do context '.diff' do it 'can generate diffs for theme changes' do theme = Theme.new(user_id: -1, name: 'bob') - theme.set_field(:mobile, :scss, 'body {.up}') - theme.set_field(:common, :scss, 'omit-dupe') + theme.set_field(target: :mobile, name: :scss, value: 'body {.up}') + theme.set_field(target: :common, name: :scss, value: 'omit-dupe') original_json = ThemeSerializer.new(theme, root: false).to_json - theme.set_field(:mobile, :scss, 'body {.down}') + theme.set_field(target: :mobile, name: :scss, value: 'body {.down}') record = StaffActionLogger.new(Discourse.system_user) .log_theme_change(original_json, theme) diff --git a/spec/controllers/admin/themes_controller_spec.rb b/spec/controllers/admin/themes_controller_spec.rb index 4280c4d5caf..c8371ecd3e7 100644 --- a/spec/controllers/admin/themes_controller_spec.rb +++ b/spec/controllers/admin/themes_controller_spec.rb @@ -14,8 +14,8 @@ describe Admin::ThemesController do context ' .index' do it 'returns success' do theme = Theme.new(name: 'my name', user_id: -1) - theme.set_field(:common, :scss, '.body{color: black;}') - theme.set_field(:desktop, :after_header, 'test') + theme.set_field(target: :common, name: :scss, value: '.body{color: black;}') + theme.set_field(target: :desktop, name: :after_header, value: 'test') theme.remote_theme = RemoteTheme.new( remote_url: 'awesome.git', @@ -71,7 +71,7 @@ describe Admin::ThemesController do it 'updates a theme' do #focus theme = Theme.new(name: 'my name', user_id: -1) - theme.set_field(:common, :scss, '.body{color: black;}') + theme.set_field(target: :common, name: :scss, value: '.body{color: black;}') theme.save child_theme = Theme.create(name: 'my name', user_id: -1) diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index 16622e401e7..7c2f1b5f321 100644 --- a/spec/models/remote_theme_spec.rb +++ b/spec/models/remote_theme_spec.rb @@ -67,7 +67,7 @@ JSON expect(@theme.theme_fields.length).to eq(3) - mapped = Hash[*@theme.theme_fields.map{|f| ["#{f.target}-#{f.name}", f.value]}.flatten] + mapped = Hash[*@theme.theme_fields.map{|f| ["#{f.target_id}-#{f.name}", f.value]}.flatten] expect(mapped["0-header"]).to eq("I AM HEADER") expect(mapped["1-scss"]).to eq("body {color: red;}") @@ -101,7 +101,7 @@ JSON expect(scheme.name).to eq("Amazing") expect(scheme.colors.find_by(name: 'love').hex).to eq('eaeaea') - mapped = Hash[*@theme.theme_fields.map{|f| ["#{f.target}-#{f.name}", f.value]}.flatten] + mapped = Hash[*@theme.theme_fields.map{|f| ["#{f.target_id}-#{f.name}", f.value]}.flatten] expect(mapped["0-header"]).to eq("I AM UPDATED") expect(mapped["1-scss"]).to eq("body {color: red;}") diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index f120526e1c1..9dd0a099772 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -9,7 +9,7 @@ describe ThemeField do badJavaScript(; HTML - field = ThemeField.create!(theme_id: 1, target: 0, name: "header", value: html) + field = ThemeField.create!(theme_id: 1, target_id: 0, name: "header", value: html) expect(field.error).not_to eq(nil) field.value = "" field.save! @@ -18,7 +18,7 @@ HTML it "correctly generates errors for transpiled css" do css = "body {" - field = ThemeField.create!(theme_id: 1, target: 0, name: "scss", value: css) + field = ThemeField.create!(theme_id: 1, target_id: 0, name: "scss", value: css) field.reload expect(field.error).not_to eq(nil) field.value = "body {color: blue};" diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index e75bdb5d8a9..d2c19dec55c 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -26,9 +26,9 @@ describe Theme do it 'can support child themes' do child = Theme.new(name: '2', user_id: user.id) - child.set_field(:common, "header", "World") - child.set_field(:desktop, "header", "Desktop") - child.set_field(:mobile, "header", "Mobile") + child.set_field(target: :common, name: "header", value: "World") + child.set_field(target: :desktop, name: "header", value: "Desktop") + child.set_field(target: :mobile, name: "header", value: "Mobile") child.save! @@ -36,15 +36,15 @@ describe Theme do expect(Theme.lookup_field(child.key, "mobile", :header)).to eq("World\nMobile") - child.set_field(:common, "header", "Worldie") + child.set_field(target: :common, name: "header", value: "Worldie") child.save! expect(Theme.lookup_field(child.key, :mobile, :header)).to eq("Worldie\nMobile") parent = Theme.new(name: '1', user_id: user.id) - parent.set_field(:common, "header", "Common Parent") - parent.set_field(:mobile, "header", "Mobile Parent") + parent.set_field(target: :common, name: "header", value: "Common Parent") + parent.set_field(target: :mobile, name: "header", value: "Mobile Parent") parent.save! @@ -68,7 +68,7 @@ describe Theme do it 'should correct bad html in body_tag_baked and head_tag_baked' do theme = Theme.new(user_id: -1, name: "test") - theme.set_field(:common, "head_tag", "I am bold") + theme.set_field(target: :common, name: "head_tag", value: "I am bold") theme.save! expect(Theme.lookup_field(theme.key, :desktop, "head_tag")).to eq("I am bold") @@ -84,7 +84,7 @@ describe Theme do HTML theme = Theme.new(user_id: -1, name: "test") - theme.set_field(:common, "header", with_template) + theme.set_field(target: :common, name: "header", value: with_template) theme.save! baked = Theme.lookup_field(theme.key, :mobile, "header") @@ -96,7 +96,7 @@ HTML it 'should create body_tag_baked on demand if needed' do theme = Theme.new(user_id: -1, name: "test") - theme.set_field(:common, :body_tag, "test") + theme.set_field(target: :common, name: :body_tag, value: "test") theme.save ThemeField.update_all(value_baked: nil) @@ -106,7 +106,7 @@ HTML context "plugin api" do def transpile(html) - f = ThemeField.create!(target: Theme.targets[:mobile], theme_id: -1, name: "after_header", value: html) + f = ThemeField.create!(target_id: Theme.targets[:mobile], theme_id: -1, name: "after_header", value: html) f.value_baked end @@ -137,6 +137,20 @@ HTML end end + context 'theme vars' do + it 'can generate scss based off theme vars' do + theme = Theme.new(name: 'theme', user_id: -1) + theme.set_field(target: :common, name: :scss, value: 'body {color: $magic; content: quote($content)}') + theme.set_field(target: :common, name: :magic, value: 'red', type: :theme_var) + theme.set_field(target: :common, name: :content, value: 'Sam\'s Test', type: :theme_var) + theme.save + + scss,_map = Stylesheet::Compiler.compile('@import "theme_variables"; @import "desktop_theme"; ', "theme.scss", theme_id: theme.id) + expect(scss).to include("red") + expect(scss).to include('"Sam\'s Test"') + end + end + it 'correctly caches theme keys' do theme = Theme.create!(name: "bob", user_id: -1) diff --git a/spec/services/staff_action_logger_spec.rb b/spec/services/staff_action_logger_spec.rb index 2be67fbfbff..b6cdc0a85b4 100644 --- a/spec/services/staff_action_logger_spec.rb +++ b/spec/services/staff_action_logger_spec.rb @@ -153,14 +153,14 @@ describe StaffActionLogger do it "logs updated site customizations" do old_json = ThemeSerializer.new(theme, root:false).to_json - theme.set_field(:common, :scss, "body{margin: 10px;}") + theme.set_field(target: :common, name: :scss, value: "body{margin: 10px;}") log_record = logger.log_theme_change(old_json, theme) expect(log_record.previous_value).to be_present json = ::JSON.parse(log_record.new_value) - expect(json['theme_fields']).to eq([{"name" => "scss", "target" => "common", "value" => "body{margin: 10px;}"}]) + expect(json['theme_fields']).to eq([{"name" => "scss", "target" => "common", "value" => "body{margin: 10px;}", "type_id" => 1}]) end end @@ -171,14 +171,14 @@ describe StaffActionLogger do it "creates a new UserHistory record" do theme = Theme.new(name: 'Banana') - theme.set_field(:common, :scss, "body{margin: 10px;}") + theme.set_field(target: :common, name: :scss, value: "body{margin: 10px;}") log_record = logger.log_theme_destroy(theme) expect(log_record.previous_value).to be_present expect(log_record.new_value).to eq(nil) json = ::JSON.parse(log_record.previous_value) - expect(json['theme_fields']).to eq([{"name" => "scss", "target" => "common", "value" => "body{margin: 10px;}"}]) + expect(json['theme_fields']).to eq([{"name" => "scss", "target" => "common", "value" => "body{margin: 10px;}", "type_id" => 1}]) end end