Charge legacy us_bank_account instruments through PaymentIntents

lib/active_merchant/billing/gateways/stripe_ach_setup_intents.rb

@@ -80,19 +80,26 @@ def void(identification, options = {})
         commit(:post, "refunds", { payment_intent: identification }, options)
       end
 
-      # Detaches the customer's us_bank_account PaymentMethod(s). The legacy StripeGateway#unstore
-      # deletes a card source (customers/{id}/cards/{id}), which does not apply to PaymentMethod-based
-      # us_bank_account profiles.
+      # Detaches the one us_bank_account PaymentMethod this profile owns. The legacy
+      # StripeGateway#unstore deletes a card source (customers/{id}/cards/{id}), which does not apply
+      # to PaymentMethod-based us_bank_account profiles. One Stripe customer can back several payment
+      # profiles, so detaching every listed method would remove instruments belonging to profiles the
+      # caller never asked about.
       def unstore(identification, options = {}, _deprecated_options = {})
-        customer_id = identification.to_s.split("|").first
-        payment_methods = list_us_bank_account_payment_methods(customer_id)
-        return Response.new(true, "No us_bank_account payment method to detach") if payment_methods.empty?
-
-        MultiResponse.run(:first) do |r|
-          payment_methods.each do |payment_method|
-            r.process { commit(:post, "payment_methods/#{CGI.escape(payment_method["id"])}/detach", {}, options) }
-          end
+        customer_id, instrument_id = identification.to_s.split("|")
+        listed = list_us_bank_account_payment_methods(customer_id).map { |pm| pm["id"] }
+        # Nothing listed, or a named instrument no longer listed: the detach already happened.
+        already_detached = listed.empty? || (instrument_id && !listed.include?(instrument_id))
+        return Response.new(true, "No us_bank_account payment method to detach") if already_detached
+
+        payment_method_id = instrument_id ||
+                            (listed.size == 1 ? listed.first : customer_default_payment_method(customer_id))
+        unless listed.include?(payment_method_id)
+          raise StripeCustomerManyPaymentMethodWithoutDefault,
+                "Customer has more than one us_bank_account payment method and none identifies this profile."
         end
+
+        commit(:post, "payment_methods/#{payment_method_id}/detach", {}, options)
       end
 
       private
@@ -192,25 +199,22 @@ def customer_acceptance(options)
       # back to the single listed us_bank_account PM. Raises on ambiguity or absence rather than
       # silently charging the wrong method.
       def us_bank_account_payment_method_for_customer(customer)
-        # Only modern PaymentMethods (pm_*) are valid as a PaymentIntent payment_method. Stripe can
-        # list legacy bank-account sources here with their original ba_* id; those must not be used.
-        payment_method_ids = list_us_bank_account_payment_methods(customer)
-          .map { |pm| pm["id"] }
-          .select { |id| id.to_s.start_with?("pm_") }
+        # Not filtered by id prefix: a legacy bank account keeps its ba_* id, and PaymentIntent takes it.
+        ids = list_us_bank_account_payment_methods(customer).map { |pm| pm["id"] }
 
-        return payment_method_ids.first if payment_method_ids.size == 1
+        raise RuntimeError, "Customer has no us_bank_account payment method." if ids.empty?
+        return ids.first if ids.size == 1
 
-        if payment_method_ids.size > 1
-          # Disambiguate by the customer's default payment method, but only when the default is
-          # itself one of the us_bank_account methods (it could be a card).
-          default = customer_default_payment_method(customer)
-          return default if default && payment_method_ids.include?(default)
+        # The default is only usable when it is itself one of the us_bank_account methods (it could be a card).
+        default = customer_default_payment_method(customer)
+        return default if ids.include?(default)
 
-          raise StripeCustomerManyPaymentMethodWithoutDefault,
-                "Customer has more than one us_bank_account payment method but no default one."
-        end
+        # A single modern PM beside a legacy ba_* charged fine before the pm_* filter went away; keep it working.
+        modern = ids.grep(/\Apm_/)
+        return modern.first if modern.size == 1
 
-        raise RuntimeError, "Customer has no us_bank_account payment method."
+        raise StripeCustomerManyPaymentMethodWithoutDefault,
+              "Customer has more than one us_bank_account payment method but no default one."
       end
 
       def list_us_bank_account_payment_methods(customer)

test/unit/gateways/stripe_ach_setup_intents_test.rb

@@ -59,13 +59,9 @@ def test_store_rejects_non_bank_account_payment
   end
 
   def test_store_creates_us_bank_account_pm_and_setup_intent_on_new_customer
-    stub_pm = stub_request_for("payment_methods", successful_payment_method_response)
-    stub_customer = stub_request_for("customers", successful_new_customer_response)
-    stub_setup_intent = stub_request_for("setup_intents", successful_setup_intent_response)
-
-    expect_ssl_for("/v1/payment_methods", stub_pm)
-    expect_ssl_for("/v1/customers", stub_customer)
-    expect_ssl_for("/v1/setup_intents", stub_setup_intent)
+    expect_ssl_for("/v1/payment_methods", successful_payment_method_response)
+    expect_ssl_for("/v1/customers", successful_new_customer_response)
+    expect_ssl_for("/v1/setup_intents", successful_setup_intent_response)
 
     response = @gateway.store(@check, @options)
 
