Correctly save policy_name and policy_group with `knife node edit`jr/knife-node-edit-save-policy_name-group

Fixes #4364

- Add specs for Chef::Knife::NodeEditor.
- Add `policy_name` and `policy_group` to properties compared in
  `NodeEditor#updated?`.
- Add `policy_name` and `policy_group` to Hash returned by
  `NodeEditor#view`.
- Use `Node.from_hash` in `NodeEditor#apply_updates` instead of
  duplicating functionality.
- Add some YARD docs for Chef::Knife::NodeEditor.

2 files changed, 267 insertions, 35 deletions

diff --git a/lib/chef/knife/core/node_editor.rb b/lib/chef/knife/core/node_editor.rb
index fe14e18d9d..7b7cb72df9 100644
--- a/lib/chef/knife/core/node_editor.rb
+++ b/lib/chef/knife/core/node_editor.rb
@@ -1,6 +1,7 @@
 #
 # Author:: Daniel DeLeo (<dan@opscode.com>)
-# Copyright:: Copyright (c) 2011 Opscode, Inc.
+# Author:: Jordan Running (<jr@chef.io>)
+# Copyright:: Copyright (c) 2011-2016 Chef Software, Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -18,81 +19,101 @@
 
 require 'chef/json_compat'
 require 'chef/node'
-require 'tempfile'
 
 class Chef
   class Knife
     class NodeEditor
+      attr_reader :node, :ui, :config
+      private :node, :ui, :config
 
-      attr_reader :node
-      attr_reader :ui
-      attr_reader :config
-
+      # @param node [Chef::Node]
+      # @param ui [Chef::Knife::UI]
+      # @param config [Hash]
       def initialize(node, ui, config)
         @node, @ui, @config = node, ui, config
       end
 
+      # Opens the node data (as JSON) in the user's editor and returns a new
+      # {Chef::Node} reflecting the user's changes.
+      #
+      # @return [Chef::Node]
       def edit_node
         abort "You specified the --disable_editing option, nothing to edit" if config[:disable_editing]
         assert_editor_set!
 
-        updated_node_data = @ui.edit_data(view)
+        updated_node_data = ui.edit_data(view)
         apply_updates(updated_node_data)
         @updated_node
       end
 
+      # Returns an array of the names of properties that have been changed or
+      # +false+ if none were changed.
+      #
+      # @return [Array<String>] if any properties have been changed.
+      # @return [false] if no properties have been changed.
       def updated?
+        return false if @updated_node.nil?
+
         pristine_copy = Chef::JSONCompat.parse(Chef::JSONCompat.to_json(node))
         updated_copy  = Chef::JSONCompat.parse(Chef::JSONCompat.to_json(@updated_node))
-        unless pristine_copy == updated_copy
-          updated_properties = %w{name normal chef_environment run_list default override automatic}.reject do |key|
-             pristine_copy[key] == updated_copy[key]
-          end
+
+        updated_properties = %w[
+          name
+          chef_environment
+          automatic
+          default
+          normal
+          override
+          policy_name
+          policy_group
+          run_list
+        ].reject do |key|
+          pristine_copy[key] == updated_copy[key]
         end
-        ( pristine_copy != updated_copy ) && updated_properties
-      end
 
-      private
+        updated_properties.any? && updated_properties
+      end
 
+      # @api private
       def view
-        result = {}
-        result["name"] = node.name
-        result["chef_environment"] = node.chef_environment
-        result["normal"] = node.normal_attrs
-        result["run_list"] = node.run_list
+        result = {
+          "name" => node.name,
+          "chef_environment" => node.chef_environment,
+          "normal" => node.normal_attrs,
+          "policy_name" => node.policy_name,
+          "policy_group" => node.policy_group,
+          "run_list" => node.run_list,
+        }
 
         if config[:all_attributes]
           result["default"]   = node.default_attrs
           result["override"]  = node.override_attrs
           result["automatic"] = node.automatic_attrs
         end
+
         result
       end
 
+      # @api private
       def apply_updates(updated_data)
         if node.name and node.name != updated_data["name"]
           ui.warn "Changing the name of a node results in a new node being created, #{node.name} will not be modified or removed."
-          confirm = ui.confirm "Proceed with creation of new node"
+          ui.confirm "Proceed with creation of new node"
         end
 
