Skip to content

Handle payment_intent.failed event#246

Merged
waiting-for-dev merged 2 commits into
masterfrom
waiting-for-dev/handle_payment_intent_failed_event
Mar 23, 2023
Merged

Handle payment_intent.failed event#246
waiting-for-dev merged 2 commits into
masterfrom
waiting-for-dev/handle_payment_intent_failed_event

Conversation

@waiting-for-dev

Copy link
Copy Markdown
Contributor

Summary

A payment_intent.failed event is published when a payment intent can't be captured from Stripe.

After we check #185, the webhook could also be triggered by an action from the backend, so we need to do nothing if the payment is already failed.

Closes #179

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@kennyadsl kennyadsl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@codecov

codecov Bot commented Mar 22, 2023

Copy link
Copy Markdown

Codecov Report

Merging #246 (5f8ccf3) into master (da4d80a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #246      +/-   ##
==========================================
+ Coverage   98.12%   98.16%   +0.03%     
==========================================
  Files          23       23              
  Lines         426      435       +9     
==========================================
+ Hits          418      427       +9     
  Misses          8        8              
Impacted Files Coverage Δ
lib/solidus_stripe/webhook/event.rb 96.96% <ø> (ø)
...olidus_stripe/webhook/payment_intent_subscriber.rb 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

payment.failure!.tap do
SolidusStripe::LogEntries.payment_log(
payment,
success: true,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be false?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right. Changed! Thanks for pointing out.

I also added another commit to test the success value for both handlers in the subscriber class.

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/handle_payment_intent_failed_event branch from bd2ba34 to 3d27136 Compare March 23, 2023 05:15
A `payment_intent.failed` event is published [1] when a payment intent
can't be captured from Stripe.

After we check #185, the webhook could also be triggered by an action
from the backend, so we need to do nothing if the payment is already
failed.

Closes #179

[1] - https://stripe.com/docs/payments/payment-intents/verifying-status#webhooks
@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/handle_payment_intent_failed_event branch from 056978f to 5f8ccf3 Compare March 23, 2023 05:40
@waiting-for-dev waiting-for-dev requested a review from elia March 23, 2023 06:50
# @param event [SolidusStripe::Webhook::Event]
def fail_payment(event)
payment = extract_payment_from_event(event)
return if payment.failed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return if payment.failed?
return if !payment.can_failure?

I'd say that checking more generally if we can transition to failed is more generic and safe. You could be on a state (like checkout) where payment is not failed but doing the transition will raise an error. Same goes for the complete! event of the same file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion, @loicginoux. If Solidus payment is in the :checkout state, but we're already getting a payment_intent.payment_failed webhook, that's probably an abnormal situation. I think it's better to be stricter so we can't detect any failure in our integration.

@waiting-for-dev waiting-for-dev merged commit bad6ed0 into master Mar 23, 2023
@waiting-for-dev waiting-for-dev deleted the waiting-for-dev/handle_payment_intent_failed_event branch March 23, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle payment_intent.succeeded webhook event

4 participants