Scenarios we need to consider:

  1. A completed payment is refunded (in whole or in part) at Stripe
  2. An authorization is voided at Stripe
  3. An authorization is captured (in whole or in part) at Stripe
  4. A confirmation of the Payment Element results in a payment intent that is processing
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

vmarchuk created an issue. See original summary.

vmarchuk’s picture

Issue summary: View changes

vmarchuk’s picture

Status: Active » Needs review
rszrama’s picture

Status: Needs review » Needs work

Reviewing this, I have some follow-ups and some questions. First the things we need to change:

  1. Let's remove the logging of incoming webhooks before merging, as we have a separate ticket for that with a settings form in #3389207: Add an option to log incoming webhooks to watchdog.
  2. We also need to ensure we don't call webhooks IPNs, as that's a PayPal specific term.
  3. Where we know we can safely ignore a webhook, we shouldn't generate a warning in the watchdog. For example, we don't need a warning when we intentionally ignore a refund webhook that gets sent in response to an authorization being voided.
  4. I also noticed this when a simple authorization + capture payment during checkout resulted in the demo site receiving a payment_intent.succeeded webhook before the site had completed the original checkout. We would not need a warning in the logs for a webhook we know we can ignore.
  5. That said, we also need some way to identify that a payment was created by Drupal during checkout so we know we can safely ignore them. We specced out different metadata properties for the capture / void / refund source, but we didn't do the same for the initial payment itself. Should it be create_source / create_uid perhaps? Can we even get those into the object in the context of the creation of the payment intent?

I also had a follow-up questions based on my review of StripePaymentElement:createPayment(): did we never arrive at a conclusion regarding the deletion of a payment method when a payment attempt failed? I still see $this->deletePaymentMethod($payment_method); in there, which surprised me, as this deletes the payment method locally and remotely, no questions asked. That still seems like overkill to me, especially if you consider that payment method may be tied to other pending payments or recurring payments in Stripe or Drupal ... do we have a separate ticket to address it perhaps?

rszrama’s picture

Interesting note: even though a partial capture will appear in the Stripe UI as a partial refund, they don't actually send a refund webhook, just the payment_intent.succeeded webhook, even though the payload will have the "refund" in its object.

vmarchuk’s picture

@rszrama

  • 1, 2, 3 - Fixed.
  • 4, 5 - we can't really do that because it would affect the scenarios we're trying to cover (for example, scenario #3), so let's talk about it
  • Regarding removing payment method in StripePaymentElement:createPayment() - new issue created
  • Interesting note: even though a partial capture will appear in the Stripe UI as a partial refund, they don't actually send a refund webhook, just the payment_intent.succeeded webhook, even though the payload will have the "refund" in its object.
    Yes, but in this case we are only interested in payment_intent.succeeded event.
rszrama’s picture

Status: Needs work » Reviewed & tested by the community

I think we can move forward without a full solution for the unnecessary logs in 4/5. I'll just remove logging from her for now on commit, and we can work that logging in via #3389207: Add an option to log incoming webhooks to watchdog.

  • rszrama committed bdd6224f on 8.x-1.x authored by vmarchuk
    Issue #3388175: Implement webhooks as needed for sync payments with...

  • rszrama committed edb3b5e7 on 8.x-1.x
    Issue #3388175 by rszrama: temporarily disable logging ignored webhooks...
rszrama’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.