Webhook development support and generic payment method slug by elia · Pull Request #271 · solidusio/solidus_stripe

elia · 2023-04-03T17:08:11Z

Summary

both after confirmation and webhook urls are under the same scope
bin/dev will now listen for the supported incoming webhooks
fixes Consider scoping all payment method routes to its slug #220

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

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.

codecov · 2023-04-05T12:35:03Z

Codecov Report

Merging #271 (dad6dd1) into master (7cfff91) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head dad6dd1 differs from pull request most recent head 731629d. Consider uploading reports for the commit 731629d to get more accurate results

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
- Coverage   99.56%   99.56%   -0.01%     
==========================================
  Files          26       26              
  Lines         462      460       -2     
==========================================
- Hits          460      458       -2     
  Misses          2        2

Impacted Files	Coverage Δ
...p/controllers/solidus_stripe/intents_controller.rb	`100.00% <ø> (ø)`
app/models/solidus_stripe/payment_method.rb	`100.00% <100.00%> (ø)`
app/models/solidus_stripe/slug_entry.rb	`100.00% <100.00%> (ø)`
config/routes.rb	`100.00% <100.00%> (ø)`
...tes/app/views/orders/payment_info/_stripe.html.erb	`100.00% <100.00%> (ø)`
lib/solidus_stripe/testing_support/factories.rb	`100.00% <100.00%> (ø)`
lib/solidus_stripe/webhook/event.rb	`96.96% <100.00%> (ø)`

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

rainerdema

I left a comment, but overall looks great! 👏

Also remove the prefix when delegating `#slug` to it.

Add a scope that can be combined with `.first!` to find a payment method by slug.

rainerdema

💯

waiting-for-dev

Thanks for working on that.

I left a posthumous review. No need to take action, but leaving my thoughts for future reference.

waiting-for-dev · 2023-04-11T03:04:03Z

      class << self
        def from_request(payload:, signature_header:, slug:, tolerance: default_tolerance)
-          payment_method = SolidusStripe::WebhookEndpoint.payment_method(slug)
+          payment_method = SolidusStripe::PaymentMethod.with_slug(slug).first!


Not sure to get why we unwrapped the procedure to get the payment method. IMHO, that opens gates to duplication and copy-paste programming.

@waiting-for-dev that's a valid concern. The reason it was moved to a slightly lower-level construct like a scope is that it's more versatile and can be mixed and matched with other scopes without generating extra queries more easily, e.g. in this case

solidus_stripe/app/controllers/solidus_stripe/intents_controller.rb

Lines 45 to 48 in 731629d

@payment_method = current_order(create_order_if_necessary: true)

.available_payment_methods

.merge(SolidusStripe::PaymentMethod.with_slug(params[:slug]))

.first!

waiting-for-dev · 2023-04-11T03:08:58Z

    end

-    private
+    def assign_slug


I think responsibilities were fairer when that method was part of the SlugEntry class, especially considering that the payment method already deals with a lot of stuff (via inheritance from upstream Solidus).

The main reason was (I think) having access to preferred_test_mode (see the other comment), personally I'm fine with both arrangements, with a preference for SlugEntry not needing to know too much about the payment method.

waiting-for-dev · 2023-04-11T03:10:44Z

+    def assign_slug
+      # If there's only one payment method, we can use a default slug.
+      slug = preferred_test_mode ? 'test' : 'live' if self.class.count == 1
+      slug = SecureRandom.hex(16) while SlugEntry.exists?(slug: slug) || slug.nil?


