Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FEATURE: Theme settings migrations (#24071) 

This commit introduces a new feature that allows theme developers to manage the transformation of theme settings over time. Similar to Rails migrations, the theme settings migration system enables developers to write and execute migrations for theme settings, ensuring a smooth transition when changes are required in the format or structure of setting values.

Example use cases for the theme settings migration system:

1. Renaming a theme setting.

2. Changing the data type of a theme setting (e.g., transforming a string setting containing comma-separated values into a proper list setting).

3. Altering the format of data stored in a theme setting.

All of these use cases and more are now possible while preserving theme setting values for sites that have already modified their theme settings.

Usage:

1. Create a top-level directory called `migrations` in your theme/component, and then within the `migrations` directory create another directory called `settings`.

2. Inside the `migrations/settings` directory, create a JavaScript file using the format `XXXX-some-name.js`, where `XXXX` is a unique 4-digit number, and `some-name` is a descriptor of your choice that describes the migration.

3. Within the JavaScript file, define and export (as the default) a function called `migrate`. This function will receive a `Map` object and must also return a `Map` object (it's acceptable to return the same `Map` object that the function received).

4. The `Map` object received by the `migrate` function will include settings that have been overridden or changed by site administrators. Settings that have never been changed from the default will not be included.

5. The keys and values contained in the `Map` object that the `migrate` function returns will replace all the currently changed settings of the theme.

6. Migrations are executed in numerical order based on the XXXX segment in the migration filenames. For instance, `0001-some-migration.js` will be executed before `0002-another-migration.js`.

Here's a complete example migration script that renames a setting from `setting_with_old_name` to `setting_with_new_name`:

```js
// File name: 0001-rename-setting.js

export default function migrate(settings) {
  if (settings.has("setting_with_old_name")) {
    settings.set("setting_with_new_name", settings.get("setting_with_old_name"));
  }
  return settings;
}
```

Internal topic: t/109980
This commit is contained in:
Osama Sayegh 2023-11-02 08:10:15 +03:00 committed by GitHub
parent d50fccfcaf
commit 3cadd6769e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
21 changed files with 1292 additions and 187 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
app/controllers/admin/themes_controller.rb
View File

@ -160,6 +160,8 @@ class Admin::ThemesController < Admin::AdminController
render_json_error I18n.t("themes.import_error.unknown_file_type"),
status: :unprocessable_entity
end
rescue Theme::SettingsMigrationError => err
render_json_error err.message
end
def index
@ -226,10 +228,14 @@ class Admin::ThemesController < Admin::AdminController
@theme.remote_theme.update_remote_version if params[:theme][:remote_check]
@theme.remote_theme.update_from_remote if params[:theme][:remote_update]
if params[:theme][:remote_update]
@theme.remote_theme.update_from_remote(raise_if_theme_save_fails: false)
else
@theme.save
end
respond_to do |format|
if @theme.save
if @theme.errors.blank?
update_default_theme
@theme = Theme.include_relations.find(@theme.id)
@ -253,6 +259,8 @@ class Admin::ThemesController < Admin::AdminController
end
rescue RemoteTheme::ImportError => e
render_json_error e.message
rescue Theme::SettingsMigrationError => e
render_json_error e.message
end
def destroy

77
app/models/remote_theme.rb
View File

@ -110,30 +110,33 @@ class RemoteTheme < ActiveRecord::Base
remote_theme = new
remote_theme.theme = theme
remote_theme.remote_url = ""
remote_theme.update_from_remote(importer, skip_update: true)
theme.save!
do_update_child_components = false
theme.transaction do
remote_theme.update_from_remote(importer, skip_update: true, already_in_transaction: true)
if existing && update_components.present? && update_components != "none"
child_components = child_components.map { |url| ThemeStore::GitImporter.new(url.strip).url }
if existing && update_components.present? && update_components != "none"
child_components = child_components.map { |url| ThemeStore::GitImporter.new(url.strip).url }
if update_components == "sync"
ChildTheme
.joins(child_theme: :remote_theme)
.where("remote_themes.remote_url NOT IN (?)", child_components)
.delete_all
if update_components == "sync"
ChildTheme
.joins(child_theme: :remote_theme)
.where("remote_themes.remote_url NOT IN (?)", child_components)
.delete_all
end
child_components -=
theme
.child_themes
.joins(:remote_theme)
.where("remote_themes.remote_url IN (?)", child_components)
.pluck("remote_themes.remote_url")
theme.child_components = child_components
do_update_child_components = true
end
child_components -=
theme
.child_themes
.joins(:remote_theme)
.where("remote_themes.remote_url IN (?)", child_components)
.pluck("remote_themes.remote_url")
theme.child_components = child_components
theme.update_child_components
end
theme.update_child_components if do_update_child_components
theme
ensure
begin
@ -160,9 +163,9 @@ class RemoteTheme < ActiveRecord::Base
remote_theme.private_key = private_key
remote_theme.branch = branch
remote_theme.remote_url = importer.url
remote_theme.update_from_remote(importer)
theme.save!
theme
ensure
begin
@ -209,7 +212,12 @@ class RemoteTheme < ActiveRecord::Base
end
end
def update_from_remote(importer = nil, skip_update: false)
def update_from_remote(
importer = nil,
skip_update: false,
raise_if_theme_save_fails: true,
already_in_transaction: false
)
cleanup = false
unless importer
@ -333,12 +341,6 @@ class RemoteTheme < ActiveRecord::Base
updated_fields << theme.set_field(**opts.merge(value: value))
end
theme.convert_settings
# Destroy fields that no longer exist in the remote theme
field_ids_to_destroy = theme.theme_fields.pluck(:id) - updated_fields.map { |tf| tf&.id }
ThemeField.where(id: field_ids_to_destroy).destroy_all
if !skip_update
self.remote_updated_at = Time.zone.now
self.remote_version = importer.version
@ -346,9 +348,28 @@ class RemoteTheme < ActiveRecord::Base
self.commits_behind = 0
end
update_theme_color_schemes(theme, theme_info["color_schemes"]) unless theme.component
transaction_block = -> do
# Destroy fields that no longer exist in the remote theme
field_ids_to_destroy = theme.theme_fields.pluck(:id) - updated_fields.map { |tf| tf&.id }
ThemeField.where(id: field_ids_to_destroy).destroy_all
update_theme_color_schemes(theme, theme_info["color_schemes"]) unless theme.component
self.save!
if raise_if_theme_save_fails
theme.save!
else
raise ActiveRecord::Rollback if !theme.save
end
theme.migrate_settings(start_transaction: false)
end
if already_in_transaction
transaction_block.call
else
self.transaction(&transaction_block)
end
self.save!
self
ensure
begin

61
app/models/theme.rb
View File

@ -8,6 +8,9 @@ class Theme < ActiveRecord::Base
BASE_COMPILER_VERSION = 77
class SettingsMigrationError < StandardError
end
attr_accessor :child_components
def self.cache
@ -33,6 +36,7 @@ class Theme < ActiveRecord::Base
through: :parent_theme_relation,
source: :parent_theme
has_many :color_schemes
has_many :theme_settings_migrations
belongs_to :remote_theme, dependent: :destroy
has_one :theme_modifier_set, dependent: :destroy
has_one :theme_svg_sprite, dependent: :destroy
@ -59,6 +63,9 @@ class Theme < ActiveRecord::Base
has_many :builder_theme_fields,
-> { where("name IN (?)", ThemeField.scss_fields) },
class_name: "ThemeField"
has_many :migration_fields,
-> { where(target_id: Theme.targets[:migrations]) },
class_name: "ThemeField"
validate :component_validations
validate :validate_theme_fields
@ -380,6 +387,7 @@ class Theme < ActiveRecord::Base
extra_scss: 5,
extra_js: 6,
tests_js: 7,
migrations: 8,
)
end
@ -828,22 +836,51 @@ class Theme < ActiveRecord::Base
contents
end
def convert_settings
settings.each do |setting|
setting_row = ThemeSetting.where(theme_id: self.id, name: setting.name.to_s).first
def migrate_settings(start_transaction: true)
block = -> do
runner = ThemeSettingsMigrationsRunner.new(self)
results = runner.run
if setting_row && setting_row.data_type != setting.type
if (
setting_row.data_type == ThemeSetting.types[:list] &&
setting.type == ThemeSetting.types[:string] && setting.json_schema.present?
)
convert_list_to_json_schema(setting_row, setting)
next if results.blank?
old_settings = self.theme_settings.pluck(:name)
self.theme_settings.destroy_all
final_result = results.last
final_result[:settings_after].each do |key, val|
self.update_setting(key.to_sym, val)
rescue Discourse::NotFound
if old_settings.include?(key)
final_result[:settings_after].delete(key)
else
Rails.logger.warn(
"Theme setting type has changed but cannot be converted. \n\n #{setting.inspect}",
)
raise Theme::SettingsMigrationError.new(
I18n.t(
"themes.import_error.migrations.unknown_setting_returned_by_migration",
name: final_result[:original_name],
setting_name: key,
),
)
end
end
results.each do |res|
record =
ThemeSettingsMigration.new(
theme_id: self.id,
version: res[:version],
name: res[:name],
theme_field_id: res[:theme_field_id],
)
record.calculate_diff(res[:settings_before], res[:settings_after])
record.save!
end
self.save!
end
if start_transaction
self.transaction(&block)
else
block.call
end
end

43
app/models/theme_field.rb
View File

@ -1,12 +1,17 @@
# frozen_string_literal: true
class ThemeField < ActiveRecord::Base
MIGRATION_NAME_PART_MAX_LENGTH = 150
belongs_to :upload
has_one :javascript_cache, dependent: :destroy
has_one :upload_reference, as: :target, dependent: :destroy
has_one :theme_settings_migration
validates :value, { length: { maximum: 1024**2 } }
validate :migration_filename_is_valid, if: :migration_field?
after_save do
if self.type_id == ThemeField.types[:theme_upload_var] && saved_change_to_upload_id?
UploadReference.ensure_exist!(upload_ids: [self.upload_id], target: self)
@ -340,7 +345,7 @@ class ThemeField < ActiveRecord::Base
types[:scss]
elsif target.to_s == "extra_scss"
types[:scss]
elsif target.to_s == "extra_js"
elsif %w[migrations extra_js].include?(target.to_s)
types[:js]
elsif target.to_s == "settings" || target.to_s == "translations"
types[:yaml]
@ -394,6 +399,10 @@ class ThemeField < ActiveRecord::Base
self.name == SvgSprite.theme_sprite_variable_name
end
def migration_field?
Theme.targets[:migrations] == self.target_id
end
def ensure_baked!
needs_baking = !self.value_baked || compiler_version != Theme.compiler_version
return unless needs_baking
@ -422,6 +431,9 @@ class ThemeField < ActiveRecord::Base
self.error = validate_svg_sprite_xml
self.value_baked = "baked"
self.compiler_version = Theme.compiler_version
elsif migration_field?
self.value_baked = "baked"
self.compiler_version = Theme.compiler_version
end
if self.will_save_change_to_value_baked? || self.will_save_change_to_compiler_version? ||
@ -603,6 +615,13 @@ class ThemeField < ActiveRecord::Base
targets: :common,
canonical: ->(h) { "assets/#{h[:name]}#{File.extname(h[:filename])}" },
),
ThemeFileMatcher.new(
regex: %r{\Amigrations/settings/(?<name>[^/]+)\.js\z},
names: nil,
types: :js,
targets: :migrations,
canonical: ->(h) { "migrations/settings/#{h[:name]}.js" },
),
]
# For now just work for standard fields
@ -716,6 +735,28 @@ class ThemeField < ActiveRecord::Base
true
end
end
def migration_filename_is_valid
if !name.match?(/\A\d{4}-[a-zA-Z0-9]+/)
self.errors.add(
:base,
I18n.t("themes.import_error.migrations.invalid_filename", filename: name),
)
return
end
# the 5 here is the length of the first 4 digits and the dash that follows
# them
if name.size - 5 > MIGRATION_NAME_PART_MAX_LENGTH
self.errors.add(
:base,
I18n.t(
"themes.import_error.migrations.name_too_long",
count: MIGRATION_NAME_PART_MAX_LENGTH,
),
)
end
end
end
# == Schema Information

