From 827b62356aade287f886be85d63897e955e7b6f7 Mon Sep 17 00:00:00 2001 From: Andrew Cantino Date: Sun, 8 Jun 2014 23:37:15 -0700 Subject: [PATCH] the import process now allows you to merge your agents with the incoming ones; next step is better UI --- .../javascripts/application.js.coffee.erb | 11 +- .../stylesheets/application.css.scss.erb | 6 +- app/assets/stylesheets/scenarios.css.scss | 7 - .../scenario_imports_controller.rb | 9 +- app/models/scenario_import.rb | 157 +++++++- .../manual_event_agent/_show.html.erb | 2 +- app/views/scenario_imports/_step_one.html.erb | 51 +-- app/views/scenario_imports/_step_two.html.erb | 200 +++++++--- app/views/scenario_imports/new.html.erb | 34 +- lib/agents_exporter.rb | 9 +- .../scenario_imports_controller_spec.rb | 5 +- spec/lib/agents_exporter_spec.rb | 5 +- spec/models/scenario_import_spec.rb | 355 +++++++++++++----- 13 files changed, 608 insertions(+), 243 deletions(-) diff --git a/app/assets/javascripts/application.js.coffee.erb b/app/assets/javascripts/application.js.coffee.erb index ee34948a..1f78c27a 100644 --- a/app/assets/javascripts/application.js.coffee.erb +++ b/app/assets/javascripts/application.js.coffee.erb @@ -9,14 +9,17 @@ #= require ./worker-checker #= require_self -window.setupJsonEditor = ($editor = $(".live-json-editor")) -> +window.setupJsonEditor = ($editors = $(".live-json-editor")) -> JSONEditor.prototype.ADD_IMG = '<%= image_path 'json-editor/add.png' %>' JSONEditor.prototype.DELETE_IMG = '<%= image_path 'json-editor/delete.png' %>' - if $editor.length + editors = [] + $editors.each -> + $editor = $(this) jsonEditor = new JSONEditor($editor, $editor.data('width') || 400, $editor.data('height') || 500) jsonEditor.doTruncation true jsonEditor.showFunctionButtons() - return jsonEditor + editors.push jsonEditor + return editors hideSchedule = -> $(".schedule-region select").hide() @@ -55,7 +58,7 @@ showEventDescriptions = -> $(document).ready -> # JSON Editor - window.jsonEditor = setupJsonEditor() + window.jsonEditor = setupJsonEditor()[0] # Flash if $(".flash").length diff --git a/app/assets/stylesheets/application.css.scss.erb b/app/assets/stylesheets/application.css.scss.erb index 3f0a6239..56cf9b64 100644 --- a/app/assets/stylesheets/application.css.scss.erb +++ b/app/assets/stylesheets/application.css.scss.erb @@ -140,7 +140,11 @@ span.not-applicable:after { opacity: 0.5; } -// Fix JSON Editor +// JSON Editor + +.live-json-editor { + font-family: "Courier New", Courier, monospace; +} .json-editor blockquote { font-size: 14px; diff --git a/app/assets/stylesheets/scenarios.css.scss b/app/assets/stylesheets/scenarios.css.scss index c6ef3bbd..83177c9f 100644 --- a/app/assets/stylesheets/scenarios.css.scss +++ b/app/assets/stylesheets/scenarios.css.scss @@ -1,9 +1,2 @@ .scenario-import { - .danger { - color: red; - font-weight: strong; - border: 1px solid red; - padding: 10px; - margin: 10px 0; - } } \ No newline at end of file diff --git a/app/controllers/scenario_imports_controller.rb b/app/controllers/scenario_imports_controller.rb index 3f0086ed..531aa19d 100644 --- a/app/controllers/scenario_imports_controller.rb +++ b/app/controllers/scenario_imports_controller.rb @@ -11,13 +11,8 @@ class ScenarioImportsController < ApplicationController render :text => 'Sorry, you cannot import a Scenario by URL from your own Huginn server.' and return end - if @scenario_import.valid? - if @scenario_import.do_import? - @scenario_import.import! - redirect_to @scenario_import.scenario, notice: "Import successful!" - else - render action: "new" - end + if @scenario_import.valid? && @scenario_import.should_import? && @scenario_import.import + redirect_to @scenario_import.scenario, notice: "Import successful!" else render action: "new" end diff --git a/app/models/scenario_import.rb b/app/models/scenario_import.rb index 1efa38ee..edf55eb8 100644 --- a/app/models/scenario_import.rb +++ b/app/models/scenario_import.rb @@ -1,4 +1,7 @@ -# This is a helper class for managing Scenario imports. +require 'ostruct' + +# This is a helper class for managing Scenario imports, used by the ScenarioImportsController. This class behaves much +# like a normal ActiveRecord object, with validations and callbacks. However, it is never persisted to the database. class ScenarioImport include ActiveModel::Model include ActiveModel::Callbacks @@ -7,7 +10,7 @@ class ScenarioImport DANGEROUS_AGENT_TYPES = %w[Agents::ShellCommandAgent] URL_REGEX = /\Ahttps?:\/\//i - attr_accessor :file, :url, :data, :do_import + attr_accessor :file, :url, :data, :do_import, :merges attr_reader :user @@ -17,13 +20,14 @@ class ScenarioImport validate :validate_presence_of_file_url_or_data validates_format_of :url, :with => URL_REGEX, :allow_nil => true, :allow_blank => true, :message => "appears to be invalid" validate :validate_data + validate :generate_diff def step_one? data.blank? end def step_two? - valid? + data.present? end def set_user(user) @@ -31,7 +35,7 @@ class ScenarioImport end def existing_scenario - @existing_scenario ||= user.scenarios.find_by_guid(parsed_data["guid"]) + @existing_scenario ||= user.scenarios.find_by(:guid => parsed_data["guid"]) end def dangerous? @@ -39,18 +43,22 @@ class ScenarioImport end def parsed_data - @parsed_data ||= data && JSON.parse(data) rescue {} + @parsed_data ||= (data && JSON.parse(data) rescue {}) || {} end - def do_import? + def agent_diffs + @agent_diffs || generate_diff + end + + def should_import? do_import == "1" end - def import!(options = {}) + def import(options = {}) + success = true guid = parsed_data['guid'] description = parsed_data['description'] name = parsed_data['name'] - agents = parsed_data['agents'] links = parsed_data['links'] source_url = parsed_data['source_url'].presence || nil @scenario = user.scenarios.where(:guid => guid).first_or_initialize @@ -58,17 +66,20 @@ class ScenarioImport :source_url => source_url, :public => false) unless options[:skip_agents] - created_agents = agents.map do |agent_data| - agent = @scenario.agents.find_by(:guid => agent_data['guid']) || Agent.build_for_type(agent_data['type'], user) - agent.guid = agent_data['guid'] - agent.attributes = { :name => agent_data['name'], - :schedule => agent_data['schedule'], - :keep_events_for => agent_data['keep_events_for'], - :propagate_immediately => agent_data['propagate_immediately'], - :disabled => agent_data['disabled'], - :options => agent_data['options'], + created_agents = agent_diffs.map do |agent_diff| + agent = agent_diff.agent || Agent.build_for_type("Agents::" + agent_diff.type.incoming, user) + agent.guid = agent_diff.guid.incoming + agent.attributes = { :name => agent_diff.name.updated, + :disabled => agent_diff.disabled.updated, # == "true" + :options => agent_diff.options.updated, :scenario_ids => [@scenario.id] } - agent.save! + agent.schedule = agent_diff.schedule.updated if agent_diff.schedule.present? + agent.keep_events_for = agent_diff.keep_events_for.updated if agent_diff.keep_events_for.present? + agent.propagate_immediately = agent_diff.propagate_immediately.updated if agent_diff.propagate_immediately.present? # == "true" + unless agent.save + success = false + errors.add(:base, "Errors when saving '#{agent_diff.name.incoming}': #{agent.errors.full_messages.to_sentence}") + end agent end @@ -78,6 +89,8 @@ class ScenarioImport receiver.sources << source unless receiver.sources.include?(source) end end + + success end def scenario @@ -119,4 +132,110 @@ class ScenarioImport errors.add(:base, "Please provide either a Scenario JSON File or a Public Scenario URL.") end end -end \ No newline at end of file + + def generate_diff + @agent_diffs = (parsed_data['agents'] || []).map.with_index do |agent_data, index| + # AgentDiff is defined at the end of this file. + agent_diff = AgentDiff.new(agent_data) + if existing_scenario + # If this Agent exists already, update the AgentDiff with the local version's information. + agent_diff.diff_with! existing_scenario.agents.find_by(:guid => agent_data['guid']) + + begin + # Update the AgentDiff with any hand-merged changes coming from the UI. This only happens when this + # Agent already exists locally and has conflicting changes. + agent_diff.update_from! merges[index.to_s] if merges + rescue JSON::ParserError + errors.add(:base, "Your updated options for '#{agent_data['name']}' were unparsable.") + end + end + agent_diff + end + end + + # AgentDiff is a helper object that encapsulates an incoming Agent. All fields will be returned as an array + # of either one or two values. The first value is the incoming value, the second is the existing value, if + # it differs from the incoming value. + class AgentDiff < OpenStruct + class FieldDiff + attr_accessor :incoming, :current, :updated + + def initialize(incoming) + @incoming = incoming + @updated = incoming + end + + def set_current(current) + @current = current + @requires_merge = (incoming != current) + end + + def requires_merge? + @requires_merge + end + end + + def initialize(agent_data) + super() + @requires_merge = false + self.agent = nil + store! agent_data + end + + BASE_FIELDS = %w[name schedule keep_events_for propagate_immediately disabled guid] + + def agent_exists? + !!agent + end + + def requires_merge? + @requires_merge + end + + def store!(agent_data) + self.type = FieldDiff.new(agent_data["type"].split("::").pop) + self.options = FieldDiff.new(agent_data['options'] || {}) + BASE_FIELDS.each do |option| + self[option] = FieldDiff.new(agent_data[option]) if agent_data.has_key?(option) + end + end + + def diff_with!(agent) + return unless agent.present? + + self.agent = agent + + type.set_current(agent.short_type) + options.set_current(agent.options || {}) + + @requires_merge ||= type.requires_merge? + @requires_merge ||= options.requires_merge? + + BASE_FIELDS.each do |field| + next unless self[field].present? + self[field].set_current(agent.send(field)) + + @requires_merge ||= self[field].requires_merge? + end + end + + def update_from!(merges) + each_field do |field, value, selection_options| + value.updated = merges[field] + end + + if options.requires_merge? + options.updated = JSON.parse(merges['options']) + end + end + + def each_field + boolean = [["True", "true"], ["False", "false"]] + yield 'name', name if name.requires_merge? + yield 'schedule', schedule, Agent::SCHEDULES.map {|s| [s.humanize.titleize, s] } if self['schedule'].present? && schedule.requires_merge? + yield 'keep_events_for', keep_events_for, Agent::EVENT_RETENTION_SCHEDULES if self['keep_events_for'].present? && keep_events_for.requires_merge? + yield 'propagate_immediately', propagate_immediately, boolean if self['propagate_immediately'].present? && propagate_immediately.requires_merge? + yield 'disabled', disabled, boolean if disabled.requires_merge? + end + end +end diff --git a/app/views/agents/agent_views/manual_event_agent/_show.html.erb b/app/views/agents/agent_views/manual_event_agent/_show.html.erb index 3aca7bd5..c9120fdf 100644 --- a/app/views/agents/agent_views/manual_event_agent/_show.html.erb +++ b/app/views/agents/agent_views/manual_event_agent/_show.html.erb @@ -14,7 +14,7 @@ - - <% if @scenario_import.dangerous? %> -
- This Scenario contains one or more potentially dangerous Agents. - These may be able to run local commands or execute code. - Please be sure that you understand the above Agent configurations before importing! -
- <% end %> - - <% if @scenario_import.existing_scenario.present? %> -
- This Scenario already exists in your system. - If you continue, the import will overwrite your existing - <%= @scenario_import.existing_scenario.name %> Scenario and the Agents in it. -
- <% end %> - -
- <%= f.label :do_import do %> - <%= f.check_box :do_import %> I confirm that I want to import these Agents. + <% if @scenario_import.existing_scenario.present? %> +
+ + This Scenario already exists in your system. The import will update your existing + <%= @scenario_import.existing_scenario.name %> Scenario's title + and + description. Below you can customize how the individual agents get updated. +
+ <% end %> + +
+

