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!)

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,

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,

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?
 

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"

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