awesomemotive / easy-digital-downloads

Sell digital downloads through WordPress
https://easydigitaldownloads.com
GNU General Public License v2.0
865 stars 475 forks source link

Allow requiring credit cards, even with 100% discount applied #5686

Closed tnorthcutt closed 6 years ago

tnorthcutt commented 7 years ago

I'm not certain if this should go here or in the edd-recurring repo; happy to move it.

I'm working with a client who'd like to be able to require credit card information, even if a 100% discount is applied. This is because with a recurring payment, they may provide a 100% off discount code for the first payment, but want to be able to charge the buyer's card at the next subscription billing period.

It looks like this is mostly controlled in edd-checkout-global.js here.

I'm thinking a reasonable approach will be to add a global EDD option toggling whether to require CC information regardless of cart value (for now; we may want to try per-product settings later on), and if that option is set, then have discount_response include that flag, and do e.g.

if( true == discount_response.require_cc ) {

  if (!inputs.is('.card-address-2')) {
    inputs.attr('required','required');
  }
  $('#edd_cc_fields,#edd_cc_address').slideDown();

}

else if( '0.00' == discount_response.total_plain ) {

    $('#edd_cc_fields,#edd_cc_address,#edd_payment_mode_select').slideUp();
    inputs.removeAttr('required');
    $('input[name="edd-gateway"]').val( 'manual' );

} else {

    if (!inputs.is('.card-address-2')) {
        inputs.attr('required','required');
    }
    $('#edd_cc_fields,#edd_cc_address').slideDown();

}

That feels a bit hacky/non-DRY, but hopefully conveys the intent.

If this is kosher, and the desired approach, I'll move forward and then open a PR for this feature.

Thanks!

tnorthcutt commented 7 years ago

FWIW, I've started putting together a PR for this, but I can't sort out how edd-checkout-global.min.js gets built from edd-checkout-global.js. Obviously I can set SCRIPT_DEBUG to true for testing, but I figure a complete PR would be nice 😄

pippinsplugins commented 7 years ago

@tnorthcutt we do all minification right before release so no need to include it in the PR :)

tnorthcutt commented 7 years ago

@pippinsplugins ok cool. Otherwise how's that approach look?

pippinsplugins commented 7 years ago

Yes, I think the approach looks good, though instead of require_cc I think it should be require_payment since it could be a credit card, bank draft, or PayPal (or similar) payment. It really just indicates that the payment gateway should still be involved.

SeanTOSCD commented 7 years ago

This is interesting. What's the expected behavior here? In my testing with a 100% discount of a recurring item, the subscription is created in EDD. There's no Transaction ID on the subscription record, which makes sense because I'm pretty sure no subscription is supposed to be created at the merchant processor (I'm testing with Stripe... nothing there). The EDD subscription and its payment record record the payment method as Free Purchase. And manually recording a renewal payment on the EDD subscription card doesn't appear to initiate anything with the merchant process.

What's the expected behavior of the functionality from start to finish?

tnorthcutt commented 7 years ago

@SDavisMedia FYI, this is on hold on my end, pending client go-ahead. But once I get that, I'll come back to this.

pippinsplugins commented 7 years ago

Hmmm, I don't love the addition of the new setting because there's not a single use case in core alone where it would be used. The description of the setting even mentions that it might be useful when purchasing a subscription, which isn't something core support.

I don't think settings should ever be added to core that are not used by core in some meaningful way.

I'm completely okay with adding the filter that permits the fields to be disabled, but let's remove the UI setting please.

tnorthcutt commented 7 years ago

@pippinsplugins roger that. Would it make sense for the edd-recurring addon to add this setting, or do you indeed want to go strictly UI-less?

tnorthcutt commented 7 years ago

Went ahead and refactored to allow requiring payment info via a filter: https://github.com/easydigitaldownloads/easy-digital-downloads/pull/5697/commits/7a7a986cd5b92e93db345863519490b97b5682e8

Let me know if this looks good, any changes needed, etc. Thanks!

