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.

1 files changed, 56 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