-
Notifications
You must be signed in to change notification settings - Fork 983
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
Fixes #37179 - Clone hostgroup facets when cloning hostgroup #10055
Fixes #37179 - Clone hostgroup facets when cloning hostgroup #10055
Conversation
a349897
to
c1bba4d
Compare
[test katello] |
c1bba4d
to
4be7be2
Compare
[test katello] |
[test integration] |
4be7be2
to
64c5b96
Compare
changed back to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well!
can you add this unit test diff --git a/test/controllers/hostgroups_controller_test.rb b/test/controllers/hostgroups_controller_test.rb
index 697729e0e..2fd26b7da 100644
--- a/test/controllers/hostgroups_controller_test.rb
+++ b/test/controllers/hostgroups_controller_test.rb
@@ -38,6 +38,24 @@ class HostgroupsControllerTest < ActionController::TestCase
assert_template 'new'
end
+ def test_copies_facets
+ hg = hostgroups(:common)
+ hg.stubs(:facet_definitions).returns([OpenStruct.new(name: :mock_facet)])
+ stubbed_facet = OpenStruct.new(attribute_one:"one", two: "two")
+ hg.instance_eval {self.class.attr_accessor :mock_facet}
+ hg.mock_facet = stubbed_facet
+
+ @controller.stubs(:find_resource)
+ @controller.instance_variable_set(:@hostgroup, hg)
+ get :clone, params: { :id => hg.id }, session: set_session_user
+ assert_template 'new'
+ cloned_hg = @controller.instance_variable_get(:@hostgroup)
+ assert_nil cloned_hg.id
+ assert_nil cloned_hg.name
+ assert_equal stubbed_facet, cloned_hg.mock_facet
+ refute_equal stubbed_facet.object_id, cloned_hg.mock_facet.object_id
+ end
+
def test_edit
get :edit, params: { :id => hostgroups(:common) }, session: set_session_user
assert_template 'edit |
64c5b96
to
1844c31
Compare
Added test 👍 |
load_vars_for_ajax | ||
# Clone all its facets | ||
@hostgroup.facet_definitions.map(&:name).each do |facet_name| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need to do the legwork manually, you can use the SelectiveClone
to do the work for you, similar to the host object:
include_in_clone facet_config.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @ShimShtein . Sounds like the entire PR can be replaced with
diff --git a/app/models/concerns/facets/hostgroup_extensions.rb b/app/models/concerns/facets/hostgroup_extensions.rb
index ba05649f2..4259504e0 100644
--- a/app/models/concerns/facets/hostgroup_extensions.rb
+++ b/app/models/concerns/facets/hostgroup_extensions.rb
@@ -6,7 +6,9 @@ module Facets
include Facets::ModelExtensionsBase
included do
- configure_facet(:hostgroup, :hostgroup, :hostgroup_id)
+ configure_facet(:hostgroup, :hostgroup, :hostgroup_id) do |facet_config|
+ include_in_clone facet_config.name
+ end
end
def hostgroup_ancestry_cache
1844c31
to
45c01e4
Compare
45c01e4
to
3796cfa
Compare
@ShimShtein updated 👍 |
@@ -38,6 +38,23 @@ def test_clone | |||
assert_template 'new' | |||
end | |||
|
|||
def test_copies_facets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test will belong with all facet behavior tests:
foreman/test/unit/facet_test.rb
Line 237 in a9b9b7d
context 'hostgroup behavior' do |
This test file already has the proper setup.
3796cfa
to
da4d6d9
Compare
@ShimShtein Moved and adjusted the test. all this time I didn't realize we were running |
Side note: this appears to still need Katello/katello#10894 in order to work; otherwise I still don't see content view / LCE populated when I clone. |
Merging. Thanks @jeremylenz ! |
I'm afraid this broke
|
When cloning a hostgroup, its facets were not being cloned. Because of this, the hostgroup form would be pre-filled with some attributes, but not all.
This change clones all registered facets when cloning a hostgroup, enabling improvements in plugins such as Katello/katello#10894.