diff --git a/app/assets/javascripts/admin/addon/components/themes-grid-card.gjs b/app/assets/javascripts/admin/addon/components/themes-grid-card.gjs index 33b50d80cf4..815f6155bf3 100644 --- a/app/assets/javascripts/admin/addon/components/themes-grid-card.gjs +++ b/app/assets/javascripts/admin/addon/components/themes-grid-card.gjs @@ -82,10 +82,10 @@ export default class ThemeCard extends Component { > <:content>
- {{#if @theme.screenshot}} + {{#if @theme.screenshot_url}} {{@theme.name}} {{else}} diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb index 7179934f7b6..1025017973b 100644 --- a/app/models/remote_theme.rb +++ b/app/models/remote_theme.rb @@ -31,6 +31,9 @@ class RemoteTheme < ActiveRecord::Base MAX_ASSET_FILE_SIZE = 8.megabytes MAX_THEME_FILE_COUNT = 1024 MAX_THEME_SIZE = 256.megabytes + MAX_THEME_SCREENSHOT_FILE_SIZE = 1.megabyte + MAX_THEME_SCREENSHOT_DIMENSIONS = [3840, 2160] # 4K resolution + THEME_SCREENSHOT_ALLOWED_FILE_TYPES = %w[.jpg .jpeg .gif .png].freeze has_one :theme, autosave: false scope :joined_remotes, @@ -250,15 +253,7 @@ class RemoteTheme < ActiveRecord::Base theme_info["assets"]&.each do |name, relative_path| if path = importer.real_path(relative_path) - new_path = "#{File.dirname(path)}/#{SecureRandom.hex}#{File.extname(path)}" - File.rename(path, new_path) # OptimizedImage has strict file name restrictions, so rename temporarily - upload = - UploadCreator.new( - File.open(new_path), - File.basename(relative_path), - for_theme: true, - ).create_for(theme.user_id) - + upload = create_upload(path, relative_path) if !upload.errors.empty? raise ImportError, I18n.t( @@ -277,6 +272,67 @@ class RemoteTheme < ActiveRecord::Base end end + # TODO (martin): Until we are ready to roll this out more + # widely, let's avoid doing this work for most sites. + if SiteSetting.theme_download_screenshots + theme_info["screenshots"] = Array.wrap(theme_info["screenshots"]).take(2) + theme_info["screenshots"].each_with_index do |relative_path, idx| + if path = importer.real_path(relative_path) + if !THEME_SCREENSHOT_ALLOWED_FILE_TYPES.include?(File.extname(path)) + raise ImportError, + I18n.t( + "themes.import_error.screenshot_invalid_type", + file_name: File.basename(path), + accepted_formats: THEME_SCREENSHOT_ALLOWED_FILE_TYPES.join(","), + ) + end + + if File.size(path) > MAX_THEME_SCREENSHOT_FILE_SIZE + raise ImportError, + I18n.t( + "themes.import_error.screenshot_invalid_size", + file_name: File.basename(path), + max_size: + ActiveSupport::NumberHelper.number_to_human_size( + MAX_THEME_SCREENSHOT_FILE_SIZE, + ), + ) + end + + screenshot_width, screenshot_height = FastImage.size(path) + if (screenshot_width.nil? || screenshot_height.nil?) || + screenshot_width > MAX_THEME_SCREENSHOT_DIMENSIONS[0] || + screenshot_height > MAX_THEME_SCREENSHOT_DIMENSIONS[1] + raise ImportError, + I18n.t( + "themes.import_error.screenshot_invalid_dimensions", + file_name: File.basename(path), + width: screenshot_width.to_i, + height: screenshot_height.to_i, + max_width: MAX_THEME_SCREENSHOT_DIMENSIONS[0], + max_height: MAX_THEME_SCREENSHOT_DIMENSIONS[1], + ) + end + + upload = create_upload(path, relative_path) + if !upload.errors.empty? + raise ImportError, + I18n.t( + "themes.import_error.screenshot", + errors: upload.errors.full_messages.join(","), + ) + end + + updated_fields << theme.set_field( + target: :common, + name: "screenshot_#{idx + 1}", + type: :theme_screenshot_upload_var, + upload_id: upload.id, + ) + end + end + end + # Update all theme attributes if this is just a placeholder if self.remote_url.present? && !self.local_version && !self.commits_behind self.theme.name = theme_info["name"] @@ -466,6 +522,19 @@ class RemoteTheme < ActiveRecord::Base def is_git? remote_url.present? end + + def create_upload(path, relative_path) + new_path = "#{File.dirname(path)}/#{SecureRandom.hex}#{File.extname(path)}" + + # OptimizedImage has strict file name restrictions, so rename temporarily + File.rename(path, new_path) + + UploadCreator.new( + File.open(new_path), + File.basename(relative_path), + for_theme: true, + ).create_for(theme.user_id) + end end # == Schema Information diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index c0c3682a516..15cb067894a 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -16,11 +16,31 @@ class ThemeField < ActiveRecord::Base 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? + if ( + self.type_id == ThemeField.types[:theme_screenshot_upload_var] || + self.type_id == ThemeField.types[:theme_upload_var] + ) && saved_change_to_upload_id? UploadReference.ensure_exist!(upload_ids: [self.upload_id], target: self) end end + after_save do + dependent_fields.each(&:invalidate_baked!) + + if upload && svg_sprite_field? + upsert_svg_sprite! + SvgSprite.expire_cache + end + end + + after_destroy do + if svg_sprite_field? + ThemeSvgSprite.where(theme_id: theme_id).delete_all + + SvgSprite.expire_cache + end + end + scope :find_by_theme_ids, ->(theme_ids) do return none if theme_ids.blank? @@ -69,6 +89,7 @@ class ThemeField < ActiveRecord::Base theme_var: 4, # No longer used yaml: 5, js: 6, + theme_screenshot_upload_var: 7, ) end @@ -717,21 +738,8 @@ class ThemeField < ActiveRecord::Base end end - after_save do - dependent_fields.each(&:invalidate_baked!) - - if upload && svg_sprite_field? - upsert_svg_sprite! - SvgSprite.expire_cache - end - end - - after_destroy do - if svg_sprite_field? - ThemeSvgSprite.where(theme_id: theme_id).delete_all - - SvgSprite.expire_cache - end + def upload_url + self.upload&.url end private diff --git a/app/serializers/theme_serializer.rb b/app/serializers/theme_serializer.rb index 4407783f0eb..febd97eb210 100644 --- a/app/serializers/theme_serializer.rb +++ b/app/serializers/theme_serializer.rb @@ -13,7 +13,8 @@ class ThemeSerializer < BasicThemeSerializer :description, :enabled?, :disabled_at, - :theme_fields + :theme_fields, + :screenshot_url has_one :color_scheme, serializer: ColorSchemeSerializer, embed: :object has_one :user, serializer: UserNameSerializer, embed: :object @@ -47,6 +48,13 @@ class ThemeSerializer < BasicThemeSerializer @include_theme_field_values || object.remote_theme_id.nil? end + def screenshot_url + object + .theme_fields + .find { |field| field.type_id == ThemeField.types[:theme_screenshot_upload_var] } + &.upload_url + end + def child_themes object.child_themes end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 317fb70adf0..2ed1a64bd31 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -85,6 +85,10 @@ en: import_error: generic: An error occurred while importing that theme upload: "Error creating upload asset: %{name}. %{errors}" + screenshot: "Error importing theme screenshots. %{errors}" + screenshot_invalid_type: "The theme screenshots must be in one of the following formats: %{accepted_formats}. The screenshot %{file_name} has an invalid format." + screenshot_invalid_size: "The theme screenshots must be less than %{max_size}. The screenshot %{file_name} is too large." + screenshot_invalid_dimensions: "The theme screenshots must be maximum %{max_width}x%{max_height}. The screenshot %{file_name} exceeds this. Its dimensions are %{width}x%{height}." about_json: "Import Error: about.json does not exist, or is invalid. Are you sure this is a Discourse Theme?" about_json_too_big: "Import Error: about.json is bigger than the %{limit} limit." about_json_values: "about.json contains invalid values: %{errors}" diff --git a/config/site_settings.yml b/config/site_settings.yml index 0ce6cdef70e..012df4fe9ab 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1525,6 +1525,9 @@ files: default: "wasm|jpg|jpeg|png|woff|woff2|svg|eot|ttf|otf|gif|webp|avif|js" type: list list_type: file_types + theme_download_screenshots: + default: false + hidden: true authorized_extensions: client: true default: "jpg|jpeg|png|gif|heic|heif|webp|avif" diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index 51f1396d761..ba4c432d49a 100644 --- a/spec/models/remote_theme_spec.rb +++ b/spec/models/remote_theme_spec.rb @@ -34,7 +34,8 @@ RSpec.describe RemoteTheme do "type": "setting", "value": "list_setting" } - } + }, + "screenshots": ["screenshots/1.jpeg", "screenshots/2.jpeg"] } JSON end @@ -72,6 +73,8 @@ RSpec.describe RemoteTheme do "settings.yaml" => settings, "locales/en.yml" => "sometranslations", "migrations/settings/0001-some-migration.js" => migration_js, + "screenshots/1.jpeg" => file_from_fixtures("logo.jpg", "images"), + "screenshots/2.jpeg" => file_from_fixtures("logo.jpg", "images"), ) end @@ -355,7 +358,7 @@ RSpec.describe RemoteTheme 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: 15, limit: 1), + I18n.t("themes.import_error.too_many_files", count: 17, limit: 1), ) end end @@ -384,6 +387,85 @@ RSpec.describe RemoteTheme do ) end end + + describe "screenshots" do + before { SiteSetting.theme_download_screenshots = true } + + it "fails if any of the provided screenshots is not an accepted file type" do + stub_const(RemoteTheme, "THEME_SCREENSHOT_ALLOWED_FILE_TYPES", [".bmp"]) do + expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error( + RemoteTheme::ImportError, + I18n.t( + "themes.import_error.screenshot_invalid_type", + file_name: "1.jpeg", + accepted_formats: ".bmp", + ), + ) + end + end + + it "fails if any of the provided screenshots is too big" do + stub_const(RemoteTheme, "MAX_THEME_SCREENSHOT_FILE_SIZE", 1.byte) do + expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error( + RemoteTheme::ImportError, + I18n.t( + "themes.import_error.screenshot_invalid_size", + file_name: "1.jpeg", + max_size: "1 Bytes", + ), + ) + end + end + + it "fails if any of the provided screenshots has dimensions that are too big" do + FastImage + .expects(:size) + .with { |arg| arg.match(%r{/screenshots/1\.jpeg}) } + .returns([512, 512]) + stub_const(RemoteTheme, "MAX_THEME_SCREENSHOT_DIMENSIONS", [1, 1]) do + expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error( + RemoteTheme::ImportError, + I18n.t( + "themes.import_error.screenshot_invalid_dimensions", + file_name: "1.jpeg", + width: 512, + height: 512, + max_width: 1, + max_height: 1, + ), + ) + end + end + + it "creates uploads and associated theme fields for all theme screenshots" do + FastImage + .stubs(:size) + .with { |arg| arg.match(%r{/screenshots/1\.jpeg}) } + .returns([800, 600]) + FastImage + .stubs(:size) + .with { |arg| arg.match(%r{/screenshots/2\.jpeg}) } + .returns([1024, 768]) + + theme = RemoteTheme.import_theme(initial_repo_url) + + screenshot_1 = theme.theme_fields.find_by(name: "screenshot_1") + screenshot_2 = theme.theme_fields.find_by(name: "screenshot_2") + + expect(screenshot_1).to be_present + expect(screenshot_1.type_id).to eq(ThemeField.types[:theme_screenshot_upload_var]) + expect(screenshot_2).to be_present + expect(screenshot_2.type_id).to eq(ThemeField.types[:theme_screenshot_upload_var]) + expect(screenshot_1.upload).to be_present + expect(screenshot_2.upload).to be_present + + expect(UploadReference.exists?(target: screenshot_1)).to eq(true) + expect(UploadReference.exists?(target: screenshot_2)).to eq(true) + + expect(screenshot_1.upload.original_filename).to eq("1.jpeg") + expect(screenshot_2.upload.original_filename).to eq("2.jpeg") + end + end end let(:github_repo) do diff --git a/spec/serializers/theme_serializer_spec.rb b/spec/serializers/theme_serializer_spec.rb index 5da8479d913..c3f1e7330a4 100644 --- a/spec/serializers/theme_serializer_spec.rb +++ b/spec/serializers/theme_serializer_spec.rb @@ -2,8 +2,9 @@ RSpec.describe ThemeSerializer do describe "load theme settings" do + fab!(:theme) + it "should add error message when settings format is invalid" do - theme = Fabricate(:theme) Theme .any_instance .stubs(:settings) @@ -16,11 +17,51 @@ RSpec.describe ThemeSerializer do it "should add errors messages from theme fields" do error = "error when compiling theme field" - theme = Fabricate(:theme) theme_field = Fabricate(:theme_field, error: error, theme: theme) serialized = ThemeSerializer.new(theme.reload).as_json[:theme] expect(serialized[:errors].count).to eq(1) expect(serialized[:errors][0]).to eq(error) end end + + describe "screenshot_url" do + fab!(:theme) + let(:serialized) { ThemeSerializer.new(theme.reload).as_json[:theme] } + + it "should include screenshot_url when there is a theme field with screenshot upload type" do + Fabricate( + :theme_field, + theme: theme, + type_id: ThemeField.types[:theme_screenshot_upload_var], + name: "theme_screenshot_1", + upload: Fabricate(:upload), + ) + expect(serialized[:screenshot_url]).to be_present + end + + it "should not include screenshot_url when there is no theme field with screenshot upload type" do + expect(serialized[:screenshot_url]).to be_nil + end + + it "should handle multiple screenshot fields and use the first one" do + first_upload = Fabricate(:upload) + second_upload = Fabricate(:upload) + Fabricate( + :theme_field, + theme: theme, + type_id: ThemeField.types[:theme_screenshot_upload_var], + name: "theme_screenshot_1", + upload: first_upload, + ) + Fabricate( + :theme_field, + theme: theme, + type_id: ThemeField.types[:theme_screenshot_upload_var], + name: "theme_screenshot_2", + upload: second_upload, + ) + + expect(serialized[:screenshot_url]).to eq(first_upload.url) + end + end end