From 6d51ece69c8ea02b5a4d9ce3185710bee28680f6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Mar 2021 12:14:47 +0000 Subject: [PATCH] Tidy up payment method params --- .../spree/admin/payment_methods_controller.rb | 45 +++++++++++-------- .../admin/payment_methods_controller_spec.rb | 41 ++++++++++++++++- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb index 5086ad64cd..aaee7fb306 100644 --- a/app/controllers/spree/admin/payment_methods_controller.rb +++ b/app/controllers/spree/admin/payment_methods_controller.rb @@ -12,13 +12,11 @@ module Spree def create force_environment - @payment_method = params[:payment_method]. - delete(:type). - constantize. - new(PermittedAttributes::PaymentMethod.new(params[:payment_method]).call) + @payment_method = payment_method_class.constantize.new(base_params) @object = @payment_method invoke_callbacks(:create, :before) + if @payment_method.save invoke_callbacks(:create, :after) flash[:success] = Spree.t(:successfully_created, resource: Spree.t(:payment_method)) @@ -34,16 +32,16 @@ module Spree force_environment invoke_callbacks(:update, :before) - payment_method_type = params[:payment_method].delete(:type) - if @payment_method['type'].to_s != payment_method_type + + if @payment_method['type'].to_s != payment_method_class @payment_method.update_columns( - type: payment_method_type, + type: payment_method_class, updated_at: Time.zone.now ) @payment_method = PaymentMethod.find(params[:id]) end - if @payment_method.update(params_for_update) + if @payment_method.update(update_params) invoke_callbacks(:update, :after) flash[:success] = Spree.t(:successfully_updated, resource: Spree.t(:payment_method)) redirect_to spree.edit_admin_payment_method_path(@payment_method) @@ -98,8 +96,12 @@ module Spree private + def payment_method_class + base_params.delete(:type) + end + def force_environment - params[:payment_method][:environment] = Rails.env unless spree_current_user.admin? + base_params[:environment] = Rails.env unless spree_current_user.admin? end def load_data @@ -143,7 +145,7 @@ module Spree @stripe_account_holder = Enterprise.find(@payment_method.preferred_enterprise_id) return if spree_current_user.enterprises.include? @stripe_account_holder - params[:payment_method][:preferred_enterprise_id] = @stripe_account_holder.id + update_params[:preferred_enterprise_id] = @stripe_account_holder.id end def stripe_payment_method? @@ -155,19 +157,26 @@ module Spree provider.name.ends_with?("StripeConnect", "StripeSCA") end + def base_params + @base_params ||= PermittedAttributes::PaymentMethod.new(params[:payment_method]). + call.to_h.with_indifferent_access + end + # Merge payment method params with gateway params like :gateway_stripe_connect # Also, remove password if present and blank - def params_for_update - gateway_params = params[ActiveModel::Naming.param_key(@payment_method)] || {} - params_for_update = params[:payment_method].merge(gateway_params) + def update_params + @update_params ||= begin + gateway_params = raw_params[ActiveModel::Naming.param_key(@payment_method)] || {} + params_for_update = base_params.merge(gateway_params) - params_for_update.each do |key, _value| - if key.include?("password") && params_for_update[key].blank? - params_for_update.delete(key) + params_for_update.each do |key, value| + if key.include?("password") && value.blank? + params_for_update.delete(key) + end end - end - PermittedAttributes::PaymentMethod.new(params_for_update).call + params_for_update + end end end end diff --git a/spec/controllers/spree/admin/payment_methods_controller_spec.rb b/spec/controllers/spree/admin/payment_methods_controller_spec.rb index 75abf32a63..1ed6f5de1a 100644 --- a/spec/controllers/spree/admin/payment_methods_controller_spec.rb +++ b/spec/controllers/spree/admin/payment_methods_controller_spec.rb @@ -52,6 +52,37 @@ module Spree end describe "#update" do + context "updating a payment method" do + let!(:payment_method) { create(:payment_method, :flat_rate) } + let(:params) { + { + id: payment_method.id, + payment_method: { + name: "Updated", + description: "Updated", + type: "Spree::PaymentMethod::Check", + calculator_attributes: { + id: payment_method.calculator.id, + preferred_amount: 456, + preferred_currency: "GBP" + } + } + } + } + + before { controller_login_as_admin } + + it "updates the payment method" do + spree_post :update, params + payment_method.reload + + expect(payment_method.name).to eq "Updated" + expect(payment_method.description).to eq "Updated" + expect(payment_method.calculator.preferred_amount).to eq 456 + expect(payment_method.calculator.preferred_currency).to eq "GBP" + end + end + context "on a StripeConnect payment method" do let!(:user) { create(:user, enterprise_limit: 2) } let!(:enterprise1) { create(:distributor_enterprise, owner: user) } @@ -61,7 +92,15 @@ module Spree before { allow(controller).to receive(:spree_current_user) { user } } context "when an attempt is made to change the stripe account holder (preferred_enterprise_id)" do - let(:params) { { id: payment_method.id, payment_method: { type: "Spree::Gateway::StripeConnect", preferred_enterprise_id: enterprise1.id } } } + let(:params) { + { + id: payment_method.id, + payment_method: { + type: "Spree::Gateway::StripeConnect", + preferred_enterprise_id: enterprise1.id + } + } + } context "as a user that does not manage the existing stripe account holder" do it "prevents the stripe account holder from being updated" do