Fix body element theme class not updating when user changes theme by haru · Pull Request #26 · haru/redmine_theme_changer

haru · 2025-12-23T13:04:30Z

…s/ directories

….1 and 7+

…r functionality

codecov · 2025-12-23T13:06:29Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (734a240) to head (288f016).
⚠️ Report is 8 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop       #26   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         6    +1     
  Lines           57        72   +15     
=========================================
+ Hits            57        72   +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR fixes issue #25 where the CSS class on the body element was not updating when users changed their theme preference. The fix introduces a new ApplicationHelper patch that overrides the body_css_classes method to inject user-specific theme classes into the HTML body element.

Key Changes:

Added new ThemeChangerApplicationHelperPatch module to override body_css_classes and inject user-specific theme CSS classes
Added comprehensive unit tests for the new body_css_classes functionality
Refactored test helper to improve fixture loading and remove deprecated simplecov-rcov dependency

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
lib/theme_changer_application_helper_patch.rb	New patch module that overrides body_css_classes to dynamically inject user's theme class into the body element
init.rb	Registers the new application helper patch module in the plugin initialization
test/unit/application_helper_patch_test.rb	New test file with unit tests for body_css_classes behavior with different user themes and anonymous users
test/test_helper.rb	Refactored to use double quotes, automatic fixture loading, and removed deprecated simplecov-rcov formatter
.gitignore	Added local development files (.claude/settings.local.json and docs/) to ignore list

Copilot · 2025-12-23T13:11:54Z

+    # Only modify theme class for logged-in users
+    return css_classes unless User.current.logged?
+
+    # Get user's theme setting
+    setting = ThemeChangerUserSetting.find_theme_by_user_id(User.current.id)
+
+    # If no setting or system setting is selected, use default behavior
+    return css_classes unless setting
+    return css_classes if setting.theme == ThemeChangerUserSetting::SYSTEM_SETTING
+
+    # Remove existing theme class from the CSS classes
+    css_classes = css_classes.gsub(/\btheme-\S+/, '').strip
+
+    # Add new theme class at the beginning if a specific theme is set
+    unless setting.theme.blank? || setting.theme == ThemeChangerUserSetting::DEFAULT_THEME
+      theme = Redmine::Themes.theme(setting.theme)
+      if theme
+        css_classes = "theme-#{theme.name.tr(' ', '_')} #{css_classes}"
+      end
+    end


The body_css_classes patch duplicates theme resolution logic that already exists in ThemeChangerThemesPatch (get_theme and current_theme methods). Instead of re-implementing theme lookup with ThemeChangerUserSetting.find_theme_by_user_id and handling SYSTEM_SETTING/DEFAULT_THEME cases, consider reusing the existing current_theme method which already handles this logic. This would make the code more maintainable and ensure consistent behavior across the plugin.

Suggested change

# Only modify theme class for logged-in users

return css_classes unless User.current.logged?

# Get user's theme setting

setting = ThemeChangerUserSetting.find_theme_by_user_id(User.current.id)

# If no setting or system setting is selected, use default behavior

return css_classes unless setting

return css_classes if setting.theme == ThemeChangerUserSetting::SYSTEM_SETTING

# Remove existing theme class from the CSS classes

css_classes = css_classes.gsub(/\btheme-\S+/, '').strip

# Add new theme class at the beginning if a specific theme is set

unless setting.theme.blank? || setting.theme == ThemeChangerUserSetting::DEFAULT_THEME

theme = Redmine::Themes.theme(setting.theme)

if theme

css_classes = "theme-#{theme.name.tr(' ', '_')} #{css_classes}"

end

end

# Use the centralized theme resolution logic from ThemeChangerThemesPatch

theme = current_theme

return css_classes unless theme

# Remove existing theme class from the CSS classes

css_classes = css_classes.gsub(/\btheme-\S+/, '').strip

# Add new theme class at the beginning based on the resolved theme

css_classes = "theme-#{theme.name.tr(' ', '_')} #{css_classes}".strip

Copilot · 2025-12-23T13:11:54Z

+  def test_body_css_classes_with_user_theme
+    User.current = User.find(1)
+    css_classes = body_css_classes
+    assert_match(/\Atheme-Classic/, css_classes)
+  end
+
+  def test_body_css_classes_with_alternate_theme
+    User.current = User.find(2)
+    css_classes = body_css_classes
+    assert_match(/\Atheme-Alternate/, css_classes)
+  end
+
+  def test_body_css_classes_when_not_logged_in
+    User.current = User.anonymous
+    with_settings :ui_theme => 'classic' do
+      css_classes = body_css_classes
+      assert_match(/\Atheme-/, css_classes)
+    end
+  end


