Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subscription gets renewed while ended #233

Open
voicecode-bv opened this issue Apr 11, 2024 · 14 comments
Open

Subscription gets renewed while ended #233

voicecode-bv opened this issue Apr 11, 2024 · 14 comments
Assignees

Comments

@voicecode-bv
Copy link

voicecode-bv commented Apr 11, 2024

Hi team,

To be honest, I'm not sure if this is an actual bug in the package, but there's something weird going on. For a number of days I'm facing quite a lot of issues with subscriptions that seem to be be cancelled, but the cashier:run command is still trying to renew it, which causes an error

src/Subscription.php:552 - Call to a member function format() on null
(it's about the from value)

 $item->description_extra_lines = [
    trans('cashier::order_item.cycle', [
        'from' => $subscription->cycle_started_at->format('Y-m-d'),
        'to' => $subscription->cycle_ends_at->format('Y-m-d'),
    ]),
];

The cycle_started_at timestamp is NULL because on line 542 it's being set to the current cycle_ends_at timestamp which is NULL.

$subscription->cycle_started_at = $subscription->cycle_ends_at;

In the database I see:

cycle_started_at = 2024-04-10 21:23:10
cycle_ends_at = NULL
ends_at = 2024-03-14 13:48:41

To cancel a subscription we simply use:

$user->subscription('main')->cancel();

How is it possible that the cycle_started_at timestamp is a date after the ends_at timestamp?

@sandervanhooft
Copy link
Collaborator

Hi @voicecode-bv ,

What package version are you using?

@voicecode-bv
Copy link
Author

Hi @sandervanhooft,

We're on v2.11.0 currently

@voicecode-bv
Copy link
Author

I was able to narrow it down. When doing some more research, I noticed this happens when for whatever reason a payment fails (chargeback, refund, revoked mandate, etc).

We listen to such events using the class below. In both cases we won't retry the payment for obvious reasons. Is this a glitch in the cancelNow() method maybe?

class ExpireSubscription
{
    /**
     * Handle the event.
     */
    public function handle(ChargebackReceived|RefundProcessed $event)
    {
        // When refund received.
        if ($event instanceof RefundProcessed) {
            // Get user and expire subscription immediately.
            $this->expireSubscription(
                User::find($event->refund->owner_id)
            );
        }

        // When chargeback received.
        if ($event instanceof ChargebackReceived) {
            // Get user and expire subscription immediately.
            $this->expireSubscription(
                User::find($event->payment->owner_id)
            );
        }
    }

    /**
     * Expire a subscription with immediate effect.
     */
    public function expireSubscription(User $user)
    {
        $user->subscription('main')->cancelNow();
    }
}

@Naoray
Copy link
Collaborator

Naoray commented Apr 22, 2024

Hi @voicecode-bv,

I am currently looking into this. So far I wasn't able to find anything odd regarding the cancelNow() method. Do you have any other Listeners setup that reacts to any event from Cashier? Do you have a custom Subscription model? Please provide any other info that you think might be helpful to solve this issue.

@Naoray
Copy link
Collaborator

Naoray commented Apr 25, 2024

@voicecode-bv I've tested the setup locally and also listened to RefundProcessed & ChargebackReceived events as you did. I wan't able to receive the same error you had.

It's odd. The exception you describe is only triggered when a new Order is created. A new Order is only created if

  • an OrderItem is due -> when calling cancelNow() this gets rid of the next dueOrderItem
  • calling any of the following methods on Subscription
    • swap()
    • updateQuantity()
    • restartCycle()

Do you call any of this methods in your code?

@voicecode-bv
Copy link
Author

@Naoray thanks for your efforts so far, appreciate it! I only use the cancelNow() method, as you can see in my previous post.

@Naoray
Copy link
Collaborator

Naoray commented Apr 25, 2024

@voicecode-bv maybe you use a custom Subscription model or have another Listener setup somewhere else in the codebase? Or you might interact with the OrderItems directly?!

It's hard to find the cause for this without having all the details of the config you use or any custom models.

@voicecode-bv
Copy link
Author

@Naoray I fully understand, here you go:

In AppServiceProvider:

public function boot()
{
    ....

    // Get Cashier plans from database.
    $this->app->bind(\Laravel\Cashier\Plan\Contracts\PlanRepository::class, CashierPlansHelper::class);

    // Cashier subscription model.
    Cashier::useSubscriptionModel(Subscription::class);
}

CashierPlanHelper.php

<?php

namespace App\Helpers;

use App\Models\Plan;
use Laravel\Cashier\Exceptions\PlanNotFoundException;
use Laravel\Cashier\Plan\Contracts\PlanRepository;

class CashierPlansHelper implements PlanRepository
{
    public static function find(string $identifier)
    {
        $plan = Plan::where('slug', $identifier)
            ->orWhere('name', $identifier)
            ->first();

        if (is_null($plan)) {
            return;
        }

        return $plan->buildCashierPlan();
    }

    public static function findOrFail(string $identifier)
    {
        if (($result = self::find($identifier)) === null) {
            throw new PlanNotFoundException;
        }

        return $result;
    }
}

Subscription.php (model)

<?php

namespace App\Models;

use Laravel\Cashier\Subscription as CashierSubscription;

class Subscription extends CashierSubscription
{
    public function currentPlan()
    {
        return $this->hasOne(Plan::class, 'name', 'plan');
    }

    public function nextPlan()
    {
        return $this->hasOne(Plan::class, 'name', 'next_plan');
    }
}

Plan.php (model)

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;
use Laravel\Cashier\Order\Order;
use Laravel\Cashier\Plan\Plan as CashierPlan;
use Spatie\Sluggable\HasSlug;
use Spatie\Sluggable\SlugOptions;

class Plan extends Model
{
    use HasSlug, SoftDeletes;

    /**
     * The attributes that are mass assignable.
     *
     * @var array
     */
    protected $fillable = [
        'name',
        'slug',
        'price',
        'interval',
        'description',
        'discount_text',
        'notes',
        'first_payment_description',
        'first_payment_webhook_url',
        'first_payment_redirect_url',
    ];

    // Builds a Cashier plan from the current model.
    public function buildCashierPlan(): CashierPlan
    {
        // Create new plan.
        $plan = new CashierPlan($this->name);

        // Update plan details.
        $plan->setAmount(
            mollie_array_to_money([
                'value' => ($this->price / 100),
                'currency' => $this->currency,
            ])
        )
            ->setInterval($this->interval)
            ->setDescription($this->description)
            ->setFirstPaymentDescription(
                'Order '.Order::numberGenerator()->generate(),
            )
            ->setFirstPaymentRedirectUrl($this->first_payment_redirect_url)
            ->setFirstPaymentWebhookUrl($this->first_payment_webhook_url);

        return $plan;
    }
}

@Naoray
Copy link
Collaborator

Naoray commented Apr 26, 2024

I am sorry @voicecode-bv, I still don't know why you experience this glitch on your side. 🙈

Do you have any observers or other code snippets that interact with the Subscription?


Some findings that you might be interested in:

class Subscription extends CashierSubscription
{
    public function currentPlan()
    {
        return $this->hasOne(Plan::class, 'name', 'plan');
    }

    public function nextPlan()
    {
        return $this->hasOne(Plan::class, 'name', 'next_plan');
    }
}

Neat! But be aware that this could break in future releases when we add the return types to those methods!

You should add the OrderItemPreprocessors to the Cashier plan, otherwise the coupons don't work for every use case

use Laravel\Cashier\Order\OrderItemPreprocessorCollection as Preprocessors;

$plan
    //...
    ->setOrderItemPreprocessors(Preprocessors::fromArray(config('cashier_plans.defaults.order_item_preprocessors')));

another small optimisation...

class ExpireSubscription
{
    /**
     * Handle the event.
     */
    public function handle(ChargebackReceived|RefundProcessed $event)
    {
        // When refund received.
        if ($event instanceof RefundProcessed) {
            // Get user and expire subscription immediately.
            $this->expireSubscription($event->refund->owner_id); // owner is a relation
        }

        // When chargeback received.
        if ($event instanceof ChargebackReceived) {
            // Get user and expire subscription immediately.
            $this->expireSubscription($event->payment->owner); // owner is a relation
        }
    }

    /**
     * Expire a subscription with immediate effect.
     */
    public function expireSubscription(User $user)
    {
        $user->subscription('main')->cancelNow();
    }
}

@voicecode-bv
Copy link
Author

Thanks for all your efforts @Naoray, really appreciate it! For now I've manually updated the database records so cashier:run no longer crashes. Let's see if it was just a sort of "one-time-glitch".

Also, thanks for the suggestions, I've implemented them accordingly. Maybe you can change the $event->refund->owner_id to $event->refund->owner too in your comment for other people reading this topic in the future?

Cheers!

Michael

@Naoray
Copy link
Collaborator

Naoray commented May 7, 2024

Thanks @voicecode-bv,

I'll close the issue for now. Feel free to open it if you have further insights or the problem appears again!

@Naoray Naoray closed this as completed May 7, 2024
@voicecode-bv
Copy link
Author

voicecode-bv commented Jul 23, 2024

Hi @Naoray,

I'm reopening this as we still having issues. I found a good example:

We have this customer that has an active subscription, but because of his credit card has insufficient fundings, the renewal has failed. We receive a webhook from Mollie and we will cancel the subscription immediately using the cancelNow() method.

Screenshot 2024-07-23 at 12 15 56

What I see happening is that the subscription is actually being cancelled. Comparing the timestamps with the webhook timestamp from Mollie it's all looking just fine.

Screenshot 2024-07-23 at 12 15 16

Here we have the record in the orders table of which the payment has failed:

Screenshot 2024-07-23 at 12 15 36

I would expect the order items that are scheduled would be removed from the database, but as you can see, record 138182 still exists.

Screenshot 2024-07-23 at 12 15 22

This record is the reason cashier:run crashes, it thinks the subscription is still active because of that scheduled order item.

Update:
Now that I'm writing this, I realise we're listening to the RefundProcessed and ChargebackReceived events, but we're not listening to the OrderPaymentFailed event, so we'll be using this to delete the scheduled order items.

@Naoray
Copy link
Collaborator

Naoray commented Aug 5, 2024

Hi @voicecode-bv,

I am trying to understand what happened here. What I noticed is that prior subscription cycles went from 15th to the 15th of the next month and according to your timestamps shown in the screenshot they were updated on the 15th as I would expect. Then starting with OrderItem 138182 somehow the created_at doesn't match the cycles' end of the 15th. Do you have any idea on why that is? I'd expect the latest OrderItem to be created on the 2024-06-15 but somehow it was created on the day the payment failed.

Following the data, that leads me to believe that there was an OrderItem which was deleted and was somehow scheduled to be processed_at 2024-06-15. But again, the data you present doesn't make any sense to me since the failed payment should only be triggered when a cycle comes to an end so there has to be something that triggered creating a payment before the cycle was due.

Another thing that is odd to me is that the updated_at of OrderItem 138182 displays 2021-07-06 20:37:11 but the processed_at of the same record displays 2024-07-15 06:49:16. The updated_at time should always display the latest timestamp on when a record was updated. But since the record's processed_at was updated after the displayed updated_at timestamp it clearly shows that something is not right with the provided data.

Another thing that doesn't match my expectation is the description_extra_lines for OrderItem:96819 ["From 2024-07-14 to 2024-08-15"] it should instead display ["From 2024-06-15 to 2024-07-15"]. Could you paste your kernel calling the cashier:run command? I am trying to figure out if you may have overlapping commands running at the same time which might be able to explain such a scenario.

Now that I'm writing this, I realise we're listening to the RefundProcessed and ChargebackReceived events, but we're not listening to the OrderPaymentFailed event, so we'll be using this to delete the scheduled order items.

When a payment fails the Subscription::handlePaymentFailed() is triggered, which in turn calls the canceledAt(now()) which deletes the current scheduled at OrderItem automatically.

@Naoray Naoray reopened this Aug 16, 2024
@mikehesse
Copy link

Hey everyone,

We also experienced the same behaviour when a payment fails: Sometimes a payment of a customer failes due to an invalid mandate and gets cancelled immediately. So far so good.

But when the customer updates his mandate, we have the following listener:

public function handleMandateUpdated(MandateUpdated $event)
    {
        if ($event->owner->validMollieMandate()) {
            $event->owner->retryFailedOrders();
        }
    }

This should automatically retry all failed orders when the mandate is restored. And in maybe 1/1000 cases the cycle_ends_at stays empty and causes the same error as above. If you need more information on this topic to find this error, feel free to contact me!

As a workaround we periodically search for subscriptions with no cycle_ends_at timestamp and fixes them, but its more a hacky workaround and not a good solution.

Thanks for your effort!

Mike

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants