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

Credit card fixes related to Wallet updates #1773

Merged
merged 1 commit into from
Apr 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions core/app/models/spree/credit_card.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,15 @@ def self.default

def default
Spree::Deprecation.warn("CreditCard.default is deprecated. Please use user.wallet.default_wallet_payment_source instead.", caller)
user.wallet.default_wallet_payment_source.payment_source == self
return false if user.nil?
user.wallet.default_wallet_payment_source.try!(:payment_source) == self
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?
Copy link
Contributor

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?

https://github.com/solidusio/solidus/blob/master/core/app/models/spree/wallet/default_payment_builder.rb#L14

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...

Copy link
Contributor Author

@jordan-brough jordan-brough Mar 16, 2017

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@jordan-brough jordan-brough Mar 16, 2017

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.

raise "Cannot set 'default' on a credit card without a user"
elsif set_as_default # setting this card as default
wallet_payment_source = user.wallet.add(self)
user.wallet.default_wallet_payment_source = wallet_payment_source
true
Expand Down
103 changes: 81 additions & 22 deletions core/spec/models/spree/credit_card_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -274,40 +274,99 @@ def self.payment_states
end
end

# TODO: Remove these specs once default is removed
describe 'default' do
around do |example|
Spree::Deprecation.silence { example.run }
def default_with_silence(card)
Spree::Deprecation.silence { card.default }
end

it 'ensures only one credit card per user is default at a time' do
user = FactoryGirl.create(:user)
first = FactoryGirl.create(:credit_card, user: user, default: true)
second = FactoryGirl.create(:credit_card, user: user, default: true)
context 'with a user' do
let(:user) { create(:user) }
let(:credit_card) { create(:credit_card, user: user) }

expect(first.reload.default).to eq false
expect(second.reload.default).to eq true
it 'uses the wallet information' do
wallet_payment_source = user.wallet.add(credit_card)
user.wallet.default_wallet_payment_source = wallet_payment_source

first.default = true
expect(default_with_silence(credit_card)).to be_truthy
end
end

context 'without a user' do
let(:credit_card) { create(:credit_card) }

expect(first.reload.default).to eq true
expect(second.reload.default).to eq false
it 'returns false' do
expect(default_with_silence(credit_card)).to eq(false)
end
end
end

it 'allows default credit cards for different users' do
first = FactoryGirl.create(:credit_card, user: FactoryGirl.create(:user), default: true)
second = FactoryGirl.create(:credit_card, user: FactoryGirl.create(:user), default: true)
# TODO: Remove these specs once default= is removed
describe 'default=' do
def default_with_silence(card)
Spree::Deprecation.silence { card.default }
end

expect(first.reload.default).to eq true
expect(second.reload.default).to eq true
context 'with a user' do
let(:user) { create(:user) }
let(:credit_card) { create(:credit_card, user: user) }

it 'updates the wallet information' do
Spree::Deprecation.silence do
credit_card.default = true
end
expect(user.wallet.default_wallet_payment_source.payment_source).to eq(credit_card)
end
end

it 'allows this card to save even if the previously default card has expired' do
user = FactoryGirl.create(:user)
first = FactoryGirl.create(:credit_card, user: user, default: true)
second = FactoryGirl.create(:credit_card, user: user, default: false)
first.update_columns(year: DateTime.current.year, month: 1.month.ago.month)
context 'with multiple cards for one user' do
let(:user) { create(:user) }
let(:first_card) { create(:credit_card, user: user) }
let(:second_card) { create(:credit_card, user: user) }

it 'ensures only one default' do
Spree::Deprecation.silence do
first_card.default = true
second_card.default = true
end

expect(default_with_silence(first_card)).to be_falsey
expect(default_with_silence(second_card)).to be_truthy

Spree::Deprecation.silence do
first_card.default = true
end

second.update_attributes!(default: true)
expect(default_with_silence(first_card)).to be_truthy
expect(default_with_silence(second_card)).to be_falsey
end
end

context 'with multiple cards for different users' do
let(:first_card) { create(:credit_card, user: create(:user)) }
let(:second_card) { create(:credit_card, user: create(:user)) }

it 'allows multiple defaults' do
Spree::Deprecation.silence do
first_card.default = true
second_card.default = true
end

expect(default_with_silence(first_card)).to be_truthy
expect(default_with_silence(second_card)).to be_truthy
end
end

context 'without a user' do
let(:credit_card) { create(:credit_card) }

it 'raises' do
expect {
Spree::Deprecation.silence do
credit_card.default = true
end
}.to raise_error("Cannot set 'default' on a credit card without a user")
end
end
end
end