54
app/models/theme_settings_migration.rb Normal file
View File

@ -0,0 +1,54 @@
# frozen_string_literal: true
class ThemeSettingsMigration < ActiveRecord::Base
belongs_to :theme
belongs_to :theme_field
validates :theme_id, presence: true
validates :theme_field_id, presence: true
validates :version, presence: true
validates :name, presence: true
validates :diff, presence: true
def calculate_diff(settings_before, settings_after)
diff = { additions: [], deletions: [] }
before_keys = settings_before.keys
after_keys = settings_after.keys
removed_keys = before_keys - after_keys
removed_keys.each { |key| diff[:deletions] << { key: key, val: settings_before[key] } }
added_keys = after_keys - before_keys
added_keys.each { |key| diff[:additions] << { key: key, val: settings_after[key] } }
common_keys = before_keys & after_keys
common_keys.each do |key|
if settings_before[key] != settings_after[key]
diff[:deletions] << { key: key, val: settings_before[key] }
diff[:additions] << { key: key, val: settings_after[key] }
end
end
self.diff = diff
end
end
# == Schema Information
#
# Table name: theme_settings_migrations
#
# id :bigint not null, primary key
# theme_id :integer not null
# theme_field_id :integer not null
# version :integer not null
# name :string(150) not null
# diff :json not null
# created_at :datetime not null
#
# Indexes
#
# index_theme_settings_migrations_on_theme_field_id (theme_field_id) UNIQUE
# index_theme_settings_migrations_on_theme_id_and_version (theme_id,version) UNIQUE
#

185
app/services/theme_settings_migrations_runner.rb Normal file
View File

