Spree::Wallet & Non credit card payment sources #1707

chrisradford · 2017-02-07T18:27:16Z

This PR takes the work from #1091 and #1414 by @jordan-brough and @peterberkenbosch, rebases it over the latest master and updates a single failing spec (squashed into another commit).

From the original PR:

This is an attempt to update Solidus to fully support non-credit-card payment sources, and to make that easier to do it also introduces the concept of Spree::Wallet (see the code comments in wallet.rb for a summary of that class).

What else is needed for this to be merged? Assuming this is still desirable.

tvdeyen

Overall a great improvement!

I have some concerns about naming, especially within the wallet class (I actually really love this abstraction).

I want to avoid to use wallet_payment_sources as much as possible and rather use payment_source for interaction with the wallet wherever we can.

Also I found some typos and incorrect deprecation usage.

Thank you for picking this up and to all other contributors to this.

tvdeyen · 2017-02-08T16:08:37Z

CHANGELOG.md

@@ -282,6 +282,9 @@
    This was unnecessary since it didn't update the order totals, and running
    order.update! would recalculate the adjustments and totals again.

+*   Filter orders by store when more than a single store is present. [#1149](https://github.com/solidusio/solidus/pull/1140)


I believe this doesn't belong here. Although I think we we should add an changelog entry.

tvdeyen · 2017-02-08T16:20:04Z

core/app/models/concerns/spree/user_methods.rb

@@ -23,6 +23,8 @@ module UserMethods
      has_many :store_credit_events, through: :store_credits
      money_methods :total_available_store_credit

+      has_many :wallet_payment_sources, foreign_key: 'user_id', class_name: 'Spree::WalletPaymentSource'
+


Can these be moved into user_payment_source module?

Personally I'm not a huge fan of that module, and most of it is going away. How about the reverse - bring the has_many from that module into here? Then the whole module goes away soon.

Absolutely!

tvdeyen · 2017-02-08T16:24:13Z

core/app/models/spree/wallet.rb

+  def wallet_payment_sources
+    user.wallet_payment_sources.to_a
+  end
+


Could we add another method?

def payment_sources wallet_payment_sources.collect(&:payment_source) end

tvdeyen · 2017-02-08T16:25:35Z

core/app/models/spree/wallet.rb

+
+  # Find the default WalletPaymentSource for this wallet, if any.
+  # @return [WalletPaymentSource]
+  def default


Can we rename this into default_payment_source and return an actual PaymentSource? wallet.default sounds like getting the default wallet and we don't actually need the wallet_payment_source object. All we want deal with is PaymentSources.

def default_wallet_payment_source

tvdeyen · 2017-02-08T16:26:30Z

core/app/models/spree/wallet.rb

+  #   It must be in the wallet already. Pass nil to clear the default.
+  # @return [WalletPaymentSource] the associated WalletPaymentSource, or nil if clearing
+  #   the default.
+  def default=(payment_source)


Can we rename this into default_payment_source= ? wallet.default= sounds like setting the default wallet.

tvdeyen · 2017-02-08T17:30:12Z

core/spec/models/spree/credit_card_spec.rb

-  end
-
-  describe "#to_active_merchant" do
+  context "#to_active_merchant" do


Can we leave this a describe block?

tvdeyen · 2017-02-08T17:32:36Z

core/spec/models/spree/credit_card_spec.rb


-    second.update_attributes!(default: true)
+    it 'allows this card to save even if the previously default card has expired' do


This test is identical to the previous one.

tvdeyen · 2017-02-08T17:35:45Z

core/spec/models/spree/payment_create_spec.rb

@@ -81,6 +81,10 @@ module Spree
        }
      end

+      around do |example|
+        ActiveSupport::Deprecation.silence { example.run }


We don't want to silence ActiveSupport::Deprecation but Spree::Deprecation here

tvdeyen · 2017-02-08T17:35:55Z

core/spec/models/spree/user_spec.rb

@@ -92,7 +92,9 @@
      end

      it "has payment sources" do
-        expect(user.payment_sources.first.gateway_customer_profile_id).not_to be_empty
+        ActiveSupport::Deprecation.silence do


We don't want to silence ActiveSupport::Deprecation but Spree::Deprecation here

tvdeyen · 2017-02-08T17:37:27Z

core/spec/support/concerns/payment_source.rb

+    it { is_expected.to respond_to(:user) }
+  end
+
+  context "#can_capture?" do


Can we use a describe block for methods?

tvdeyen

Thanks

tvdeyen

Thanks. The specs will pass, if you fix the foreign key usage in the migrations