@@ -83,23 +79,15 @@ def test_store_creates_payment_method_with_us_bank_account_fields_and_mapped_hol
         post.include?("us_bank_account[account_holder_type]=individual") && # personal -> individual
         post.include?("us_bank_account[account_type]=checking")
     end.returns(successful_payment_method_response)
-    @gateway.stubs(:ssl_request).with do |_method, endpoint, _post, _headers|
-      endpoint.start_with?("https://api.stripe.com/v1/customers")
-    end.returns(successful_new_customer_response)
-    @gateway.stubs(:ssl_request).with do |_method, endpoint, _post, _headers|
-      endpoint.start_with?("https://api.stripe.com/v1/setup_intents")
-    end.returns(successful_setup_intent_response)
+    stub_ssl_for("/v1/customers", successful_new_customer_response)
+    stub_ssl_for("/v1/setup_intents", successful_setup_intent_response)
 
     assert_success @gateway.store(@check, @options)
   end
 
   def test_store_uses_online_mandate_for_browser_channel
-    @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/payment_methods")
-    end.returns(successful_payment_method_response)
-    @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/customers")
-    end.returns(successful_new_customer_response)
+    stub_ssl_for("/v1/payment_methods", successful_payment_method_response)
+    stub_ssl_for("/v1/customers", successful_new_customer_response)
     @gateway.expects(:ssl_request).with do |_m, endpoint, post, _h|
       endpoint.start_with?("https://api.stripe.com/v1/setup_intents") &&
         post.include?("mandate_data[customer_acceptance][type]=online") &&
@@ -111,26 +99,18 @@ def test_store_uses_online_mandate_for_browser_channel
   end
 
   def test_store_uses_offline_mandate_for_api_channel
-    @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/payment_methods")
-    end.returns(successful_payment_method_response)
-    @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/customers")
-    end.returns(successful_new_customer_response)
-    @gateway.expects(:ssl_request).with do |_m, endpoint, post, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/setup_intents") &&
-        post.include?("mandate_data[customer_acceptance][type]=offline")
-    end.returns(successful_setup_intent_response)
+    stub_ssl_for("/v1/payment_methods", successful_payment_method_response)
+    stub_ssl_for("/v1/customers", successful_new_customer_response)
+    expect_ssl_for("/v1/setup_intents", successful_setup_intent_response,
+                   "mandate_data[customer_acceptance][type]=offline")
 
     assert_success @gateway.store(@check, @options.merge(channel: "api"))
   end
 
   # --- purchase ------------------------------------------------------------------------------
 
   def test_purchase_creates_off_session_payment_intent_with_resolved_bank_pm
