mirror of
https://github.com/discourse/discourse.git
synced 2025-06-25 00:31:34 +08:00
FIX: N+1 in admin themes page (#32763)
`strict_loading` was added to prevent it happening in the future. Few adjustments had to be made: - include color_scheme and color_scheme_colors, also for parent and child themes; - internal translations were using preload_fields, but it was too deep to correctly use preloaded tables. I had to pass `preloaded_locale_fields` manually; - include theme in color_scheme. Before: <img width="663" alt="Screenshot 2025-05-15 at 3 43 47 pm" src="https://github.com/user-attachments/assets/b55ce11e-80cb-43eb-8e31-940b0e9859f3" /> After: <img width="665" alt="Screenshot 2025-05-16 at 11 29 00 am" src="https://github.com/user-attachments/assets/f00bac19-f64b-4048-b220-4d0a9d90a929" />
This commit is contained in:

committed by
GitHub

parent
b823fb297f
commit
76e1373b04
@ -169,8 +169,10 @@ class Admin::ThemesController < Admin::AdminController
|
|||||||
end
|
end
|
||||||
|
|
||||||
def index
|
def index
|
||||||
@themes = Theme.include_relations.order(:name)
|
@themes = Theme.strict_loading.include_relations.order(:name)
|
||||||
@color_schemes = ColorScheme.all.includes(:theme, color_scheme_colors: :color_scheme).to_a
|
|
||||||
|
@color_schemes =
|
||||||
|
ColorScheme.strict_loading.all.includes(:theme, color_scheme_colors: :color_scheme).to_a
|
||||||
|
|
||||||
payload = {
|
payload = {
|
||||||
themes: serialize_data(@themes, ThemeSerializer),
|
themes: serialize_data(@themes, ThemeSerializer),
|
||||||
|
@ -84,11 +84,11 @@ class Theme < ActiveRecord::Base
|
|||||||
scope :include_relations,
|
scope :include_relations,
|
||||||
-> do
|
-> do
|
||||||
include_basic_relations.includes(
|
include_basic_relations.includes(
|
||||||
:child_themes,
|
|
||||||
:theme_settings,
|
:theme_settings,
|
||||||
:settings_field,
|
:settings_field,
|
||||||
:color_scheme,
|
color_scheme: %i[color_scheme_colors],
|
||||||
theme_fields: %i[upload theme_settings_migration],
|
theme_fields: %i[upload theme_settings_migration],
|
||||||
|
child_themes: %i[color_scheme locale_fields theme_translation_overrides],
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
|
||||||
@ -99,7 +99,8 @@ class Theme < ActiveRecord::Base
|
|||||||
:user,
|
:user,
|
||||||
:locale_fields,
|
:locale_fields,
|
||||||
:theme_translation_overrides,
|
:theme_translation_overrides,
|
||||||
parent_themes: %i[locale_fields theme_translation_overrides],
|
color_scheme: %i[theme],
|
||||||
|
parent_themes: %i[color_scheme locale_fields theme_translation_overrides],
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
|
||||||
@ -658,15 +659,16 @@ class Theme < ActiveRecord::Base
|
|||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def internal_translations
|
def internal_translations(preloaded_locale_fields: nil)
|
||||||
@internal_translations ||= translations(internal: true)
|
@internal_translations ||=
|
||||||
|
translations(internal: true, preloaded_locale_fields: preloaded_locale_fields)
|
||||||
end
|
end
|
||||||
|
|
||||||
def translations(internal: false)
|
def translations(internal: false, preloaded_locale_fields: nil)
|
||||||
fallbacks = I18n.fallbacks[I18n.locale]
|
fallbacks = I18n.fallbacks[I18n.locale]
|
||||||
begin
|
begin
|
||||||
data =
|
data =
|
||||||
locale_fields.first&.translation_data(
|
(preloaded_locale_fields&.first || locale_fields.first)&.translation_data(
|
||||||
with_overrides: false,
|
with_overrides: false,
|
||||||
internal: internal,
|
internal: internal,
|
||||||
fallback_fields: locale_fields,
|
fallback_fields: locale_fields,
|
||||||
|
@ -12,6 +12,9 @@ class BasicThemeSerializer < ApplicationSerializer
|
|||||||
end
|
end
|
||||||
|
|
||||||
def description
|
def description
|
||||||
object.internal_translations.find { |t| t.key == "theme_metadata.description" }&.value
|
object
|
||||||
|
.internal_translations(preloaded_locale_fields: object.locale_fields)
|
||||||
|
.find { |t| t.key == "theme_metadata.description" }
|
||||||
|
&.value
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -8,10 +8,6 @@ class ColorSchemeSerializer < ApplicationSerializer
|
|||||||
object.theme&.name
|
object.theme&.name
|
||||||
end
|
end
|
||||||
|
|
||||||
def theme_id
|
|
||||||
object.theme&.id
|
|
||||||
end
|
|
||||||
|
|
||||||
def colors
|
def colors
|
||||||
db_colors = object.colors.index_by(&:name)
|
db_colors = object.colors.index_by(&:name)
|
||||||
object.resolved_colors.map do |name, default|
|
object.resolved_colors.map do |name, default|
|
||||||
|
@ -476,6 +476,44 @@ RSpec.describe Admin::ThemesController do
|
|||||||
|
|
||||||
expect(theme_json["remote_theme"]["remote_version"]).to eq("7")
|
expect(theme_json["remote_theme"]["remote_version"]).to eq("7")
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it "does not result in N+1 queries" do
|
||||||
|
# warmup
|
||||||
|
get "/admin/themes.json"
|
||||||
|
expect(response.status).to eq(200)
|
||||||
|
|
||||||
|
theme = Fabricate(:theme, color_scheme: Fabricate(:color_scheme))
|
||||||
|
Fabricate(
|
||||||
|
:theme_field,
|
||||||
|
target_id: Theme.targets[:translations],
|
||||||
|
theme: theme,
|
||||||
|
name: "en",
|
||||||
|
value:
|
||||||
|
"en:\n theme_metadata:\n description: \"A simple, beautiful theme that improves the out of the box experience for Discourse sites.\"\n topic_pinned: \"Pinned\"\n topic_hot: \"Hot\"\n user_replied: \"replied\"\n user_posted: \"posted\"\n user_updated: \"updated\"\n",
|
||||||
|
)
|
||||||
|
first_request_queries =
|
||||||
|
track_sql_queries do
|
||||||
|
get "/admin/themes.json"
|
||||||
|
expect(response.status).to eq(200)
|
||||||
|
end
|
||||||
|
|
||||||
|
theme_2 = Fabricate(:theme, color_scheme: Fabricate(:color_scheme))
|
||||||
|
Fabricate(
|
||||||
|
:theme_field,
|
||||||
|
target_id: Theme.targets[:translations],
|
||||||
|
theme: theme_2,
|
||||||
|
name: "en",
|
||||||
|
value:
|
||||||
|
"en:\n theme_metadata:\n description: \"A simple, beautiful theme that improves the out of the box experience for Discourse sites.\"\n topic_pinned: \"Pinned\"\n topic_hot: \"Hot\"\n user_replied: \"replied\"\n user_posted: \"posted\"\n user_updated: \"updated\"\n",
|
||||||
|
)
|
||||||
|
second_request_queries =
|
||||||
|
track_sql_queries do
|
||||||
|
get "/admin/themes.json"
|
||||||
|
expect(response.status).to eq(200)
|
||||||
|
end
|
||||||
|
|
||||||
|
expect(first_request_queries.count).to eq(second_request_queries.count)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
it "allows themes and components to be edited" do
|
it "allows themes and components to be edited" do
|
||||||
|
Reference in New Issue
Block a user