From 33de5abb6ec98a568e1b23f2a400e6c8d36c9965 Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Thu, 20 Jun 2024 11:33:46 -0400 Subject: [PATCH] DEV: Extract theme resolution to a helper (#27426) This ensures that the theme id is resolved as early as possible in the request cycle. This is necessary for the custom homepage to skip preloading the wrong data. --- app/controllers/application_controller.rb | 32 ++------------------ app/helpers/application_helper.rb | 2 +- config/routes.rb | 4 ++- lib/homepage_constraint.rb | 6 +++- lib/homepage_helper.rb | 4 +-- lib/theme_resolver.rb | 36 +++++++++++++++++++++++ spec/lib/homepage_helper_spec.rb | 2 +- 7 files changed, 51 insertions(+), 35 deletions(-) create mode 100644 lib/theme_resolver.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 177ccc97ed1..001a7735139 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -9,6 +9,7 @@ class ApplicationController < ActionController::Base include GlobalPath include Hijack include ReadOnlyMixin + include ThemeResolver include VaryHeader attr_reader :theme_id @@ -478,34 +479,7 @@ class ApplicationController < ActionController::Base resolve_safe_mode return if request.env[NO_THEMES] - theme_id = nil - - if (preview_theme_id = request[:preview_theme_id]&.to_i) && - guardian.allow_themes?([preview_theme_id], include_preview: true) - theme_id = preview_theme_id - end - - user_option = current_user&.user_option - - if theme_id.blank? - ids, seq = cookies[:theme_ids]&.split("|") - id = ids&.split(",")&.map(&:to_i)&.first - if id.present? && seq && seq.to_i == user_option&.theme_key_seq.to_i - theme_id = id if guardian.allow_themes?([id]) - end - end - - if theme_id.blank? - ids = user_option&.theme_ids || [] - theme_id = ids.first if guardian.allow_themes?(ids) - end - - if theme_id.blank? && SiteSetting.default_theme_id != -1 && - guardian.allow_themes?([SiteSetting.default_theme_id]) - theme_id = SiteSetting.default_theme_id - end - - @theme_id = request.env[:resolved_theme_id] = theme_id + @theme_id ||= ThemeResolver.resolve_theme_id(request, guardian, current_user) end def guardian @@ -518,7 +492,7 @@ class ApplicationController < ActionController::Base end def current_homepage - current_user&.user_option&.homepage || HomepageHelper.resolve(request, current_user) + current_user&.user_option&.homepage || HomepageHelper.resolve(@theme_id, current_user) end def serialize_data(obj, serializer, opts = nil) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index e0a0d22a1ef..9817ac85a06 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -560,7 +560,7 @@ module ApplicationHelper end def current_homepage - current_user&.user_option&.homepage || HomepageHelper.resolve(request, current_user) + current_user&.user_option&.homepage || HomepageHelper.resolve(theme_id, current_user) end def build_plugin_html(name) diff --git a/config/routes.rb b/config/routes.rb index 59f2a7d9ebc..bde57811bbd 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1598,7 +1598,9 @@ Discourse::Application.routes.draw do constraints: HomePageConstraint.new("finish_installation"), as: "installation_redirect" - root to: "custom#index", constraints: HomePageConstraint.new("custom"), as: "custom_index" + root to: "custom_homepage#index", + constraints: HomePageConstraint.new("custom"), + as: "custom_index" get "/user-api-key/new" => "user_api_keys#new" post "/user-api-key" => "user_api_keys#create" diff --git a/lib/homepage_constraint.rb b/lib/homepage_constraint.rb index 3944bf1593c..3c6fbd2bd40 100644 --- a/lib/homepage_constraint.rb +++ b/lib/homepage_constraint.rb @@ -9,7 +9,11 @@ class HomePageConstraint return @filter == "finish_installation" if SiteSetting.has_login_hint? current_user = CurrentUser.lookup_from_env(request.env) - homepage = current_user&.user_option&.homepage || HomepageHelper.resolve(request, current_user) + + # ensures we resolve the theme id as early as possible + theme_id = ThemeResolver.resolve_theme_id(request, Guardian.new(current_user), current_user) + + homepage = current_user&.user_option&.homepage || HomepageHelper.resolve(theme_id, current_user) homepage == @filter rescue Discourse::InvalidAccess, Discourse::ReadOnly false diff --git a/lib/homepage_helper.rb b/lib/homepage_helper.rb index c4a77a24482..9eb684508f8 100644 --- a/lib/homepage_helper.rb +++ b/lib/homepage_helper.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true class HomepageHelper - def self.resolve(request = nil, current_user = nil) - return "custom" if ThemeModifierHelper.new(request: request).custom_homepage + def self.resolve(theme_id = nil, current_user = nil) + return "custom" if ThemeModifierHelper.new(theme_ids: theme_id).custom_homepage current_user ? SiteSetting.homepage : SiteSetting.anonymous_homepage end diff --git a/lib/theme_resolver.rb b/lib/theme_resolver.rb new file mode 100644 index 00000000000..8bae0f7018e --- /dev/null +++ b/lib/theme_resolver.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module ThemeResolver + def self.resolve_theme_id(request, guardian, current_user) + return request.env[:resolved_theme_id] if request.env[:resolved_theme_id] != nil + + theme_id = nil + + if (preview_theme_id = request[:preview_theme_id]&.to_i) && + guardian.allow_themes?([preview_theme_id], include_preview: true) + theme_id = preview_theme_id + end + + user_option = current_user&.user_option + + if theme_id.blank? && request.cookie_jar[:theme_ids].present? + ids, seq = request.cookie_jar[:theme_ids]&.split("|") + id = ids&.split(",")&.map(&:to_i)&.first + if id.present? && seq && seq.to_i == user_option&.theme_key_seq.to_i + theme_id = id if guardian.allow_themes?([id]) + end + end + + if theme_id.blank? + ids = user_option&.theme_ids || [] + theme_id = ids.first if guardian.allow_themes?(ids) + end + + if theme_id.blank? && SiteSetting.default_theme_id != -1 && + guardian.allow_themes?([SiteSetting.default_theme_id]) + theme_id = SiteSetting.default_theme_id + end + + request.env[:resolved_theme_id] = theme_id + end +end diff --git a/spec/lib/homepage_helper_spec.rb b/spec/lib/homepage_helper_spec.rb index c17cbc6c139..e0ab66b028f 100644 --- a/spec/lib/homepage_helper_spec.rb +++ b/spec/lib/homepage_helper_spec.rb @@ -14,7 +14,7 @@ RSpec.describe HomepageHelper do expect(HomepageHelper.resolve).to eq("custom") end - context "when first item in top menu is no valid for anons" do + context "when first item in top menu is not valid for anons" do it "distinguishes between auth homepage and anon homepage" do SiteSetting.top_menu = "new|top|latest|unread"