Skip to content

Add setup intents support to stripe dashboard link helper#212

Merged
elia merged 2 commits into
masterfrom
elia/stripe-dashboard-link
Mar 9, 2023
Merged

Add setup intents support to stripe dashboard link helper#212
elia merged 2 commits into
masterfrom
elia/stripe-dashboard-link

Conversation

@elia

@elia elia commented Mar 9, 2023

Copy link
Copy Markdown
Member

Summary

We added payment and setup intents classes attached to the order and we should provide dashboard links in accordance to it.

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.

@elia elia requested a review from waiting-for-dev March 9, 2023 11:26
@elia elia self-assigned this Mar 9, 2023
Comment thread app/models/solidus_stripe/payment_method.rb

@waiting-for-dev waiting-for-dev left a comment

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.

Thanks, @elia! I left a couple of suggestions, although feel free to go ahead if you think that's premature optimization before we have a more defined interface.

Comment on lines +171 to +172
SolidusStripe::PaymentIntent.where(order: payment.order).pick(:stripe_payment_intent_id) ||
SolidusStripe::SetupIntent.where(order: payment.order).pick(:stripe_setup_intent_id)

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.

Thoughts on extracting the logic to the classes? Like:

class SolidusStripe::PaymentIntent
  def self.stripe_id(order)
    where(order: order).pick(:stripe_payment_intent_id)
  end

That could help duck-typing at a later stage if we're not ready yet.

Comment on lines +175 to +178
when /^pi_/
"https://dashboard.stripe.com#{path_prefix}/payments/#{intent_id}"
when /^seti_/
"https://dashboard.stripe.com#{path_prefix}/setup_intents/#{intent_id}"

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.

Similarly, we could duck type here, although the problem is the payment.transaction_id getting in the way.

@waiting-for-dev

Copy link
Copy Markdown
Contributor

@elia, linting is failing, though 🙂

@elia

elia commented Mar 9, 2023

Copy link
Copy Markdown
Member Author

@elia, linting is failing, though 🙂

@waiting-for-dev yeah, I dared to skip the draft at my own peril 😅 – fixing it 👍

@elia

elia commented Mar 9, 2023

Copy link
Copy Markdown
Member Author

@waiting-for-dev I'll keep the suggestions for the strategy class, for this one I just wanted to have a working link for manual tests 🙌

@elia elia force-pushed the elia/stripe-dashboard-link branch from cf55521 to 9fe3784 Compare March 9, 2023 14:24
@elia elia merged commit 5c79be7 into master Mar 9, 2023
@elia elia deleted the elia/stripe-dashboard-link branch March 9, 2023 14:36
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.

2 participants