diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 7b910d6f7b..5232eb2c5e 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -47,7 +47,6 @@ Metrics/LineLength: - app/controllers/spree/admin/base_controller_decorator.rb - app/controllers/spree/admin/orders_controller_decorator.rb - app/controllers/spree/admin/payments_controller_decorator.rb - - app/controllers/spree/admin/payment_methods_controller_decorator.rb - app/controllers/spree/admin/reports_controller_decorator.rb - app/controllers/spree/api/products_controller_decorator.rb - app/controllers/spree/credit_cards_controller.rb @@ -400,8 +399,8 @@ Metrics/AbcSize: - app/controllers/spree/admin/orders/customer_details_controller_decorator.rb - app/controllers/spree/admin/orders_controller_decorator.rb - app/controllers/spree/admin/overview_controller_decorator.rb + - app/controllers/spree/admin/payment_methods_controller.rb - app/controllers/spree/admin/payments_controller_decorator.rb - - app/controllers/spree/admin/payment_methods_controller_decorator.rb - app/controllers/spree/admin/products_controller_decorator.rb - app/controllers/spree/admin/reports_controller_decorator.rb - app/controllers/spree/admin/search_controller_decorator.rb @@ -607,8 +606,8 @@ Metrics/MethodLength: - app/controllers/checkout_controller.rb - app/controllers/shop_controller.rb - app/controllers/spree/admin/orders/customer_details_controller_decorator.rb + - app/controllers/spree/admin/payment_methods_controller.rb - app/controllers/spree/admin/payments_controller_decorator.rb - - app/controllers/spree/admin/payment_methods_controller_decorator.rb - app/controllers/spree/admin/products_controller_decorator.rb - app/controllers/spree/admin/reports_controller_decorator.rb - app/controllers/spree/admin/search_controller_decorator.rb @@ -696,6 +695,7 @@ Metrics/ClassLength: - app/controllers/admin/order_cycles_controller.rb - app/controllers/admin/subscriptions_controller.rb - app/controllers/checkout_controller.rb + - app/controllers/spree/admin/payment_methods_controller.rb - app/models/enterprise.rb - app/models/order_cycle.rb - app/models/product_import/entry_processor.rb diff --git a/app/controllers/spree/admin/payment_methods_controller_decorator.rb b/app/controllers/spree/admin/payment_methods_controller.rb similarity index 56% rename from app/controllers/spree/admin/payment_methods_controller_decorator.rb rename to app/controllers/spree/admin/payment_methods_controller.rb index 53a1404bc9..a858c0d651 100644 --- a/app/controllers/spree/admin/payment_methods_controller_decorator.rb +++ b/app/controllers/spree/admin/payment_methods_controller.rb @@ -1,12 +1,63 @@ module Spree module Admin - PaymentMethodsController.class_eval do - before_filter :restrict_stripe_account_change, only: [:update] - before_filter :force_environment, only: [:create, :update] - skip_before_filter :load_resource, only: [:show_provider_preferences] + class PaymentMethodsController < ResourceController + skip_before_filter :load_resource, only: [:create, :show_provider_preferences] + before_filter :load_data + before_filter :validate_payment_method_provider, only: [:create] before_filter :load_hubs, only: [:new, :edit, :update] create.before :load_hubs + respond_to :html + + def create + force_environment + + @payment_method = params[:payment_method]. + delete(:type). + constantize. + new(params[:payment_method]) + @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)) + redirect_to edit_admin_payment_method_path(@payment_method) + else + invoke_callbacks(:create, :fails) + respond_with(@payment_method) + end + end + + def update + restrict_stripe_account_change + force_environment + + invoke_callbacks(:update, :before) + payment_method_type = params[:payment_method].delete(:type) + if @payment_method['type'].to_s != payment_method_type + @payment_method.update_column(:type, payment_method_type) + @payment_method = PaymentMethod.find(params[:id]) + end + + payment_method_params = params[ActiveModel::Naming.param_key(@payment_method)] || {} + attributes = params[:payment_method].merge(payment_method_params) + attributes.each do |k, _v| + if k.include?("password") && attributes[k].blank? + attributes.delete(k) + end + end + + if @payment_method.update_attributes(attributes) + invoke_callbacks(:update, :after) + flash[:success] = Spree.t(:successfully_updated, resource: Spree.t(:payment_method)) + redirect_to edit_admin_payment_method_path(@payment_method) + else + invoke_callbacks(:update, :fails) + respond_with(@payment_method) + end + end + # Only show payment methods that user has access to and sort by distributor name # ! Redundant code copied from Spree::Admin::ResourceController with modifications marked def collection @@ -62,6 +113,13 @@ module Spree @calculators = PaymentMethod.calculators.sort_by(&:name) end + def validate_payment_method_provider + valid_payment_methods = Rails.application.config.spree.payment_methods.map(&:to_s) + return if valid_payment_methods.include?(params[:payment_method][:type]) + flash[:error] = Spree.t(:invalid_payment_provider) + redirect_to new_admin_payment_method_path + end + def load_hubs # rubocop:disable Style/TernaryParentheses @hubs = Enterprise.managed_by(spree_current_user).is_distributor.sort_by! do |d| @@ -73,7 +131,8 @@ module Spree # Show Stripe as an option if enabled, or if the # current payment_method is already a Stripe method def show_stripe? - Spree::Config.stripe_connect_enabled || @payment_method.try(:type) == "Spree::Gateway::StripeConnect" + Spree::Config.stripe_connect_enabled || + @payment_method.try(:type) == "Spree::Gateway::StripeConnect" end def restrict_stripe_account_change diff --git a/spec/controllers/spree/admin/payment_methods_controller_spec.rb b/spec/controllers/spree/admin/payment_methods_controller_spec.rb index c5811f4fe7..2a2f03dbcc 100644 --- a/spec/controllers/spree/admin/payment_methods_controller_spec.rb +++ b/spec/controllers/spree/admin/payment_methods_controller_spec.rb @@ -1,127 +1,177 @@ require 'spec_helper' -describe Spree::Admin::PaymentMethodsController, type: :controller do - describe "#update" do - context "on a StripeConnect payment method" do - let!(:user) { create(:user, enterprise_limit: 2) } - let!(:enterprise1) { create(:distributor_enterprise, owner: user) } - let!(:enterprise2) { create(:distributor_enterprise, owner: create(:user)) } - let!(:payment_method) { create(:stripe_payment_method, distributor_ids: [enterprise1.id, enterprise2.id], preferred_enterprise_id: enterprise2.id) } +module Spree + class GatewayWithPassword < PaymentMethod + attr_accessible :preferred_password + preference :password, :string, default: "password" + end + + describe Admin::PaymentMethodsController, type: :controller do + describe "#create and #update" do + let!(:enterprise) { create(:distributor_enterprise, owner: user) } + let(:payment_method) { GatewayWithPassword.create!(name: "Bogus", preferred_password: "haxme", distributor_ids: [enterprise.id], preferred_enterprise_id: enterprise.id) } + let!(:user) { create(:user) } 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 } } } + it "does not clear password on update" do + expect(payment_method.preferred_password).to eq "haxme" + spree_put :update, id: payment_method.id, payment_method: { type: payment_method.class.to_s, preferred_password: "" } + expect(response).to redirect_to spree.edit_admin_payment_method_path(payment_method) - context "as a user that does not manage the existing stripe account holder" do - it "prevents the stripe account holder from being updated" do - spree_put :update, params - expect(payment_method.reload.preferred_enterprise_id).to eq enterprise2.id - end + payment_method.reload + expect(payment_method.preferred_password).to eq "haxme" + end + + context "tries to save invalid payment" do + it "doesn't break, responds nicely" do + expect { + spree_post :create, payment_method: { name: "", type: "Spree::Gateway::Bogus" } + }.not_to raise_error end + end - context "as a user that manages the existing stripe account holder" do - before { enterprise2.update_attributes!(owner_id: user.id) } + it "can create a payment method of a valid type" do + expect { + spree_post :create, payment_method: { name: "Test Method", type: "Spree::Gateway::Bogus", distributor_ids: [enterprise.id], preferred_enterprise_id: enterprise.id } + }.to change(Spree::PaymentMethod, :count).by(1) - it "allows the stripe account holder to be updated" do - spree_put :update, params - expect(payment_method.reload.preferred_enterprise_id).to eq enterprise1.id + expect(response).to be_redirect + expect(response).to redirect_to spree.edit_admin_payment_method_path(assigns(:payment_method)) + end + + it "can not create a payment method of an invalid type" do + expect { + spree_post :create, payment_method: { name: "Invalid Payment Method", type: "Spree::InvalidType", distributor_ids: [enterprise.id], preferred_enterprise_id: enterprise.id } + }.to change(Spree::PaymentMethod, :count).by(0) + + expect(response).to be_redirect + expect(response).to redirect_to spree.new_admin_payment_method_path + end + end + + describe "#update" do + context "on a StripeConnect payment method" do + let!(:user) { create(:user, enterprise_limit: 2) } + let!(:enterprise1) { create(:distributor_enterprise, owner: user) } + let!(:enterprise2) { create(:distributor_enterprise, owner: create(:user)) } + let!(:payment_method) { create(:stripe_payment_method, distributor_ids: [enterprise1.id, enterprise2.id], preferred_enterprise_id: enterprise2.id) } + + 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 } } } + + context "as a user that does not manage the existing stripe account holder" do + it "prevents the stripe account holder from being updated" do + spree_put :update, params + expect(payment_method.reload.preferred_enterprise_id).to eq enterprise2.id + end end - context "when no enterprise is selected as the account holder" do - before { payment_method.update_attribute(:preferred_enterprise_id, nil) } + context "as a user that manages the existing stripe account holder" do + before { enterprise2.update_attributes!(owner_id: user.id) } - context "id not provided at all" do - before { params[:payment_method].delete(:preferred_enterprise_id) } - - it "does not save the payment method" do - spree_put :update, params - expect(response).to render_template :edit - expect(assigns(:payment_method).errors.messages[:stripe_account_owner]).to include I18n.t(:error_required) - end + it "allows the stripe account holder to be updated" do + spree_put :update, params + expect(payment_method.reload.preferred_enterprise_id).to eq enterprise1.id end - context "enterprise_id of 0" do - before { params[:payment_method][:preferred_enterprise_id] = 0 } + context "when no enterprise is selected as the account holder" do + before { payment_method.update_attribute(:preferred_enterprise_id, nil) } - it "does not save the payment method" do - spree_put :update, params - expect(response).to render_template :edit - expect(assigns(:payment_method).errors.messages[:stripe_account_owner]).to include I18n.t(:error_required) + context "id not provided at all" do + before { params[:payment_method].delete(:preferred_enterprise_id) } + + it "does not save the payment method" do + spree_put :update, params + expect(response).to render_template :edit + expect(assigns(:payment_method).errors.messages[:stripe_account_owner]).to include I18n.t(:error_required) + end + end + + context "enterprise_id of 0" do + before { params[:payment_method][:preferred_enterprise_id] = 0 } + + it "does not save the payment method" do + spree_put :update, params + expect(response).to render_template :edit + expect(assigns(:payment_method).errors.messages[:stripe_account_owner]).to include I18n.t(:error_required) + end end end end end end end - end - context "Requesting provider preference fields" do - let(:enterprise) { create(:distributor_enterprise) } - let(:user) do - new_user = create(:user, email: 'enterprise@hub.com', password: 'blahblah', password_confirmation: 'blahblah', ) - new_user.spree_roles = [] # for some reason unbeknown to me, this new user gets admin permissions by default. - new_user.enterprise_roles.build(enterprise: enterprise).save - new_user.save - new_user - end + context "Requesting provider preference fields" do + let(:enterprise) { create(:distributor_enterprise) } + let(:user) do + new_user = create(:user, email: 'enterprise@hub.com', password: 'blahblah', password_confirmation: 'blahblah', ) + new_user.spree_roles = [] # for some reason unbeknown to me, this new user gets admin permissions by default. + new_user.enterprise_roles.build(enterprise: enterprise).save + new_user.save + new_user + end - before do - allow(controller).to receive_messages spree_current_user: user - end + before do + allow(controller).to receive_messages spree_current_user: user + end - context "on an existing payment method" do - let(:payment_method) { create(:payment_method) } + context "on an existing payment method" do + let(:payment_method) { create(:payment_method) } - context "where I have permission" do - before do - payment_method.distributors << user.enterprises.is_distributor.first + context "where I have permission" do + before do + payment_method.distributors << user.enterprises.is_distributor.first + end + + context "without an altered provider type" do + it "renders provider settings with same payment method" do + spree_get :show_provider_preferences, + pm_id: payment_method.id, + provider_type: "Spree::PaymentMethod::Check" + expect(assigns(:payment_method)).to eq payment_method + expect(response).to render_template partial: '_provider_settings' + end + end + + context "with an altered provider type" do + it "renders provider settings with a different payment method" do + spree_get :show_provider_preferences, + pm_id: payment_method.id, + provider_type: "Spree::Gateway::Bogus" + expect(assigns(:payment_method)).not_to eq payment_method + expect(response).to render_template partial: '_provider_settings' + end + end end - context "without an altered provider type" do - it "renders provider settings with same payment method" do + context "where I do not have permission" do + before do + payment_method.distributors = [] + end + + it "renders unauthorised" do spree_get :show_provider_preferences, pm_id: payment_method.id, provider_type: "Spree::PaymentMethod::Check" expect(assigns(:payment_method)).to eq payment_method - expect(response).to render_template partial: '_provider_settings' - end - end - - context "with an altered provider type" do - it "renders provider settings with a different payment method" do - spree_get :show_provider_preferences, - pm_id: payment_method.id, - provider_type: "Spree::Gateway::Bogus" - expect(assigns(:payment_method)).not_to eq payment_method - expect(response).to render_template partial: '_provider_settings' + expect(flash[:error]).to eq "Authorization Failure" end end end - context "where I do not have permission" do - before do - payment_method.distributors = [] - end - - it "renders unauthorised" do + context "on a new payment method" do + it "renders provider settings with a new payment method of type" do spree_get :show_provider_preferences, - pm_id: payment_method.id, - provider_type: "Spree::PaymentMethod::Check" - expect(assigns(:payment_method)).to eq payment_method - expect(flash[:error]).to eq "Authorization Failure" + pm_id: "", + provider_type: "Spree::Gateway::Bogus" + expect(assigns(:payment_method)).to be_a_new Spree::Gateway::Bogus + expect(response).to render_template partial: '_provider_settings' end end end - - context "on a new payment method" do - it "renders provider settings with a new payment method of type" do - spree_get :show_provider_preferences, - pm_id: "", - provider_type: "Spree::Gateway::Bogus" - expect(assigns(:payment_method)).to be_a_new Spree::Gateway::Bogus - expect(response).to render_template partial: '_provider_settings' - end - end end end