@ -0,0 +1,185 @@
# frozen_string_literal: true
class ThemeSettingsMigrationsRunner
Migration = Struct.new(:version, :name, :original_name, :code, :theme_field_id)
MIGRATION_ENTRY_POINT_JS = <<~JS
const migrate = require("discourse/theme/migration")?.default;
function main(settingsObj) {
if (!migrate) {
throw new Error("no_exported_migration_function");
}
if (typeof migrate !== "function") {
throw new Error("default_export_is_not_a_function");
}
const map = new Map(Object.entries(settingsObj));
const updatedMap = migrate(map);
if (!updatedMap) {
throw new Error("migration_function_no_returned_value");
}
if (!(updatedMap instanceof Map)) {
throw new Error("migration_function_wrong_return_type");
}
return Object.fromEntries(updatedMap.entries());
}
JS
private_constant :Migration, :MIGRATION_ENTRY_POINT_JS
def self.loader_js_lib_content
@loader_js_lib_content ||=
File.read(
File.join(
Rails.root,
"app/assets/javascripts/node_modules/loader.js/dist/loader/loader.js",
),
)
end
def initialize(theme, limit: 100, timeout: 100, memory: 2.megabytes)
@theme = theme
@limit = limit
@timeout = timeout
@memory = memory
end
def run
fields = lookup_pending_migrations_fields
count = fields.count
return [] if count == 0
raise_error("themes.import_error.migrations.too_many_pending_migrations") if count > @limit
migrations = convert_fields_to_migrations(fields)
migrations.sort_by!(&:version)
current_migration_version =
@theme.theme_settings_migrations.order(version: :desc).pick(:version)
current_migration_version ||= -Float::INFINITY
current_settings = lookup_overriden_settings
migrations.map do |migration|
if migration.version <= current_migration_version
raise_error(
"themes.import_error.migrations.out_of_sequence",
name: migration.original_name,
current: current_migration_version,
)
end
migrated_settings = execute(migration, current_settings)
results = {
version: migration.version,
name: migration.name,
original_name: migration.original_name,
theme_field_id: migration.theme_field_id,
settings_before: current_settings,
settings_after: migrated_settings,
}
current_settings = migrated_settings
current_migration_version = migration.version
results
rescue DiscourseJsProcessor::TranspileError => error
raise_error(
"themes.import_error.migrations.syntax_error",
name: migration.original_name,
error: error.message,
)
rescue MiniRacer::V8OutOfMemoryError
raise_error(
"themes.import_error.migrations.exceeded_memory_limit",
name: migration.original_name,
)
rescue MiniRacer::ScriptTerminatedError
raise_error("themes.import_error.migrations.timed_out", name: migration.original_name)
rescue MiniRacer::RuntimeError => error
message = error.message
if message.include?("no_exported_migration_function")
raise_error(
"themes.import_error.migrations.no_exported_function",
name: migration.original_name,
)
elsif message.include?("default_export_is_not_a_function")
raise_error(
"themes.import_error.migrations.default_export_not_a_function",
name: migration.original_name,
)
elsif message.include?("migration_function_no_returned_value")
raise_error(
"themes.import_error.migrations.no_returned_value",
name: migration.original_name,
)
elsif message.include?("migration_function_wrong_return_type")
raise_error(
"themes.import_error.migrations.wrong_return_type",
name: migration.original_name,
)
else
raise_error(
"themes.import_error.migrations.runtime_error",
name: migration.original_name,
error: message,
)
end
end
end
private
def lookup_pending_migrations_fields
@theme
.migration_fields
.left_joins(:theme_settings_migration)
.where(theme_settings_migration: { id: nil })
end
def convert_fields_to_migrations(fields)
fields.map do |field|
match_data = /\A(?<version>\d{4})-(?<name>.+)/.match(field.name)
if !match_data
raise_error("themes.import_error.migrations.invalid_filename", filename: field.name)
end
version = match_data[:version].to_i
name = match_data[:name]
original_name = field.name
Migration.new(
version: version,
name: name,
original_name: original_name,
code: field.value,
theme_field_id: field.id,
)
end
end
def lookup_overriden_settings
hash = {}
@theme.theme_settings.each { |row| hash[row.name] = ThemeSettingsManager.cast_row_value(row) }
hash
end
def execute(migration, settings)
context = MiniRacer::Context.new(timeout: @timeout, max_memory: @memory)
context.eval(self.class.loader_js_lib_content, filename: "loader.js")
context.eval(
DiscourseJsProcessor.transpile(migration.code, "", "discourse/theme/migration"),
filename: "theme-#{@theme.id}-migration.js",
)
context.eval(MIGRATION_ENTRY_POINT_JS, filename: "migration-entrypoint.js")
context.call("main", settings)
ensure
context&.dispose
end
def raise_error(message_key, **i18n_args)
raise Theme::SettingsMigrationError.new(I18n.t(message_key, **i18n_args))
end
end

3
app/services/themes_install_task.rb
View File

@ -74,8 +74,7 @@ class ThemesInstallTask
end
def update
@remote_theme.update_from_remote
@theme.save
@remote_theme.update_from_remote(raise_if_theme_save_fails: false)
add_component_to_all_themes
@remote_theme.last_error_text.nil?
end

16
config/locales/server.en.yml
View File

@ -92,6 +92,22 @@ en:
not_allowed_theme: "`%{repo}` is not in the list of allowed themes (check `allowed_theme_repos` global setting)."
ssh_key_gone: "You waited too long to install the theme and SSH key expired. Please try again."
too_many_files: "The number of files (%{count}) in the theme has exceeded the maximum allowed number of files (%{limit})"
migrations:
invalid_filename: "Invalid filename for migration file: %{filename}. Filenames must begin with 4 digits followed by a hyphen and then a name that only contains alphanumeric characters with hyphens."
name_too_long:
one: "Migration name is too long. It shouldn't exceed %{count} character."
other: "Migration name is too long. It shouldn't exceed %{count} characters."
too_many_pending_migrations: "There are too many pending migrations in this theme. Themes are not allowed to introduce more than 100 migrations in a single update"
out_of_sequence: "Migration '%{name}' is out of sequence. The last migration for this theme had version number %{current} which is higher than the new migration"
syntax_error: "Failed to run migration '%{name}' because it has a syntax error: %{error}"
exceeded_memory_limit: "Migration '%{name}' failed because it exceeded the memory limit"
timed_out: "Migration '%{name}' timed out"
no_exported_function: "Migration '%{name}' doesn't export a function to perform the migration"
default_export_not_a_function: "Migration '%{name}' has a default export that's not a function. The default export must be the function that performs the migration"
no_returned_value: "Migration '%{name}' didn't return any value (or returned null or undefined). It must return a Map object"
wrong_return_type: "Migration '%{name}' returned an unknown data type. It must return a Map object"
runtime_error: "Migration '%{name}' encountered the following runtime error: %{error}"
unknown_setting_returned_by_migration: "Migrations '%{name}' returned a setting '%{setting_name}' which is not declared in the theme's settings.yml file"
errors:
component_no_user_selectable: "Theme components can't be user-selectable"
component_no_default: "Theme components can't be default theme"

17
db/migrate/20230907225057_create_theme_settings_migrations.rb Normal file
View File

@ -0,0 +1,17 @@
# frozen_string_literal: true
class CreateThemeSettingsMigrations < ActiveRecord::Migration[7.0]
def change
create_table :theme_settings_migrations do |t|
t.integer :theme_id, null: false
t.integer :theme_field_id, null: false
t.integer :version, null: false
t.string :name, null: false, limit: 150
t.json :diff, null: false
t.datetime :created_at, null: false
end
add_index :theme_settings_migrations, %i[theme_id version], unique: true
add_index :theme_settings_migrations, :theme_field_id, unique: true
end
end

3
lib/tasks/themes.rake
View File

@ -70,8 +70,7 @@ def update_themes
remote_theme.update_remote_version
if remote_theme.out_of_date?
puts "updating from #{remote_theme.local_version[0..7]} to #{remote_theme.remote_version[0..7]}"
remote_theme.update_from_remote
theme.save!
remote_theme.update_from_remote(already_in_transaction: true)
else
puts "up to date"
end

34
lib/theme_settings_manager.rb
View File

