From c51e4d657a5a1adbe8377c243b2b3bf84d60dc92 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 14 Oct 2019 23:09:23 +0100
Subject: [PATCH 1/6] Bring payment method controller from spree_backend

---
 .../spree/admin/payment_methods_controller.rb | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 app/controllers/spree/admin/payment_methods_controller.rb

diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb
new file mode 100644
index 0000000000..84e1327d78
--- /dev/null
+++ b/app/controllers/spree/admin/payment_methods_controller.rb
@@ -0,0 +1,65 @@
+module Spree
+  module Admin
+    class PaymentMethodsController < ResourceController
+      skip_before_filter :load_resource, :only => [:create]
+      before_filter :load_data
+      before_filter :validate_payment_method_provider, :only => :create
+
+      respond_to :html
+
+      def create
+        @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
+        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
+
+      private
+
+      def load_data
+        @providers = Gateway.providers.sort{|p1, p2| p1.name <=> p2.name }
+      end
+
+      def validate_payment_method_provider
+        valid_payment_methods = Rails.application.config.spree.payment_methods.map(&:to_s)
+        if !valid_payment_methods.include?(params[:payment_method][:type])
+          flash[:error] = Spree.t(:invalid_payment_provider)
+          redirect_to new_admin_payment_method_path
+        end
+      end
+    end
+  end
+end

From 1c257cca3ffddcd8945dc8494415c68f634b97ff Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 14 Oct 2019 23:13:33 +0100
Subject: [PATCH 2/6] Merge payment methods controller brought from
 spree_backend with its decorator that was in OFN

---
 .rubocop_manual_todo.yml                      |  3 -
 .../spree/admin/payment_methods_controller.rb | 81 ++++++++++++++++-
 .../payment_methods_controller_decorator.rb   | 91 -------------------
 3 files changed, 80 insertions(+), 95 deletions(-)
 delete mode 100644 app/controllers/spree/admin/payment_methods_controller_decorator.rb

diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml
index 0804602b70..b5ed5a0b0c 100644
--- a/.rubocop_manual_todo.yml
+++ b/.rubocop_manual_todo.yml
@@ -48,7 +48,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
@@ -399,7 +398,6 @@ Metrics/AbcSize:
   - app/controllers/spree/admin/orders_controller_decorator.rb
   - app/controllers/spree/admin/overview_controller_decorator.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
@@ -579,7 +577,6 @@ Metrics/MethodLength:
   - app/controllers/shop_controller.rb
   - app/controllers/spree/admin/orders/customer_details_controller_decorator.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
diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb
index 84e1327d78..05f44b4d7c 100644
--- a/app/controllers/spree/admin/payment_methods_controller.rb
+++ b/app/controllers/spree/admin/payment_methods_controller.rb
@@ -4,6 +4,11 @@ module Spree
       skip_before_filter :load_resource, :only => [:create]
       before_filter :load_data
       before_filter :validate_payment_method_provider, :only => :create
+      before_filter :restrict_stripe_account_change, only: [:update]
+      before_filter :force_environment, only: [:create, :update]
+      skip_before_filter :load_resource, only: [:show_provider_preferences]
+      before_filter :load_hubs, only: [:new, :edit, :update]
+      create.before :load_hubs
 
       respond_to :html
 
@@ -47,10 +52,59 @@ module Spree
         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
+        return parent.public_send(controller_name) if parent_data.present?
+        collection = if model_class.respond_to?(:accessible_by) &&
+                        !current_ability.has_block?(params[:action], model_class)
+
+                       model_class.accessible_by(current_ability, action)
+
+                     else
+                       model_class.scoped
+                     end
+
+        collection = collection.managed_by(spree_current_user).by_name # This line added
+
+        # This block added
+        if params.key? :enterprise_id
+          distributor = Enterprise.find params[:enterprise_id]
+          collection = collection.for_distributor(distributor)
+        end
+
+        collection
+      end
+
+      def show_provider_preferences
+        if params[:pm_id].present?
+          @payment_method = PaymentMethod.find(params[:pm_id])
+          authorize! :show_provider_preferences, @payment_method
+          payment_method_type = params[:provider_type]
+          if @payment_method['type'].to_s != payment_method_type
+            @payment_method.update_column(:type, payment_method_type)
+            @payment_method = PaymentMethod.find(params[:pm_id])
+          end
+        else
+          @payment_method = params[:provider_type].constantize.new
+        end
+        render partial: 'provider_settings'
+      end
+
       private
 
+      def force_environment
+        params[:payment_method][:environment] = Rails.env unless spree_current_user.admin?
+      end
+
       def load_data
