Recurring billing support

[ruimarinho]

Hello,

I am currently adding support for Recurring Billing API's and since that requires some changes to the core plugin, I'd like to know if my approach is adequate for a possible PR.

Basically, Recurring Billing starts with the definition of an Agreement. This agreement is what allows applications to either setup regular billing periods (as is the case with PayPal and WorldPay) or debit (un)limited amounts of payments. This latter variable agreements are very useful when dealing with memberships since after an agreement is setup, the system can continually charge the user without requiring additional user interaction (think of it like direct debit). To my knowledge, only WorldPay features such functionality.

The Agreement setup also varies from provider to provider. On PayPal, you can choose ExpressCheckout to create it or use DirectPayment where you pass the information of the credit card. On both situations, you can persist the Agreement before communicating with the payment system. This is not the case with WorldPay, since the Agreement setup is solicited by a direct POST to a WorldPay-hosted page. You do get an instant callback (similar to PayPal IPN) if you have setup one - from there you will be able to create an agreement with the ID that WorldPay sends back.

Seeing that this bundle provides most of the base functionality for payments, I think it would be a good addition to support recurring payments, since they fit perfectly on the PaymentInstruction > Payments > FinancialTransaction flow. The only difference is that, for convenience, I'd like to add an Agreement reference to PaymentInstruction in order to know how many payments were made under a certain agreement. This is very important for billing reports.

Because the integration with the different API's varies substantially, the Plugin architecture used for payments is also a perfect fit for Agreements. In summary, here are some of the changes I'd like to introduce :

1. Add a new AgreementEntityPluginController / AgreementPluginController with a simple API like createAgreement, cancelAgreement, modifyAgreementStartDate, debit and a couple more.
2. Add the possibility of registering new agreement plugins by using the agreement.plugin_controller tag.
3. Add a new Agreement class that is used to setup agreements. Some information regarding the next payment date, if the agreement is a regular one, should be included in this class - I already normalized most of the features of PayPal and WorldPay on this class (things like interval multipliers and units, initial payment amount, etc).
4. Add a ManyToOne relation on the PaymentInstruction class to Agreement.
5. Ensuring a PaymentInstruction knows about an Agreement and deals with it.

I'll be starting this approach today but I'm looking forward for improvements and suggestions.

[ClementGautier]

:+1:

[schmittjoh]

First, thanks for the thorough write-up, sounds pretty good. I've just a few minor implementation remarks:

to 1) Do we really need to create a separate AgreementEntityPluginController? I'd just add the functionality to the existing controllers, and maybe add a new interface.

to 2) Similar to 1), I'd just use the existing system and add a RecurringPluginInterface which extends the current interface.

to 4/5) Is it really necessary to make the PaymentInstruction aware of an agreement? Instead, I'd just copy over all data from the agreement to the instruction that you need there each time.

[ruimarinho]

1) My first thought was to extend the EntityPluginController with a new interface (AgreementEntityPluginController), but I hesitated in doing so to avoid cluttering the original class too much.

2) As a consequence of 1), I opted for the new tag but, if we agree on the new interface for the PluginController, then it makes sense for a plugin to implement the RecurringPluginInterface and to continue registering itself as payment.plugin. Sounds good?

4/5) I would prefer not to, but do you see any better alternative to retrieve all PaymentInstruction's generated by an Agreement? I wouldn't mind using ExtendedData for that, but then you can't run queries on it.

[schmittjoh]

Ok, then let's add it to the existing class for now, and only add a new interface.

Regarding 4/5), I'd add a OneToMany association on Agreement.

[ruimarinho]

Ok, PluginController now implements AgreementPluginControllerInterface.

1) Does it make sense to have Agreement management-related API calls run through FinancialTransaction's (modifying dates, cycles, etc)? The good thing about it is that we can use the current code structure to handle API successes and failures (ie, response_code and reason_code) according to the transaction result state but, to get there, we'd need to build the tree - Payment < PaymentInstruction - and since those operations do not involve money, it doesn't look right. Another option is to add a relation (agreement_id) to a FinancialTransaction, like Credit. What do you think?

2) Abstract class Plugin implements the PluginInterface, which contains several methods that a recurring billing plugin does not need. What about moving those parts to a PaymentPluginInterface and let RecurringBillingPlugin implement PluginInterface and RecurringBillingPluginInterface?