@ -7,12 +7,22 @@ class ThemeSettingsManager
ThemeSetting.types
end
def self.cast_row_value(row)
type_name = self.types.invert[row.data_type].downcase.capitalize
klass = "ThemeSettingsManager::#{type_name}".constantize
klass.cast(row.value)
end
def self.create(name, default, type, theme, opts = {})
type_name = self.types.invert[type].downcase.capitalize
klass = "ThemeSettingsManager::#{type_name}".constantize
klass.new(name, default, theme, opts)
end
def self.cast(value)
value
end
def initialize(name, default, theme, opts = {})
@name = name.to_sym
@default = default
@ -128,23 +138,31 @@ class ThemeSettingsManager
end
class Bool < self
def self.cast(value)
[true, "true"].include?(value)
end
def value
[true, "true"].include?(super)
self.class.cast(super)
end
def value=(new_value)
new_value = ([true, "true"].include?(new_value)).to_s
new_value = (self.class.cast(new_value)).to_s
super(new_value)
end
end
class Integer < self
def self.cast(value)
value.to_i
end
def value
super.to_i
self.class.cast(super)
end
def value=(new_value)
super(new_value.to_i)
super(self.class.cast(new_value))
end
def is_valid_value?(new_value)
@ -153,12 +171,16 @@ class ThemeSettingsManager
end
class Float < self
def self.cast(value)
value.to_f
end
def value
super.to_f
self.class.cast(super)
end
def value=(new_value)
super(new_value.to_f)
super(self.class.cast(new_value))
end
def is_valid_value?(new_value)

21
spec/fabricators/theme_field_fabricator.rb
View File

@ -7,3 +7,24 @@ Fabricator(:theme_field) do
value { ".test {color: blue;}" }
error { nil }
end
Fabricator(:migration_theme_field, from: :theme_field) do
transient :version
type_id ThemeField.types[:js]
target_id Theme.targets[:migrations]
name do |attrs|
version = attrs[:version] || sequence("theme_#{attrs[:theme].id}_migration_field", 1)
"#{version.to_s.rjust(4, "0")}-some-name"
end
value <<~JS
export default function migrate(settings) {
return settings;
}
JS
end
Fabricator(:settings_theme_field, from: :theme_field) do
type_id ThemeField.types[:yaml]
target_id Theme.targets[:settings]
name "yaml"
end

9
spec/fabricators/theme_settings_migration_fabricator.rb Normal file
View File

@ -0,0 +1,9 @@
# frozen_string_literal: true
Fabricator(:theme_settings_migration) do
theme
theme_field
version { |attrs| sequence("theme_#{attrs[:theme].id}_migrations", 1) }
name { |attrs| "migration-n-#{attrs[:version]}" }
diff { { "additions" => [], "deletions" => [] } }
end

24
spec/lib/theme_store/zip_exporter_spec.rb
View File

