Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stripe: Sprout Initial Stripe Utility #1003

Merged
merged 7 commits into from
Dec 27, 2022

Conversation

zspencer
Copy link
Member

#831 #1002

This is mostly because I want to be able to move forward with connecting Vendor and Distributor accounts in the Marketplace, and to get started fiddling with the Stripe Connect bits.

This also hopefully helps set the stage to getting rid of Utility classes, and instead have those inherit fro UtilityHookup (kind of like how new Furniture inherits from FurniturePlacement

To be honest, I should probably either do a rename pass to more clearly indicate the old-way vs now-and-forward way of defining furniture, or do a final cleanup pass on both of those before starting this, but this was the nearest-mess-to-muck-with ( not clean... just.. uhhh... muck with)

Anyway!

#831
#1002

This is mostly because I want to be able to move forward with connecting
Vendor and Distributor accounts in the Marketplace, and to get started
fiddling with the Stripe Connect bits.

This also hopefully helps set the stage to getting rid of `Utility`
classes, and instead have those inherit fro `UtilityHookup` (kind of
like how new `Furniture` inherits from `FurniturePlacement`

To be honest, I should probably either do a rename pass to more clearly
indicate the old-way vs now-and-forward way of defining furniture, or do
a final cleanup pass on both of those before starting this, but this was
the nearest-mess-to-muck-with ( not clean... just.. uhhh... muck with)

Anyway!
app/policies/utility_hookup_policy.rb Outdated Show resolved Hide resolved
app/models/utility_hookup.rb Outdated Show resolved Hide resolved
app/models/utility_hookup.rb Outdated Show resolved Hide resolved
@zspencer zspencer marked this pull request as draft December 19, 2022 06:09
@zspencer
Copy link
Member Author

CI Failed, back-to-draft with ye.

Thoughts on the trajectory v much welcome.

@zspencer zspencer marked this pull request as ready for review December 22, 2022 04:10
@zspencer
Copy link
Member Author

This is an intermediary step to at least get the Stripe API Token settable; but I pulled back from refactoring the Utilities structure.

I'm going to try and finish unfucking the Furniture structure(s) before I start unfucking Utility.

@anaulin
Copy link
Member

anaulin commented Dec 23, 2022

@zspencer do you still want to merge this Utility, or are we now preferring the approach in #1009? Happy to review this if you still want a review.

@zspencer
Copy link
Member Author

I'm not entirely sure, tbh. I think we have open questions about whether the Stripe API Key wants to live in the Marketplace, or at the Space level. Storing it at the Marketplace for now is probably the straightest path towards a working feature; but having the Utility available/merged in could be useful for when a Space has many Marketplaces.

Copy link
Member

@anaulin anaulin left a comment

Choose a reason for hiding this comment

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

💰 🔑 ✅

@@ -9,9 +9,10 @@ class UtilityHookup < ApplicationRecord
# has multiple {UtilityHookup}s.
# @return [String]
attribute :name, :string
validates :name, presence: :true, uniqueness: { scope: :space_id }
Copy link
Member

Choose a reason for hiding this comment

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

This is ok for now, but I can imagine wanting to have more than one Stripe account associated with a space in the future. But maybe the hookup itself will support multiple API keys. We'll cross that bridge when we get to it.

Copy link
Member Author

@zspencer zspencer Dec 27, 2022

Choose a reason for hiding this comment

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

This is validating the name given to the utility to disambiguate it against different utilities on the same space. I.e. Stripe Account - Jim's Beans vs Stripe Account - Jane's Jams

Maybe we should rename the field to label? Does that feel more clear than name?

Copy link
Member

Choose a reason for hiding this comment

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

Oh 🤦🏼‍♀️ Bad code reviewer, no donut for me.

@zspencer zspencer merged commit 36e372c into main Dec 27, 2022
@zspencer zspencer deleted the marketplace/vendor-connects-stripe-account branch December 27, 2022 18:10
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