tvdeyen · 2017-02-20T19:37:30Z

core/db/migrate/20160420044191_create_spree_wallet_payment_sources.rb

@@ -1,7 +1,7 @@
 class CreateSpreeWalletPaymentSources < ActiveRecord::Migration[4.2]
  def change
    create_table :spree_wallet_payment_sources do |t|
-      t.references :user, index: true, null: false
+      t.references :user, foreign_key: true


You need to pass {to_table: Spree.user_class.table_name} to foreign_key

Ah, thanks!

mamhoff

I'm good with this, except for the commits that fix things for Rails 5 and that fix the migration's foreign key. Those should be merged with the commit that introduces the code they're fixing (this will probably also make sure all commits are green again).

tvdeyen

Go

Extracted from these commits from mamhoff: - spree/spree@7f706d0 - spree/spree@a3b66ae

Based largely on this commit from mamhoff: - spree/spree@a3b66ae

To help tell whether payment sources should be saved in the Wallet or just reused in general (e.g. for unreturned exchange charges).

TODO: Use this in frontend and deprecate `existing_card_id`.

TODO: Deprecate existing_card and existing_card_id

And deprecate User#default_credit_card

For any stores using legacy payment sources that do not inherit from PaymentSource.

- Update Spree::StoreCredit to inherit from Spree::PaymentSource - Fix comment text in wallet

- Rename wallet_source#source to payment_source - Reflected the name change in the migrations

- Ensure the `payment_source` is a `Spree::PaymentSource` - Add spec for wallet_payment_source#payment_source validation - Add locale for invalid payment_source

The "inverse_of" relationship required some extra work for this

This updates the `Spree::Gateway#reusable_sources method to fetch those sources from the wallet on the user instead of only looking for credit cards. See Solidus PR 1442.

- Update the comment on the payment builder so it's clear that this will use the default source in the user's wallet. - Fix issue with more then 1 payment being created. Added back the check on payments that exist on the order with the same payment source

- Return the actual reusable payment sources - Deprecate Gateway#sources_by_order and renamed it to reusable_payment_sources to reflect the purpose of the method better

Fix deprecation warnings on calling the PaymentCreate with passing in the existing_credit_card_id. Replace that with the wallet_payment_source_id.

- Add specs for `Spree::Wallet` - Update payment spec to remove source_attributes usage

Rather than ActiveSupport::Deprecation

- And the same with the setter. - Update related specs.

…nt source And add Wallet::Unauthorized when attempting to set a default that doesn't belong to the wallet. ../core/spec/models/spree/order/checkout_spec.rb

peterberkenbosch mentioned this pull request Feb 7, 2017

Wallet & Support for non-credit card payment sources - rebased on solidus:master #1414

Closed

jordan-brough mentioned this pull request Feb 8, 2017

[RFC] Wallet & Support for non-credit card payment sources #1091

Closed

3 tasks

tvdeyen requested changes Feb 8, 2017

View reviewed changes

tvdeyen approved these changes Feb 20, 2017

View reviewed changes

tvdeyen requested changes Feb 20, 2017

View reviewed changes

jordan-brough force-pushed the non-credit-card-payment-sources branch from 032a302 to 79618fc Compare February 24, 2017 19:43

jordan-brough mentioned this pull request Feb 24, 2017

Disable rubocop Metrics/BlockLength #1744

Merged

mamhoff suggested changes Mar 1, 2017

View reviewed changes

tvdeyen approved these changes Mar 1, 2017

View reviewed changes

jordan-brough changed the title ~~Non credit card payment sources~~ Spree::Wallet & Non credit card payment sources Mar 1, 2017

jordan-brough force-pushed the non-credit-card-payment-sources branch from 0d88ab1 to 0e00b48 Compare March 10, 2017 12:29

jordan-brough and others added 17 commits March 10, 2017 06:50

Move common payment source specs into shared example file

515a405

Extracted from these commits from mamhoff: - spree/spree@7f706d0 - spree/spree@a3b66ae

Move common code from CreditCard into PaymentSource

c08f334

Based largely on this commit from mamhoff: - spree/spree@a3b66ae

Introduce Wallet and WalletSource classes

a4434e0

Add PaymentSource#reusable? concept

cb64c89

To help tell whether payment sources should be saved in the Wallet or just reused in general (e.g. for unreturned exchange charges).

StoreCredit temporary workaround

1f4dbf6

Rename temporary_credit_card -> temporary_payment_source

2dd5293

Update persist_user_credit_card for the Wallet

8cdcfc2

Update assign_default_credit_card for the Wallet

9beab0a

Allow passing wallet_source_id instead of existing_card_id

e9c86f5

TODO: Use this in frontend and deprecate `existing_card_id`.

Use wallet_source_id instead of existing_card in frontend

2435109

TODO: Deprecate existing_card and existing_card_id

Deprecate existing_card and existing_card_id parameters

f516b90

Allow payment sources that don't have belongs_to :user

b43e95e

Migrate CreditCard#default to Wallet, migrate data, drop column

458a947

And deprecate User#default_credit_card

Deprecate User#payment_sources

97d3f73

Update add_after_order_complete for non-PaymentSource sources

7dd840d

For any stores using legacy payment sources that do not inherit from PaymentSource.

Update Spree::StoreCredit to inherit from Spree::PaymentSource

60ef289

- Update Spree::StoreCredit to inherit from Spree::PaymentSource - Fix comment text in wallet

Rename the wallet_source to wallet_payment_source

abb73a5

- Rename wallet_source#source to payment_source - Reflected the name change in the migrations

jordan-brough force-pushed the non-credit-card-payment-sources branch 2 times, most recently from ff2d5f6 to d6e9585 Compare March 10, 2017 14:24

peterberkenbosch and others added 13 commits March 10, 2017 09:12

Add validation for payment_source on the wallet_payment_source.

c0329e0

- Ensure the `payment_source` is a `Spree::PaymentSource` - Add spec for wallet_payment_source#payment_source validation - Add locale for invalid payment_source

Add another spec for WalletPaymentSource

5881475

The "inverse_of" relationship required some extra work for this

Fetch reusable_sources from the wallet

224a9bc

This updates the `Spree::Gateway#reusable_sources method to fetch those sources from the wallet on the user instead of only looking for credit cards. See Solidus PR 1442.