@ -29,6 +29,14 @@ RSpec.describe ThemeStore::ZipExporter do
upload_id: upload.id,
type: :theme_upload_var,
)
theme.set_field(target: :migrations, name: "0201-some-migration", type: :js, value: <<~JS)
export default function migrate(settings) {
settings.set("aa", 1);
return settings;
}
JS
theme.build_remote_theme(
remote_url: "",
about_url: "abouturl",
@ -94,7 +102,14 @@ RSpec.describe ThemeStore::ZipExporter do
`rm #{file}`
folders = Dir.glob("**/*").reject { |f| File.file?(f) }
expect(folders).to contain_exactly("assets", "common", "locales", "mobile")
expect(folders).to contain_exactly(
"assets",
"common",
"locales",
"mobile",
"migrations",
"migrations/settings",
)
files = Dir.glob("**/*").reject { |f| File.directory?(f) }
expect(files).to contain_exactly(
@ -105,6 +120,7 @@ RSpec.describe ThemeStore::ZipExporter do
"locales/en.yml",
"mobile/mobile.scss",
"settings.yml",
"migrations/settings/0201-some-migration.js",
)
expect(JSON.parse(File.read("about.json")).deep_symbolize_keys).to eq(
@ -145,6 +161,12 @@ RSpec.describe ThemeStore::ZipExporter do
expect(File.read("locales/en.yml")).to eq(
{ en: { key: "value" } }.deep_stringify_keys.to_yaml,
)
expect(File.read("migrations/settings/0201-some-migration.js")).to eq(<<~JS)
export default function migrate(settings) {
settings.set("aa", 1);
return settings;
}
JS
theme.update!(name: "Discourse Header Icons")
exporter = ThemeStore::ZipExporter.new(theme)

179
spec/models/remote_theme_spec.rb
View File

@ -1,7 +1,7 @@
# frozen_string_literal: true
RSpec.describe RemoteTheme do
describe "#import_remote" do
describe "#import_theme" do
def about_json(
love_color: "FAFAFA",
tertiary_low_color: "FFFFFF",
@ -35,6 +35,12 @@ RSpec.describe RemoteTheme do
"@font-face { font-family: magic; src: url($font)}; body {color: $color; content: $name;}"
end
let(:migration_js) { <<~JS }
export default function migrate(settings) {
return settings;
}
JS
let :initial_repo do
setup_git_repo(
"about.json" => about_json,
@ -51,6 +57,7 @@ RSpec.describe RemoteTheme do
"assets/font.woff2" => "FAKE FONT",
"settings.yaml" => "boolean_setting: true",
"locales/en.yml" => "sometranslations",
"migrations/settings/0001-some-migration.js" => migration_js,
)
end
@ -62,14 +69,102 @@ RSpec.describe RemoteTheme do
around(:each) { |group| MockGitImporter.with_mock { group.run } }
it "run pending theme settings migrations" do
add_to_git_repo(initial_repo, "migrations/settings/0002-another-migration.js" => <<~JS)
export default function migrate(settings) {
settings.set("boolean_setting", false);
return settings;
}
JS
theme = RemoteTheme.import_theme(initial_repo_url)
migrations = theme.theme_settings_migrations.order(:version)
expect(migrations.size).to eq(2)
first_migration = migrations[0]
second_migration = migrations[1]
expect(first_migration.version).to eq(1)
expect(second_migration.version).to eq(2)
expect(first_migration.name).to eq("some-migration")
expect(second_migration.name).to eq("another-migration")
expect(first_migration.diff).to eq("additions" => [], "deletions" => [])
expect(second_migration.diff).to eq(
"additions" => [{ "key" => "boolean_setting", "val" => false }],
"deletions" => [],
)
expect(theme.get_setting(:boolean_setting)).to eq(false)
expect(first_migration.theme_field.value).to eq(<<~JS)
export default function migrate(settings) {
return settings;
}
JS
expect(second_migration.theme_field.value).to eq(<<~JS)
export default function migrate(settings) {
settings.set("boolean_setting", false);
return settings;
}
JS
end
it "doesn't create theme if a migration fails" do
add_to_git_repo(initial_repo, "migrations/settings/0002-another-migration.js" => <<~JS)
export default function migrate(s) {
return null;
}
JS
expect do RemoteTheme.import_theme(initial_repo_url) end.to raise_error(
Theme::SettingsMigrationError,
).and not_change(Theme, :count).and not_change(RemoteTheme, :count)
end
it "doesn't partially update the theme when a migration fails" do
theme = RemoteTheme.import_theme(initial_repo_url)
add_to_git_repo(
initial_repo,
"about.json" =>
JSON
.parse(about_json(about_url: "https://updated.site.com"))
.tap { |h| h[:component] = true }
.to_json,
"stylesheets/file.scss" => ".class3 { color: green; }",
"common/header.html" => "I AM UPDATED HEADER",
"migrations/settings/0002-new-failing-migration.js" => <<~JS,
export default function migrate(settings) {
null.toString();
return settings;
}
JS
)
expect do theme.remote_theme.update_from_remote end.to raise_error(
Theme::SettingsMigrationError,
)
theme.reload
expect(theme.component).to eq(false)
expect(theme.remote_theme.about_url).to eq("https://www.site.com/about")
expect(theme.theme_fields.find_by(name: "header").value).to eq("I AM HEADER")
expect(
theme.theme_fields.find_by(type_id: ThemeField.types[:scss], name: "file").value,
).to eq(".class1{color:red}")
end
it "can correctly import a remote theme" do
time = Time.new("2000")
freeze_time time
@theme = RemoteTheme.import_theme(initial_repo_url)
remote = @theme.remote_theme
theme = RemoteTheme.import_theme(initial_repo_url)
remote = theme.remote_theme
expect(@theme.name).to eq("awesome theme")
expect(theme.name).to eq("awesome theme")
expect(remote.remote_url).to eq(initial_repo_url)
expect(remote.remote_version).to eq(`cd #{initial_repo} && git rev-parse HEAD`.strip)
expect(remote.local_version).to eq(`cd #{initial_repo} && git rev-parse HEAD`.strip)
@ -79,11 +174,11 @@ RSpec.describe RemoteTheme do
expect(remote.theme_version).to eq("1.0")
expect(remote.minimum_discourse_version).to eq("1.0.0")
expect(@theme.theme_modifier_set.serialize_topic_excerpts).to eq(true)
expect(theme.theme_modifier_set.serialize_topic_excerpts).to eq(true)
expect(@theme.theme_fields.length).to eq(11)
expect(theme.theme_fields.length).to eq(12)
mapped = Hash[*@theme.theme_fields.map { |f| ["#{f.target_id}-#{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(scss_data)
@ -96,21 +191,24 @@ RSpec.describe RemoteTheme do
expect(mapped["4-en"]).to eq("sometranslations")
expect(mapped["7-acceptance/theme-test.js"]).to eq("assert.ok(true);")
expect(mapped["8-0001-some-migration"]).to eq(
"export default function migrate(settings) {\n return settings;\n}\n",
)
expect(mapped.length).to eq(11)
expect(mapped.length).to eq(12)
expect(@theme.settings.length).to eq(1)
expect(@theme.settings.first.value).to eq(true)
expect(theme.settings.length).to eq(1)
expect(theme.settings.first.value).to eq(true)
expect(remote.remote_updated_at).to eq_time(time)
scheme = ColorScheme.find_by(theme_id: @theme.id)
scheme = ColorScheme.find_by(theme_id: theme.id)
expect(scheme.name).to eq("Amazing")
expect(scheme.colors.find_by(name: "love").hex).to eq("fafafa")
expect(scheme.colors.find_by(name: "tertiary-low").hex).to eq("ffffff")
expect(@theme.color_scheme_id).to eq(scheme.id)
@theme.update(color_scheme_id: nil)
expect(theme.color_scheme_id).to eq(scheme.id)
theme.update(color_scheme_id: nil)
File.write("#{initial_repo}/common/header.html", "I AM UPDATED")
File.write(
@ -133,15 +231,14 @@ RSpec.describe RemoteTheme do
expect(remote.remote_version).to eq(`cd #{initial_repo} && git rev-parse HEAD`.strip)
remote.update_from_remote
@theme.save!
@theme.reload
theme.reload
scheme = ColorScheme.find_by(theme_id: @theme.id)
scheme = ColorScheme.find_by(theme_id: theme.id)
expect(scheme.name).to eq("Amazing")
expect(scheme.colors.find_by(name: "love").hex).to eq("eaeaea")
expect(@theme.color_scheme_id).to eq(nil) # Should only be set on first import
expect(theme.color_scheme_id).to eq(nil) # Should only be set on first import
mapped = Hash[*@theme.theme_fields.map { |f| ["#{f.target_id}-#{f.name}", f.value] }.flatten]
mapped = Hash[*theme.theme_fields.map { |f| ["#{f.target_id}-#{f.name}", f.value] }.flatten]
# Scss file was deleted
expect(mapped["5-file"]).to eq(nil)
@ -149,8 +246,8 @@ RSpec.describe RemoteTheme do
expect(mapped["0-header"]).to eq("I AM UPDATED")
expect(mapped["1-scss"]).to eq(scss_data)
expect(@theme.settings.length).to eq(1)
expect(@theme.settings.first.value).to eq(32)
expect(theme.settings.length).to eq(1)
expect(theme.settings.first.value).to eq(32)
expect(remote.remote_updated_at).to eq_time(time)
expect(remote.about_url).to eq("https://newsite.com/about")
@ -163,13 +260,12 @@ RSpec.describe RemoteTheme do
`cd #{initial_repo} && git commit -am "update"`
remote.update_from_remote
@theme.save
@theme.reload
theme.reload
scheme_count = ColorScheme.where(theme_id: @theme.id).count
scheme_count = ColorScheme.where(theme_id: theme.id).count
expect(scheme_count).to eq(1)
scheme = ColorScheme.find_by(theme_id: @theme.id)
scheme = ColorScheme.find_by(theme_id: theme.id)
expect(scheme.colors.find_by(name: "tertiary_low_color")).to eq(nil)
end
@ -196,11 +292,44 @@ RSpec.describe RemoteTheme do
expect(remote.reload.commits_behind).to eq(-1)
end
it "runs only new migrations when updating a theme" do
add_to_git_repo(initial_repo, "settings.yaml" => <<~YAML)
first_integer_setting: 1
second_integer_setting: 2
YAML
add_to_git_repo(initial_repo, "migrations/settings/0002-another-migration.js" => <<~JS)
export default function migrate(settings) {
settings.set("first_integer_setting", 101);
return settings;
}
JS
theme = RemoteTheme.import_theme(initial_repo_url)
expect(theme.get_setting(:first_integer_setting)).to eq(101)
expect(theme.get_setting(:second_integer_setting)).to eq(2)
theme.update_setting(:first_integer_setting, 110)
add_to_git_repo(initial_repo, "migrations/settings/0003-yet-another-migration.js" => <<~JS)
export default function migrate(settings) {
settings.set("second_integer_setting", 201);
return settings;
}
JS
theme.remote_theme.update_from_remote
theme.reload
expect(theme.get_setting(:first_integer_setting)).to eq(110)
expect(theme.get_setting(:second_integer_setting)).to eq(201)
end
it "fails if theme has too many files" do
stub_const(RemoteTheme, "MAX_THEME_FILE_COUNT", 1) do
expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error(
RemoteTheme::ImportError,
I18n.t("themes.import_error.too_many_files", count: 14, limit: 1),
I18n.t("themes.import_error.too_many_files", count: 15, limit: 1),
)
end
end

63
spec/models/theme_field_spec.rb
View File

@ -775,4 +775,67 @@ HTML
expect(theme.scss_variables).not_to include("theme_uploads")
end
end
describe "migration JavaScript field" do
it "must match a specific format for filename" do
field = Fabricate(:migration_theme_field, theme: theme)
field.name = "12-some-name"
expect(field.valid?).to eq(false)
expect(field.errors.full_messages).to contain_exactly(
I18n.t("themes.import_error.migrations.invalid_filename", filename: "12-some-name"),
)
field.name = "00012-some-name"
expect(field.valid?).to eq(false)
expect(field.errors.full_messages).to contain_exactly(
I18n.t("themes.import_error.migrations.invalid_filename", filename: "00012-some-name"),
)
field.name = "0012some-name"
expect(field.valid?).to eq(false)
expect(field.errors.full_messages).to contain_exactly(
I18n.t("themes.import_error.migrations.invalid_filename", filename: "0012some-name"),
)
field.name = "0012"
expect(field.valid?).to eq(false)
expect(field.errors.full_messages).to contain_exactly(
I18n.t("themes.import_error.migrations.invalid_filename", filename: "0012"),
)
field.name = "0012-something"
expect(field.valid?).to eq(true)
end
it "doesn't allow weird characters in the name" do
field = Fabricate(:migration_theme_field, theme: theme)
field.name = "0012-ëèard"
expect(field.valid?).to eq(false)
expect(field.errors.full_messages).to contain_exactly(
I18n.t("themes.import_error.migrations.invalid_filename", filename: "0012-ëèard"),
)
end
it "imposes a limit on the name part in the filename" do
stub_const(ThemeField, "MIGRATION_NAME_PART_MAX_LENGTH", 10) do
field = Fabricate(:migration_theme_field, theme: theme)
field.name = "0012-#{"a" * 11}"
expect(field.valid?).to eq(false)
expect(field.errors.full_messages).to contain_exactly(
I18n.t("themes.import_error.migrations.name_too_long", count: 10),
)
field.name = "0012-#{"a" * 10}"
expect(field.valid?).to eq(true)
end
end
end
end

14
spec/models/theme_settings_migration_spec.rb Normal file
View File

@ -0,0 +1,14 @@
# frozen_string_literal: true
RSpec.describe ThemeSettingsMigration do
describe "Validations" do
subject(:badge) { Fabricate.build(:theme_settings_migration) }
it { is_expected.to validate_presence_of(:theme_id) }
it { is_expected.to validate_presence_of(:theme_field_id) }
it { is_expected.to validate_presence_of(:version) }
it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_presence_of(:diff) }
end
end

334
spec/models/theme_spec.rb
View File

@ -12,7 +12,7 @@ RSpec.describe Theme do
let(:guardian) { Guardian.new(user) }
let(:theme) { Fabricate(:theme, user: user) }
fab!(:theme) { Fabricate(:theme, user: user) }
let(:child) { Fabricate(:theme, user: user, component: true) }
it "can properly clean up color schemes" do
@ -440,9 +440,8 @@ HTML
end
it "correctly caches theme ids" do
Theme.destroy_all
Theme.where.not(id: theme.id).destroy_all
theme
theme2 = Fabricate(:theme)
expect(Theme.theme_ids).to contain_exactly(theme.id, theme2.id)
@ -562,7 +561,7 @@ HTML
end
it "includes theme_uploads in settings" do
Theme.destroy_all
Theme.where.not(id: theme.id).destroy_all
upload = UploadCreator.new(file_from_fixtures("logo.png"), "logo.png").create_for(-1)
theme.set_field(type: :theme_upload_var, target: :common, name: "bob", upload_id: upload.id)
@ -574,7 +573,7 @@ HTML
end
it "does not break on missing uploads in settings" do
Theme.destroy_all
Theme.where.not(id: theme.id).destroy_all
upload = UploadCreator.new(file_from_fixtures("logo.png"), "logo.png").create_for(-1)
theme.set_field(type: :theme_upload_var, target: :common, name: "bob", upload_id: upload.id)
@ -589,7 +588,7 @@ HTML
it "uses CDN url for theme_uploads in settings" do
set_cdn_url("http://cdn.localhost")
Theme.destroy_all
Theme.where.not(id: theme.id).destroy_all
upload = UploadCreator.new(file_from_fixtures("logo.png"), "logo.png").create_for(-1)
theme.set_field(type: :theme_upload_var, target: :common, name: "bob", upload_id: upload.id)
@ -602,7 +601,7 @@ HTML
it "uses CDN url for settings of type upload" do
set_cdn_url("http://cdn.localhost")
Theme.destroy_all
Theme.where.not(id: theme.id).destroy_all
upload = UploadCreator.new(file_from_fixtures("logo.png"), "logo.png").create_for(-1)
theme.set_field(target: :settings, name: "yaml", value: <<~YAML)
@ -623,99 +622,6 @@ HTML
expect(json["my_upload"]).to eq("http://cdn.localhost#{upload.url}")
end
describe "convert_settings" do
it "can migrate a list field to a string field with json schema" do
theme.set_field(
target: :settings,
name: :yaml,
value: "valid_json_schema_setting:\n default: \"green,globe\"\n type: \"list\"",
)
theme.save!
setting = theme.settings.find { |s| s.name == :valid_json_schema_setting }
setting.value = "red,globe|green,cog|brown,users"
theme.save!
expect(setting.type).to eq(ThemeSetting.types[:list])
yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/valid_settings.yaml")
theme.set_field(target: :settings, name: "yaml", value: yaml)
theme.save!
theme.convert_settings
setting = theme.settings.find { |s| s.name == :valid_json_schema_setting }
expect(JSON.parse(setting.value)).to eq(
JSON.parse(
'[{"color":"red","icon":"globe"},{"color":"green","icon":"cog"},{"color":"brown","icon":"users"}]',
),
)
expect(setting.type).to eq(ThemeSetting.types[:string])
end
it "does not update setting if data does not validate against json schema" do
theme.set_field(
target: :settings,
name: :yaml,
value: "valid_json_schema_setting:\n default: \"green,globe\"\n type: \"list\"",
)
theme.save!
setting = theme.settings.find { |s| s.name == :valid_json_schema_setting }
# json_schema_settings.yaml defines only two properties per object and disallows additionalProperties
setting.value = "red,globe,hey|green,cog,hey|brown,users,nay"
theme.save!
yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/valid_settings.yaml")
theme.set_field(target: :settings, name: "yaml", value: yaml)
theme.save!
expect { theme.convert_settings }.to raise_error("Schema validation failed")
setting.value = "red,globe|green,cog|brown"
theme.save!
expect { theme.convert_settings }.not_to raise_error
setting = theme.settings.find { |s| s.name == :valid_json_schema_setting }
expect(setting.type).to eq(ThemeSetting.types[:string])
end
it "warns when the theme has modified the setting type but data cannot be converted" do
begin
@orig_logger = Rails.logger
Rails.logger = @fake_logger = FakeLogger.new
theme.set_field(
target: :settings,
name: :yaml,
value: "valid_json_schema_setting:\n default: \"\"\n type: \"list\"",
)
theme.save!
setting = theme.settings.find { |s| s.name == :valid_json_schema_setting }
setting.value = "red,globe"
theme.save!
theme.set_field(
target: :settings,
name: :yaml,
value: "valid_json_schema_setting:\n default: \"\"\n type: \"string\"",
)
theme.save!
theme.convert_settings
expect(setting.value).to eq("red,globe")
expect(@fake_logger.warnings[0]).to include(
"Theme setting type has changed but cannot be converted.",
)
ensure
Rails.logger = @orig_logger
end
end
end
describe "theme translations" do
it "can list working theme_translation_manager objects" do
en_translation =
@ -1125,6 +1031,234 @@ HTML
end
end
describe "#migrate_settings" do
fab!(:settings_field) { Fabricate(:settings_theme_field, theme: theme, value: <<~YAML) }
integer_setting: 1
list_setting: "aa,bb"
YAML
fab!(:migration_field) { Fabricate(:migration_theme_field, theme: theme, version: 1) }
it "persists the results of the last pending migration to the database" do
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
settings.set("integer_setting", 1033);
settings.set("list_setting", "cc,dd");
return settings;
}
JS
Fabricate(:migration_theme_field, theme: theme, value: <<~JS, version: 2)
export default function migrate(settings) {
settings.set("integer_setting", 9909);
settings.set("list_setting", "ee,ff");
return settings;
}
JS
theme.migrate_settings
expect(theme.get_setting("integer_setting")).to eq(9909)
expect(theme.get_setting("list_setting")).to eq("ee,ff")
end
it "doesn't allow arbitrary settings to be saved in the database" do
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
settings.set("unknown_setting", 8834);
return settings;
}
JS
expect do theme.migrate_settings end.to raise_error(
Theme::SettingsMigrationError,
I18n.t(
"themes.import_error.migrations.unknown_setting_returned_by_migration",
name: "0001-some-name",
setting_name: "unknown_setting",
),
)
end
it "allows changing a setting's type" do
theme.update_setting(:list_setting, "zz,aa")
theme.save!
setting_record = theme.theme_settings.where(name: "list_setting").first
expect(setting_record.data_type).to eq(ThemeSetting.types[:string])
expect(setting_record.value).to eq("zz,aa")
settings_field.update!(value: <<~YAML)
integer_setting: 1
list_setting:
default: aa|bb
type: list
YAML
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
settings.set("list_setting", "zz|aa");
return settings;
}
JS
theme.reload
theme.migrate_settings
expect(theme.theme_settings.where(name: "list_setting").count).to eq(1)
setting_record = theme.theme_settings.where(name: "list_setting").first
expect(setting_record.data_type).to eq(ThemeSetting.types[:list])
expect(setting_record.value).to eq("zz|aa")
expect(
theme.theme_settings_migrations.where(theme_field_id: migration_field.id).first.diff,
).to eq(
"additions" => [{ "key" => "list_setting", "val" => "zz|aa" }],
"deletions" => [{ "key" => "list_setting", "val" => "zz,aa" }],
)
end
it "allows renaming a setting" do
theme.update_setting(:integer_setting, 11)
theme.save!
setting_record = theme.theme_settings.where(name: "integer_setting").first
expect(setting_record.value).to eq("11")
settings_field.update!(value: <<~YAML)
integer_setting_updated: 1
list_setting: "aa,bb"
YAML
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
settings.set("integer_setting_updated", settings.get("integer_setting"));
return settings;
}
JS
theme.reload
theme.migrate_settings
expect(theme.theme_settings.where(name: "integer_setting").exists?).to eq(false)
setting_record = theme.theme_settings.where(name: "integer_setting_updated").first
expect(setting_record.value).to eq("11")
expect(
theme.theme_settings_migrations.where(theme_field_id: migration_field.id).first.diff,
).to eq(
"additions" => [{ "key" => "integer_setting_updated", "val" => 11 }],
"deletions" => [{ "key" => "integer_setting", "val" => 11 }],
)
end
it "creates a ThemeSettingsMigration record for each migration" do
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
settings.set("integer_setting", 2);
settings.set("list_setting", "cc,dd");
return settings;
}
JS
second_migration_field =
Fabricate(:migration_theme_field, theme: theme, value: <<~JS, version: 2)
export default function migrate(settings) {
settings.set("integer_setting", 3);
settings.set("list_setting", "ee,ff");
return settings;
}
JS
third_migration_field =
Fabricate(:migration_theme_field, theme: theme, value: <<~JS, version: 3)
export default function migrate(settings) {
settings.set("integer_setting", 4);
settings.set("list_setting", "gg,hh");
return settings;
}
JS
theme.migrate_settings
records = theme.theme_settings_migrations.order(:version)
expect(records.count).to eq(3)
expect(records[0].version).to eq(1)
expect(records[0].name).to eq("some-name")
expect(records[0].theme_field_id).to eq(migration_field.id)
expect(records[0].diff).to eq(
"additions" => [
{ "key" => "integer_setting", "val" => 2 },
{ "key" => "list_setting", "val" => "cc,dd" },
],
"deletions" => [],
)
expect(records[1].version).to eq(2)
expect(records[1].name).to eq("some-name")
expect(records[1].theme_field_id).to eq(second_migration_field.id)
expect(records[1].diff).to eq(
"additions" => [
{ "key" => "integer_setting", "val" => 3 },
{ "key" => "list_setting", "val" => "ee,ff" },
],
"deletions" => [
{ "key" => "integer_setting", "val" => 2 },
{ "key" => "list_setting", "val" => "cc,dd" },
],
)
expect(records[2].version).to eq(3)
expect(records[2].name).to eq("some-name")
expect(records[2].theme_field_id).to eq(third_migration_field.id)
expect(records[2].diff).to eq(
"additions" => [
{ "key" => "integer_setting", "val" => 4 },
{ "key" => "list_setting", "val" => "gg,hh" },
],
"deletions" => [
{ "key" => "integer_setting", "val" => 3 },
{ "key" => "list_setting", "val" => "ee,ff" },
],
)
end
it "allows removing an old setting that no longer exists" do
settings_field.update!(value: <<~YAML)
setting_that_will_be_removed: 1
YAML
theme.update_setting(:setting_that_will_be_removed, 1023)
theme.save!
settings_field.update!(value: <<~YAML)
new_setting: 1
YAML
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
if (settings.get("setting_that_will_be_removed") !== 1023) {
throw new Error(`expected setting_that_will_be_removed to be 1023, but it was instead ${settings.get("setting_that_will_be_removed")}.`);
}
settings.delete("setting_that_will_be_removed");
return settings;
}
JS
theme.reload
theme.migrate_settings
theme.reload
expect(theme.theme_settings.count).to eq(0)
records = theme.theme_settings_migrations
expect(records.size).to eq(1)
expect(records[0].diff).to eq(
"additions" => [],
"deletions" => [{ "key" => "setting_that_will_be_removed", "val" => 1023 }],
)
end
end
describe "development experience" do
it "sends 'development-mode-theme-changed event when non-css fields are updated" do
Theme.any_instance.stubs(:should_refresh_development_clients?).returns(true)