-        @updated_node = Node.new.tap do |n|
-          n.name( updated_data["name"] )
-          n.chef_environment( updated_data["chef_environment"] )
-          n.run_list( updated_data["run_list"])
-          n.normal_attrs = updated_data["normal"]
-
-          if config[:all_attributes]
-            n.default_attrs   = updated_data["default"]
-            n.override_attrs  = updated_data["override"]
-            n.automatic_attrs = updated_data["automatic"]
-          else
-            n.default_attrs   = node.default_attrs
-            n.override_attrs  = node.override_attrs
-            n.automatic_attrs = node.automatic_attrs
-          end
+        data = updated_data.dup
+
+        unless config[:all_attributes]
+          data['automatic'] = node.automatic_attrs
+          data['default'] = node.default_attrs
+          data['override'] = node.override_attrs
         end
+
+        @updated_node = Node.from_hash(data)
       end
 
+      private
+
       def abort(message)
         ui.error(message)
         exit 1
diff --git a/spec/unit/knife/core/node_editor_spec.rb b/spec/unit/knife/core/node_editor_spec.rb
new file mode 100644
index 0000000000..52e3aeadd5
--- /dev/null
+++ b/spec/unit/knife/core/node_editor_spec.rb
@@ -0,0 +1,211 @@
+#
+# Author:: Jordan Running (<jr@chef.io>)
+# Copyright:: Copyright (c) 2016 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'spec_helper'
+require 'chef/knife/core/node_editor'
+
+describe Chef::Knife::NodeEditor do
+  let(:node_data) do
+    { 'name' => 'test_node',
+      'chef_environment' => 'production',
+      'automatic' => { 'foo' => 'bar' },
+      'default'   => { 'alpha' => { 'bravo' => 'charlie', 'delta' => 'echo' } },
+      'normal'    => { 'alpha' => { 'bravo' => 'hotel' }, 'tags' => [] },
+      'override'  => { 'alpha' => { 'bravo' => 'foxtrot', 'delta' => 'golf' } },
+      'policy_name'  => nil,
+      'policy_group' => nil,
+      'run_list' => %w(role[comedy] role[drama] recipe[mystery])
+    }
+  end
+
+  let(:node) { Chef::Node.from_hash(node_data) }
+
+  let(:ui) { double 'ui' }
+  let(:base_config) { { editor: 'cat' } }
+  let(:config) { base_config.merge(all_attributes: false) }
+
+  subject { described_class.new(node, ui, config) }
+
+  describe '#view' do
+    it 'returns a Hash with only the name, chef_environment, normal, ' +
+       'policy_name, policy_group, and run_list properties' do
+      expected = node_data.select do |key,|
+        %w[ name chef_environment normal
+            policy_name policy_group run_list ].include?(key)
+      end
+
+      expect(subject.view).to eq(expected)
+    end
+
+    context 'when config[:all_attributes] == true' do
+      let(:config) { base_config.merge(all_attributes: true) }
+
+      it 'returns a Hash with all of the node\'s properties' do
+        expect(subject.view).to eq(node_data)
+      end
+    end
+  end
+
+  describe '#apply_updates' do
+    context 'when the node name is changed' do
+      before(:each) do
+        allow(ui).to receive(:warn)
+        allow(ui).to receive(:confirm).and_return(true)
+      end
+
+      it 'emits a warning and prompts for confirmation' do
+        data = subject.view.merge('name' => 'foo_new_name_node')
+        updated_node = subject.apply_updates(data)
+
+        expect(ui).to have_received(:warn)
+          .with 'Changing the name of a node results in a new node being ' +
+                'created, test_node will not be modified or removed.'
+
+        expect(ui).to have_received(:confirm)
+          .with('Proceed with creation of new node')
+
+        expect(updated_node).to be_a(Chef::Node)
+      end
+    end
+
+    context 'when config[:all_attributes] == false' do
+      let(:config) { base_config.merge(all_attributes: false) }
+
+      let(:updated_data) do
+        subject.view.merge(
+          'normal' => { 'alpha' => { 'bravo' => 'hotel2' }, 'tags' => [ 'xyz' ] },
+          'policy_name' => 'mypolicy',
+          'policy_group' => 'prod',
+          'run_list' => %w(role[drama] recipe[mystery])
+        )
+      end
+
+      it 'returns a node with run_list and normal_attrs changed' do
+        updated_node = subject.apply_updates(updated_data)
+        expect(updated_node).to be_a(Chef::Node)
+
+        # Expected to have been changed
+        expect(updated_node.chef_environment).to eql(updated_data['chef_environment'])
+        expect(updated_node.normal_attrs).to eql(updated_data['normal'])
+        expect(updated_node.policy_name).to eql(updated_data['policy_name'])
+        expect(updated_node.policy_group).to eql(updated_data['policy_group'])
+        expect(updated_node.run_list.map(&:to_s)).to eql(updated_data['run_list'])
+
+        # Expected not to have changed
+        expect(updated_node.default_attrs).to eql(node.default_attrs)
+        expect(updated_node.override_attrs).to eql(node.override_attrs)
+        expect(updated_node.automatic_attrs).to eql(node.automatic_attrs)
+      end
+    end
+
+    context 'when config[:all_attributes] == true' do
+      let(:config) { base_config.merge(all_attributes: true) }
+
+      let(:updated_data) do
+        subject.view.merge(
+          'default'   => { 'alpha' => { 'bravo' => 'charlie2', 'delta' => 'echo2' } },
+          'normal'    => { 'alpha' => { 'bravo' => 'hotel2' }, 'tags' => [ 'xyz' ] },
+          'override'  => { 'alpha' => { 'bravo' => 'foxtrot2', 'delta' => 'golf2' } },
+          'policy_name' => 'mypolicy',
+          'policy_group' => 'prod',
+          'run_list'  => %w(role[drama] recipe[mystery])
+        )
+      end
+
+      it 'returns a node with all editable properties changed' do
+        updated_node = subject.apply_updates(updated_data)
+        expect(updated_node).to be_a(Chef::Node)
+
+        expect(updated_node.chef_environment).to eql(updated_data['chef_environment'])
+        expect(updated_node.automatic_attrs).to eql(updated_data['automatic'])
+        expect(updated_node.normal_attrs).to eql(updated_data['normal'])
+        expect(updated_node.default_attrs).to eql(updated_data['default'])
+        expect(updated_node.override_attrs).to eql(updated_data['override'])
+        expect(updated_node.policy_name).to eql(updated_data['policy_name'])
+        expect(updated_node.policy_group).to eql(updated_data['policy_group'])
+        expect(updated_node.run_list.map(&:to_s)).to eql(updated_data['run_list'])
+      end
+    end
+  end
+
+  describe '#updated?' do
+    context 'before the node has been edited' do
+      it 'returns false' do
+        expect(subject.updated?).to be false
+      end
+    end
+
+    context 'after the node has been edited' do
+      context 'and changes were made' do
+        let(:updated_data) do
+          subject.view.merge(
+            'default'   => { 'alpha' => { 'bravo' => 'charlie2', 'delta' => 'echo2' } },
+            'normal'    => { 'alpha' => { 'bravo' => 'hotel2' }, 'tags' => [ 'xyz' ] },
+            'override'  => { 'alpha' => { 'bravo' => 'foxtrot2', 'delta' => 'golf2' } },
+            'policy_name'  => 'mypolicy',
+            'policy_group' => 'prod',
+            'run_list'  => %w(role[drama] recipe[mystery])
+          )
+        end
+
+        context 'and changes affect only editable properties' do
+          before(:each) do
+            allow(ui).to receive(:edit_data)
+              .with(subject.view)
+              .and_return(updated_data)
+
+            subject.edit_node
+          end
+
+          it 'returns an array of the changed property names' do
+            expect(subject.updated?).to eql %w[ normal policy_name policy_group run_list ]
+          end
+        end
+
+        context 'and the changes include non-editable properties' do
+          before(:each) do
+            data = updated_data.merge('bad_property' => 'bad_value')
+
+            allow(ui).to receive(:edit_data)
+              .with(subject.view)
+              .and_return(data)
+
+            subject.edit_node
+          end
+
+          it 'returns an array of property names that doesn\'t include ' +
+             'the non-editable properties' do
+            expect(subject.updated?).to eql %w[ normal policy_name policy_group run_list ]
+          end
+        end
+      end
+
+      context 'and changes were not made' do
+        before(:each) do
+          allow(ui).to receive(:edit_data)
+            .with(subject.view)
+            .and_return(subject.view.dup)
+
+          subject.edit_node
+        end
+
+        it { is_expected.not_to be_updated }
+      end
+    end
+  end
+end