Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose multi-CV feature downstream #85

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/concerns/setting_branding.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ def replace_keywords(keyword)
end

def [](name)
if UpstreamOnlySettings::SETTINGS.include?(name.to_s)
if UpstreamOnlySettings.include?(name.to_s)
Rails.logger.debug "Setting '#{name}' is not available in Satellite; ignoring"
return nil
end
Expand Down
11 changes: 9 additions & 2 deletions app/models/concerns/upstream_only_settings.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
# Settings to hide in downstream (will return nil for all values)
module UpstreamOnlySettings
class UpstreamOnlySettings
SETTINGS = %w[
allow_multiple_content_views
].freeze

def self.include?(key)
new.include?(key)
end

def include?(key)
SETTINGS.include?(key.to_s)
end
end
22 changes: 19 additions & 3 deletions test/unit/setting_registry_branding_test.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'test_plugin_helper'

class SettingRegistryBrandingTest < ActiveSupport::TestCase
# rubocop:disable Metrics/BlockLength
describe 'stubbed creation' do
test 'it replaces value to a branded one using DSL' do
Setting.where(name: 'dsl_setting').delete_all
Expand All @@ -27,14 +28,29 @@ class SettingRegistryBrandingTest < ActiveSupport::TestCase
end

test 'hides upstream-only settings' do
assert_nil Setting['allow_multiple_content_views']
Foreman::SettingManager.define('test2') do
category(:Content) do
setting(
:test_setting,
description: N_("Upstream-only setting test"),
default: 'definitely not falsey',
full_name: N_('Upstream-only setting test'),
type: :string
)
end
end
Foreman.settings.load
UpstreamOnlySettings.expects(:include?).with('test_setting').returns(true)
assert_nil Setting['test_setting']
Comment on lines +43 to +44
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You never add test_setting to UpstreamOnlySettings::SETTINGS, so this test is not testing what it says to be testing.
Wouldn't you need to first declare that setting with a default value, then add it to UpstreamOnlySettings and then assert the result is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't

UpstreamOnlySettings::SETTINGS << 'test_setting'

due to

FrozenError: can't modify frozen Array: []

But I did add the setting definition in the test.
(and did confirm it does fail when I comment out the

UpstreamOnlySettings.expects(:include?).with('test_setting').returns(true)

)

does that look okay?

end
end
# rubocop:enable Metrics/BlockLength
end

class SettingBrandingTest < ActiveSupport::TestCase
test 'replaces warning for upstream-only settings' do
Rails.logger.expects(:debug).with('Setting \'allow_multiple_content_views\' is not available in Satellite; ignoring')
Setting['allow_multiple_content_views']
UpstreamOnlySettings.expects(:include?).with('test_setting').returns(true)
Rails.logger.expects(:debug).with('Setting \'test_setting\' is not available in Satellite; ignoring')
Setting['test_setting']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not defining the test_setting in settings here. Are you relying on the test execution order?

Also not related to this specific PR: why do we have two classes in a single file? Can we separate them please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this relies on the fact that Setting[something] always queries the Settings registry and triggers the warning code, even if the setting is not actually defined. (The test worked also before the setting definition in the previous test that I asked for)

end
end