Update Spree::Gateway for payment sources

547d459

- Return the actual reusable payment sources - Deprecate Gateway#sources_by_order and renamed it to reusable_payment_sources to reflect the purpose of the method better

Fix deprecation warnings in PaymentSpec

ad2a966

Fix deprecation warnings on calling the PaymentCreate with passing in the existing_credit_card_id. Replace that with the wallet_payment_source_id.

Add specs for Spree::Wallet

b66c08e

- Add specs for `Spree::Wallet` - Update payment spec to remove source_attributes usage

Update credit_cards relation to be in UserPaymentSource module

88b3097

Update deprecations to include replacement and deprecator

a2a2b5c

Update specs to use Spree::Deprecation.silence

a4585c2

Rather than ActiveSupport::Deprecation

Rename wallet #default to #default_wallet_payment_source

1b7bba7

- And the same with the setter. - Update related specs.

Update Wallet deprecations to use Spree::Deprecation

b11cf28

Update Wallet#default_wallet_payment_source= to accept a wallet payme…

2e3bccf

…nt source And add Wallet::Unauthorized when attempting to set a default that doesn't belong to the wallet. ../core/spec/models/spree/order/checkout_spec.rb

jordan-brough force-pushed the non-credit-card-payment-sources branch from 28edc9a to c0329e0 Compare March 10, 2017 16:33

mamhoff approved these changes Mar 10, 2017

View reviewed changes

jordan-brough merged commit 7289efd into solidusio:master Mar 13, 2017

jordan-brough deleted the non-credit-card-payment-sources branch March 13, 2017 11:15

jhawthorn mentioned this pull request Mar 13, 2017

Migrate all reusable credit card to wallet sources #1765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spree::Wallet & Non credit card payment sources #1707

Spree::Wallet & Non credit card payment sources #1707

chrisradford commented Feb 7, 2017

tvdeyen left a comment

tvdeyen Feb 8, 2017

tvdeyen Feb 8, 2017

jordan-brough Feb 8, 2017

tvdeyen Feb 8, 2017

tvdeyen Feb 8, 2017

tvdeyen Feb 8, 2017

chrisradford Feb 17, 2017

tvdeyen Feb 8, 2017

tvdeyen Feb 8, 2017

tvdeyen Feb 8, 2017

tvdeyen Feb 8, 2017

tvdeyen Feb 8, 2017

tvdeyen Feb 8, 2017

tvdeyen left a comment

tvdeyen left a comment

tvdeyen Feb 20, 2017

chrisradford Feb 21, 2017

mamhoff left a comment

tvdeyen left a comment


		second.update_attributes!(default: true)
		it 'allows this card to save even if the previously default card has expired' do

Spree::Wallet & Non credit card payment sources #1707

Spree::Wallet & Non credit card payment sources #1707

Conversation

chrisradford commented Feb 7, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamhoff left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment