Migrate all reusable credit card to wallet sources #1765
Conversation
| default: credit_card.read_attribute(:default), | ||
| ) | ||
| payment_source: credit_card | ||
| ) do |wallet_source| |
| def up | ||
| credit_cards = Spree::CreditCard. | ||
| where(default: true). | ||
| where("gateway_customer_profile_id IS NOT NULL"). |
There was a problem hiding this comment.
where.not(gateway_customer_profile_id: nil)
| class WalletPaymentSource < ActiveRecord::Base | ||
| self.table_name = 'spree_wallet_payment_sources' | ||
| end | ||
|
|
||
| def up | ||
| credit_cards = Spree::CreditCard. |
There was a problem hiding this comment.
Did you intend to use CreditCard here instead of Spree::CreditCard?
| where.not(user_id: nil) | ||
|
|
||
| credit_cards.find_each do |credit_card| | ||
| Spree::WalletPaymentSource.create!( | ||
| Spree::WalletPaymentSource.find_or_create_by!( |
There was a problem hiding this comment.
WalletPaymentSource instead of Spree::WalletPaymentSource?
| ) | ||
| payment_source: credit_card | ||
| ) do |wallet_source| | ||
| wallet_source.default = credit_card[:default] |
There was a problem hiding this comment.
why is this in a block instead of alongside the other attributes?
There was a problem hiding this comment.
if you use your in-migration classes you can just do credit_card.default right?
There was a problem hiding this comment.
Passing a block to find_or_create_by behaves the same as passing a block to create if the record doesn't already exist.
We want to do this here, because we absolutely only want to have one WalletPaymentSource per credit card, the block specifies the values if one doesn't already exist. This makes the up migration idempotent, which we want since the down migration doesn't remove WalletPaymentSources.
There was a problem hiding this comment.
👍 I stared at this at least 10 times and still managed to miss the fact that you'd changed it from create to find_or_create_by :)
| @@ -19,7 +27,7 @@ def down | |||
| add_column :spree_credit_cards, :default, :boolean, default: false, null: false | |||
|
|
|||
| wallet_payment_sources = Spree::WalletPaymentSource. | |||
| where(default: true, source_type: 'Spree::CreditCard'). | |||
| where(default: true, payment_source_type: 'Spree::CreditCard'). | |||
| includes(:source) | |||
There was a problem hiding this comment.
we'll need some tweaking for the includes to work with the in-migration classes right?
| payment_source: credit_card | ||
| ) do |wallet_source| | ||
| wallet_source.default = credit_card[:default] | ||
| end | ||
| end | ||
|
|
||
| remove_column :spree_credit_cards, :default, :boolean |
There was a problem hiding this comment.
I wonder if we should hold off on actually dropping this column for another release.
Looking at it again right now I feel nervous about dropping a payment-related column like that. Holding off would make the "down" migration simpler also.
Thoughts?
There was a problem hiding this comment.
👍 As long as we provide deprecations.
There was a problem hiding this comment.
The Spree::CreditCard#default method does have deprecations, but if someone was doing a Spree::CreditCard.where(default: true) I'm not sure how we could add a deprecation for that. Perhaps we could rename the column to deprecated_default instead of dropping it? Or just go ahead and drop it?
jhawthorn
force-pushed
the
wallet_migration
branch
from
8d8d472 to
5c16a23
Compare
| user_id: credit_card.user_id, | ||
| payment_source: credit_card | ||
| ) do |wallet_source| | ||
| wallet_source.default = credit_card.default] |
|
@jordan-brough I think I have addressed your feedback. It no longer removes the |
The behaviour prior to adding the Wallet was to allow reuse of any CreditCard which had a payment profile. However the migration was only migrating CreditCards marked "default". This now migrates all credit cards with an associated gateway_customer_profile_id. This also no longer removes the "default" column from the credit card, as we decided this was safer to leave in for at least one release.
jhawthorn
force-pushed
the
wallet_migration
branch
from
5c16a23 to
0c37df7
Compare
Follow up to #1707
The behaviour prior to adding the Wallet was to allow reuse of any
CreditCardwhich had a payment profile. However the migration was only migratingCreditCards markeddefault.This now migrates all credit cards with an associated
gateway_customer_profile_id.