From d2e4b32c8755f8927a1a99c026642d2824e38481 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 12 Sep 2023 07:38:47 +0800 Subject: [PATCH] DEV: Add support for uploading a theme from a directory in system tests (#23402) Why this change? Currently, we do not have an easy way to test themes and theme components using Rails system tests. While we support QUnit acceptance tests for themes and theme components, QUnit acceptance tests stubs out the server and setting up the fixtures for server responses is difficult and can lead to a frustrating experience. System tests on the other hand allow authors to set up the test fixtures using our fabricator system which is much easier to use. What does this change do? In order for us to allow authors to run system tests with their themes installed, we are adding a `upload_theme` helper that is made available when writing system tests. The `upload_theme` helper requires a single `directory` parameter where `directory` is the directory of the theme locally and returns a `Theme` record. --- app/controllers/admin/themes_controller.rb | 2 - app/models/remote_theme.rb | 27 ++++++++++-- lib/theme_store/base_importer.rb | 41 ++++++++++++++++++ lib/theme_store/directory_importer.rb | 14 ++++++ lib/theme_store/git_importer.rb | 40 ++--------------- lib/theme_store/zip_importer.rb | 33 +------------- .../themes/discourse-test-theme/about.json | 21 +++++++++ .../discourse-test-theme/assets/logo.png | Bin 0 -> 2297 bytes .../discourse-test-theme/common/body_tag.html | 1 + .../discourse-test-theme/locales/en.yml | 3 ++ .../discourse-test-theme/mobile/mobile.scss | 1 + .../themes/discourse-test-theme/settings.yml | 1 + spec/models/remote_theme_spec.rb | 11 +++++ spec/requests/admin/themes_controller_spec.rb | 15 ------- spec/support/system_helpers.rb | 4 ++ 15 files changed, 126 insertions(+), 88 deletions(-) create mode 100644 lib/theme_store/base_importer.rb create mode 100644 lib/theme_store/directory_importer.rb create mode 100644 spec/fixtures/themes/discourse-test-theme/about.json create mode 100644 spec/fixtures/themes/discourse-test-theme/assets/logo.png create mode 100644 spec/fixtures/themes/discourse-test-theme/common/body_tag.html create mode 100644 spec/fixtures/themes/discourse-test-theme/locales/en.yml create mode 100644 spec/fixtures/themes/discourse-test-theme/mobile/mobile.scss create mode 100644 spec/fixtures/themes/discourse-test-theme/settings.yml diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index 2eddce7d13c..b61df316e6a 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -142,13 +142,11 @@ class Admin::ThemesController < Admin::AdminController bundle = params[:bundle] || params[:theme] theme_id = params[:theme_id] update_components = params[:components] - match_theme_by_name = !!params[:bundle] && !params.key?(:theme_id) # Old theme CLI behavior, match by name. Remove Jan 2020 begin @theme = RemoteTheme.update_zipped_theme( bundle.path, bundle.original_filename, - match_theme: match_theme_by_name, user: theme_user, theme_id: theme_id, update_components: update_components, diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb index f80bbf26980..8e9c8d3ce4b 100644 --- a/app/models/remote_theme.rb +++ b/app/models/remote_theme.rb @@ -51,16 +51,34 @@ class RemoteTheme < ActiveRecord::Base def self.update_zipped_theme( filename, original_filename, - match_theme: false, user: Discourse.system_user, theme_id: nil, update_components: nil ) - importer = ThemeStore::ZipImporter.new(filename, original_filename) + update_theme( + ThemeStore::ZipImporter.new(filename, original_filename), + user:, + theme_id:, + update_components:, + ) + end + + # This is only used in the tests environment and is currently not supported for other environments + if Rails.env.test? + def self.import_theme_from_directory(directory) + update_theme(ThemeStore::DirectoryImporter.new(directory)) + end + end + + def self.update_theme( + importer, + user: Discourse.system_user, + theme_id: nil, + update_components: nil + ) importer.import! theme_info = RemoteTheme.extract_theme_info(importer) - theme = Theme.find_by(name: theme_info["name"]) if match_theme # Old theme CLI method, remove Jan 2020 theme = Theme.find_by(id: theme_id) if theme_id # New theme CLI method existing = true @@ -107,6 +125,7 @@ class RemoteTheme < ActiveRecord::Base Rails.logger.warn("Failed cleanup remote path #{e}") end end + private_class_method :update_theme def self.import_theme(url, user = Discourse.system_user, private_key: nil, branch: nil) importer = ThemeStore::GitImporter.new(url.strip, private_key: private_key, branch: branch) @@ -232,6 +251,7 @@ class RemoteTheme < ActiveRecord::Base METADATA_PROPERTIES.each do |property| self.public_send(:"#{property}=", theme_info[property.to_s]) end + if !self.valid? raise ImportError, I18n.t( @@ -246,6 +266,7 @@ class RemoteTheme < ActiveRecord::Base theme_info.dig("modifiers", modifier_name.to_s), ) end + if !theme.theme_modifier_set.valid? raise ImportError, I18n.t( diff --git a/lib/theme_store/base_importer.rb b/lib/theme_store/base_importer.rb new file mode 100644 index 00000000000..feb36bcd701 --- /dev/null +++ b/lib/theme_store/base_importer.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module ThemeStore + class BaseImporter + def import! + raise "Not implemented" + end + + def [](value) + fullpath = real_path(value) + return nil unless fullpath + File.read(fullpath) + end + + def real_path(relative) + fullpath = "#{temp_folder}/#{relative}" + return nil unless File.exist?(fullpath) + + # careful to handle symlinks here, don't want to expose random data + fullpath = Pathname.new(fullpath).realpath.to_s + + if fullpath && fullpath.start_with?(temp_folder) + fullpath + else + nil + end + end + + def all_files + Dir.glob("**/**", base: temp_folder).reject { |f| File.directory?(File.join(temp_folder, f)) } + end + + def cleanup! + FileUtils.rm_rf(temp_folder) + end + + def temp_folder + @temp_folder ||= "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}" + end + end +end diff --git a/lib/theme_store/directory_importer.rb b/lib/theme_store/directory_importer.rb new file mode 100644 index 00000000000..50c041e406c --- /dev/null +++ b/lib/theme_store/directory_importer.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module ThemeStore + class DirectoryImporter < BaseImporter + def initialize(theme_dir) + @theme_dir = theme_dir + end + + def import! + FileUtils.mkdir_p(temp_folder) + FileUtils.cp_r("#{@theme_dir}/.", temp_folder) + end + end +end diff --git a/lib/theme_store/git_importer.rb b/lib/theme_store/git_importer.rb index f23127a02c2..407dcfadbea 100644 --- a/lib/theme_store/git_importer.rb +++ b/lib/theme_store/git_importer.rb @@ -1,16 +1,12 @@ # frozen_string_literal: true -module ThemeStore -end - -class ThemeStore::GitImporter +class ThemeStore::GitImporter < ThemeStore::BaseImporter COMMAND_TIMEOUT_SECONDS = 20 attr_reader :url def initialize(url, private_key: nil, branch: nil) @url = GitUrl.normalize(url) - @temp_folder = "#{Pathname.new(Dir.tmpdir).realpath}/discourse_theme_#{SecureRandom.hex}" @private_key = private_key @branch = branch end @@ -18,7 +14,7 @@ class ThemeStore::GitImporter def import! clone! - if version = Discourse.find_compatible_git_resource(@temp_folder) + if version = Discourse.find_compatible_git_resource(temp_folder) begin execute "git", "cat-file", "-e", version rescue RuntimeError => e @@ -56,34 +52,6 @@ class ThemeStore::GitImporter execute("git", "rev-parse", "HEAD").strip end - def cleanup! - FileUtils.rm_rf(@temp_folder) - end - - def real_path(relative) - fullpath = "#{@temp_folder}/#{relative}" - return nil unless File.exist?(fullpath) - - # careful to handle symlinks here, don't want to expose random data - fullpath = Pathname.new(fullpath).realpath.to_s - - if fullpath && fullpath.start_with?(@temp_folder) - fullpath - else - nil - end - end - - def all_files - Dir.glob("**/*", base: @temp_folder).reject { |f| File.directory?(File.join(@temp_folder, f)) } - end - - def [](value) - fullpath = real_path(value) - return nil unless fullpath - File.read(fullpath) - end - protected def redirected_uri @@ -135,7 +103,7 @@ class ThemeStore::GitImporter args.concat(["--single-branch", "-b", @branch]) if @branch.present? - args.concat([url, @temp_folder]) + args.concat([url, temp_folder]) args end @@ -196,6 +164,6 @@ class ThemeStore::GitImporter end def execute(*args) - Discourse::Utils.execute_command(*args, chdir: @temp_folder, timeout: COMMAND_TIMEOUT_SECONDS) + Discourse::Utils.execute_command(*args, chdir: temp_folder, timeout: COMMAND_TIMEOUT_SECONDS) end end diff --git a/lib/theme_store/zip_importer.rb b/lib/theme_store/zip_importer.rb index aed20bb83e2..ac75a482c5e 100644 --- a/lib/theme_store/zip_importer.rb +++ b/lib/theme_store/zip_importer.rb @@ -2,10 +2,7 @@ require "compression/engine" -module ThemeStore -end - -class ThemeStore::ZipImporter +class ThemeStore::ZipImporter < ThemeStore::BaseImporter attr_reader :url def initialize(filename, original_filename) @@ -30,10 +27,6 @@ class ThemeStore::ZipImporter raise RemoteTheme::ImportError, I18n.t("themes.import_error.file_too_big") end - def cleanup! - FileUtils.rm_rf(@temp_folder) - end - def version "" end @@ -44,28 +37,4 @@ class ThemeStore::ZipImporter FileUtils.mv(Dir.glob("#{@temp_folder}/*/*"), @temp_folder) end end - - def real_path(relative) - fullpath = "#{@temp_folder}/#{relative}" - return nil unless File.exist?(fullpath) - - # careful to handle symlinks here, don't want to expose random data - fullpath = Pathname.new(fullpath).realpath.to_s - - if fullpath && fullpath.start_with?(@temp_folder) - fullpath - else - nil - end - end - - def all_files - Dir.glob("**/**", base: @temp_folder).reject { |f| File.directory?(File.join(@temp_folder, f)) } - end - - def [](value) - fullpath = real_path(value) - return nil unless fullpath - File.read(fullpath) - end end diff --git a/spec/fixtures/themes/discourse-test-theme/about.json b/spec/fixtures/themes/discourse-test-theme/about.json new file mode 100644 index 00000000000..333d16932e4 --- /dev/null +++ b/spec/fixtures/themes/discourse-test-theme/about.json @@ -0,0 +1,21 @@ +{ + "name": "Header Icons", + "about_url": "abouturl", + "license_url": "licenseurl", + "component": false, + "assets": { + "logo": "assets/logo.png" + }, + "color_schemes": { + "Orphan Color Scheme": { + "header_primary": "F0F0F0", + "header_background": "1E1E1E", + "tertiary": "858585" + }, + "Theme Color Scheme": { + "header_primary": "F0F0F0", + "header_background": "1E1E1E", + "tertiary": "858585" + } + } +} \ No newline at end of file diff --git a/spec/fixtures/themes/discourse-test-theme/assets/logo.png b/spec/fixtures/themes/discourse-test-theme/assets/logo.png new file mode 100644 index 0000000000000000000000000000000000000000..5c900b61f70312ad23e30265c92dca79feba84b4 GIT binary patch literal 2297 zcmb7_`9IT-1IOR)Na)gxVMifyQ?6_|HrFT%Ni0`Oj>6=~(P%5|L*>Z5IdhF_jSyX; znvBZXq%vnSA9s`b^!*dQkH_ovdOlu1zkYb#a>Jr`$$(`50NCY#u|4&dtABlBhuGir z^xs(t03u{JXPn*N3RhGAzXa5}0Sp088S$?V?9WdIP8B+;jF>+P=i9<9)SydVumwwa z8U(h@D_Kjmd#nOif8+zy#X>cY0QGs&>Fq#W_Lp!f$F1jk3OnQV)YKQ9;e9FyH3Rbvy5ABG_LF%<>TWO4K%3D(2;5)* zgQs{p9B6o?!0^}=OBXNx^1&7%-~3hNFixTSRU zHzCz83-DQA%JgS;tAGGNYLA003K#3kE97;xo6Ax>(;Jamp*`HSnZ?D$@L?`jkeqb= z0Y`G=*al>Lht3lr3t}#b*@4)@=o47WI#@DoW;b75>g7@n1OKqBpQG5}*7|Q{h zYYGpJ*sLm6q>`O|2K>m@ZtPCz;aW3Hd%>;rZ>cYJ>YOxEpR>EMfwJO6cRY2mq@b+K zuV{4Z482K8pOw|{*pONRg1k!P*CP{}s#>pZyh?9vf*_Bok z>sz&#e4AVff)1f3K0qa1T#}>2HH?%}9u}HhD$I<8NiWDPzj4aHO0_NI5*N1DKW@sF zS8uWkOk~6Hs;=332cOa2MQtS6e>7$On3JeCI5#p5K=CRz@3X zh4rvEzCDxF(;oX{7HL9eoIh)5rie;bGU$hZw1zgU>lpTDp4aYVY9Lp3uRK4-L_4m% zi!d~XYo&<%>wF8y^wx4Aa%Vn1nAFy(2oR+6|vxk$ik> zW=k_wthnK7Wqj$U7Mmds<%ibzZBU}?WLwxe>(Y27+b18!fH9ev&dnxngVbktKUn;A^ z2dm4ZmM#X0GjO>sE)$5_u%m{JJcY)QW+xL-j*hmM!_&5b1HyLo9m7}LtF^!H8+%-* zy@OS>U5L)zhq5*&EnQm;4%`&Uqv91xhl&h>@|4R0xbU%qL zaWO9y%KLi9R|ITG&Q0w0=R*2&OGLLO+-bw!A5PQy>o^_;#;z%CnU)(qX`qf3c@c~G zv9xm%V|nx&dCgU9+sP-8YmTZ*E$g_H+ru zwIR%JxS{SPS9RwTk`pOhH|{PIF9DI^xWSzvp`>CM|REGz4`G5$;+BIGom*^_xhHrEMyg@H+m;@89HwG zgO2;XNB4u2*#m=6icBKY#AG}x?#vaYySNMHf`qEh#Jl#kt(Lq6D4Wf!Ds|)gc3&6u&3XF4A^Ot9HCh-cCe2uxi+MU7YqfzJx|D#=2WJ=P{CEcWV|IG3NM~2xb%0VVU zmb&ZmlTiO)@$H3B#}482*LLdBCX#b~Q^3^HI&tb{xvjl{QK|EE&&yXh#z^bLca#R1 zd^UYc#IL6UBfFho`>W?$33aQ*bErj?^7ZusuN%`Z`1Mmab1i0Xj}n=D#z;0KaJSTm z#SEJol#S_wSi6jVoOD?`vx<^ahP0C&CxZ za-&zL&^lHrS%ER7H%f)w#%_(EgVY}W8v&M5U(&dj#C1(VxlhHg+abq`;-i@=DCFz;ZTHne#+Wf>~3v6!IQ9YhANvur+0& RsqXJ{0uFXqTgC}I=|2V|H;4cL literal 0 HcmV?d00001 diff --git a/spec/fixtures/themes/discourse-test-theme/common/body_tag.html b/spec/fixtures/themes/discourse-test-theme/common/body_tag.html new file mode 100644 index 00000000000..da472912a6f --- /dev/null +++ b/spec/fixtures/themes/discourse-test-theme/common/body_tag.html @@ -0,0 +1 @@ +testtheme1 \ No newline at end of file diff --git a/spec/fixtures/themes/discourse-test-theme/locales/en.yml b/spec/fixtures/themes/discourse-test-theme/locales/en.yml new file mode 100644 index 00000000000..0670a76c42f --- /dev/null +++ b/spec/fixtures/themes/discourse-test-theme/locales/en.yml @@ -0,0 +1,3 @@ +--- +en: + key: value diff --git a/spec/fixtures/themes/discourse-test-theme/mobile/mobile.scss b/spec/fixtures/themes/discourse-test-theme/mobile/mobile.scss new file mode 100644 index 00000000000..160017232d4 --- /dev/null +++ b/spec/fixtures/themes/discourse-test-theme/mobile/mobile.scss @@ -0,0 +1 @@ +body {background-color: $background_color; font-size: $font-size} \ No newline at end of file diff --git a/spec/fixtures/themes/discourse-test-theme/settings.yml b/spec/fixtures/themes/discourse-test-theme/settings.yml new file mode 100644 index 00000000000..deb5d034a24 --- /dev/null +++ b/spec/fixtures/themes/discourse-test-theme/settings.yml @@ -0,0 +1 @@ +somesetting: test \ No newline at end of file diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index 659bd5a3324..6a052a10d7a 100644 --- a/spec/models/remote_theme_spec.rb +++ b/spec/models/remote_theme_spec.rb @@ -281,4 +281,15 @@ RSpec.describe RemoteTheme do expect(described_class.unreachable_themes).to eq([]) end end + + describe ".import_theme_from_directory" do + let(:theme_dir) { "#{Rails.root}/spec/fixtures/themes/discourse-test-theme" } + + it "imports a theme from a directory" do + theme = RemoteTheme.import_theme_from_directory(theme_dir) + + expect(theme.name).to eq("Header Icons") + expect(theme.theme_fields.count).to eq(5) + end + end end diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index 75571da04f5..36f0711be63 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -326,21 +326,6 @@ RSpec.describe Admin::ThemesController do expect(UserHistory.where(action: UserHistory.actions[:change_theme]).count).to eq(1) end - it "updates an existing theme from an archive by name" do - # Old theme CLI method, remove Jan 2020 - _existing_theme = Fabricate(:theme, name: "Header Icons") - - expect do - post "/admin/themes/import.json", params: { bundle: theme_archive } - end.to change { Theme.count }.by (0) - expect(response.status).to eq(201) - json = response.parsed_body - - expect(json["theme"]["name"]).to eq("Header Icons") - expect(json["theme"]["theme_fields"].length).to eq(5) - expect(UserHistory.where(action: UserHistory.actions[:change_theme]).count).to eq(1) - end - it "updates an existing theme from an archive by id" do # Used by theme CLI _existing_theme = Fabricate(:theme, name: "Header Icons") diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb index 294276f49db..f59851c2a61 100644 --- a/spec/support/system_helpers.rb +++ b/spec/support/system_helpers.rb @@ -22,6 +22,10 @@ module SystemHelpers expect(page).to have_content("Signed in to #{user.encoded_username} successfully") end + def upload_theme(theme_dir) + RemoteTheme.import_theme_from_directory(theme_dir) + end + def setup_system_test SiteSetting.login_required = false SiteSetting.has_login_hint = false