+ <%= @scenario_import.parsed_data["name"] %> + (<%= pluralize @scenario_import.parsed_data["agents"].length, "Agent" %> + ; exported <%= time_ago_in_words Time.parse(@scenario_import.parsed_data["exported_at"]) %> ago) +

+
+ + <% if @scenario_import.parsed_data["description"].present? %> +
<%= @scenario_import.parsed_data["description"] %>
<% end %> -
-
- <%= f.submit "Finish Import", :class => "btn btn-primary" %>
+ +
+ <% @scenario_import.agent_diffs.each.with_index do |agent_diff, index| %> +
+ +
+
+

+ <%= agent_diff.name.incoming %> + + (<%= agent_diff.type.incoming %><% " -- WARNING: this Agent's type has been changed. This import will likely fail!" if agent_diff.type.requires_merge? %>) + +

+ + <% if agent_diff.agent_exists? %> +
+ This Agent exists in your Huginn system. + + <% if agent_diff.requires_merge? %> + Here are the differences between the incoming version and the one you have now. For each field, please + select which value you'd like to keep. + <% else %> + It's already up-to-date. + <% end %> +
+ <% end %> +
+
+ + + + <% agent_diff.each_field do |field, value, selection_options| %> +
+
+
+ <%= label_tag "scenario_import[merges][#{index}][#{field}]", field.titleize %> + <% if selection_options.present? %> +
+ Your current Agent's value is: + <%= selection_options.find { |s| s.last.to_s == value.current.to_s }.first %> +
+ <%= select_tag "scenario_import[merges][#{index}][#{field}]", options_for_select(selection_options, value.updated), :class => 'form-control' %> + <% else %> +
+ Your current Agent's value is: <%= value.current.to_s %> +
+ <%= text_field_tag "scenario_import[merges][#{index}][#{field}]", value.updated, :class => 'form-control' %> + <% end %> +
+
+
+ <% end %> + +
+ <% if agent_diff.options.requires_merge? %> +
+ +
+ +
+ +
+ +
+
+ Your current options: +
+ 
<%= Utils.pretty_jsonify agent_diff.options.current %>
+
+ <% end %> +
+
+ <% end %> +
+ +
+
+
+ <%= f.label :do_import do %> + <%= f.check_box :do_import %> I confirm that I want to import these Agents. + <% end %> +
+ +
+ <%= f.submit "Finish Import", :class => "btn btn-primary" %> +
+
+
+ + + diff --git a/app/views/scenario_imports/new.html.erb b/app/views/scenario_imports/new.html.erb index c3dbcad7..7e2d5576 100644 --- a/app/views/scenario_imports/new.html.erb +++ b/app/views/scenario_imports/new.html.erb @@ -1,6 +1,6 @@
-
-
+
+
<% if @scenario_import.errors.any? %>

