From aadf8ce9ff7da61673c63dd30faf7d50ffa7ec33 Mon Sep 17 00:00:00 2001 From: David Laprade Date: Tue, 13 Mar 2018 14:49:09 -0400 Subject: [PATCH 1/5] Enable dynamic parameter update and persistence --- app/interactors/cangaroo/perform_flow.rb | 3 +- .../cangaroo/persist_parameters.rb | 36 +++++++++++++++++++ app/jobs/cangaroo/base_job.rb | 1 + 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 app/interactors/cangaroo/persist_parameters.rb diff --git a/app/interactors/cangaroo/perform_flow.rb b/app/interactors/cangaroo/perform_flow.rb index d1bace4..e6b5a56 100644 --- a/app/interactors/cangaroo/perform_flow.rb +++ b/app/interactors/cangaroo/perform_flow.rb @@ -4,6 +4,7 @@ class PerformFlow organize ValidateJsonSchema, CountJsonObject, - PerformJobs + PerformJobs, + PersistParameters end end diff --git a/app/interactors/cangaroo/persist_parameters.rb b/app/interactors/cangaroo/persist_parameters.rb new file mode 100644 index 0000000..a829b26 --- /dev/null +++ b/app/interactors/cangaroo/persist_parameters.rb @@ -0,0 +1,36 @@ +module Cangaroo + class PersistParameters + include Interactor + + def call + return if request_params.empty? + unless connection.update(parameters: new_params) + context.fail!( + message: "could not update #{context.flow.class.name} parameters: #{connection.errors.full_messages.to_sentence}", + error_code: 500 + ) + end + end + + private + + def new_params + persisted_params + .merge(request_params) + .slice(*persisted_params.keys) + end + + def connection + context.flow.send(:destination_connection) + end + + def persisted_params + connection.parameters.stringify_keys + end + + def request_params + context.parameters.to_h.select{|k,v| v.present?}.stringify_keys + end + + end +end diff --git a/app/jobs/cangaroo/base_job.rb b/app/jobs/cangaroo/base_job.rb index e28ab30..b15c8f2 100644 --- a/app/jobs/cangaroo/base_job.rb +++ b/app/jobs/cangaroo/base_job.rb @@ -20,6 +20,7 @@ def restart_flow(response) return unless process_response command = Cangaroo::PerformFlow.call( + flow: self, source_connection: destination_connection, json_body: response, jobs: Rails.configuration.cangaroo.jobs.reject{ |job| job == self.class } From 7c046a8c5e68012d26c68e462fd32ff8061a839d Mon Sep 17 00:00:00 2001 From: David Laprade Date: Tue, 13 Mar 2018 15:31:59 -0400 Subject: [PATCH 2/5] Fix spec failures caused by PersistParameters interactor --- spec/interactors/cangaroo/perform_flow_spec.rb | 3 ++- spec/jobs/cangaroo/push_job_spec.rb | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/interactors/cangaroo/perform_flow_spec.rb b/spec/interactors/cangaroo/perform_flow_spec.rb index e963fe8..03c4be1 100644 --- a/spec/interactors/cangaroo/perform_flow_spec.rb +++ b/spec/interactors/cangaroo/perform_flow_spec.rb @@ -9,7 +9,8 @@ let(:interactors) do [Cangaroo::ValidateJsonSchema, Cangaroo::CountJsonObject, - Cangaroo::PerformJobs] + Cangaroo::PerformJobs, + Cangaroo::PersistParameters] end it { is_expected.to eql interactors } diff --git a/spec/jobs/cangaroo/push_job_spec.rb b/spec/jobs/cangaroo/push_job_spec.rb index a61238d..b833bab 100644 --- a/spec/jobs/cangaroo/push_job_spec.rb +++ b/spec/jobs/cangaroo/push_job_spec.rb @@ -47,7 +47,8 @@ module Cangaroo job.perform_now expect(Cangaroo::PerformFlow).to have_received(:call) .once - .with(source_connection: destination_connection, + .with(flow: job, + source_connection: destination_connection, json_body: connection_response, jobs: Rails.configuration.cangaroo.jobs) end From 5d0566e507ccfe2a23ee98d26657f11c6d685182 Mon Sep 17 00:00:00 2001 From: David Laprade Date: Wed, 14 Mar 2018 09:23:53 -0400 Subject: [PATCH 3/5] Test dynamic parameter persistence --- .../cangaroo/persist_parameters.rb | 16 +- spec/fixtures/json_payload_parameters.json | 27 +++ .../cangaroo/persist_parameters_spec.rb | 154 ++++++++++++++++++ 3 files changed, 191 insertions(+), 6 deletions(-) create mode 100644 spec/fixtures/json_payload_parameters.json create mode 100644 spec/interactors/cangaroo/persist_parameters_spec.rb diff --git a/app/interactors/cangaroo/persist_parameters.rb b/app/interactors/cangaroo/persist_parameters.rb index a829b26..275fbb9 100644 --- a/app/interactors/cangaroo/persist_parameters.rb +++ b/app/interactors/cangaroo/persist_parameters.rb @@ -5,10 +5,7 @@ class PersistParameters def call return if request_params.empty? unless connection.update(parameters: new_params) - context.fail!( - message: "could not update #{context.flow.class.name} parameters: #{connection.errors.full_messages.to_sentence}", - error_code: 500 - ) + fail_context! end end @@ -16,6 +13,7 @@ def call def new_params persisted_params + .with_indifferent_access .merge(request_params) .slice(*persisted_params.keys) end @@ -25,12 +23,18 @@ def connection end def persisted_params - connection.parameters.stringify_keys + connection.parameters end def request_params - context.parameters.to_h.select{|k,v| v.present?}.stringify_keys + context.parameters.to_h.select{|k,v| v.present?} end + def fail_context! + context.fail!( + message: "could not update #{context.flow.class.name} parameters: #{connection.errors.full_messages.to_sentence}", + error_code: 500 + ) + end end end diff --git a/spec/fixtures/json_payload_parameters.json b/spec/fixtures/json_payload_parameters.json new file mode 100644 index 0000000..33471d2 --- /dev/null +++ b/spec/fixtures/json_payload_parameters.json @@ -0,0 +1,27 @@ +{ + "orders":[ + { + "id":"O154085346172", + "state":"cart" + }, + { + "id":"O154085343224", + "state":"payed" + } + ], + "shipments":[ + { + "id":"S53454325", + "state":"shipped" + }, + { + "id":"S53565543", + "state":"waiting" + } + ], + "line_items": [], + "parameters": { + "first": 1521034044, + "second": "oh hai" + } +} diff --git a/spec/interactors/cangaroo/persist_parameters_spec.rb b/spec/interactors/cangaroo/persist_parameters_spec.rb new file mode 100644 index 0000000..b0c2dbd --- /dev/null +++ b/spec/interactors/cangaroo/persist_parameters_spec.rb @@ -0,0 +1,154 @@ +require 'rails_helper' + +class JobC < Cangaroo::BaseJob + connection :job_c_connection +end +class JobD < Cangaroo::BaseJob; end +class JobE < Cangaroo::BaseJob + connection :job_e_connection +end + +describe Cangaroo::PersistParameters do + let!(:connection) { create(:cangaroo_connection, name: JobC.connection.to_s) } + let(:flow) { JobC.new(source_connection: nil, type: 'orders', payload: {}) } + let(:parameters) { Hash.new } + subject do + described_class.new( + flow: flow, + parameters: parameters + ) + end + + describe 'integration with other interactors' do + let(:json_body) { JSON.parse(load_fixture('json_payload_parameters.json')) } + before(:each) do + job_d = double(:job_d, perform?: true, enqueue: true) + allow(JobD).to receive(:new).and_return(job_d) + end + context 'connection with parameters' do + before(:each) { expect(connection.parameters).to_not be_empty } + it "persists the new parameters" do + expect{ + Cangaroo::PerformFlow.call( + flow: flow, + json_body: json_body, + jobs: [JobD], + source_connection: connection + ) + }.to change{ + connection.reload.parameters + }.to(json_body["parameters"]) + end + end + context 'connection without parameters' do + let!(:connection) { create(:store, name: JobC.connection.to_s) } + before(:each) { expect(connection.parameters).to_not be_present } + it "doesn't persist params for a connection without them" do + expect{ + Cangaroo::PerformFlow.call( + flow: flow, + json_body: json_body, + jobs: [JobD], + source_connection: connection + ) + }.to_not change{ + connection.reload.parameters + }.from({}) + end + + end + end + + describe '#call' do + context "new parameters" do + let(:parameters) { { connection.parameters.keys.first => "new value" } } + it 'persists the new parameters' do + old_params = connection.parameters + new_params = connection.parameters.with_indifferent_access.merge(parameters) + expect{ + subject.call + }.to change{ + connection.reload.parameters + }.from(old_params).to(new_params) + end + it 'fails if updates cannot be persisted' do + allow_any_instance_of(Cangaroo::Connection).to receive(:update) { false } + context = described_class.call(flow: flow, parameters: parameters) + expect(context).to_not be_success + expect(context).to be_failure + end + end + it 'does not update the DB if there are no request params' do + expect(parameters).to be_empty + expect{ subject.call }.to_not change{ connection.reload.parameters } + end + end + + describe '#connection' do + it "is the flow's connection" do + expect(subject.send(:connection)).to eq connection + end + end + + describe '#request_params' do + context 'nil params' do + let(:parameters) { nil } + it "does not throw an error" do + expect(subject.send(:request_params)).to eq({}) + end + end + it "handles empty params" do + expect(parameters).to be_empty + expect(subject.send(:request_params)).to eq({}) + end + context "empty param values" do + let(:parameters) { {"a" => nil, "b" => 3, "c" => ""} } + it "removes them" do + expect(subject.send(:request_params)).to eq({"b" => 3}) + end + end + end + + describe "#new_params" do + let(:new_params) { subject.send(:new_params) } + context "empty request params" do + before(:each) { expect(parameters).to be_empty } + it "returns the persisted params" do + persisted_params = connection.parameters + persisted_params.each do |key, value| + expect(new_params.fetch(key)).to eq(persisted_params.fetch(key)) + end + end + end + context "extra request params" do + let(:parameters) { connection.parameters.merge("a" => 1) } + it "removes them" do + expect(new_params).to_not have_key("a") + end + end + context "missing request params" do + let(:param_key) { connection.parameters.keys.first } + let(:parameters) { { param_key => "new value" } } + it "ignores them" do + old_params = connection.parameters + missing_keys = old_params.keys - [param_key] + missing_keys.each do |missing_key| + expect(new_params[missing_key]).to eq(old_params[missing_key]) + end + end + it "sets the present key" do + expect(new_params[param_key]).to eq(parameters[param_key]) + end + end + context "symbol request params" do + let(:param_key) { connection.parameters.keys.first } + let(:parameters) { { param_key.to_sym => "new value" } } + before(:each) do + connection.update(parameters: connection.parameters.stringify_keys) + end + it "still matches them with their string counterparts" do + expect(new_params[param_key]).to eq(parameters[param_key.to_sym]) + end + end + end +end From 0e6d3fe1341b07a8c5901ba41b33790ed5ae4732 Mon Sep 17 00:00:00 2001 From: David Laprade Date: Wed, 14 Mar 2018 09:46:08 -0400 Subject: [PATCH 4/5] Resolve test deprecation warnings --- app/controllers/cangaroo/endpoint_controller.rb | 2 +- bin/setup | 2 +- cangaroo.gemspec | 2 +- spec/factories/cangaroo_connections.rb | 2 +- spec/jobs/cangaroo/poll_job_spec.rb | 1 + spec/support/factory_girl.rb | 4 ++-- 6 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/controllers/cangaroo/endpoint_controller.rb b/app/controllers/cangaroo/endpoint_controller.rb index ae90752..fe5455a 100644 --- a/app/controllers/cangaroo/endpoint_controller.rb +++ b/app/controllers/cangaroo/endpoint_controller.rb @@ -33,7 +33,7 @@ def handle_request def ensure_json_request return if request.headers['Content-Type'] == 'application/json' - render nothing: true, status: 406 + head 406 end def key diff --git a/bin/setup b/bin/setup index 87175bf..a63211b 100755 --- a/bin/setup +++ b/bin/setup @@ -13,4 +13,4 @@ ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../../Gemfile", require "rubygems" require "bundler/setup" -load Gem.bin_path("factory_girl_rails", "setup") +load Gem.bin_path("factory_bot_rails", "setup") diff --git a/cangaroo.gemspec b/cangaroo.gemspec index e247027..a9f2e96 100644 --- a/cangaroo.gemspec +++ b/cangaroo.gemspec @@ -29,7 +29,7 @@ Gem::Specification.new do |s| s.add_development_dependency 'appraisal' s.add_development_dependency 'codeclimate-test-reporter' s.add_development_dependency 'database_cleaner' - s.add_development_dependency 'factory_girl_rails' + s.add_development_dependency 'factory_bot_rails' s.add_development_dependency 'pry-byebug' s.add_development_dependency 'rake' s.add_development_dependency 'rspec-rails' diff --git a/spec/factories/cangaroo_connections.rb b/spec/factories/cangaroo_connections.rb index 48f9470..e98393d 100644 --- a/spec/factories/cangaroo_connections.rb +++ b/spec/factories/cangaroo_connections.rb @@ -1,6 +1,6 @@ require 'securerandom' -FactoryGirl.define do +FactoryBot.define do factory :cangaroo_connection, class: 'Cangaroo::Connection' do name :store url 'www.store.com' diff --git a/spec/jobs/cangaroo/poll_job_spec.rb b/spec/jobs/cangaroo/poll_job_spec.rb index 55f4d22..17057e4 100644 --- a/spec/jobs/cangaroo/poll_job_spec.rb +++ b/spec/jobs/cangaroo/poll_job_spec.rb @@ -90,6 +90,7 @@ class FakePollJob < Cangaroo::PollJob allow_any_instance_of(Cangaroo::Webhook::Client) .to receive(:post) .and_return(parse_fixture('json_payload_ok.json')) + allow_any_instance_of(Cangaroo::Webhook::Client).to receive(:post).and_return(parse_fixture('json_payload_ok.json')) allow(Cangaroo::PerformFlow).to receive(:call).and_return(double(success?: false, message: 'bad failure')) diff --git a/spec/support/factory_girl.rb b/spec/support/factory_girl.rb index 196b203..7c9ba1c 100644 --- a/spec/support/factory_girl.rb +++ b/spec/support/factory_girl.rb @@ -1,5 +1,5 @@ -require 'factory_girl_rails' +require 'factory_bot_rails' RSpec.configure do |config| - config.include FactoryGirl::Syntax::Methods + config.include FactoryBot::Syntax::Methods end From 5e38affc5ad0e75b9f3be18d5b61939ea1a19f2c Mon Sep 17 00:00:00 2001 From: David Laprade Date: Thu, 15 Mar 2018 09:33:34 -0400 Subject: [PATCH 5/5] Add another dynamic param persistence test case --- app/interactors/cangaroo/persist_parameters.rb | 2 +- spec/interactors/cangaroo/persist_parameters_spec.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/interactors/cangaroo/persist_parameters.rb b/app/interactors/cangaroo/persist_parameters.rb index 275fbb9..687c1b5 100644 --- a/app/interactors/cangaroo/persist_parameters.rb +++ b/app/interactors/cangaroo/persist_parameters.rb @@ -3,7 +3,7 @@ class PersistParameters include Interactor def call - return if request_params.empty? + return if request_params.empty? || new_params.empty? unless connection.update(parameters: new_params) fail_context! end diff --git a/spec/interactors/cangaroo/persist_parameters_spec.rb b/spec/interactors/cangaroo/persist_parameters_spec.rb index b0c2dbd..951d203 100644 --- a/spec/interactors/cangaroo/persist_parameters_spec.rb +++ b/spec/interactors/cangaroo/persist_parameters_spec.rb @@ -78,6 +78,13 @@ class JobE < Cangaroo::BaseJob expect(context).to be_failure end end + context 'different request parameters' do + let(:parameters) { { "different" => "param" } } + it 'does not update the DB' do + expect(connection.parameters.keys).to_not include(parameters.keys.first) + expect{ subject.call }.to_not change{ connection.reload.parameters.with_indifferent_access } + end + end it 'does not update the DB if there are no request params' do expect(parameters).to be_empty expect{ subject.call }.to_not change{ connection.reload.parameters }