pippinsplugins commented 6 years ago

I've been playing around with this and I have some concerns.

First, the experience of adding a 100% discount code and then still seeing the payment methods is weird. As a customer I'm immediately wondering why I see the option to enter payment details and why I see PayPal, in my example, as an option. screencapture-edd-checkout-page-1511058186221 This is compounded by the fact that the payment methods do not actually do anything. Even if I click on PayPal and complete my free purchase, nothing extra happens. I don't go to PayPal to authorize anything; I'm just sent to the success page.

This is even worse if a customer clicks to pay with a credit card because they see the credit card radio button but no fields are shown to enter their card details:

screencapture-edd-checkout-page-1511058210393

These concerns could be alleviated by updating payment gateways to better understand what to do in this scenario, but that leads me into my second concern.

Payment gateways will all have to be updated to support this. It won't just magically work. For example, Stripe is already setup to hide the card fields if the payment amount is 0, so until it is updated to permit the display of the card fields when the amount is 0 and edd_require_payment_info is to to true, it won't work.

Overall I think the direction of this is fine, but in order for us to merge this in it needs to be fully flushed out to account for the nuances in the customer experience and the updates need to be simultaneously carried over to at least the top three payment gateways.

tnorthcutt commented 6 years ago

This definitely doesn't make sense with PayPal, since (AFAIK) PayPal doesn't support the concept of "capturing" payment info for later use.

In my testing with only Stripe activated, the card fields are displayed. It looks like adding PayPal as an option interferes with this.

However, it looks like there are some issues with the card info getting captured in Stripe; I'm looking into this.

Keeping in mind the use-case for this (a recurring subscription where the first term is discounted 100% with a discount code, and only enabled via a filter – so not something a non-dev would be doing), is it absolutely necessary that this work with every payment gateway?

chriscct7 commented 6 years ago

It seems like this should be built in as an option to the Stripe extension not EDD core

cklosowski commented 6 years ago

I was thinking the same thing here. I think EDD core should 'allow' a way to have gateways collect this data on checkout, but it's something the gateways specifically have to enable and capture.

It was a very odd experience in my testing when no gateways that support this were enabled.

tnorthcutt commented 6 years ago

@chriscct7 @cklosowski roger that, makes sense.

Do you happen to know what EDD + Stripe + EDD Recurring will (currently) do if a purchase for a recurring product is 100% discounted? Will the renewal simply fail because there's no payment method stored?

cklosowski commented 6 years ago

Correct, if an item is 100% discounted, Recurring does not require payment info, so Stripe is never loaded and it's processed as a 'manual' payment. The subscription record in EDD is created, but it will expire at the end of the term.

tnorthcutt commented 6 years ago

@cklosowski thinking about this some more, would it be better to add it to EDD Recurring, since it's only for recurring payments?

cklosowski commented 6 years ago

That's one logical place, but since recurring requires the gateway extensions to be installed, doing it in the gateway itself allows for non-recurring users to also do this, either for fraud detection, etc.

Also, I think that's the most logical, for future possible integrations, so we don't end up with a conflict with it if we decide it needs to be in the gateway later. I'd also like to see if we can do this as an 'option' or a filter, something like this in the gateway would seem logical:

function edd_stripe_require_payment_on_free() {
    return apply_filters( 'edd_stripe_require_payment_on_free', false );
}

This would allow for no changes for current users, and anyone that wants to can drop a one-liner in to their integrations: apply_filters( 'edd_stripe_require_payment_on_free', '__return_true' );

tnorthcutt commented 6 years ago

Cool, I like that. I'll move in that direction.

tnorthcutt commented 6 years ago

@cklosowski this is ready for a look now 😅

https://github.com/easydigitaldownloads/edd-stripe/pull/284

cklosowski commented 6 years ago

@tnorthcutt So with that change in Stripe, does that mean we can close this out in EDD core itself since we're passing that responsibility off to the gateway?

tnorthcutt commented 6 years ago

Yep, thanks for the bump here