35
spec/requests/admin/themes_controller_spec.rb
View File

@ -235,6 +235,32 @@ RSpec.describe Admin::ThemesController do
expect(response.status).to eq(201)
end
it "responds with suitable error message when a migration fails" do
repo_path =
setup_git_repo(
"about.json" => { name: "test theme" }.to_json,
"settings.yaml" => "boolean_setting: true",
"migrations/settings/0001-some-migration.js" => <<~JS,
export default function migrate(settings) {
settings.set("unknown_setting", "dsad");
return settings;
}
JS
)
repo_url = MockGitImporter.register("https://example.com/initial_repo.git", repo_path)
post "/admin/themes/import.json", params: { remote: repo_url }
expect(response.status).to eq(422)
expect(response.parsed_body["errors"]).to contain_exactly(
I18n.t(
"themes.import_error.migrations.unknown_setting_returned_by_migration",
name: "0001-some-migration",
setting_name: "unknown_setting",
),
)
end
it "fails to import with a failing status" do
post "/admin/themes/import.json", params: { remote: "non-existent" }
@ -250,16 +276,9 @@ RSpec.describe Admin::ThemesController do
it "can lookup a private key by public key" do
Discourse.redis.setex("ssh_key_abcdef", 1.hour, "rsa private key")
ThemeStore::GitImporter.any_instance.stubs(:import!)
RemoteTheme.stubs(:extract_theme_info).returns(
"name" => "discourse-brand-header",
"component" => true,
)
RemoteTheme.any_instance.stubs(:update_from_remote)
post "/admin/themes/import.json",
params: {
remote: " https://github.com/discourse/discourse-brand-header.git ",
remote: " #{repo_url} ",
public_key: "abcdef",
}

