-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Credit card fixes related to Wallet updates #1773
Credit card fixes related to Wallet updates #1773
Conversation
The deprecated methods didn't handle all cases.
b49889e
to
dbc95cb
Compare
end | ||
|
||
def default=(set_as_default) | ||
Spree::Deprecation.warn("CreditCard.default= is deprecated. Please use user.wallet.default_wallet_payment_source= instead.", caller) | ||
if set_as_default # setting this card as default | ||
if user.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work reasonably with the current default behavior and guest checkouts?
Its tough to trace all the way through easily (with all the aliases and whatnot) but I think we'll get the default set to nil if there is no user? Though none of our specs seem to be failing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbrunsdon that code you referenced (and the add_default_payment_from_wallet
method that calls it) is a no-op when order.user
is nil, right? Is that what you're asking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the AddPaymentSourcesToWallet code is a no-op if order.user
is nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also checked our permitted attrs and it looks like they prevent setting credit card defaults via the API so I don't think there should be a problem in that direction.
And the admin doesn't allow setting the default
attribute either.
I'm not seeing any other code that tries to set the default on a credit card, other than the above items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
The deprecated methods didn't handle all cases.