From 98fbc019a3578950f40c86a923a3c2155929cc1b Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 29 Aug 2019 15:47:08 +0100 Subject: [PATCH] FIX: Ensure live-reloading of theme CSS works first time (#8052) The client-side theme-selector would always apply the first in a series of file change notifications. This has been fixed, so it now applies the most recent notification. Duplicate notifications were being sent because - The remote_theme autosave was causing every change notification to be doubled - Color scheme change notifications were being sent every time a theme was uploaded, even if the colors were unchanged These duplicate notifications have been fixed, and a spec added to ensure it does not regress in future --- .../discourse/lib/theme-selector.js.es6 | 12 ++++-------- app/models/remote_theme.rb | 6 +++--- app/models/theme.rb | 2 +- spec/models/theme_spec.rb | 1 + spec/requests/admin/themes_controller_spec.rb | 15 ++++++++++++--- 5 files changed, 21 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/discourse/lib/theme-selector.js.es6 b/app/assets/javascripts/discourse/lib/theme-selector.js.es6 index 60d61c20c53..e0ba455d4ff 100644 --- a/app/assets/javascripts/discourse/lib/theme-selector.js.es6 +++ b/app/assets/javascripts/discourse/lib/theme-selector.js.es6 @@ -43,16 +43,12 @@ export function setLocalTheme(ids, themeSeq) { } } -export function refreshCSS(node, hash, newHref, options) { +export function refreshCSS(node, hash, newHref) { let $orig = $(node); if ($orig.data("reloading")) { - if (options && options.force) { - clearTimeout($orig.data("timeout")); - $orig.data("copy").remove(); - } else { - return; - } + clearTimeout($orig.data("timeout")); + $orig.data("copy").remove(); } if (!$orig.data("orig")) { @@ -99,7 +95,7 @@ export function previewTheme(ids = []) { `link[rel=stylesheet][data-target=${theme.target}]` )[0]; if (node) { - refreshCSS(node, null, theme.new_href, { force: true }); + refreshCSS(node, null, theme.new_href); } }); } diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb index 4949f3b0366..38da1d054f9 100644 --- a/app/models/remote_theme.rb +++ b/app/models/remote_theme.rb @@ -21,7 +21,7 @@ class RemoteTheme < ActiveRecord::Base GITHUB_REGEXP = /^https?:\/\/github\.com\// GITHUB_SSH_REGEXP = /^git@github\.com:/ - has_one :theme + has_one :theme, autosave: false scope :joined_remotes, -> { joins("JOIN themes ON themes.remote_theme_id = remote_themes.id").where.not(remote_url: "") } @@ -211,7 +211,7 @@ class RemoteTheme < ActiveRecord::Base color_scheme_color = scheme.color_scheme_colors.to_a.find { |c| c.name == color[:name] } || scheme.color_scheme_colors.build(name: color[:name]) color_scheme_color.hex = override || color[:hex] - theme.notify_color_change(color_scheme_color) + theme.notify_color_change(color_scheme_color) if color_scheme_color.hex_changed? end # Update advanced colors @@ -221,7 +221,7 @@ class RemoteTheme < ActiveRecord::Base if override color_scheme_color ||= scheme.color_scheme_colors.build(name: variable_name) color_scheme_color.hex = override - theme.notify_color_change(color_scheme_color) + theme.notify_color_change(color_scheme_color) if color_scheme_color.hex_changed? elsif color_scheme_color # No longer specified in about.json, delete record scheme.color_scheme_colors.delete(color_scheme_color) theme.notify_color_change(nil, scheme: scheme) diff --git a/app/models/theme.rb b/app/models/theme.rb index a307d3ac05a..c4bda99cc7a 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -22,7 +22,7 @@ class Theme < ActiveRecord::Base has_many :child_themes, -> { order(:name) }, through: :child_theme_relation, source: :child_theme has_many :parent_themes, -> { order(:name) }, through: :parent_theme_relation, source: :parent_theme has_many :color_schemes - belongs_to :remote_theme, autosave: true, dependent: :destroy + belongs_to :remote_theme, dependent: :destroy has_one :settings_field, -> { where(target_id: Theme.targets[:settings], name: "yaml") }, class_name: 'ThemeField' has_one :javascript_cache, dependent: :destroy diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index fb47cfb50df..749c366f14b 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -62,6 +62,7 @@ describe Theme do 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") + theme.save! expect(theme.supported?).to eq(false) expect(Theme.transform_ids([theme.id])).to be_empty diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index 43cc16cd8b8..36fd5f68db5 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -136,12 +136,20 @@ describe Admin::ThemesController do existing_theme = Fabricate(:theme, name: "Header Icons") other_existing_theme = Fabricate(:theme, name: "Some other name") - expect do - post "/admin/themes/import.json", params: { bundle: theme_archive, theme_id: other_existing_theme.id } - end.to change { Theme.count }.by (0) + messages = MessageBus.track_publish do + expect do + post "/admin/themes/import.json", params: { bundle: theme_archive, theme_id: other_existing_theme.id } + end.to change { Theme.count }.by (0) + end expect(response.status).to eq(201) json = ::JSON.parse(response.body) + # Ensure only one refresh message is sent. + # More than 1 is wasteful, and can trigger unusual race conditions in the client + # If this test fails, it probably means `theme.save` is being called twice - check any 'autosave' relations + file_change_messages = messages.filter { |m| m[:channel] == "/file-change" } + expect(file_change_messages.count).to eq(1) + expect(json["theme"]["name"]).to eq("Some other name") expect(json["theme"]["id"]).to eq(other_existing_theme.id) expect(json["theme"]["theme_fields"].length).to eq(5) @@ -383,6 +391,7 @@ describe Admin::ThemesController do it 'handles import errors on update' do theme.create_remote_theme!(remote_url: "https://example.com/repository") + theme.save! # RemoteTheme is extensively tested, and setting up the test scaffold is a large overhead # So use a stub here to test the controller