For sure opinionated, but IMHO using recursion made it simpler (for instance, we didn't need to check for slug.nil?) 🙂

I think it was originally moved in order to check the .count and .preferred_test_mode on the payment method, but could be simplified for sure, e.g. the following is probably equivalent:

return if slug_entry.present? slug = preferred_test_mode ? 'test' : 'live' slug = SecureRandom.hex(16) while SlugEntry.exists?(slug: slug)

Although given how rare it is I agree we should do it for readability (vs. performance).

elia

@waiting-for-dev I hope I provided some context on the code changes you highlighted, please feel free to send PRs for refining rough edges if there's anything you particularly care about 🙌

elia · 2023-04-12T11:11:52Z

      class << self
        def from_request(payload:, signature_header:, slug:, tolerance: default_tolerance)
-          payment_method = SolidusStripe::WebhookEndpoint.payment_method(slug)
+          payment_method = SolidusStripe::PaymentMethod.with_slug(slug).first!


@waiting-for-dev that's a valid concern. The reason it was moved to a slightly lower-level construct like a scope is that it's more versatile and can be mixed and matched with other scopes without generating extra queries more easily, e.g. in this case

solidus_stripe/app/controllers/solidus_stripe/intents_controller.rb

Lines 45 to 48 in 731629d

@payment_method = current_order(create_order_if_necessary: true)

.available_payment_methods

.merge(SolidusStripe::PaymentMethod.with_slug(params[:slug]))

.first!

elia · 2023-04-12T11:19:32Z

+    def assign_slug
+      # If there's only one payment method, we can use a default slug.
+      slug = preferred_test_mode ? 'test' : 'live' if self.class.count == 1
+      slug = SecureRandom.hex(16) while SlugEntry.exists?(slug: slug) || slug.nil?


I think it was originally moved in order to check the .count and .preferred_test_mode on the payment method, but could be simplified for sure, e.g. the following is probably equivalent:

return if slug_entry.present? slug = preferred_test_mode ? 'test' : 'live' slug = SecureRandom.hex(16) while SlugEntry.exists?(slug: slug)

Although given how rare it is I agree we should do it for readability (vs. performance).

elia · 2023-04-12T11:24:31Z

    end

-    private
+    def assign_slug


The main reason was (I think) having access to preferred_test_mode (see the other comment), personally I'm fine with both arrangements, with a preference for SlugEntry not needing to know too much about the payment method.

elia self-assigned this Apr 3, 2023

elia force-pushed the elia/webhook-dev-support branch 8 times, most recently from 3844979 to dad6dd1 Compare April 5, 2023 12:20

elia marked this pull request as ready for review April 5, 2023 12:46

elia requested a review from rainerdema April 5, 2023 12:46

rainerdema reviewed Apr 5, 2023

View reviewed changes

Comment thread app/models/solidus_stripe/slug_entry.rb

elia added 5 commits April 5, 2023 16:00

Rename WebhookEndpoint to SlugEntry

58902e7

Also remove the prefix when delegating `#slug` to it.

Move the slug generation to the payment_method

3b0dc86

Move the payment method finder by slug to payment method

34380ef

Add a scope that can be combined with `.first!` to find a payment method by slug.

Automatically start a stripe listener for webhooks to bin/dev

7e977c0

Use the slug as a general scope for the payment method

731629d

elia force-pushed the elia/webhook-dev-support branch from dad6dd1 to 731629d Compare April 5, 2023 14:02

elia requested a review from rainerdema April 5, 2023 14:18

rainerdema approved these changes Apr 5, 2023

View reviewed changes

elia merged commit be652e6 into master Apr 5, 2023

elia deleted the elia/webhook-dev-support branch April 5, 2023 14:45

waiting-for-dev reviewed Apr 11, 2023

View reviewed changes

elia commented Apr 12, 2023

View reviewed changes

waiting-for-dev mentioned this pull request Apr 28, 2023

Allow the use of non-card Stripe payment methods #295

Merged

3 tasks

	@payment_method = current_order(create_order_if_necessary: true)
	.available_payment_methods
	.merge(SolidusStripe::PaymentMethod.with_slug(params[:slug]))
	.first!

Uh oh!

Uh oh!

Conversation

elia commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

codecov Bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rainerdema left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rainerdema left a comment

Choose a reason for hiding this comment

Uh oh!

waiting-for-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elia commented Apr 3, 2023 •

edited

Loading

codecov Bot commented Apr 5, 2023 •

edited

Loading