The test suite is missing coverage for important edge cases of the body_css_classes method. Consider adding tests for: 1) when a user has SYSTEM_SETTING selected (should use system default), 2) when a user has DEFAULT_THEME selected (should use default behavior), 3) when a user's saved theme is invalid or no longer available (the theme lookup returns nil), and 4) when a user has a theme setting but the theme value is blank.

Copilot · 2025-12-23T13:11:55Z

+    # If no setting or system setting is selected, use default behavior
+    return css_classes unless setting
+    return css_classes if setting.theme == ThemeChangerUserSetting::SYSTEM_SETTING
+
+    # Remove existing theme class from the CSS classes
+    css_classes = css_classes.gsub(/\btheme-\S+/, '').strip
+
+    # Add new theme class at the beginning if a specific theme is set
+    unless setting.theme.blank? || setting.theme == ThemeChangerUserSetting::DEFAULT_THEME


The logic for handling DEFAULT_THEME may not work as intended. When setting.theme equals DEFAULT_THEME, the method returns the original css_classes without modifications. However, the original css_classes from super already contains a theme class based on the system default. This means DEFAULT_THEME behaves identically to SYSTEM_SETTING. If DEFAULT_THEME is meant to represent a different behavior (e.g., no theme or a specific default), the current logic may not implement that correctly. Clarify whether DEFAULT_THEME should have distinct behavior from SYSTEM_SETTING.

Suggested change

# If no setting or system setting is selected, use default behavior

return css_classes unless setting

return css_classes if setting.theme == ThemeChangerUserSetting::SYSTEM_SETTING

# Remove existing theme class from the CSS classes

css_classes = css_classes.gsub(/\btheme-\S+/, '').strip

# Add new theme class at the beginning if a specific theme is set

unless setting.theme.blank? || setting.theme == ThemeChangerUserSetting::DEFAULT_THEME

# If no setting, system setting, or default theme is selected, use default behavior

return css_classes unless setting

return css_classes if setting.theme == ThemeChangerUserSetting::SYSTEM_SETTING ||

setting.theme == ThemeChangerUserSetting::DEFAULT_THEME

# Remove existing theme class from the CSS classes

css_classes = css_classes.gsub(/\btheme-\S+/, '').strip

# Add new theme class at the beginning if a specific theme is set

unless setting.theme.blank?

Copilot · 2025-12-23T13:11:55Z

+    # Remove existing theme class from the CSS classes
+    css_classes = css_classes.gsub(/\btheme-\S+/, '').strip
+
+    # Add new theme class at the beginning if a specific theme is set
+    unless setting.theme.blank? || setting.theme == ThemeChangerUserSetting::DEFAULT_THEME
+      theme = Redmine::Themes.theme(setting.theme)
+      if theme
+        css_classes = "theme-#{theme.name.tr(' ', '_')} #{css_classes}"


The regex pattern /\btheme-\S+/ used to remove existing theme classes will match and remove any class starting with 'theme-' followed by non-whitespace characters. This could potentially match theme-related classes beyond just the main theme class (e.g., 'theme-switcher', 'theme-custom-class'). Consider using a more specific pattern that only matches the exact theme class format, or verify that no other classes in Redmine start with 'theme-' prefix to avoid unintended removals.

Suggested change

# Remove existing theme class from the CSS classes

css_classes = css_classes.gsub(/\btheme-\S+/, '').strip

# Add new theme class at the beginning if a specific theme is set

unless setting.theme.blank? || setting.theme == ThemeChangerUserSetting::DEFAULT_THEME

theme = Redmine::Themes.theme(setting.theme)

if theme

css_classes = "theme-#{theme.name.tr(' ', '_')} #{css_classes}"

# Remove existing theme class from the CSS classes.

# Only remove classes that correspond to actual Redmine themes.

css_class_tokens = css_classes.split(/\s+/)

css_class_tokens.reject! do |klass|

next false unless klass.start_with?('theme-')

theme_name = klass.sub(/\Atheme-/, '')

!theme_name.empty? && Redmine::Themes.theme(theme_name)

end

css_classes = css_class_tokens.join(' ')

# Add new theme class at the beginning if a specific theme is set

unless setting.theme.blank? || setting.theme == ThemeChangerUserSetting::DEFAULT_THEME

theme = Redmine::Themes.theme(setting.theme)

if theme

css_classes = "theme-#{theme.name.tr(' ', '_')} #{css_classes}".strip

Copilot · 2025-12-23T13:11:55Z

+  fixtures = Dir.glob("*.yml").map { |s| s.gsub(/.yml$/, "") }
+end
+ActiveRecord::FixtureSet.create_fixtures(File.dirname(__FILE__) + "/fixtures/", fixtures)