<%= pluralize(@scenario_import.errors.count, "error") %> prohibited this Scenario from being imported:

@@ -9,26 +9,24 @@ <% end %>
<% end %> +
+
- <%= form_for @scenario_import, :multipart => true do |f| %> - <%= f.hidden_field :data %> + <%= form_for @scenario_import, :multipart => true do |f| %> + <%= f.hidden_field :data %> -
- <% if @scenario_import.step_one? %> - <%= render 'step_one', :f => f %> - <% elsif @scenario_import.step_two? %> - <%= render 'step_two', :f => f %> - <% end %> -
- <% end %> + <% if @scenario_import.step_one? %> + <%= render 'step_one', :f => f %> + <% elsif @scenario_import.step_two? %> + <%= render 'step_two', :f => f %> + <% end %> + <% end %> -
+
-
-
- <%= link_to ' Back'.html_safe, scenarios_path, class: "btn btn-default" %> -
-
+
+
+ <%= link_to ' Back'.html_safe, scenarios_path, class: "btn btn-default" %>
\ No newline at end of file diff --git a/lib/agents_exporter.rb b/lib/agents_exporter.rb index 4302c0e6..1aa77d63 100644 --- a/lib/agents_exporter.rb +++ b/lib/agents_exporter.rb @@ -42,12 +42,13 @@ class AgentsExporter { :type => agent.type, :name => agent.name, - :schedule => agent.schedule, - :keep_events_for => agent.keep_events_for, - :propagate_immediately => agent.propagate_immediately, :disabled => agent.disabled, :guid => agent.guid, :options => agent.options - } + }.tap do |options| + options[:schedule] = agent.schedule if agent.can_be_scheduled? + options[:keep_events_for] = agent.keep_events_for if agent.can_create_events? + options[:propagate_immediately] = agent.propagate_immediately if agent.can_receive_events? + end end end \ No newline at end of file diff --git a/spec/controllers/scenario_imports_controller_spec.rb b/spec/controllers/scenario_imports_controller_spec.rb index 898b9f02..a56e0ded 100644 --- a/spec/controllers/scenario_imports_controller_spec.rb +++ b/spec/controllers/scenario_imports_controller_spec.rb @@ -1,10 +1,6 @@ require 'spec_helper' describe ScenarioImportsController do - def valid_attributes(options = {}) - { :name => "some_name" }.merge(options) - end - before do sign_in users(:bob) end @@ -22,6 +18,7 @@ describe ScenarioImportsController do post :create, :scenario_import => { :url => "bad url" } assigns(:scenario_import).user.should == users(:bob) assigns(:scenario_import).url.should == "bad url" + assigns(:scenario_import).should_not be_valid response.should render_template(:new) end end diff --git a/spec/lib/agents_exporter_spec.rb b/spec/lib/agents_exporter_spec.rb index 1dfb3f2d..974784fe 100644 --- a/spec/lib/agents_exporter_spec.rb +++ b/spec/lib/agents_exporter_spec.rb @@ -21,9 +21,12 @@ describe AgentsExporter do data[:links].should == [{ :source => 0, :receiver => 1 }] data[:agents].should == agent_list.map { |agent| exporter.agent_as_json(agent) } data[:agents].all? { |agent_json| agent_json[:guid].present? && agent_json[:type].present? && agent_json[:name].present? }.should be_true + + data[:agents][0].should_not have_key(:propagate_immediately) # can't receive events + data[:agents][1].should_not have_key(:schedule) # can't be scheduled end - it "does not output links to other agents" do + it "does not output links to other agents outside of the incoming set" do Link.create!(:source_id => agents(:jane_weather_agent).id, :receiver_id => agents(:jane_website_agent).id) Link.create!(:source_id => agents(:jane_website_agent).id, :receiver_id => agents(:jane_rain_notifier_agent).id) diff --git a/spec/models/scenario_import_spec.rb b/spec/models/scenario_import_spec.rb index 4c77e28a..836083f0 100644 --- a/spec/models/scenario_import_spec.rb +++ b/spec/models/scenario_import_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' describe ScenarioImport do + let(:user) { users(:bob) } let(:guid) { "somescenarioguid" } let(:description) { "This is a cool Huginn Scenario that does something useful!" } let(:name) { "A useful Scenario" } @@ -22,6 +23,28 @@ describe ScenarioImport do 'message' => "Looks like rain!" } } + let(:valid_parsed_weather_agent_data) do + { + :type => "Agents::WeatherAgent", + :name => "a weather agent", + :schedule => "5pm", + :keep_events_for => 14, + :disabled => true, + :guid => "a-weather-agent", + :options => weather_agent_options + } + end + let(:valid_parsed_trigger_agent_data) do + { + :type => "Agents::TriggerAgent", + :name => "listen for weather", + :keep_events_for => 0, + :propagate_immediately => true, + :disabled => false, + :guid => "a-trigger-agent", + :options => trigger_agent_options + } + end let(:valid_parsed_data) do { :name => name, @@ -30,26 +53,8 @@ describe ScenarioImport do :source_url => source_url, :exported_at => 2.days.ago.utc.iso8601, :agents => [ - { - :type => "Agents::WeatherAgent", - :name => "a weather agent", - :schedule => "5pm", - :keep_events_for => 14, - :propagate_immediately => false, - :disabled => false, - :guid => "a-weather-agent", - :options => weather_agent_options - }, - { - :type => "Agents::TriggerAgent", - :name => "listen for weather", - :schedule => nil, - :keep_events_for => 0, - :propagate_immediately => true, - :disabled => true, - :guid => "a-trigger-agent", - :options => trigger_agent_options - } + valid_parsed_weather_agent_data, + valid_parsed_trigger_agent_data ], :links => [ { :source => 0, :receiver => 1 } @@ -66,7 +71,11 @@ describe ScenarioImport do end describe "validations" do - subject { ScenarioImport.new } + subject do + _import = ScenarioImport.new + _import.set_user(user) + _import + end it "is not valid when none of file, url, or data are present" do subject.should_not be_valid @@ -145,7 +154,7 @@ describe ScenarioImport do end end - describe "#import!" do + describe "#import and #generate_diff" do let(:scenario_import) do _import = ScenarioImport.new(:data => valid_data) _import.set_user users(:bob) @@ -153,107 +162,249 @@ describe ScenarioImport do end context "when this scenario has never been seen before" do - it "makes a new scenario" do - lambda { - scenario_import.import!(:skip_agents => true) - }.should change { users(:bob).scenarios.count }.by(1) + describe "#import" do + it "makes a new scenario" do + lambda { + scenario_import.import(:skip_agents => true) + }.should change { users(:bob).scenarios.count }.by(1) - scenario_import.scenario.name.should == name - scenario_import.scenario.description.should == description - scenario_import.scenario.guid.should == guid - scenario_import.scenario.source_url.should == source_url - scenario_import.scenario.public.should be_false + scenario_import.scenario.name.should == name + scenario_import.scenario.description.should == description + scenario_import.scenario.guid.should == guid + scenario_import.scenario.source_url.should == source_url + scenario_import.scenario.public.should be_false + end + + it "creates the Agents" do + lambda { + scenario_import.import + }.should change { users(:bob).agents.count }.by(2) + + weather_agent = scenario_import.scenario.agents.find_by(:guid => "a-weather-agent") + trigger_agent = scenario_import.scenario.agents.find_by(:guid => "a-trigger-agent") + + weather_agent.name.should == "a weather agent" + weather_agent.schedule.should == "5pm" + weather_agent.keep_events_for.should == 14 + weather_agent.propagate_immediately.should be_false + weather_agent.should be_disabled + weather_agent.memory.should be_empty + weather_agent.options.should == weather_agent_options + + trigger_agent.name.should == "listen for weather" + trigger_agent.sources.should == [weather_agent] + trigger_agent.schedule.should be_nil + trigger_agent.keep_events_for.should == 0 + trigger_agent.propagate_immediately.should be_true + trigger_agent.should_not be_disabled + trigger_agent.memory.should be_empty + trigger_agent.options.should == trigger_agent_options + end + + it "creates new Agents, even if one already exists with the given guid (so that we don't overwrite a user's work outside of the scenario)" do + agents(:bob_weather_agent).update_attribute :guid, "a-weather-agent" + + lambda { + scenario_import.import + }.should change { users(:bob).agents.count }.by(2) + end end - it "creates the Agents" do - lambda { - scenario_import.import! - }.should change { users(:bob).agents.count }.by(2) + describe "#generate_diff" do + it "returns AgentDiff objects for the incoming Agents" do + scenario_import.should be_valid - weather_agent = scenario_import.scenario.agents.find_by(:guid => "a-weather-agent") - trigger_agent = scenario_import.scenario.agents.find_by(:guid => "a-trigger-agent") + agent_diffs = scenario_import.agent_diffs - weather_agent.name.should == "a weather agent" - weather_agent.schedule.should == "5pm" - weather_agent.keep_events_for.should == 14 - weather_agent.propagate_immediately.should be_false - weather_agent.should_not be_disabled - weather_agent.memory.should be_empty - weather_agent.options.should == weather_agent_options + weather_agent_diff = agent_diffs[0] + trigger_agent_diff = agent_diffs[1] - trigger_agent.name.should == "listen for weather" - trigger_agent.sources.should == [weather_agent] - trigger_agent.schedule.should be_nil - trigger_agent.keep_events_for.should == 0 - trigger_agent.propagate_immediately.should be_true - trigger_agent.should be_disabled - trigger_agent.memory.should be_empty - trigger_agent.options.should == trigger_agent_options - end + valid_parsed_weather_agent_data.each do |key, value| + if key == :type + value = value.split("::").last + end + weather_agent_diff.should respond_to(key) + field = weather_agent_diff.send(key) + field.should be_a(ScenarioImport::AgentDiff::FieldDiff) + field.incoming.should == value + field.updated.should == value + field.current.should be_nil + end + weather_agent_diff.should_not respond_to(:propagate_immediately) - it "creates new Agents, even if one already exists with the given guid (so that we don't overwrite a user's work outside of the scenario)" do - agents(:bob_weather_agent).update_attribute :guid, "a-weather-agent" - - lambda { - scenario_import.import! - }.should change { users(:bob).agents.count }.by(2) + valid_parsed_trigger_agent_data.each do |key, value| + if key == :type + value = value.split("::").last + end + trigger_agent_diff.should respond_to(key) + field = trigger_agent_diff.send(key) + field.should be_a(ScenarioImport::AgentDiff::FieldDiff) + field.incoming.should == value + field.updated.should == value + field.current.should be_nil + end + trigger_agent_diff.should_not respond_to(:schedule) + end end end context "when an a scenario already exists with the given guid" do - let!(:existing_scenario) { - _existing_scenerio = users(:bob).scenarios.build(:name => "an existing scenario") + let!(:existing_scenario) do + _existing_scenerio = users(:bob).scenarios.build(:name => "an existing scenario", :description => "something") _existing_scenerio.guid = guid _existing_scenerio.save! + + agents(:bob_weather_agent).update_attribute :guid, "a-weather-agent" + agents(:bob_weather_agent).scenarios << _existing_scenerio + _existing_scenerio - } - - it "uses the existing scenario, updating it's data" do - lambda { - scenario_import.import!(:skip_agents => true) - scenario_import.scenario.should == existing_scenario - }.should_not change { users(:bob).scenarios.count } - - existing_scenario.reload - existing_scenario.guid.should == guid - existing_scenario.description.should == description - existing_scenario.name.should == name - existing_scenario.source_url.should == source_url - existing_scenario.public.should be_false end - it "updates any existing agents in the scenario, and makes new ones as needed" do - agents(:bob_weather_agent).update_attribute :guid, "a-weather-agent" - agents(:bob_weather_agent).scenarios << existing_scenario + describe "#import" do + it "uses the existing scenario, updating its data" do + lambda { + scenario_import.import(:skip_agents => true) + scenario_import.scenario.should == existing_scenario + }.should_not change { users(:bob).scenarios.count } - lambda { - # Shouldn't matter how many times we do it! - scenario_import.import! - scenario_import.import! - scenario_import.import! - }.should change { users(:bob).agents.count }.by(1) + existing_scenario.reload + existing_scenario.guid.should == guid + existing_scenario.description.should == description + existing_scenario.name.should == name + existing_scenario.source_url.should == source_url + existing_scenario.public.should be_false + end - weather_agent = existing_scenario.agents.find_by(:guid => "a-weather-agent") - trigger_agent = existing_scenario.agents.find_by(:guid => "a-trigger-agent") + it "updates any existing agents in the scenario, and makes new ones as needed" do + scenario_import.should be_valid - weather_agent.should == agents(:bob_weather_agent) + lambda { + scenario_import.import + }.should change { users(:bob).agents.count }.by(1) # One, because the weather agent already existed. - weather_agent.name.should == "a weather agent" - weather_agent.schedule.should == "5pm" - weather_agent.keep_events_for.should == 14 - weather_agent.propagate_immediately.should be_false - weather_agent.should_not be_disabled - weather_agent.memory.should be_empty - weather_agent.options.should == weather_agent_options + weather_agent = existing_scenario.agents.find_by(:guid => "a-weather-agent") + trigger_agent = existing_scenario.agents.find_by(:guid => "a-trigger-agent") - trigger_agent.name.should == "listen for weather" - trigger_agent.sources.should == [weather_agent] - trigger_agent.schedule.should be_nil - trigger_agent.keep_events_for.should == 0 - trigger_agent.propagate_immediately.should be_true - trigger_agent.should be_disabled - trigger_agent.memory.should be_empty - trigger_agent.options.should == trigger_agent_options + weather_agent.should == agents(:bob_weather_agent) + + weather_agent.name.should == "a weather agent" + weather_agent.schedule.should == "5pm" + weather_agent.keep_events_for.should == 14 + weather_agent.propagate_immediately.should be_false + weather_agent.should be_disabled + weather_agent.memory.should be_empty + weather_agent.options.should == weather_agent_options + + trigger_agent.name.should == "listen for weather" + trigger_agent.sources.should == [weather_agent] + trigger_agent.schedule.should be_nil + trigger_agent.keep_events_for.should == 0 + trigger_agent.propagate_immediately.should be_true + trigger_agent.should_not be_disabled + trigger_agent.memory.should be_empty + trigger_agent.options.should == trigger_agent_options + end + + it "honors updates coming from the UI" do + scenario_import.merges = { + "0" => { + "name" => "updated name", + "schedule" => "6pm", + "keep_events_for" => "2", + "disabled" => "false", + "options" => weather_agent_options.merge("api_key" => "foo").to_json + } + } + + scenario_import.should be_valid + + scenario_import.import.should be_true + + weather_agent = existing_scenario.agents.find_by(:guid => "a-weather-agent") + weather_agent.name.should == "updated name" + weather_agent.schedule.should == "6pm" + weather_agent.keep_events_for.should == 2 + weather_agent.should_not be_disabled + weather_agent.options.should == weather_agent_options.merge("api_key" => "foo") + end + + it "adds errors when updated agents are invalid" do + scenario_import.merges = { + "0" => { + "name" => "", + "schedule" => "foo", + "keep_events_for" => "2", + "options" => weather_agent_options.merge("api_key" => "").to_json + } + } + + scenario_import.import.should be_false + + errors = scenario_import.errors.full_messages.to_sentence + errors.should =~ /Name can't be blank/ + errors.should =~ /api_key is required/ + errors.should =~ /Schedule is not a valid schedule/ + end + end + + describe "#generate_diff" do + it "returns AgentDiff objects that include 'current' values from any agents that already exist" do + agent_diffs = scenario_import.agent_diffs + weather_agent_diff = agent_diffs[0] + trigger_agent_diff = agent_diffs[1] + + # Already exists + weather_agent_diff.agent.should == agents(:bob_weather_agent) + valid_parsed_weather_agent_data.each do |key, value| + next if key == :type + weather_agent_diff.send(key).current.should == agents(:bob_weather_agent).send(key) + end + + # Doesn't exist yet + valid_parsed_trigger_agent_data.each do |key, value| + trigger_agent_diff.send(key).current.should be_nil + end + end + + it "sets the 'updated' FieldDiff values based on any feedback from the user" do + scenario_import.merges = { + "0" => { + "name" => "a new name", + "schedule" => "6pm", + "keep_events_for" => "2", + "disabled" => "true", + "options" => weather_agent_options.merge("api_key" => "foo").to_json + }, + "1" => { + "name" => "another new name" + } + } + + scenario_import.should be_valid + + agent_diffs = scenario_import.agent_diffs + weather_agent_diff = agent_diffs[0] + trigger_agent_diff = agent_diffs[1] + + weather_agent_diff.name.current.should == agents(:bob_weather_agent).name + weather_agent_diff.name.incoming.should == valid_parsed_weather_agent_data[:name] + weather_agent_diff.name.updated.should == "a new name" + + weather_agent_diff.schedule.updated.should == "6pm" + weather_agent_diff.keep_events_for.updated.should == "2" + weather_agent_diff.disabled.updated.should == "true" + weather_agent_diff.options.updated.should == weather_agent_options.merge("api_key" => "foo") + end + + it "adds errors on validation when updated options are unparsable" do + scenario_import.merges = { + "0" => { + "options" => '{' + } + } + scenario_import.should_not be_valid + scenario_import.should have(1).error_on(:base) + end end end end