From b1207289992c752d55abbbbf7215fa37a0a5b2b2 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 28 Nov 2019 16:19:01 +1100 Subject: [PATCH] FEATURE: Ability to add components to all themes (#8404) * FEATURE: Ability to add components to all themes This is the first and functional step from that topic https://dev.discourse.org/t/adding-a-theme-component-is-too-much-work/15398/16 The idea here is that when a new component is added, the user can easily assign it to all themes (parents). To achieve that, I needed to change a site-setting component to accept `setDefaultValues` action and `setDefaultValuesLabel` translated label. Also, I needed to add `allowAny` option to disable that for theme selector. I also refactored backend to accept both parent and child ids with one method to avoid duplication (Renamed `add_child_theme!` to more general `add_relative_theme!`) * FIX: Improvement after code review * FIX: Improvement after code review2 * FIX: use mapBy and filterBy directly --- .../theme-setting-relatives-selector.js.es6 | 26 ++++++++++++ .../admin-customize-themes-show.js.es6 | 35 ++++++++++++++-- .../admin/mixins/setting-component.js.es6 | 16 ++++++-- .../javascripts/admin/models/theme.js.es6 | 9 +++++ .../templates/components/site-setting.hbs | 5 ++- .../components/site-settings/compact-list.hbs | 2 +- .../admin/templates/customize-themes-show.hbs | 17 ++++++-- .../stylesheets/common/admin/customize.scss | 3 ++ app/controllers/admin/themes_controller.rb | 40 +++++++++++++------ app/models/theme.rb | 9 ++++- config/locales/client.en.yml | 2 + spec/components/guardian_spec.rb | 2 +- spec/components/stylesheet/importer_spec.rb | 2 +- spec/components/stylesheet/manager_spec.rb | 4 +- spec/components/svg_sprite/svg_sprite_spec.rb | 2 +- spec/models/theme_spec.rb | 23 ++++++----- spec/requests/admin/themes_controller_spec.rb | 10 +++++ spec/requests/application_controller_spec.rb | 2 +- spec/services/user_updater_spec.rb | 2 +- 19 files changed, 168 insertions(+), 43 deletions(-) create mode 100644 app/assets/javascripts/admin/components/theme-setting-relatives-selector.js.es6 diff --git a/app/assets/javascripts/admin/components/theme-setting-relatives-selector.js.es6 b/app/assets/javascripts/admin/components/theme-setting-relatives-selector.js.es6 new file mode 100644 index 00000000000..8ba638076c8 --- /dev/null +++ b/app/assets/javascripts/admin/components/theme-setting-relatives-selector.js.es6 @@ -0,0 +1,26 @@ +import Component from "@ember/component"; +import BufferedContent from "discourse/mixins/buffered-content"; +import SettingComponent from "admin/mixins/setting-component"; + +export default Component.extend(BufferedContent, SettingComponent, { + layoutName: "admin/templates/components/site-setting", + + _save() { + return this.model + .save({ [this.setting.setting]: this.convertNamesToIds() }) + .then(() => this.store.findAll("theme")); + }, + + convertNamesToIds() { + return this.get("buffered.value") + .split("|") + .filter(Boolean) + .map(themeName => { + if (themeName !== "") { + return this.setting.allThemes.find(theme => theme.name === themeName) + .id; + } + return themeName; + }); + } +}); diff --git a/app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6 b/app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6 index acf195e3a98..a7308865cea 100644 --- a/app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-customize-themes-show.js.es6 @@ -1,5 +1,11 @@ import { makeArray } from "discourse-common/lib/helpers"; -import { empty, notEmpty, match } from "@ember/object/computed"; +import { + empty, + filterBy, + match, + mapBy, + notEmpty +} from "@ember/object/computed"; import Controller from "@ember/controller"; import { default as discourseComputed } from "discourse-common/utils/decorators"; import { url } from "discourse/lib/computed"; @@ -15,6 +21,9 @@ export default Controller.extend({ previewUrl: url("model.id", "/admin/themes/%@/preview"), addButtonDisabled: empty("selectedChildThemeId"), editRouteName: "adminCustomizeThemes.edit", + parentThemesNames: mapBy("model.parentThemes", "name"), + availableParentThemes: filterBy("allThemes", "component", false), + availableThemesNames: mapBy("availableParentThemes", "name"), @discourseComputed("model.editedFields") editedFieldsFormatted() { @@ -50,6 +59,24 @@ export default Controller.extend({ } }, + @discourseComputed("model.parentThemes.[]") + relativesSelectorSettings() { + return Ember.Object.create({ + list_type: "compact", + type: "list", + preview: null, + anyValue: false, + setting: "parent_theme_ids", + label: I18n.t("admin.customize.theme.component_on_themes"), + choices: this.availableThemesNames, + default: this.parentThemesNames.join("|"), + value: this.parentThemesNames.join("|"), + defaultValues: this.availableThemesNames.join("|"), + allThemes: this.allThemes, + setDefaultValuesLabel: I18n.t("admin.customize.theme.add_all_themes") + }); + }, + @discourseComputed("allThemes", "model.component", "model") availableChildThemes(allThemes) { if (!this.get("model.component")) { @@ -241,7 +268,7 @@ export default Controller.extend({ addChildTheme() { let themeId = parseInt(this.selectedChildThemeId, 10); let theme = this.allThemes.findBy("id", themeId); - this.model.addChildTheme(theme); + this.model.addChildTheme(theme).then(() => this.store.findAll("theme")); }, removeUpload(upload) { @@ -258,7 +285,9 @@ export default Controller.extend({ }, removeChildTheme(theme) { - this.model.removeChildTheme(theme); + this.model + .removeChildTheme(theme) + .then(() => this.store.findAll("theme")); }, destroy() { diff --git a/app/assets/javascripts/admin/mixins/setting-component.js.es6 b/app/assets/javascripts/admin/mixins/setting-component.js.es6 index 6da9f010da8..9cec8194049 100644 --- a/app/assets/javascripts/admin/mixins/setting-component.js.es6 +++ b/app/assets/javascripts/admin/mixins/setting-component.js.es6 @@ -51,7 +51,6 @@ export default Mixin.create({ }); } } - let preview = setting.get("preview"); if (preview) { return new Handlebars.SafeString( @@ -67,9 +66,9 @@ export default Mixin.create({ return componentType.replace(/\_/g, "-"); }, - @discourseComputed("setting.setting") - settingName(setting) { - return setting.replace(/\_/g, " "); + @discourseComputed("setting.setting", "setting.label") + settingName(setting, label) { + return label || setting.replace(/\_/g, " "); }, @discourseComputed("type") @@ -91,6 +90,11 @@ export default Mixin.create({ return "site-settings/" + typeClass; }, + @discourseComputed("setting.anyValue") + allowAny(anyValue) { + return anyValue !== false; + }, + @discourseComputed("setting.default", "buffered.value") overridden(settingDefault, bufferedValue) { return settingDefault !== bufferedValue; @@ -209,6 +213,10 @@ export default Mixin.create({ toggleSecret() { this.toggleProperty("isSecret"); + }, + + setDefaultValues() { + this.set("buffered.value", this.get("setting.defaultValues")); } } }); diff --git a/app/assets/javascripts/admin/models/theme.js.es6 b/app/assets/javascripts/admin/models/theme.js.es6 index 94ccf5f9b0e..fb00497c257 100644 --- a/app/assets/javascripts/admin/models/theme.js.es6 +++ b/app/assets/javascripts/admin/models/theme.js.es6 @@ -269,6 +269,15 @@ const Theme = RestModel.extend({ return this.saveChanges("child_theme_ids"); }, + addParentTheme(theme) { + let parentThemes = this.parentThemes; + if (!parentThemes) { + parentThemes = []; + this.set("parentThemes", parentThemes); + } + parentThemes.addObject(theme); + }, + @discourseComputed("name", "default") description: function(name, isDefault) { if (isDefault) { diff --git a/app/assets/javascripts/admin/templates/components/site-setting.hbs b/app/assets/javascripts/admin/templates/components/site-setting.hbs index 980c30986cd..888a132a26c 100644 --- a/app/assets/javascripts/admin/templates/components/site-setting.hbs +++ b/app/assets/javascripts/admin/templates/components/site-setting.hbs @@ -1,8 +1,11 @@

{{unbound settingName}}

+ {{#if setting.defaultValues }} + {{setting.setDefaultValuesLabel}} + {{/if}}
- {{component componentName setting=setting value=buffered.value validationMessage=validationMessage preview=preview isSecret=isSecret}} + {{component componentName setting=setting value=buffered.value validationMessage=validationMessage preview=preview isSecret=isSecret allowAny=allowAny}}
{{#if dirty}}
diff --git a/app/assets/javascripts/admin/templates/components/site-settings/compact-list.hbs b/app/assets/javascripts/admin/templates/components/site-settings/compact-list.hbs index e741bea5ed1..551ca3d45d3 100644 --- a/app/assets/javascripts/admin/templates/components/site-settings/compact-list.hbs +++ b/app/assets/javascripts/admin/templates/components/site-settings/compact-list.hbs @@ -1,3 +1,3 @@ -{{list-setting settingValue=value choices=setting.choices settingName=setting.setting}} +{{list-setting settingValue=value choices=setting.choices settingName=setting.setting allowAny=allowAny}} {{setting-validation-message message=validationMessage}}
{{{unbound setting.description}}}
diff --git a/app/assets/javascripts/admin/templates/customize-themes-show.hbs b/app/assets/javascripts/admin/templates/customize-themes-show.hbs index 4b28949ad26..c90e73e7a52 100644 --- a/app/assets/javascripts/admin/templates/customize-themes-show.hbs +++ b/app/assets/javascripts/admin/templates/customize-themes-show.hbs @@ -59,16 +59,16 @@ {{#if model.remote_theme.remote_url}} {{#if sourceIsHttp}} - {{i18n "admin.customize.theme.source_url"}} {{d-icon "link"}} + {{i18n "admin.customize.theme.source_url"}}{{d-icon "link"}} {{else}}
{{model.remote_theme.remote_url}}
{{/if}} {{/if}} {{#if model.remote_theme.about_url}} - {{i18n "admin.customize.theme.about_theme"}} {{d-icon "link"}} + {{i18n "admin.customize.theme.about_theme"}}{{d-icon "link"}} {{/if}} {{#if model.remote_theme.license_url}} - {{i18n "admin.customize.theme.license"}} {{d-icon "link"}} + {{i18n "admin.customize.theme.license"}}{{d-icon "link"}} {{/if}} {{#if model.description}} @@ -156,6 +156,17 @@
{{/if}} + {{#if model.component }} +
+
{{i18n "admin.customize.theme.title"}}
+ {{#d-section class="form-horizontal theme settings"}} +
+ {{theme-setting-relatives-selector setting=relativesSelectorSettings model=model class="theme-setting"}} +
+ {{/d-section}} +
+ {{/if}} +
{{i18n "admin.customize.theme.css_html"}}
{{#if model.hasEditedFields}} diff --git a/app/assets/stylesheets/common/admin/customize.scss b/app/assets/stylesheets/common/admin/customize.scss index 4495cf2dede..d626ba2c8b6 100644 --- a/app/assets/stylesheets/common/admin/customize.scss +++ b/app/assets/stylesheets/common/admin/customize.scss @@ -150,6 +150,9 @@ a.license-url { display: inline-block; margin-right: 10px; + .d-icon { + margin-left: 5px; + } } .mini-title { diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index 43df7efca48..9db615e5199 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -176,19 +176,11 @@ class Admin::ThemesController < Admin::AdminController end if theme_params.key?(:child_theme_ids) - expected = theme_params[:child_theme_ids].map(&:to_i) + add_relative_themes!(:child, theme_params[:child_theme_ids]) + end - @theme.child_theme_relation.to_a.each do |child| - if expected.include?(child.child_theme_id) - expected.reject! { |id| id == child.child_theme_id } - else - child.destroy - end - end - - Theme.where(id: expected).each do |theme| - @theme.add_child_theme!(theme) - end + if theme_params.key?(:parent_theme_ids) + add_relative_themes!(:parent, theme_params[:parent_theme_ids]) end set_fields @@ -294,6 +286,26 @@ class Admin::ThemesController < Admin::AdminController private + def add_relative_themes!(kind, ids) + expected = ids.map(&:to_i) + + relation = kind == :child ? @theme.child_theme_relation : @theme.parent_theme_relation + + relation.to_a.each do |relative| + if kind == :child && expected.include?(relative.child_theme_id) + expected.reject! { |id| id == relative.child_theme_id } + elsif kind == :parent && expected.include?(relative.parent_theme_id) + expected.reject! { |id| id == relative.parent_theme_id } + else + relative.destroy + end + end + + Theme.where(id: expected).each do |theme| + @theme.add_relative_theme!(kind, theme) + end + end + def update_default_theme if theme_params.key?(:default) is_default = theme_params[:default].to_s == "true" @@ -310,6 +322,7 @@ class Admin::ThemesController < Admin::AdminController begin # deep munge is a train wreck, work around it for now params[:theme][:child_theme_ids] ||= [] if params[:theme].key?(:child_theme_ids) + params[:theme][:parent_theme_ids] ||= [] if params[:theme].key?(:parent_theme_ids) params.require(:theme).permit( :name, @@ -321,7 +334,8 @@ class Admin::ThemesController < Admin::AdminController settings: {}, translations: {}, theme_fields: [:name, :target, :value, :upload_id, :type_id], - child_theme_ids: [] + child_theme_ids: [], + parent_theme_ids: [] ) end end diff --git a/app/models/theme.rb b/app/models/theme.rb index 0597cda86fe..f0476e67a99 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -376,10 +376,15 @@ class Theme < ActiveRecord::Base fields.values end - def add_child_theme!(theme) - new_relation = child_theme_relation.new(child_theme_id: theme.id) + def add_relative_theme!(kind, theme) + new_relation = if kind == :child + child_theme_relation.new(child_theme_id: theme.id) + else + parent_theme_relation.new(parent_theme_id: theme.id) + end if new_relation.save child_themes.reload + parent_themes.reload save! Theme.clear_cache! else diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 87879ab05cd..477d7c86c93 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3590,6 +3590,7 @@ en: color_scheme_select: "Select colors to be used by theme" custom_sections: "Custom sections:" theme_components: "Theme Components" + add_all_themes: "Add all themes" convert: "Convert" convert_component_alert: "Are you sure you want to convert this component to theme? It will be removed as a component from %{relatives}." convert_component_tooltip: "Convert this component to theme" @@ -3622,6 +3623,7 @@ en: edit_css_html: "Edit CSS/HTML" edit_css_html_help: "You have not edited any CSS or HTML" delete_upload_confirm: "Delete this upload? (Theme CSS may stop working!)" + component_on_themes: "Include component on these themes" import_web_tip: "Repository containing theme" import_web_advanced: "Advanced..." import_file_tip: ".tar.gz, .zip, or .dcstyle.json file containing theme" diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 34a2eae70eb..46ef188bccd 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -2890,7 +2890,7 @@ describe Guardian do expect(user_guardian.allow_themes?([theme.id, theme2.id])).to eq(false) theme2.update!(user_selectable: false, component: true) - theme.add_child_theme!(theme2) + theme.add_relative_theme!(:child, theme2) expect(user_guardian.allow_themes?([theme.id, theme2.id])).to eq(true) expect(user_guardian.allow_themes?([theme2.id])).to eq(false) end diff --git a/spec/components/stylesheet/importer_spec.rb b/spec/components/stylesheet/importer_spec.rb index f143bd0b396..21c70032fcd 100644 --- a/spec/components/stylesheet/importer_spec.rb +++ b/spec/components/stylesheet/importer_spec.rb @@ -74,7 +74,7 @@ describe Stylesheet::Importer do t.component = true t.set_field(target: :extra_scss, name: "my_files/moremagic", value: child_scss) t.save! - theme.add_child_theme!(t) + theme.add_relative_theme!(:child, t) }} let(:importer) { described_class.new(theme: theme) } diff --git a/spec/components/stylesheet/manager_spec.rb b/spec/components/stylesheet/manager_spec.rb index 95ca08b6abb..9b782657e24 100644 --- a/spec/components/stylesheet/manager_spec.rb +++ b/spec/components/stylesheet/manager_spec.rb @@ -40,7 +40,7 @@ describe Stylesheet::Manager do child_theme.set_field(target: :common, name: "embedded_scss", value: ".child_embedded{.scss{color: red;}}") child_theme.save! - theme.add_child_theme!(child_theme) + theme.add_relative_theme!(:child, child_theme) old_link = Stylesheet::Manager.stylesheet_link_tag(:desktop_theme, 'all', theme.id) @@ -88,7 +88,7 @@ describe Stylesheet::Manager do it "can correctly account for settings in theme's components" do theme = Fabricate(:theme) child = Fabricate(:theme, component: true) - theme.add_child_theme!(child) + theme.add_relative_theme!(:child, child) child.set_field(target: :settings, name: :yaml, value: "childcolor: red") child.set_field(target: :common, name: :scss, value: "body {background-color: $childcolor}") diff --git a/spec/components/svg_sprite/svg_sprite_spec.rb b/spec/components/svg_sprite/svg_sprite_spec.rb index 2885699b4a4..f8acc0028b8 100644 --- a/spec/components/svg_sprite/svg_sprite_spec.rb +++ b/spec/components/svg_sprite/svg_sprite_spec.rb @@ -138,7 +138,7 @@ describe SvgSprite do theme.update(component: true) theme.save! parent_theme = Fabricate(:theme) - parent_theme.add_child_theme!(theme) + parent_theme.add_relative_theme!(:child, theme) expect(SvgSprite.all_icons([parent_theme.id])).to include("dragon") end diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index c7fa086adea..ae162773e37 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -53,12 +53,17 @@ describe Theme do parent.save! - parent.add_child_theme!(child) + parent.add_relative_theme!(:child, child) expect(Theme.lookup_field(parent.id, :mobile, "header")).to eq("Common Parent\nMobile Parent\nWorldie\nMobile") end + it 'can support parent themes' do + child.add_relative_theme!(:parent, theme) + expect(child.parent_themes).to eq([theme]) + end + it "can automatically disable for mismatching version" do expect(theme.supported?).to eq(true) theme.create_remote_theme!(remote_url: "", minimum_discourse_version: "99.99.99") @@ -74,7 +79,7 @@ describe Theme do end it '#transform_ids filters out disabled components' do - theme.add_child_theme!(child) + theme.add_relative_theme!(:child, child) expect(Theme.transform_ids([theme.id], extend: true)).to eq([theme.id, child.id]) child.update!(enabled: false) expect(Theme.transform_ids([theme.id], extend: true)).to eq([theme.id]) @@ -85,11 +90,11 @@ describe Theme do grandparent = Fabricate(:theme, user: user) expect do - child.add_child_theme!(grandchild) + child.add_relative_theme!(:child, grandchild) end.to raise_error(Discourse::InvalidParameters, I18n.t("themes.errors.no_multilevels_components")) expect do - grandparent.add_child_theme!(theme) + grandparent.add_relative_theme!(:child, theme) end.to raise_error(Discourse::InvalidParameters, I18n.t("themes.errors.no_multilevels_components")) end @@ -198,7 +203,7 @@ HTML describe "#switch_to_component!" do it "correctly converts a theme to component" do - theme.add_child_theme!(child) + theme.add_relative_theme!(:child, child) scheme = ColorScheme.create!(name: 'test') theme.update!(color_scheme_id: scheme.id, user_selectable: true) theme.set_default! @@ -216,7 +221,7 @@ HTML describe "#switch_to_theme!" do it "correctly converts a component to theme" do - theme.add_child_theme!(child) + theme.add_relative_theme!(:child, child) child.switch_to_theme! theme.reload @@ -236,8 +241,8 @@ HTML let!(:orphan4) { Fabricate(:theme, component: true) } before do - theme.add_child_theme!(child) - theme.add_child_theme!(child2) + theme.add_relative_theme!(:child, child) + theme.add_relative_theme!(:child, child2) end it "returns an empty array if no ids are passed" do @@ -575,7 +580,7 @@ HTML child.set_field(target: :settings, name: "yaml", value: "integer_setting: 54") child.save! - theme.add_child_theme!(child) + theme.add_relative_theme!(:child, child) json = cached_settings(theme.id) expect(json).to match(/\"boolean_setting\":false/) diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index 4be50292ecb..edba250cf77 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -321,6 +321,16 @@ describe Admin::ThemesController do expect(UserHistory.where(action: UserHistory.actions[:change_theme]).count).to eq(1) end + it 'updates a child theme' do + child_theme = Fabricate(:theme, component: true) + put "/admin/themes/#{child_theme.id}.json", params: { + theme: { + parent_theme_ids: [theme.id], + } + } + expect(child_theme.parent_themes).to eq([theme]) + end + it 'can update translations' do theme.set_field(target: :translations, name: :en, value: { en: { somegroup: { somestring: "defaultstring" } } }.deep_stringify_keys.to_yaml) theme.save! diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 372d1cf78f1..bfc4c440668 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -374,7 +374,7 @@ RSpec.describe ApplicationController do expect(controller.theme_ids).to eq([theme2.id]) theme2.update!(user_selectable: false, component: true) - theme.add_child_theme!(theme2) + theme.add_relative_theme!(:child, theme2) cookies['theme_ids'] = "#{theme.id},#{theme2.id}|#{user.user_option.theme_key_seq}" get "/" diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb index 52dc56b5ef0..c6a9194e7c0 100644 --- a/spec/services/user_updater_spec.rb +++ b/spec/services/user_updater_spec.rb @@ -225,7 +225,7 @@ describe UserUpdater do theme = Fabricate(:theme) child = Fabricate(:theme, component: true) - theme.add_child_theme!(child) + theme.add_relative_theme!(:child, child) theme.set_default! updater.update(theme_ids: [theme.id.to_s, child.id.to_s, "", nil])