-    @gateway.expects(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer")
-    end.returns(payment_methods_list_with_single_bank_account)
+    expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123"))
     @gateway.expects(:ssl_request).with do |_m, endpoint, post, _h|
       endpoint.start_with?("https://api.stripe.com/v1/payment_intents") &&
         post.include?("off_session=true") &&
@@ -146,26 +126,47 @@ def test_purchase_creates_off_session_payment_intent_with_resolved_bank_pm
   end
 
   def test_purchase_treats_processing_payment_intent_as_success
-    @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer")
-    end.returns(payment_methods_list_with_single_bank_account)
-    @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/payment_intents")
-    end.returns(successful_payment_intent_response("processing"))
+    stub_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123"))
+    stub_ssl_for("/v1/payment_intents", successful_payment_intent_response("processing"))
 
     assert_success @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH"))
   end
 
   def test_purchase_raises_when_customer_has_no_us_bank_account_pm
-    @gateway.stubs(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer")
-    end.returns(empty_payment_methods_list)
+    stub_ssl_for("payment_methods?customer", payment_methods_list)
 
     assert_raises(RuntimeError) do
       @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH"))
     end
   end
 
+  def test_purchase_uses_a_legacy_bank_account_id_as_the_payment_method
+    expect_ssl_for("payment_methods?customer", payment_methods_list("ba_legacy123"))
+    expect_ssl_for("/v1/payment_intents", successful_payment_intent_response("processing"),
+                   "payment_method=ba_legacy123")
+
+    assert_success @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH"))
+  end
+
+  # Documents why Stripe::AchRoutingState keeps a customer with several legacy instruments on the
+  # legacy path: here there is nothing to disambiguate them with, so the charge cannot be made at all.
+  def test_purchase_raises_when_several_instruments_have_no_usable_default
+    stub_ssl_for("payment_methods?customer", payment_methods_list("ba_legacy123", "ba_legacy999"))
+    stub_ssl_for("/v1/customers/", customer_without_default_payment_method)
+
+    assert_raises(ActiveMerchant::Billing::StripeCustomerManyPaymentMethodWithoutDefault) do
+      @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH"))
+    end
+  end
+
+  def test_purchase_prefers_the_single_modern_payment_method_over_a_legacy_sibling
+    stub_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123", "ba_legacy123"))
+    stub_ssl_for("/v1/customers/", customer_without_default_payment_method)
+    expect_ssl_for("/v1/payment_intents", successful_payment_intent_response("processing"), "payment_method=pm_bank123")
+
+    assert_success @gateway.purchase(@amount, nil, @options.merge(customer: "cus_ACH"))
+  end
+
   # --- refund --------------------------------------------------------------------------------
 
   def test_refund_refunds_by_payment_intent
@@ -183,36 +184,62 @@ def test_refund_refunds_by_payment_intent
   # --- unstore (detach) ----------------------------------------------------------------------
 
   def test_unstore_detaches_us_bank_account_payment_methods
-    @gateway.expects(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer")
-    end.returns(payment_methods_list_with_single_bank_account)
-    @gateway.expects(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint == "https://api.stripe.com/v1/payment_methods/pm_bank123/detach"
-    end.returns(successful_detach_response)
+    expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123"))
+    expect_ssl_for("payment_methods/pm_bank123/detach", successful_detach_response)
 
     assert_success @gateway.unstore("cus_ACH")
   end
 
   def test_unstore_is_a_noop_when_no_bank_account_pm_exists
-    @gateway.expects(:ssl_request).with do |_m, endpoint, _p, _h|
-      endpoint.start_with?("https://api.stripe.com/v1/payment_methods?customer")
-    end.returns(empty_payment_methods_list)
+    expect_ssl_for("payment_methods?customer", payment_methods_list)
 
     assert_success @gateway.unstore("cus_ACH")
   end
 
-  private
+  def test_unstore_detaches_only_the_payment_method_named_in_the_identification
+    expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123", "pm_bank999"))
+    expect_ssl_for("payment_methods/pm_bank999/detach", successful_detach_response)
+
+    assert_success @gateway.unstore("cus_ACH|pm_bank999")
+  end
+
+  def test_unstore_detaches_a_legacy_bank_account_id
+    expect_ssl_for("payment_methods?customer", payment_methods_list("ba_legacy123"))
+    expect_ssl_for("payment_methods/ba_legacy123/detach", successful_detach_response)
+
+    assert_success @gateway.unstore("cus_ACH|ba_legacy123")
+  end
+
+  def test_unstore_is_a_noop_when_the_named_instrument_is_already_detached
+    expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123"))
+
+    response = @gateway.unstore("cus_ACH|ba_gone123")
 
-  def stub_request_for(_endpoint, response)
-    response
+    assert_success response
+    assert_equal "No us_bank_account payment method to detach", response.message
+  end
+
+  def test_unstore_raises_rather_than_detaching_every_method_when_the_profile_is_ambiguous
+    expect_ssl_for("payment_methods?customer", payment_methods_list("pm_bank123", "pm_bank999"))
+    expect_ssl_for("/v1/customers/cus_ACH", customer_without_default_payment_method)
+
+    assert_raises(StripeCustomerManyPaymentMethodWithoutDefault) do
+      @gateway.unstore("cus_ACH")
+    end
   end
 
-  def expect_ssl_for(path_fragment, response)
-    @gateway.expects(:ssl_request).with do |_method, endpoint, _post, _headers|
-      endpoint.include?(path_fragment)
+  private
+
+  def expect_ssl_for(path_fragment, response, post_fragment = nil, mode: :expects)
+    @gateway.send(mode, :ssl_request).with do |_method, endpoint, post, _headers|
+      endpoint.include?(path_fragment) && (post_fragment.nil? || post.to_s.include?(post_fragment))
     end.returns(response)
   end
 
+  def stub_ssl_for(path_fragment, response, post_fragment = nil)
+    expect_ssl_for(path_fragment, response, post_fragment, mode: :stubs)
+  end
+
   # --- fixtures ------------------------------------------------------------------------------
 
   def successful_payment_method_response
@@ -239,11 +266,12 @@ def successful_detach_response
     %({"id": "pm_bank123", "object": "payment_method", "customer": null, "livemode": false})
   end
 
-  def payment_methods_list_with_single_bank_account
-    %({"object": "list", "data": [{"id": "pm_bank123", "type": "us_bank_account"}], "livemode": false})
+  def payment_methods_list(*ids)
+    data = ids.map { |id| %({"id": "#{id}", "type": "us_bank_account"}) }.join(", ")
+    %({"object": "list", "data": [#{data}], "livemode": false})
   end
 
-  def empty_payment_methods_list
-    %({"object": "list", "data": [], "livemode": false})
+  def customer_without_default_payment_method
+    %({"id": "cus_ACH", "object": "customer", "invoice_settings": {"default_payment_method": null}, "livemode": false})
   end
 end