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

[jordan-brough]

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

I've based this on top of three other PRs that I hope will be merged first:

• Deprecate User#drop_payment_source & Gateway#disable_customer_profile #1023
• Combine Order#token_based? and Gateway#supports? #1033
• Interfaces for persist_user_credit_card and assign_default_credit_card #1086

This is based on and heavily influenced by @mamhoff's Spree PR from last year:
spree/spree#6831
The major difference is the "Wallet" concept that he and I have discussed a bunch in the past month.

Comments and suggestions are very welcome.

I have not added new specs. If we think this general direction looks good then I will add them. There are also a couple todos that I will tackle if this looks generally OK.

[mamhoff]

I'm good with this, but we also paired on it so it doesn't count as team thumbs-up. :)

[jordan-brough]

@jhawthorn did you mean to close this?
Edit: I think the branch I based it on got deleted and that automatically closed this. (I know I did this PR in a funky way. I'll think of something better next time)

[mamhoff]

@jordan-brough Now the PR is targeting a non-master branch. Is this intentional?

[cbrunsdon]

I really like this change. Looks very sensible, will start simplifying a lot of junk in a lot of extensions.

[peterberkenbosch]

@jordan-brough @mamhoff checking this out now, what's the status? Would love to help out where I can. I have some real life samples to work with as well.

[jordan-brough]

@peterberkenbosch great! Things on my list:

• Add specs for all new functionality
• StoreCredit check-ups
  • Update StoreCredit to inherit from PaymentSource (i.e. this). I really just haven't even looked at this yet. It may be quite easy or there may be some gotchas.
  • Verify that this line works correctly in the presence of store credits. i.e. I'm not sure what happens for a new order if a user has some store credit available but not enough to cover the whole order.
• Try using this in a live store

Does any of that, or anything else sound interesting to you? I'm happy to have any help available.

[jordan-brough]

@mamhoff yes the non-master branch is intentional. I wanted to exclude the commits from the three PRs that I mentioned in the description from showing up here. I need to get that last one figured out and then I'll reopen this against master.

[peterberkenbosch]

Thanks @jordan-brough, will put some time in this the next few days.

[Kingdutch]

to should probably be from

[jordan-brough]

@Kingdutch updated. Thanks!

[jrochkind]

These comments focus on the data modelling, especially how it's reflected in the ActiveRecord classes and their names.

1. Instead of WalletSource, spree_wallet_sources, and WalletSource#source, I would suggest WalletPaymentSource, spree_wallet_payment_sources, and WalletPaymentSource#payment_source. The current naming makes it confusing what a WalletSource#source is -- what sort of object/class? @jordan-brough mentions he was being consistent with Payment#source, which I might do different if we had it to do over too, but at least there, there's an orthographic relationship between Payment#source and PaymentSource, that isn't there between Wallet#source and PaymentSource. I'd leave Payment#source the same for backwards compat, but WalletPaymentSource#payment_source for clarity.
2. On the topic of "it's unclear what a WalletSource#source actually is", right now an unrestricted polymorphic association there will let you set any ActiveRecord model as the source/payment_source. Suggest a validation and/or a setter argument checker that prevents you from setting anything but is_a? Spree::PaymentSource. Or else you can set a User, an Order, a Store, anything as a source, which will lead to disaster.
3. In a CC only world, we had User#credit_cards -- which was every CC a user had ever used, expired or not, not necessarily just those meant to be in the current 'wallet' (a concept we didn't have before). But are we sure we'll never need User#payment_methods, all payment methods ever used by a user, whether in current wallet or not? Maybe a user wants to see all the payment methods they've ever used? Or an Admin does? (for fraud detect purposes?). If we're pretty sure this will never be needed, that's fine. But if it were needed, going through all the user's Orders probably isn't great. If it were needed, then we might want user_payment_sources with a boolean in_wallet, instead of just wallet_payment_sources. The danger is if we later discover it's needed, we'll have to rename all these tables/models/associations again, which we don't want to have to do. The flip side is added un-needed complexity for something we might not need.
4. It might be worth considering using ActiveRecord Single Table Inheritance for PaymentSources, instead of an abstract_class = true. STI would mean all payment sources would live in the same spree_payment_sources table, instead of each living in their own table.
   • The downside is, the downside of STI, they're all in the same table, which has every column in it needed for any payment source, could get large and confusing, and extensions adding new PaymentSource sub-classes would have to make sure not to interfere with each other's column names (with possibly different types!).
   • The upside is the WalletSource#source/WalletPaymentSource#payment_source association would not need to be polymorphic -- it would be an ordinary belongs_to :payment_source, and STI takes care of it. Avoiding a polymorphic association there would also allow you to actually have User#wallet_payment_sources as an AR association, with all the goodness you get from AR associations (caching, includes/preloading) -- with the polymorphic belongs_to on the join table as current, I would be scared to try to get AR to actually do a User has_many :through PaymentSource, I don't think it will do it well (and pre-loading or other efficiencies are obviously impossible anyway). It would also make a bunch of the above points kind of settle themselves:
     • You wouldn't need to worry about 2 above, it's no longer a polymorphic association.
     • it would be easier to have both User#payment_sources and a User#wallet_payment_sources has_many :through associations, as plain old AR associations (using a scope on the in_wallet boolean in the join table, AR can handle that), to accommodate point 3 above.
     • especially if doing the previous bullet point, the naming would make sense as UserPaymentSource, spree_user_payment_sources, UserPaymentSource#payment_source, standard AR conventions.

[mamhoff]

Different payment source can have very different fields, and as soon as we start providing support for e.g. SEPA payments, we'll run into a big table with lots of accessors that don't make sense in many contexts. We have the solution @jordan-brough is proposing running in production, and this is the schema for just credit cards and SEPA payments:

  create_table "spree_credit_cards", force: :cascade do |t|
    t.string   "month"
    t.string   "year"
    t.string   "cc_type"
    t.string   "last_digits"
    t.integer  "address_id"
    t.string   "gateway_customer_profile_id"
    t.string   "gateway_payment_profile_id"
    t.datetime "created_at"
    t.datetime "updated_at"
    t.string   "name"
    t.integer  "payment_method_id"
  end

  create_table "payone_sepa_mandates", force: :cascade do |t|
    t.string   "bank_country"
    t.string   "bank_account"
    t.string   "bank_code"
    t.datetime "created_at"
    t.datetime "updated_at"
    t.integer  "payment_method_id"
    t.string   "iban"
    t.string   "bic"
    t.string   "gateway_customer_profile_id"
    t.string   "mandate_identification"
    t.string   "mandate_status"
    t.text     "mandate_text"
    t.string   "creditor_identifier"
    t.string   "street"
    t.string   "city"
    t.string   "company"
    t.string   "zip"
    t.string   "country"
    t.string   "email"
    t.string   "name"
  end

I would not want to merge these two into the same table.

As for the preloading upside: Usually, users have very few payment sources. I do not see a performance issue with loading a few of them, even if it were n+1.

Thanks a lot for taking the time to comment here, especially the naming suggestions are very much appreciated.

[jordan-brough]

1 ... I would suggest WalletPaymentSource, spree_wallet_payment_sources, and WalletPaymentSource#payment_source

Sounds reasonable to me. I'll update to this naming unless anyone objects. @mamhoff @cbrunsdon any preferences?

[jordan-brough]

2 ... Suggest a validation and/or a setter argument checker that prevents you from setting anything but is_a? Spree::PaymentSource

Great idea, I'll add that.

[jordan-brough]

3 ... are we sure we'll never need ... all payment methods ever used by a user, whether in current wallet or not?

I'm not completely opposed to this idea but I think it expands the scope right now unnecessarily without some solid use cases. A table like user_payment_sources also feels like something that is harder to get right, or at least more important to "not miss anything" in. i.e. I feel like we don't just need to make sure wallet entries are in there, we need to make sure all payment sources get in there, which feels like a greater burden for backfilling, and for scenarios that allow guest checkout and then subsequently allow guests to sign up and claim orders. I'd feel like we'd need to make sure user_payment_sources got updated, but I wouldn't be so worried about wallet_payment_sources.

Part of the reason to provide the PORO Wallet interface in this PR is to decrease the need for future coding. The provided interface could work equally well with either DB implementation so I'd say leave it as-is for now and I'd be happy to see someone submit a PR exploring the idea of user_payment_sources.

I do think one thing that might be nice to do right now though is to add belongs_to :user on PaymentSource so that people know to have a user_id field on their payment sources and to connect them to their users (except in cases of guest checkout). Anyone have thoughts on that?

[mamhoff]

@jordan-brough I'm unsure what the user_id field does that the WalletPaymentSource does not do?

[mamhoff]

👍 on adding Payment between User and Wallet.

[mamhoff]

@jrochkind Coming to think of it, most of that data does not have to be particularly query-able, so we might get do well with STI and a JSON data column, actually. @jordan-brough I know this throws over a lot of the stuff we've done so far, but it would prevent us from column creep and give us all the goodies of a more restricted association.

[dhonig]

@mamhoff I don't know that STI is used anywhere else in Solidus, do we really want to open this box? When is a JSON data structure vs tables the answer and when is it not? ( And I am pro STI, in general I think it leads to elegance in design, just not sure we want to introduce it if it is not already used.)

[jrochkind]

There's no ideal solution, but I think STI is probably less complex than an approximation of a polymorphic many-to-many, which is kind of what's there now. (AR can't actually do a polymorphic many-to-many reliably, which is maybe why the many-to-many association isn't actually defined here, despite the many-to-many-like join table). But there are definitely plusses and minuses either way, I can see either decision, none we have in front of us seem ideal.

I would say the relevant pattern that maybe makes the JSON data structure @mamhoff suggests appropriate, is when you have a bunch of rows in the same table that need wildly different attributes, and those attributes do not (ever!) need to be queryable. One place this is used in current Solidus is PromotionRule preferences -- which are actually also STI, I think? (So STI is not totally new to Solidus, I think?) (And any other place that use Spree::Preferences::Preferable is also using a JSON data structure. Hard to figure out where that might be, the way it's being done right now).

Serialized-in-one-column data struture is basically a hacky workaround escape hatch, but sometimes you need a workaround escape hatch. :) But yeah, I'm not sure which is the best decision here, just that it's probably worth considering -- whatever ends up merged is gonna be with us a while (hopefully, we don't want another backwards breaking change soon), and is part of the core bones of solidus so everyone will be dealing with it.

[dhonig]

@jrochkind exactly. There are so many places we could potentially use STI, that I think its important we know exactly why we are introducing it, and when it is justified. Once it is introduced, then you will see lots of proposals using STI. If we are going to introduce this design, then we musn't let it fall into the pervasive documentation vacuum that exists. We should clearly document it and say, this design emerged for specific conditions, etc. Its totally true that every payment method will have an unpredictable field set in a wallet that might contain, NFC, direct banking such as ACH and credit card sources. So this is a sensible hack from that point.

[jordan-brough]

I don't know that STI is used anywhere else in Solidus

Some places we currently use STI:

• PaymentMethod subclasses (e.g. Gateway and all its subclasses in solidus_gateway)
• Asset subclasses (Image)
• Calculator subclasses
• PromotionAction subclasses
• PromotionRule subclasses
• ReimbursementType subclasses

I'm not sure whether it's the right choice here, but definitely worth considering.

[jrochkind]

@dhonig So STI has already been 'introduced', this would not be introducing it. If Spree::PaymentMethod and Spree::Gateway are both already STI, that does seem rather tantalizingly analagous, doesn't it? (Unless it hasn't worked out well and seems a bad idea in retrospect for them!)

The part of this PR that moves further toward abstracting away from CreditCard to a generalized polymorphic PaymentSource is, I'd suggest, more significant (and important to get right) than the Wallet part.

Order is gonna need an association to PaymentSource too, isn't it? At least eventually. I don't completely follow how it's being done in this PR. And Order to PaymentSource might even need to be many-to-many --- you can theoretically pay for an order with more than one PaymentSource, right?

Wouldn't it be nice if the Order/PaymentSource relationship were an ordinary rails many-to-many (has_many :through) to an STI table, instead of a weird kind-of polymorphic many-to-many, like the Wallet/User->PaymentSource relationship in this PR? If a true generalization of CC to PaymentSource is to be accomplished, then PaymentSource probably needs to be a full participant in the ecology, including ability to make has_many and has_many :through associations to PaymentSources -- which probably requires STI.

[jordan-brough]

Order is gonna need an association to PaymentSource too, isn't it?

I don't see the need for anything to change in that area. Order already connects to payment sources via order.payments.map(&:source), which is already polymorphic (from Payment to Source). Moving to STI is actually more of a change than continuing to use polymorphism in that regard.

[jordan-brough]

Re polymorphic vs STI: I think I lean toward continuing with polymorphic relationships, with the addition of the validation that @jrochkind suggested so that WalletPaymentSource.source has to be a PaymentSource. It feels easier to migrate to (for our own code, for other existing extensions out there, and database-change-wise) and most similar to what we're already doing for Payment.source. I feel like my experience with the very similar Payment.source polymorphic relationship has been fine query-wise and usage-wise and performance-wise so it seems nice to be consistent there if possible.

[jordan-brough]

One extra thought: One part of Payment.source that I haven't been happy with usage-wise is the general confusion I personally have with polymorphic relationships (or STI) in general. e.g., with polymorphic relationships in particular – not knowing what kind of object at all that I might be getting, and what methods are OK to call on it. But I feel like @jrochkind's suggestion of a validation makes that part better since we'll have an explicit common interface for all wallet payment sources.

[jordan-brough]

Re polymorphic vs STI: I chatted with @jhawthorn today and he is also in favor of using an abstract base class and a polymorphic relationship (instead of STI). Among other things, he made the point that it's likely that we'll want to search by these provider-unique fields (e.g. the email address of a PayPal account) and cross-db support for json fields is complex and/or not all there right now. So the STI+JSON idea might not work very well, and STI w/o JSON doesn't seem like a great fit for a table where the fields vary greatly between child classes.

[jrochkind]

Makes sense, thanks @jordan-brough

[peterberkenbosch]

@jordan-brough should we merge in (or rebase) master on this PR? The dependent PR's are merged in, would love to see a green CI build here as well :)

[jordan-brough]

Closing this. @peterberkenbosch is taking this over and pushing it forward in #1414.

[jordan-brough]

Note: This is now being moved forward by #1707