[ruimarinho]

First of all, I'd like to apologise for the rather large comment, but I think it is necessary to explain the reasoning behind certain core modifications.

I've just pushed the new code to a forked repository [https://github.com/seegno/JMSPaymentCoreBundle/commit/22e87268c6db79ecf6e80f3ac339f26851bd9e18] but, due to the number of changes on the core plugin, I'd like to hear your feedback first before being comfortable in submitting a PR.

I have made a considerable effort to add recurring billing respecting the bundle structure, be it in terms of model as well as code organization. As a result, I had to adapt some of the existing code to fit the recurring subscriptions model and the type of operations one can execute on them.

Summary of changes:

Model/Entities

Not directly related to Recurring Billing, but I have decoupled the entities class from the model and removed unnecessary information from the mapping such as the table name property since, in my opinion, this kind of naming should be left for the DoctrineNamingStrategy implementation to decide.

Moved ExtendedDataType to the suggested Doctrine structure, which is under the DBAL directory.

Agreement

Represents an agreement between the payment system, the end user and the application.

AgreementPlan

Typically, you are presented with a list of plans, where you are invited to chose one. After selecting the plan that fits you better, an Agreement is created based on that AgreementPlan. This form selection, similar to choose_payment_method_type, has not been implemented yet, but the idea is to retrieve a concrete plan from the form data, just like the documented PaymentInstruction example.

PaymentInstruction

An Agreement can have many PaymentInstruction's, each for operation and payment. Similar to what payment systems do, I have tried to recreate PaymentInstruction's of zero amount to indicate management operations. This allows for current model re-use, instead of having to dedicate a single table to management log (create, cancel and edit an Agreement).

The part that I feel less confident about is retrieving the PaymentInstruction that was in the origin of a "create" transaction when the callback arrives by fetching a FinancialTransaction of that type.

FinancialTransaction

Added three more transaction types, each for the implemented Agreement operations (create, cancel, debit).

Result

Added a way to retrieve an Agreement as part of a result.

<User/Company/...>

Usually where the application can have a one-to-one relation to an Agreement.

PluginController

Added methods to accomodate create, cancel and debit Agreements. This last case is only possible when the Agreement is of type "variable" (instead of "regular"). This is a special type of recurring billing where debits are not regular. I plan to add support for the normal case in the future.

Added an option to pass a ParameterBag with callback information. Unfortunately, I haven't found any other elegant way to pass this information from the controller to the plugin concrete implementation. The workflow has to match a regular "approve and deposit" transaction in two steps, except one of the steps is not completed by the client, but by a callback.

Passing data via ExtendedData data is not possible unless you're fine with persisting extended data temporarily. The reason for this is that recurring payments callbacks only pass the "merchant reference ID", ie, typically the Agreement ID on your database so that you can process the transaction attached to that Agreement. This is what the controller knows about and nothing more.

Added some "xxxByReferenceNumber()" methods in order to retrieve objects that haven't been persisted yet but that are POST'ed via a callback. For example, you can take a payment from an Agreement via a merchant interface, and you receive a callback with a new transaction ID (equivalent to a FinancialTransaction referenceNumber). You need to test if this transaction is on the database or not to decided wether you need to create or just process the callback response.

Callbacks

Recurring billing and credit card debiting rely on callback notifications for processing. Some systems more than others, but in general it is very likely you'll have to work with them.

I have considered a plugin-like architecture where each payment processing plugin would be able to register "callback handlers" for a single point-of-entry controlled by the core infrastructure. A catch-all URL ("/payments/callbacks/{paymentSystemName}") would allow for a generic structure response:

1. Handshake - some payment systems require the application to GET or POST specific parameters to the API endpoint.
2. Validation - to verify the validity of callbacks, some payment systems implement hash mechanism to guarantee data integrity, while others make use of a shared secret between the two parties to authenticate the request.
3. Processing - fetching related entities by a given merchant identification number, defined upon payment, that allows the application to know which transaction/agreement a certain callback applies to.

