From 4cdb892c75f4c17a9d61e5ee2f56cbeb5f3e597c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 2 Dec 2021 14:43:12 +0000 Subject: [PATCH] Refactor formatting of credit card brand names and reformat automatically when saving This little bit of "translation" between what we might receive as input and what ActiveMerchant requires is important, but currently the responsibility for this job is spread all over the code base. It can now just live in the CreditCard model (in one place) and doesn't need to be duplicated anywhere else (like several different places in frontend Javascript!) --- .../services/stripe_elements.js.coffee | 22 +----- .../services/stripe_elements.js.coffee | 22 +----- app/models/spree/credit_card.rb | 36 ++++++---- .../services/stripe_elements_spec.js.coffee | 18 +---- spec/models/spree/credit_card_spec.rb | 71 ++++++++++++------- 5 files changed, 72 insertions(+), 97 deletions(-) diff --git a/app/assets/javascripts/admin/payments/services/stripe_elements.js.coffee b/app/assets/javascripts/admin/payments/services/stripe_elements.js.coffee index d5c20e8668..03808bbbfc 100644 --- a/app/assets/javascripts/admin/payments/services/stripe_elements.js.coffee +++ b/app/assets/javascripts/admin/payments/services/stripe_elements.js.coffee @@ -17,7 +17,7 @@ angular.module("admin.payments").factory 'AdminStripeElements', ($rootScope, Sta console.error(JSON.stringify(response.error)) else secrets.token = response.token.id - secrets.cc_type = @mapTokenApiCardBrand(response.token.card.brand) + secrets.cc_type = response.token.card.brand secrets.card = response.token.card submit() @@ -33,28 +33,10 @@ angular.module("admin.payments").factory 'AdminStripeElements', ($rootScope, Sta console.error(JSON.stringify(response.error)) else secrets.token = response.paymentMethod.id - secrets.cc_type = @mapPaymentMethodsApiCardBrand(response.paymentMethod.card.brand) + secrets.cc_type = response.paymentMethod.card.brand secrets.card = response.paymentMethod.card submit() - # Maps the brand returned by Stripe's tokenAPI to that required by activemerchant - mapTokenApiCardBrand: (cardBrand) -> - switch cardBrand - when 'MasterCard' then return 'master' - when 'Visa' then return 'visa' - when 'American Express' then return 'american_express' - when 'Discover' then return 'discover' - when 'JCB' then return 'jcb' - when 'Diners Club' then return 'diners_club' - - # Maps the brand returned by Stripe's paymentMethodsAPI to that required by activemerchant - mapPaymentMethodsApiCardBrand: (cardBrand) -> - switch cardBrand - when 'mastercard' then return 'master' - when 'amex' then return 'american_express' - when 'diners' then return 'diners_club' - else return cardBrand # a few brands are equal, for example, visa - # It doesn't matter if any of these are nil, all are optional. makeCardData: (secrets) -> {'name': secrets.name, diff --git a/app/assets/javascripts/darkswarm/services/stripe_elements.js.coffee b/app/assets/javascripts/darkswarm/services/stripe_elements.js.coffee index 9d66876716..fba76cdf00 100644 --- a/app/assets/javascripts/darkswarm/services/stripe_elements.js.coffee +++ b/app/assets/javascripts/darkswarm/services/stripe_elements.js.coffee @@ -16,7 +16,7 @@ angular.module('Darkswarm').factory 'StripeElements', ($rootScope, Messages) -> @reportError(response.error, t("error") + ": #{response.error.message}") else secrets.token = response.token.id - secrets.cc_type = @mapTokenApiCardBrand(response.token.card.brand) + secrets.cc_type = response.token.card.brand secrets.card = response.token.card submit() .catch (response) => @@ -37,7 +37,7 @@ angular.module('Darkswarm').factory 'StripeElements', ($rootScope, Messages) -> @reportError(response.error, t("error") + ": #{response.error.message}") else secrets.token = response.paymentMethod.id - secrets.cc_type = @mapPaymentMethodsApiCardBrand(response.paymentMethod.card.brand) + secrets.cc_type = response.paymentMethod.card.brand secrets.card = response.paymentMethod.card submit() .catch (response) => @@ -56,24 +56,6 @@ angular.module('Darkswarm').factory 'StripeElements', ($rootScope, Messages) -> # $evalAsync is improved way of triggering a digest without calling $apply $rootScope.$evalAsync() - # Maps the brand returned by Stripe's tokenAPI to that required by activemerchant - mapTokenApiCardBrand: (cardBrand) -> - switch cardBrand - when 'MasterCard' then return 'master' - when 'Visa' then return 'visa' - when 'American Express' then return 'american_express' - when 'Discover' then return 'discover' - when 'JCB' then return 'jcb' - when 'Diners Club' then return 'diners_club' - - # Maps the brand returned by Stripe's paymentMethodsAPI to that required by activemerchant - mapPaymentMethodsApiCardBrand: (cardBrand) -> - switch cardBrand - when 'mastercard' then return 'master' - when 'amex' then return 'american_express' - when 'diners' then return 'diners_club' - else return cardBrand # a few brands are equal, for example, visa - # It doesn't matter if any of these are nil, all are optional. makeCardData: (secrets) -> {'name': secrets.name, diff --git a/app/models/spree/credit_card.rb b/app/models/spree/credit_card.rb index 1145cb06d2..9bf584ea46 100644 --- a/app/models/spree/credit_card.rb +++ b/app/models/spree/credit_card.rb @@ -7,6 +7,7 @@ module Spree has_many :payments, as: :source + before_validation :reformat_card_type! before_save :set_last_digits attr_accessor :verification_value @@ -39,20 +40,8 @@ module Spree end end - # cc_type is set by jquery.payment, which helpfully provides different - # types from Active Merchant. Converting them is necessary. def cc_type=(type) - real_type = case type - when 'mastercard', 'maestro' - 'master' - when 'amex' - 'american_express' - when 'dinersclub' - 'diners_club' - else - type - end - self[:cc_type] = real_type + reformat_card_type!(type) end def set_last_digits @@ -129,6 +118,27 @@ module Spree private + def reformat_card_type!(type = nil) + self[:cc_type] = active_merchant_card_type(type || cc_type) + end + + # ActiveMerchant requires certain credit card brand names to be stored in a specific format. + # See: https://github.com/activemerchant/active_merchant/blob/master/lib/active_merchant/billing/credit_card.rb#L89 + def active_merchant_card_type(type) + card_type = type.to_s.downcase + + case card_type + when "mastercard", "maestro", "master card" + "master" + when "amex", "american express" + "american_express" + when "dinersclub", "diners club" + "diners_club" + else + card_type + end + end + def expiry_not_in_the_past return unless year.present? && month.present? diff --git a/spec/javascripts/unit/darkswarm/services/stripe_elements_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/stripe_elements_spec.js.coffee index d7e7fa249a..d901537ba2 100644 --- a/spec/javascripts/unit/darkswarm/services/stripe_elements_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/stripe_elements_spec.js.coffee @@ -34,7 +34,7 @@ describe 'StripeElements Service', -> StripeElements.requestToken(secrets, submit) $rootScope.$digest() # required for #then to by called expect(secrets.token).toEqual "token" - expect(secrets.cc_type).toEqual "master" + expect(secrets.cc_type).toEqual "MasterCard" expect(submit).toHaveBeenCalled() describe "with unsatifactory data", -> @@ -52,19 +52,3 @@ describe 'StripeElements Service', -> expect(Loading.clear).toHaveBeenCalled() expect(RailsFlashLoader.loadFlash).toHaveBeenCalledWith({error: "Error: There was a problem"}) expect(Bugsnag.notify).toHaveBeenCalled() - - describe 'mapTokenApiCardBrand', -> - it "maps the brand returned by Stripe's tokenAPI to that required by activemerchant", -> - expect(StripeElements.mapTokenApiCardBrand('MasterCard')).toEqual "master" - expect(StripeElements.mapTokenApiCardBrand('Visa')).toEqual "visa" - expect(StripeElements.mapTokenApiCardBrand('American Express')).toEqual "american_express" - expect(StripeElements.mapTokenApiCardBrand('Discover')).toEqual "discover" - expect(StripeElements.mapTokenApiCardBrand('JCB')).toEqual "jcb" - expect(StripeElements.mapTokenApiCardBrand('Diners Club')).toEqual "diners_club" - - describe 'mapPaymentMethodsApiCardBrand', -> - it "maps the brand returned by Stripe's paymentMethodsAPI to that required by activemerchant", -> - expect(StripeElements.mapPaymentMethodsApiCardBrand('mastercard')).toEqual "master" - expect(StripeElements.mapPaymentMethodsApiCardBrand('amex')).toEqual "american_express" - expect(StripeElements.mapPaymentMethodsApiCardBrand('diners')).toEqual "diners_club" - expect(StripeElements.mapPaymentMethodsApiCardBrand('visa')).toEqual "visa" diff --git a/spec/models/spree/credit_card_spec.rb b/spec/models/spree/credit_card_spec.rb index 3a8bd54ec1..23f40b3489 100644 --- a/spec/models/spree/credit_card_spec.rb +++ b/spec/models/spree/credit_card_spec.rb @@ -4,16 +4,16 @@ require 'spec_helper' module Spree describe CreditCard do - describe "original specs from Spree" do - let(:valid_credit_card_attributes) { - { - number: '4111111111111111', - verification_value: '123', - month: 12, - year: Time.zone.now.year + 1 - } + let(:valid_credit_card_attributes) { + { + number: '4111111111111111', + verification_value: '123', + month: 12, + year: Time.zone.now.year + 1 } + } + describe "original specs from Spree" do def self.payment_states Spree::Payment.state_machine.states.keys end @@ -151,25 +151,6 @@ module Spree end end - context "#cc_type=" do - it "converts between the different types" do - credit_card.cc_type = 'mastercard' - expect(credit_card.cc_type).to eq 'master' - - credit_card.cc_type = 'maestro' - expect(credit_card.cc_type).to eq 'master' - - credit_card.cc_type = 'amex' - expect(credit_card.cc_type).to eq 'american_express' - - credit_card.cc_type = 'dinersclub' - expect(credit_card.cc_type).to eq 'diners_club' - - credit_card.cc_type = 'some_outlandish_cc_type' - expect(credit_card.cc_type).to eq 'some_outlandish_cc_type' - end - end - context "#associations" do it "should be able to access its payments" do expect { credit_card.payments.to_a }.not_to raise_error @@ -198,6 +179,42 @@ module Spree end end + describe "formatting the card type for ActiveMerchant" do + context "#cc_type=" do + let(:credit_card) { build(:credit_card) } + + it "converts the card type format" do + credit_card.cc_type = 'mastercard' + expect(credit_card.cc_type).to eq 'master' + + credit_card.cc_type = 'maestro' + expect(credit_card.cc_type).to eq 'master' + + credit_card.cc_type = 'amex' + expect(credit_card.cc_type).to eq 'american_express' + + credit_card.cc_type = 'dinersclub' + expect(credit_card.cc_type).to eq 'diners_club' + + credit_card.cc_type = 'some_outlandish_cc_type' + expect(credit_card.cc_type).to eq 'some_outlandish_cc_type' + end + end + + context "on save" do + it "converts the card type format" do + expect_any_instance_of(Spree::CreditCard).to receive(:reformat_card_type!). + at_least(:once).and_call_original + + credit_card = Spree::CreditCard.create( + valid_credit_card_attributes.merge(cc_type: "Master Card") + ) + + expect(credit_card.cc_type).to eq "master" + end + end + end + describe "setting default credit card for a user" do let(:user) { create(:user) } let(:onetime_card_attrs) do