The fixture loading logic uses Dir.glob to load all .yml files from the fixtures directory, but this will only load fixtures from this plugin's test/fixtures directory. If the tests depend on Redmine core fixtures (such as users.yml), those need to be explicitly listed or already loaded by the parent test_helper. Verify that all required fixtures (especially 'users' referenced in line 22 of application_helper_patch_test.rb) are available when tests run.

Suggested change

# Ensure Redmine core fixtures needed by plugin tests are available (e.g. users.yml)

core_fixtures_path = File.expand_path(File.dirname(__FILE__) + "/../../../test/fixtures")

users_fixture_file = File.join(core_fixtures_path, "users.yml")

if File.exist?(users_fixture_file)

ActiveRecord::FixtureSet.create_fixtures(core_fixtures_path, ["users"])

end

Copilot · 2025-12-23T13:11:55Z

+    assert_match(/\Atheme-Classic/, css_classes)
+  end
+
+  def test_body_css_classes_with_alternate_theme
+    User.current = User.find(2)
+    css_classes = body_css_classes
+    assert_match(/\Atheme-Alternate/, css_classes)


The test assertion uses a regex pattern that only checks if the CSS classes start with 'theme-Classic', but doesn't verify the exact theme class or that it's properly placed in the string. Consider using a more specific assertion that verifies the exact theme class is present, such as checking for the complete class name or using assert_includes to verify 'theme-Classic' is in the space-separated list of classes.

Suggested change

assert_match(/\Atheme-Classic/, css_classes)

end

def test_body_css_classes_with_alternate_theme

User.current = User.find(2)

css_classes = body_css_classes

assert_match(/\Atheme-Alternate/, css_classes)

assert_includes css_classes.split(/\s+/), 'theme-Classic'

end

def test_body_css_classes_with_alternate_theme

User.current = User.find(2)

css_classes = body_css_classes

assert_includes css_classes.split(/\s+/), 'theme-Alternate'

Copilot · 2025-12-23T13:11:56Z

+  def test_body_css_classes_with_alternate_theme
+    User.current = User.find(2)
+    css_classes = body_css_classes
+    assert_match(/\Atheme-Alternate/, css_classes)


The test assertion uses a regex pattern that only checks if the CSS classes start with 'theme-Alternate', but doesn't verify the exact theme class or that it's properly placed in the string. Consider using a more specific assertion that verifies the exact theme class is present, such as checking for the complete class name or using assert_includes to verify 'theme-Alternate' is in the space-separated list of classes.

Suggested change

assert_match(/\Atheme-Alternate/, css_classes)

css_class_list = css_classes.to_s.split(/\s+/)

assert_includes css_class_list, 'theme-Alternate'

Copilot · 2025-12-23T13:11:56Z

+    User.current = User.anonymous
+    with_settings :ui_theme => 'classic' do
+      css_classes = body_css_classes
+      assert_match(/\Atheme-/, css_classes)


The test only verifies that a theme class exists (starts with 'theme-') but doesn't check which theme is being used. This test would pass even if the wrong theme is applied. Consider asserting that the system default theme is being used by checking the actual value from Setting.ui_theme or verifying the absence of user-specific theme classes.

haru added 4 commits December 23, 2025 12:35

fix: update .gitignore to include .claude/settings.local.json and doc…

382fe54

…s/ directories

Fix body element theme class not updating when user changes theme (#25)

9b06918

fix: update fixture path configuration for compatibility with Rails 6…

1f5b02a

….1 and 7+

fix: update test helper and application helper tests for theme change…

288f016

…r functionality

haru requested a review from Copilot December 23, 2025 13:05

Copilot started reviewing on behalf of haru December 23, 2025 13:07 View session

Copilot AI reviewed Dec 23, 2025

View reviewed changes

haru merged commit 7130449 into develop Dec 23, 2025
132 of 134 checks passed

haru deleted the bugfix/25-body-class-update branch December 23, 2025 13:17

haru added the bug label Dec 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix body element theme class not updating when user changes theme#26

Fix body element theme class not updating when user changes theme#26
haru merged 4 commits into
developfrom
bugfix/25-body-class-update

haru commented Dec 23, 2025

Uh oh!

codecov Bot commented Dec 23, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Copilot AI Dec 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

+# Ensure Redmine core fixtures needed by plugin tests are available (e.g. users.yml)
+core_fixtures_path = File.expand_path(File.dirname(__FILE__) + "/../../../test/fixtures")
+users_fixture_file = File.join(core_fixtures_path, "users.yml")
+if File.exist?(users_fixture_file)
+  ActiveRecord::FixtureSet.create_fixtures(core_fixtures_path, ["users"])
+end

	assert_match(/\Atheme-Alternate/, css_classes)
	css_class_list = css_classes.to_s.split(/\s+/)
	assert_includes css_class_list, 'theme-Alternate'

Conversation

haru commented Dec 23, 2025

Uh oh!

codecov Bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Dec 23, 2025 •

edited

Loading