Besides this shared structure, some logic also applies to every payment system. For instance, they all send a parameter ("cartId", "rp_invoice_id", etc.) that identifies the relation between the two systems. We could leverage this knowledge about these systems to create more generic patterns that would allow us to build shared code for more thorough source validation (e.g. DNS and IP records), data validation and data retrieval. It would be up to the plugin to implement concrete cases where those offered by the core controller would not fit.

While thinking about such system, I have encountered numerous obstacles that are a hurdle to this vision. The most obvious one is that payment systems only allows you to register one URL (e.g. http://localhost/payments/callbacks/paypal), while plugins register each implementation by functionality ("paypal_express_checkout"). While this is required to handle payment system methods, it is not feasible to process requests in order to find the matching plugin. A callback request may apply for a variety of plugins ("paypal_direct_payment", "paypal_recurring_billing" and so on) so that logic would have to ask each registered plugin if the request is supported by it and which type of callback would it like to process.

I have experimented with this solution and it's not that elegant. It breaks a lot of layers, because the PluginController is used to identify the plugin, which in turns identifies the type of transaction, and then it would be up to the PluginController to take charge again.

At this point, I think a simpler solution is to delegate the callback implementation for the plugin. Perhaps an abstract implementation is more suitable to aid developers?

Some more items up for discussion:

1. Should "Subscription" be favored instead of "Agreement"?
2. Storing raw API responses. It is very useful for auditing purposes. For now, I'm doing that using ExtendedData.
3. Feedback on interfaces, especially in reducing PluginController/Plugin complexity.
4. To discuss whether a ParameterBag or a Request object are passed to the PluginController and alternatives.

As far as changes go, I've been able to successfully implement WorldPay's Limited Agreement use case on top of them. For all that matters, please consider this a WIP. I am sure there is still a lot to improve.

[schmittjoh]

Would you mind reverting all changes that are not directly related to recurring payment support? The smaller a diff, the more easy to review, and the faster we can actually merge it. We can discuss the Model <-> Entity separation as far as I can see the entities are still inheriting from the model; I'm not sure what the benefit of this is, but anyway I'd like to discuss this separately of recurring payment support.

I'd also like to ask you to revert all other non-related changes like the removal of the table names, and the namespace changes. You can send them as separate pull requests, but since they would break BC (like the removal of the table names), there should be a really good reason to make them.

Now to the "real" changes :)

The separation between Agreement and AgreementPlan seems reasonable. However, does AgreementPlan have to be an entity, or could this be generated from the plugins at runtime?

Creating PaymentInstructions with a zero amount, just to be able to re-use financial transactions for logging the state of agreements, looks like a hack to me. I'd prefer to have a separate log, but I'm wondering whether we need a log at all? There is no money transferred after all, so just keeping the current state of the agreement seems sufficient to me. Also, for auditing you likely have a application-wide strategy which you want to use (like beberlei's EntityAudit).

Regarding callbacks, I'd prefer if we can discuss this in a separate pull request as this is also not strictly related to recurring payment support, but also relevant for one-time payments. In general, it is always better to send small improvements. Unfortunately, it is very hard to review bigger pull requests if you have not written the code yourself.

I agree that improvements of the API are necessary as well, but that would be a topic for a new version of this bundle. I'm also happy to discuss ideas here, but let's not make this in the context of recurring payment support, but in a separate issue.

[ClementGautier]

Hi, I agree with Johannes,

I think You can split this in 3 issues / pull request.

The "callbacks" part is a really important thing, In https://github.com/ETSGlobal/ETSPaymentDotpayBundle/ I had to handle it manually and I had to dispatch a specific event ( https://github.com/ETSGlobal/ETSPaymentDotpayBundle/blob/master/Controller/CallbackController.php ). I think we can move some parts in the CoreBundle to fit all needs but we should do this in another issue.

For all entities, I am writing a Propel version of the CorePlugin so just be careful and use Interfaces ;)

[ruimarinho]

Thanks for the great feedback!

Agreement/AgreementPlan

I suggest keeping AgreementPlan as an entity because usually the responsability of creating or updating plans is not on the developer, but on the staff supporting the application. This makes it very simple to update the current offers without having to touch code.

PaymentInstructions with a zero amount

This is a very tricky point and I've also given this some thought before starting the actual work. The current implementation was based on what looks like payment providers do, although I must admit I avoided introducing new tables/entities unless they were absolutely necessary.

