pronamic / wp-pronamic-pay

The Pronamic Pay plugin allows you to easily accept payments with payment methods like credit card, iDEAL, Bancontact and Sofort through a variety of payment providers on your WordPress website.
https://pronamicpay.com
34 stars 14 forks source link

Delete legacy payment subscription property `$payment->recurring` #228

Closed remcotolsma closed 2 years ago

remcotolsma commented 2 years ago

Depends a bit on https://github.com/pronamic/wp-pronamic-pay/issues/227, i don't think the $payment->recurring property is needed. Via $payment->get_subscription() and maybe $subscription->get_first_payment() we can determine it's a follow-up payment.

rvdsteege commented 2 years ago

$subscription->get_first_payment() could result in an empty result as first payments are deleted, which may be confusing.

remcotolsma commented 2 years ago

Via $payment->get_subscription() and maybe $subscription->get_first_payment() we can determine it's a follow-up payment.

We should probably go for $payment->get_subscriptions() instead of $payment->get_subscription(), because 1 payment can theoretically be linked to multiple subscriptions.

$subscription->get_first_payment() could result in an empty result as first payments are deleted, which may be confusing.

Yes, that's something to keep in mind.

remcotolsma commented 2 years ago

$subscription->get_first_payment() could result in an empty result as first payments are deleted, which may be confusing.

I noticed we already have a $subscription->get_first_payment() function: https://github.com/pronamic/wp-pay-core/blob/adec7dcd1acab7e0268fad27f1021c390a04b535/src/Subscriptions/Subscription.php#L578-L602

It's implemented like this:

    /**
     * Get the first payment of this subscription.
     *
     * @return Payment|null
     */
    public function get_first_payment() {
        if ( null === $this->id ) {
            return null;
        }

        // Query arguments to get first payment.
        $args = array(
            'posts_per_page' => 1,
            'orderby'        => 'post_date',
            'order'          => 'ASC',
        );

        $first_payment = get_pronamic_payments_by_meta( '_pronamic_payment_subscription_id', $this->id, $args );

        if ( ! empty( $first_payment ) ) {
            return $first_payment[0];
        }

        return null;
    }

If the first payment is deleted it will return the second subscription payment?

Is it a good idea to keep track of the first payment in the subscription object?

subscription {
    "first_payment": {
        "$ref": "..."
    }
}

If the first payment has been removed, we can assume that it concerns a follow-up payment?

rvdsteege commented 2 years ago

If the first payment has been removed, we can assume that it concerns a follow-up payment?

No, could also be a 'first payment', for example when updating the payment method through SubscriptionsModule::handle_subscription_mandate().

remcotolsma commented 2 years ago

I think we should see that as 2 different things:

Changing a Mollie mandate ID always requires a sequenceType = first value?

Maybe we can move more of the mandate change logic to the Mollie gateway library?

We now have some specific Mollie stuff in core:

                /*
                 * Use payment method minimum amount for verification payment.
                 *
                 * @link https://help.mollie.com/hc/en-us/articles/115000667365-What-are-the-minimum-and-maximum-amounts-per-payment-method-
                 */
                switch ( $payment->get_payment_method() ) {
                    case PaymentMethods::DIRECT_DEBIT_BANCONTACT:
                        $amount = 0.02;

                        break;
                    case PaymentMethods::DIRECT_DEBIT_SOFORT:
                        $amount = 0.10;

                        break;
                    default:
                        $amount = 0.01;
                }

If the first payment has been removed, we can assume that it concerns a follow-up payment?

@rvdsteege What if we don't take mandate changes in consideration?

remcotolsma commented 2 years ago

Also talked about payment.automatic just like the Stripe payout object property payout.automatic documented like:

Returns true if the payout was created by an automated payout schedule, and false if it was requested manually.

https://stripe.com/docs/api/payouts/object#payout_object-automatic

Or we could use something like GitHub actions event name GITHUB_EVENT_NAME:

if: github.event_name == 'schedule' && github.event.schedule != '0 0 * * *'
Property name Type Description
github.event object The full event webhook payload. For more information, see "Events that trigger workflows." You can access individual properties of the event using this context.

We could also extend the payment object with an event property that contains information about what event triggered the payment. I also mentioned the use of origin:

payment {
    "origin": {
        "post_id": 123,
        "type": "web" // or `cron`
    }
}

For just Mollie we maybe even don't need the first payment, if there is no mandate ID we use sequenceType first and otherwise recurring.

For Gravity Forms there is currently an action that is triggered only for successful follow-up payments.