-        @providers = Gateway.providers.sort{|p1, p2| p1.name <=> p2.name }
+        @providers = if spree_current_user.admin? || Rails.env.test?
+                       Gateway.providers.sort_by(&:name)
+                     else
+                       Gateway.providers.reject{ |p| p.name.include? "Bogus" }.sort_by(&:name)
+                     end
+        @providers.reject!{ |p| p.name.ends_with? "StripeConnect" } unless show_stripe?
+        @calculators = PaymentMethod.calculators.sort_by(&:name)
       end
 
       def validate_payment_method_provider
@@ -60,6 +114,31 @@ module Spree
           redirect_to new_admin_payment_method_path
         end
       end
+
+      def load_hubs
+        # rubocop:disable Style/TernaryParentheses
+        @hubs = Enterprise.managed_by(spree_current_user).is_distributor.sort_by! do |d|
+          [(@payment_method.has_distributor? d) ? 0 : 1, d.name]
+        end
+        # rubocop:enable Style/TernaryParentheses
+      end
+
+      # 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"
+      end
+
+      def restrict_stripe_account_change
+        return unless @payment_method
+        return unless @payment_method.type == "Spree::Gateway::StripeConnect"
+        return unless @payment_method.preferred_enterprise_id.andand > 0
+
+        @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
+      end
     end
   end
 end
diff --git a/app/controllers/spree/admin/payment_methods_controller_decorator.rb b/app/controllers/spree/admin/payment_methods_controller_decorator.rb
deleted file mode 100644
index 53a1404bc9..0000000000
--- a/app/controllers/spree/admin/payment_methods_controller_decorator.rb
+++ /dev/null
@@ -1,91 +0,0 @@
-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]
-      before_filter :load_hubs, only: [:new, :edit, :update]
-      create.before :load_hubs
-
-      # 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
-        return parent.public_send(controller_name) if parent_data.present?
-        collection = if model_class.respond_to?(:accessible_by) &&
-                        !current_ability.has_block?(params[:action], model_class)
-
-                       model_class.accessible_by(current_ability, action)
-
-                     else
-                       model_class.scoped
-                     end
-
-        collection = collection.managed_by(spree_current_user).by_name # This line added
-
-        # This block added
-        if params.key? :enterprise_id
-          distributor = Enterprise.find params[:enterprise_id]
-          collection = collection.for_distributor(distributor)
-        end
-
-        collection
-      end
-
-      def show_provider_preferences
-        if params[:pm_id].present?
-          @payment_method = PaymentMethod.find(params[:pm_id])
-          authorize! :show_provider_preferences, @payment_method
-          payment_method_type = params[:provider_type]
-          if @payment_method['type'].to_s != payment_method_type
-            @payment_method.update_column(:type, payment_method_type)
-            @payment_method = PaymentMethod.find(params[:pm_id])
-          end
-        else
-          @payment_method = params[:provider_type].constantize.new
-        end
-        render partial: 'provider_settings'
-      end
-
-      private
-
-      def force_environment
-        params[:payment_method][:environment] = Rails.env unless spree_current_user.admin?
-      end
-
-      def load_data
-        @providers = if spree_current_user.admin? || Rails.env.test?
-                       Gateway.providers.sort_by(&:name)
-                     else
-                       Gateway.providers.reject{ |p| p.name.include? "Bogus" }.sort_by(&:name)
-                     end
-        @providers.reject!{ |p| p.name.ends_with? "StripeConnect" } unless show_stripe?
-        @calculators = PaymentMethod.calculators.sort_by(&:name)
-      end
-
-      def load_hubs
-        # rubocop:disable Style/TernaryParentheses
-        @hubs = Enterprise.managed_by(spree_current_user).is_distributor.sort_by! do |d|
-          [(@payment_method.has_distributor? d) ? 0 : 1, d.name]
-        end
-        # rubocop:enable Style/TernaryParentheses
-      end
-
-      # 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"
-      end
-
-      def restrict_stripe_account_change
-        return unless @payment_method
-        return unless @payment_method.type == "Spree::Gateway::StripeConnect"
-        return unless @payment_method.preferred_enterprise_id.andand > 0
-
-        @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
-      end
-    end
-  end
-end

From e48ac64d37370b87a143db960e653f77ec76dbc2 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 14 Oct 2019 23:17:43 +0100
Subject: [PATCH 3/6] Prepare spec to integrate some specs coming from
 spree_backend

---
 .../admin/payment_methods_controller_spec.rb  | 184 +++++++++---------
 1 file changed, 93 insertions(+), 91 deletions(-)

diff --git a/spec/controllers/spree/admin/payment_methods_controller_spec.rb b/spec/controllers/spree/admin/payment_methods_controller_spec.rb
index c5811f4fe7..8ace9b9ff5 100644
--- a/spec/controllers/spree/admin/payment_methods_controller_spec.rb
+++ b/spec/controllers/spree/admin/payment_methods_controller_spec.rb
@@ -1,127 +1,129 @@
 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