285
spec/services/theme_settings_migrations_runner_spec.rb Normal file
View File

@ -0,0 +1,285 @@
# frozen_string_literal: true
describe ThemeSettingsMigrationsRunner do
fab!(:theme) { Fabricate(:theme) }
fab!(:migration_field) { Fabricate(:migration_theme_field, version: 1, theme: theme) }
fab!(:settings_field) { Fabricate(:settings_theme_field, theme: theme, value: <<~YAML) }
integer_setting: 1
boolean_setting: true
string_setting: ""
YAML
describe "#run" do
it "passes values of overridden settings only to migrations" do
theme.update_setting(:integer_setting, 1)
theme.update_setting(:string_setting, "osama")
theme.save!
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
if (settings.get("integer_setting") !== 1) {
throw new Error(`expected integer_setting to equal 1, but it's actually ${settings.get("integer_setting")}`);
}
if (settings.get("string_setting") !== "osama") {
throw new Error(`expected string_setting to equal "osama", but it's actually "${settings.get("string_setting")}"`);
}
if (settings.size !== 2) {
throw new Error(`expected the settings map to have only 2 keys, but instead got ${settings.size} keys`);
}
return settings;
}
JS
results = described_class.new(theme).run
expect(results.first[:theme_field_id]).to eq(migration_field.id)
expect(results.first[:settings_before]).to eq(
{ "integer_setting" => 1, "string_setting" => "osama" },
)
end
it "passes the output of the previous migration as input to the next one" do
theme.update_setting(:integer_setting, 1)
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
settings.set("integer_setting", 111);
return settings;
}
JS
another_migration_field =
Fabricate(:migration_theme_field, theme: theme, version: 2, value: <<~JS)
export default function migrate(settings) {
if (settings.get("integer_setting") !== 111) {
throw new Error(`expected integer_setting to equal 111, but it's actually ${settings.get("integer_setting")}`);
}
settings.set("integer_setting", 222);
return settings;
}
JS
results = described_class.new(theme).run
expect(results.size).to eq(2)
expect(results[0][:theme_field_id]).to eq(migration_field.id)
expect(results[1][:theme_field_id]).to eq(another_migration_field.id)
expect(results[0][:settings_before]).to eq({})
expect(results[0][:settings_after]).to eq({ "integer_setting" => 111 })
expect(results[1][:settings_before]).to eq({ "integer_setting" => 111 })
expect(results[1][:settings_after]).to eq({ "integer_setting" => 222 })
end
it "doesn't run migrations that have already been ran" do
Fabricate(:theme_settings_migration, theme: theme, theme_field: migration_field)
pending_field = Fabricate(:migration_theme_field, theme: theme, version: 23)
results = described_class.new(theme).run
expect(results.size).to eq(1)
expect(results.first[:version]).to eq(23)
expect(results.first[:theme_field_id]).to eq(pending_field.id)
end
it "doesn't error when no migrations have been ran yet" do
results = described_class.new(theme).run
expect(results.size).to eq(1)
expect(results.first[:version]).to eq(1)
expect(results.first[:theme_field_id]).to eq(migration_field.id)
end
it "doesn't error when there are no pending migrations" do
Fabricate(:theme_settings_migration, theme: theme, theme_field: migration_field)
results = described_class.new(theme).run
expect(results.size).to eq(0)
end
it "raises an error when there are too many pending migrations" do
Fabricate(:migration_theme_field, theme: theme, version: 2)
expect do described_class.new(theme, limit: 1).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.too_many_pending_migrations"),
)
end
it "raises an error if a migration field has a badly formatted name" do
migration_field.update_attribute(:name, "020-some-name")
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.invalid_filename", filename: "020-some-name"),
)
migration_field.update_attribute(:name, "0020some-name")
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.invalid_filename", filename: "0020some-name"),
)
migration_field.update_attribute(:name, "0020")
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.invalid_filename", filename: "0020"),
)
end
it "raises an error if a pending migration has version lower than the last ran migration" do
migration_field.update!(name: "0020-some-name")
Fabricate(:theme_settings_migration, theme: theme, theme_field: migration_field, version: 20)
Fabricate(:migration_theme_field, theme: theme, version: 19, name: "0019-failing-migration")
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t(
"themes.import_error.migrations.out_of_sequence",
name: "0019-failing-migration",
current: 20,
),
)
end
it "detects bad syntax in migrations and raises an error" do
migration_field.update!(value: <<~JS)
export default function migrate() {
JS
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t(
"themes.import_error.migrations.syntax_error",
name: "0001-some-name",
error:
'SyntaxError: "/discourse/theme/migration: Unexpected token (2:0)\n\n 1 | export default function migrate() {\n> 2 |\n | ^"',
),
)
end
it "imposes memory limit on migrations and raises an error if they exceed the limit" do
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
let a = new Array(10000);
while(true) {
a = a.concat(new Array(10000));
}
return settings;
}
JS
expect do described_class.new(theme, memory: 10.kilobytes).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.exceeded_memory_limit", name: "0001-some-name"),
)
end
it "imposes time limit on migrations and raises an error if they exceed the limit" do
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
let a = 1;
while(true) {
a += 1;
}
return settings;
}
JS
expect do described_class.new(theme, timeout: 10).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.timed_out", name: "0001-some-name"),
)
end
it "raises a clear error message when the migration file doesn't export anything" do
migration_field.update!(value: <<~JS)
function migrate(settings) {
return settings;
}
JS
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.no_exported_function", name: "0001-some-name"),
)
end
it "raises a clear error message when the migration file exports the default as something that's not a function" do
migration_field.update!(value: <<~JS)
export function migrate(settings) {
return settings;
}
const AA = 1;
export default AA;
JS
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t(
"themes.import_error.migrations.default_export_not_a_function",
name: "0001-some-name",
),
)
end
it "raises a clear error message when the migration function doesn't return anything" do
migration_field.update!(value: <<~JS)
export default function migrate(settings) {}
JS
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.no_returned_value", name: "0001-some-name"),
)
end
it "raises a clear error message when the migration function doesn't return a Map" do
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
return {};
}
JS
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t("themes.import_error.migrations.wrong_return_type", name: "0001-some-name"),
)
end
it "surfaces runtime errors that occur within the migration" do
migration_field.update!(value: <<~JS)
export default function migrate(settings) {
null.toString();
return settings;
}
JS
expect do described_class.new(theme).run end.to raise_error(
Theme::SettingsMigrationError,
I18n.t(
"themes.import_error.migrations.runtime_error",
name: "0001-some-name",
error: "TypeError: Cannot read properties of null (reading 'toString')",
),
)
end
it "returns a list of objects that each has data representing the migration and the results" do
results = described_class.new(theme).run
expect(results[0][:version]).to eq(1)
expect(results[0][:name]).to eq("some-name")
expect(results[0][:original_name]).to eq("0001-some-name")
expect(results[0][:theme_field_id]).to eq(migration_field.id)
expect(results[0][:settings_before]).to eq({})
expect(results[0][:settings_after]).to eq({})
end
end
end

10
spec/support/helpers.rb
View File

@ -184,6 +184,16 @@ module Helpers
repo_dir
end
def add_to_git_repo(repo_dir, files)
files.each do |name, data|
FileUtils.mkdir_p(Pathname.new("#{repo_dir}/#{name}").dirname)
File.write("#{repo_dir}/#{name}", data)
`cd #{repo_dir} && git add #{name}`
end
`cd #{repo_dir} && git commit -am 'add #{files.size} files'`
repo_dir
end
def stub_const(target, const, value)
old = target.const_get(const)
target.send(:remove_const, const)