Interfaces for persist_user_credit_card and assign_default_credit_card #1086
Conversation
jordan-brough
force-pushed
the
configurable-wallet-behavior
branch
from
847675f
to
0a51f7f
Compare
|
I'm torn on having the "add after order complete" being a separate objects. I almost think it should be supported somehow by the wallet API itself to keep the api more cohese and clear for anyone using it. I'd almost expect something more generic to be called like: wallet.events.order_complete(order)which users could do as they wish with (including persist it, if desired). The second one, the add_default_payment_source I'm weirded out about for another reason. It seems weird that a Wallet:: class would be responsible for creating a payment on an order after a transition. But thats maybe mostly related to how ugly it is now. |
| @@ -0,0 +1,24 @@ | |||
| class Spree::Wallet::AddDefaultPayment | |||
There was a problem hiding this comment.
We should consider to call this class DefaultPaymentBuilder as we actually don't add something, we build something. Otherwise great!
There was a problem hiding this comment.
And with that name change, the method could be named just build I think.
There was a problem hiding this comment.
@tvdeyen @peterberkenbosch updated. Thanks!
|
Good step in the right direction. I Like that. |
|
I see a Love to discuss this further and move forward :) |
jordan-brough
force-pushed
the
configurable-wallet-behavior
branch
from
0a51f7f
to
f03d5c1
Compare
|
@cbrunsdon I hear you on the weirdness and current ugliness. I'd lean toward getting this in since it solves the biggest problem of stores being able to customize this logic without monkey patching and then revisit during or after the main Wallet PR. What do you think though?
I like that. How about I make that change in the main Wallet PR after we merge this? (e.g. the |
|
Also, this PR is pointing to master right now. Should it be towards this branch? solidusio:jordan/non-credit-card-payment-sources-base |
@peterberkenbosch I did want to point it at master. I did this in a weird way...but this was one of three things in that other branch that I wanted to get into master before the main Wallet PR. |
|
Got it @jordan-brough, I missed the add commit with the |
|
wdyt @cbrunsdon @jhawthorn |
| # AddAfterOrderComplete is called after an order transitions to complete. It | ||
| # is responsible for saving payment sources in the user's "wallet" for future | ||
| # use. | ||
| def add_to_wallet |
There was a problem hiding this comment.
Given this comment (https://github.com/solidusio/solidus/pull/1086/files#diff-a9fae1594c22d1ec005ac8501d80d7afR356) and the importance of documenting these interfaces, could we add some more YARD docs to this method?
|
I would find this PR helpful. We've got these customizations hacked into our app now and would love to have a nice interface for it! |
|
I've thought about this one for a few days and can't think of anything that would improve it, and I think its better than what we have now, so I'm at a 👍 if @jhawthorn is. |
|
Thanks @cbrunsdon! @jhawthorn wdyt? :) |
|
@jhawthorn updated as per our chat |
jordan-brough
force-pushed
the
configurable-wallet-behavior
branch
from
646012c
to
4584a56
Compare
|
👍 Looks good |
Extract the logic for persist_user_credit_card and assign_default_credit_card into configurable classes.
jordan-brough
force-pushed
the
configurable-wallet-behavior
branch
from
4584a56
to
504b486
Compare
Extract the logic for persist_user_credit_card and
assign_default_credit_card into configurable classes.
Does this seem like something we'd like to do? This is from a pairing session with
@mamhoff around non-credit card payment sources. If it seems good then I'll
add some specs around it.