+  describe 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) }
 
-      before { allow(controller).to receive(:spree_current_user) { 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 } } }
+        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 "as a user that manages the existing stripe account holder" do
-          before { enterprise2.update_attributes!(owner_id: user.id) }
-
-          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
+          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
+end
\ No newline at end of file

From 1eafb1a3fe2ffb04fa4b4583aa1997672c3114aa Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 14 Oct 2019 23:41:18 +0100
Subject: [PATCH 4/6] Bring specs from spree_backend to payment methods
 controller spec

---
 .../admin/payment_methods_controller_spec.rb  | 50 ++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/spec/controllers/spree/admin/payment_methods_controller_spec.rb b/spec/controllers/spree/admin/payment_methods_controller_spec.rb
index 8ace9b9ff5..2a2f03dbcc 100644
--- a/spec/controllers/spree/admin/payment_methods_controller_spec.rb
+++ b/spec/controllers/spree/admin/payment_methods_controller_spec.rb
@@ -1,7 +1,55 @@
 require 'spec_helper'
 
 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 } }
+
+      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)
+
+        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
+
+      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)
+
+        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) }
@@ -126,4 +174,4 @@ module Spree
       end
     end
   end
-end
\ No newline at end of file
+end

From 9a0f1adfd2a3bdfc1e0916cd964e700f688a5ab9 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 14 Oct 2019 23:45:47 +0100
Subject: [PATCH 5/6] Reorganize/simplify before filters

---
 .../spree/admin/payment_methods_controller.rb        | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb
index 05f44b4d7c..a4d251be91 100644
--- a/app/controllers/spree/admin/payment_methods_controller.rb
+++ b/app/controllers/spree/admin/payment_methods_controller.rb
@@ -1,18 +1,17 @@
 module Spree
   module Admin
     class PaymentMethodsController < ResourceController
-      skip_before_filter :load_resource, :only => [:create]
+      skip_before_filter :load_resource, only: [:create, :show_provider_preferences]
       before_filter :load_data
-      before_filter :validate_payment_method_provider, :only => :create
-      before_filter :restrict_stripe_account_change, only: [:update]
-      before_filter :force_environment, only: [:create, :update]
-      skip_before_filter :load_resource, only: [:show_provider_preferences]
+      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)
@@ -27,6 +26,9 @@ module Spree
       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

From 6bc0d505e4484b7c531f2a5d682d89fa9aacfae4 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Mon, 14 Oct 2019 23:52:00 +0100
Subject: [PATCH 6/6] Fix some rubocop issues

---
 .rubocop_manual_todo.yml                      |  3 +++
 .../spree/admin/payment_methods_controller.rb | 22 +++++++++++--------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml
index b5ed5a0b0c..d55030429b 100644
--- a/.rubocop_manual_todo.yml
+++ b/.rubocop_manual_todo.yml
@@ -397,6 +397,7 @@ 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/products_controller_decorator.rb
   - app/controllers/spree/admin/reports_controller_decorator.rb
@@ -576,6 +577,7 @@ 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/products_controller_decorator.rb
   - app/controllers/spree/admin/reports_controller_decorator.rb
@@ -653,6 +655,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.rb b/app/controllers/spree/admin/payment_methods_controller.rb
index a4d251be91..a858c0d651 100644
--- a/app/controllers/spree/admin/payment_methods_controller.rb
+++ b/app/controllers/spree/admin/payment_methods_controller.rb
@@ -12,12 +12,16 @@ module Spree
       def create
         force_environment
 
-        @payment_method = params[:payment_method].delete(:type).constantize.new(params[:payment_method])
+        @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))
+          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)
@@ -38,7 +42,7 @@ module Spree
 
         payment_method_params = params[ActiveModel::Naming.param_key(@payment_method)] || {}
         attributes = params[:payment_method].merge(payment_method_params)
-        attributes.each do |k,v|
+        attributes.each do |k, _v|
           if k.include?("password") && attributes[k].blank?
             attributes.delete(k)
           end
@@ -46,7 +50,7 @@ module Spree
 
         if @payment_method.update_attributes(attributes)
           invoke_callbacks(:update, :after)
-          flash[:success] = Spree.t(:successfully_updated, :resource => Spree.t(:payment_method))
+          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)
@@ -111,10 +115,9 @@ module Spree
 
       def validate_payment_method_provider
         valid_payment_methods = Rails.application.config.spree.payment_methods.map(&:to_s)
-        if !valid_payment_methods.include?(params[:payment_method][:type])
-          flash[:error] = Spree.t(:invalid_payment_provider)
-          redirect_to new_admin_payment_method_path
-        end
+        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
@@ -128,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