Now, looking at this WorldPay callback for a new agreement,

region=&authAmountString=&_SP.charEnc=UTF-8&desc=XXXXXXX+membership&tel=XXXXXXX&address1=&countryMatch=B&address2=&cartId=XXXXXXX&address3=&lang=en&callbackPW=&rawAuthCode=A&transStatus=Y&amountString=A$0.00&authCost=&currency=AUD&installation=XXXXXXX&amount=0.00&countryString=Australia&displayAddress=XXXXXXX&name=XXXXXXX&testMode=100&routeKey=VISA-SSL&ipAddress=XXXXXXX&fax=&rawAuthMessage=trans.fpCreatedNoAuth&instId=XXXXXXX&AVS=0000&compName=XXXXXXX&futurePayId=XXXXXXX&authAmount=&postcode=2537&cardType=Visa&cost=0.00&authCurrency=USD&country=AU&charenc=UTF-8&email=XXXXXXX&address=XXXXXXX&msgType=authResult&town=&authMode=A;

you can see that it looks like a typical financial transaction but with amount $0.00. I haven't confirmed this on WorldPay, but I think that if you take an initial payment, it will generate a separate callback response.

On PayPal, I haven't setup initial payments yet, but from an IPN test string I've found, it looks like it's possible to immediately charge an Agreement upon creation:

cmd=_notify-validate&payment_cycle=Daily&txn_type=recurring_payment_profile_created&last_name=test&initial_payment_status=Completed&next_payment_date=02%3A00%3A00+Feb+11%2C+2010+PST&residence_country=US&initial_payment_amount=1.00&rp_invoice_id=xxxxxx&currency_code=USD&time_created=16%3A26%3A04+Feb+11%2C+2010+PST&verify_sign=A3RRzMcmZmo4KBAa6ajA6X1oydvDA9LajwyP7Jbv1tWLuJHDILsLZZOT&period_type=+Regular&payer_status=unverified&test_ipn=1&tax=0.00&first_name=test&receiver_email=email@domain.com&payer_id=WFGKJQ4E6YNCC&product_type=1&initial_payment_txn_id=4EU52048AM030681T&shipping=0.00&amount_per_cycle=1.00&profile_status=Active&charset=UTF-8&notify_version=2.9&amount=1.00&outstanding_balance=0.00&recurring_payment_id=I-V7VTKNPC6GG7&product_name=live+500

This suggests that an Agreement profile may actually be created with a positive PaymentInstruction - something that the current version does not support yet.

If this scenario is possible, then I think it makes sense to wrap those calls in PaymentInstruction's, but I don't have experience with other recurring billing API's (Chargify, Stripe, etc.).

@ClementGautier, I actually took at look at the bundle to investigate wether a generic callback handler would work but, as mentioned above, I don't think it's worth it. We could work on an abstract implementation though.

Also, understandably, the commit is too big for a proper review. Since it was tightly coupled to an actual implementation, I had to deliver it like this, but I will try to separate this into separate PRs.

Last, but not least, there is a considerable amount of effort going on at Omnipay (https://github.com/adrianmacneil/omnipay), an "agnostic multi-gateway payment processing library for PHP 5.3+". I am a real apologist of this kind of PHP focused libraries - it's definitely worth a look.

[makasim]

@ruimarinho I am going to add support of reoccurring payments (paypal express checkout would be first) to payum? We could work together. Contact me if you are interesting in it.

Last, but not least, there is a considerable amount of effort going on at Omnipay

It is already supported via the bridge . Not fully though.

[makasim]

I've just added support of recurring payments to payum lib. It works for paypal express checkout.

Here's the doc and sandbox symfony 2 example.

Hope you find it useful.

[brunoroux]

Sorry for the topic revival, but I am quite interested in recurring billing. Any news on this ?

[ruimarinho]

@brunoroux very sorry for the late reply, but I haven't had the time to complete the aforementioned changes to the original PR. While the original (forked) implementation still works, it's not yet totally aligned with the plugin architecture, as the comments point out. I plan on checking the Payum integration by @makasim (thank you!) when I have the chance to work on this topic again. I shall reopen this issue or reference it if and when that is possible.