fix(stripe): add error handling and resilience to subscription operations
- Record refunds immediately before cancellation to prevent retry issues if cancel fails - Wrap Stripe API calls in try-catch for refunds and quantity reverts with internal notifications - Add null check in Team.subscriptionEnded() to prevent NPE when subscription doesn't exist - Fix control flow bug in StripeProcessJob (add missing break statement) - Cap dynamic server limit with MAX_SERVER_LIMIT in subscription updates - Add comprehensive tests for refund failures, event handling, and null safety
This commit is contained in:
parent
23f9156c73
commit
566744b2e0
9 changed files with 350 additions and 18 deletions
|
|
@ -99,16 +99,27 @@ public function execute(Team $team): array
|
|||
'payment_intent' => $paymentIntentId,
|
||||
]);
|
||||
|
||||
$this->stripe->subscriptions->cancel($subscription->stripe_subscription_id);
|
||||
// Record refund immediately so it cannot be retried if cancel fails
|
||||
$subscription->update([
|
||||
'stripe_refunded_at' => now(),
|
||||
'stripe_feedback' => 'Refund requested by user',
|
||||
'stripe_comment' => 'Full refund processed within 30-day window at '.now()->toDateTimeString(),
|
||||
]);
|
||||
|
||||
try {
|
||||
$this->stripe->subscriptions->cancel($subscription->stripe_subscription_id);
|
||||
} catch (\Exception $e) {
|
||||
\Log::critical("Refund succeeded but subscription cancel failed for team {$team->id}: ".$e->getMessage());
|
||||
send_internal_notification(
|
||||
"CRITICAL: Refund succeeded but cancel failed for subscription {$subscription->stripe_subscription_id}, team {$team->id}. Manual intervention required."
|
||||
);
|
||||
}
|
||||
|
||||
$subscription->update([
|
||||
'stripe_cancel_at_period_end' => false,
|
||||
'stripe_invoice_paid' => false,
|
||||
'stripe_trial_already_ended' => false,
|
||||
'stripe_past_due' => false,
|
||||
'stripe_feedback' => 'Refund requested by user',
|
||||
'stripe_comment' => 'Full refund processed within 30-day window at '.now()->toDateTimeString(),
|
||||
'stripe_refunded_at' => now(),
|
||||
]);
|
||||
|
||||
$team->subscriptionEnded();
|
||||
|
|
|
|||
|
|
@ -153,12 +153,19 @@ public function execute(Team $team, int $quantity): array
|
|||
\Log::warning("Subscription {$subscription->stripe_subscription_id} quantity updated but invoice not paid (status: {$latestInvoice->status}) for team {$team->name}. Reverting to {$previousQuantity}.");
|
||||
|
||||
// Revert subscription quantity on Stripe
|
||||
$this->stripe->subscriptions->update($subscription->stripe_subscription_id, [
|
||||
'items' => [
|
||||
['id' => $item->id, 'quantity' => $previousQuantity],
|
||||
],
|
||||
'proration_behavior' => 'none',
|
||||
]);
|
||||
try {
|
||||
$this->stripe->subscriptions->update($subscription->stripe_subscription_id, [
|
||||
'items' => [
|
||||
['id' => $item->id, 'quantity' => $previousQuantity],
|
||||
],
|
||||
'proration_behavior' => 'none',
|
||||
]);
|
||||
} catch (\Exception $revertException) {
|
||||
\Log::critical("Failed to revert Stripe quantity for subscription {$subscription->stripe_subscription_id}, team {$team->id}. Stripe may have quantity {$quantity} but local is {$previousQuantity}. Error: ".$revertException->getMessage());
|
||||
send_internal_notification(
|
||||
"CRITICAL: Stripe quantity revert failed for subscription {$subscription->stripe_subscription_id}, team {$team->id}. Manual reconciliation required."
|
||||
);
|
||||
}
|
||||
|
||||
// Void the unpaid invoice
|
||||
if ($latestInvoice->id) {
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
namespace App\Jobs;
|
||||
|
||||
use App\Actions\Stripe\UpdateSubscriptionQuantity;
|
||||
use App\Models\Subscription;
|
||||
use App\Models\Team;
|
||||
use Illuminate\Contracts\Queue\ShouldBeEncrypted;
|
||||
|
|
@ -238,6 +239,7 @@ public function handle(): void
|
|||
'stripe_invoice_paid' => false,
|
||||
]);
|
||||
}
|
||||
break;
|
||||
case 'customer.subscription.updated':
|
||||
$teamId = data_get($data, 'metadata.team_id');
|
||||
$userId = data_get($data, 'metadata.user_id');
|
||||
|
|
@ -272,14 +274,14 @@ public function handle(): void
|
|||
$comment = data_get($data, 'cancellation_details.comment');
|
||||
$lookup_key = data_get($data, 'items.data.0.price.lookup_key');
|
||||
if (str($lookup_key)->contains('dynamic')) {
|
||||
$quantity = data_get($data, 'items.data.0.quantity', 2);
|
||||
$quantity = min((int) data_get($data, 'items.data.0.quantity', 2), UpdateSubscriptionQuantity::MAX_SERVER_LIMIT);
|
||||
$team = data_get($subscription, 'team');
|
||||
if ($team) {
|
||||
$team->update([
|
||||
'custom_server_limit' => $quantity,
|
||||
]);
|
||||
ServerLimitCheckJob::dispatch($team);
|
||||
}
|
||||
ServerLimitCheckJob::dispatch($team);
|
||||
}
|
||||
$subscription->update([
|
||||
'stripe_feedback' => $feedback,
|
||||
|
|
|
|||
|
|
@ -82,12 +82,9 @@ public function handle(): void
|
|||
'stripe_past_due' => false,
|
||||
]);
|
||||
|
||||
// Trigger subscription ended logic if canceled
|
||||
if ($stripeSubscription->status === 'canceled') {
|
||||
$team = $this->subscription->team;
|
||||
if ($team) {
|
||||
$team->subscriptionEnded();
|
||||
}
|
||||
$team = $this->subscription->team;
|
||||
if ($team) {
|
||||
$team->subscriptionEnded();
|
||||
}
|
||||
break;
|
||||
|
||||
|
|
|
|||
|
|
@ -197,6 +197,10 @@ public function isAnyNotificationEnabled()
|
|||
|
||||
public function subscriptionEnded()
|
||||
{
|
||||
if (! $this->subscription) {
|
||||
return;
|
||||
}
|
||||
|
||||
$this->subscription->update([
|
||||
'stripe_subscription_id' => null,
|
||||
'stripe_cancel_at_period_end' => false,
|
||||
|
|
|
|||
|
|
@ -251,6 +251,56 @@
|
|||
expect($result['error'])->toContain('No payment intent');
|
||||
});
|
||||
|
||||
test('records refund and proceeds when cancel fails', function () {
|
||||
$stripeSubscription = (object) [
|
||||
'status' => 'active',
|
||||
'start_date' => now()->subDays(10)->timestamp,
|
||||
];
|
||||
|
||||
$this->mockSubscriptions
|
||||
->shouldReceive('retrieve')
|
||||
->with('sub_test_123')
|
||||
->andReturn($stripeSubscription);
|
||||
|
||||
$invoiceCollection = (object) ['data' => [
|
||||
(object) ['payment_intent' => 'pi_test_123'],
|
||||
]];
|
||||
|
||||
$this->mockInvoices
|
||||
->shouldReceive('all')
|
||||
->with([
|
||||
'subscription' => 'sub_test_123',
|
||||
'status' => 'paid',
|
||||
'limit' => 1,
|
||||
])
|
||||
->andReturn($invoiceCollection);
|
||||
|
||||
$this->mockRefunds
|
||||
->shouldReceive('create')
|
||||
->with(['payment_intent' => 'pi_test_123'])
|
||||
->andReturn((object) ['id' => 're_test_123']);
|
||||
|
||||
// Cancel throws — simulating Stripe failure after refund
|
||||
$this->mockSubscriptions
|
||||
->shouldReceive('cancel')
|
||||
->with('sub_test_123')
|
||||
->andThrow(new \Exception('Stripe cancel API error'));
|
||||
|
||||
$action = new RefundSubscription($this->mockStripe);
|
||||
$result = $action->execute($this->team);
|
||||
|
||||
// Should still succeed — refund went through
|
||||
expect($result['success'])->toBeTrue();
|
||||
expect($result['error'])->toBeNull();
|
||||
|
||||
$this->subscription->refresh();
|
||||
// Refund timestamp must be recorded
|
||||
expect($this->subscription->stripe_refunded_at)->not->toBeNull();
|
||||
// Subscription should still be marked as ended locally
|
||||
expect($this->subscription->stripe_invoice_paid)->toBeFalsy();
|
||||
expect($this->subscription->stripe_subscription_id)->toBeNull();
|
||||
});
|
||||
|
||||
test('fails when subscription is past refund window', function () {
|
||||
$stripeSubscription = (object) [
|
||||
'status' => 'active',
|
||||
|
|
|
|||
143
tests/Feature/Subscription/StripeProcessJobTest.php
Normal file
143
tests/Feature/Subscription/StripeProcessJobTest.php
Normal file
|
|
@ -0,0 +1,143 @@
|
|||
<?php
|
||||
|
||||
use App\Jobs\ServerLimitCheckJob;
|
||||
use App\Jobs\StripeProcessJob;
|
||||
use App\Models\Subscription;
|
||||
use App\Models\Team;
|
||||
use App\Models\User;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
|
||||
uses(RefreshDatabase::class);
|
||||
|
||||
beforeEach(function () {
|
||||
config()->set('constants.coolify.self_hosted', false);
|
||||
config()->set('subscription.provider', 'stripe');
|
||||
config()->set('subscription.stripe_api_key', 'sk_test_fake');
|
||||
config()->set('subscription.stripe_excluded_plans', '');
|
||||
|
||||
$this->team = Team::factory()->create();
|
||||
$this->user = User::factory()->create();
|
||||
$this->team->members()->attach($this->user->id, ['role' => 'owner']);
|
||||
});
|
||||
|
||||
describe('customer.subscription.created does not fall through to updated', function () {
|
||||
test('created event creates subscription without setting stripe_invoice_paid to true', function () {
|
||||
Queue::fake();
|
||||
|
||||
$event = [
|
||||
'type' => 'customer.subscription.created',
|
||||
'data' => [
|
||||
'object' => [
|
||||
'customer' => 'cus_new_123',
|
||||
'id' => 'sub_new_123',
|
||||
'metadata' => [
|
||||
'team_id' => $this->team->id,
|
||||
'user_id' => $this->user->id,
|
||||
],
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$job = new StripeProcessJob($event);
|
||||
$job->handle();
|
||||
|
||||
$subscription = Subscription::where('team_id', $this->team->id)->first();
|
||||
|
||||
expect($subscription)->not->toBeNull();
|
||||
expect($subscription->stripe_subscription_id)->toBe('sub_new_123');
|
||||
expect($subscription->stripe_customer_id)->toBe('cus_new_123');
|
||||
// Critical: stripe_invoice_paid must remain false — payment not yet confirmed
|
||||
expect($subscription->stripe_invoice_paid)->toBeFalsy();
|
||||
});
|
||||
});
|
||||
|
||||
describe('customer.subscription.updated clamps quantity to MAX_SERVER_LIMIT', function () {
|
||||
test('quantity exceeding MAX is clamped to 100', function () {
|
||||
Queue::fake();
|
||||
|
||||
Subscription::create([
|
||||
'team_id' => $this->team->id,
|
||||
'stripe_subscription_id' => 'sub_existing',
|
||||
'stripe_customer_id' => 'cus_clamp_test',
|
||||
'stripe_invoice_paid' => true,
|
||||
]);
|
||||
|
||||
$event = [
|
||||
'type' => 'customer.subscription.updated',
|
||||
'data' => [
|
||||
'object' => [
|
||||
'customer' => 'cus_clamp_test',
|
||||
'id' => 'sub_existing',
|
||||
'status' => 'active',
|
||||
'metadata' => [
|
||||
'team_id' => $this->team->id,
|
||||
'user_id' => $this->user->id,
|
||||
],
|
||||
'items' => [
|
||||
'data' => [[
|
||||
'subscription' => 'sub_existing',
|
||||
'plan' => ['id' => 'price_dynamic_monthly'],
|
||||
'price' => ['lookup_key' => 'dynamic_monthly'],
|
||||
'quantity' => 999,
|
||||
]],
|
||||
],
|
||||
'cancel_at_period_end' => false,
|
||||
'cancellation_details' => ['feedback' => null, 'comment' => null],
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$job = new StripeProcessJob($event);
|
||||
$job->handle();
|
||||
|
||||
$this->team->refresh();
|
||||
expect($this->team->custom_server_limit)->toBe(100);
|
||||
|
||||
Queue::assertPushed(ServerLimitCheckJob::class);
|
||||
});
|
||||
});
|
||||
|
||||
describe('ServerLimitCheckJob dispatch is guarded by team check', function () {
|
||||
test('does not dispatch ServerLimitCheckJob when team is null', function () {
|
||||
Queue::fake();
|
||||
|
||||
// Create subscription without a valid team relationship
|
||||
$subscription = Subscription::create([
|
||||
'team_id' => 99999,
|
||||
'stripe_subscription_id' => 'sub_orphan',
|
||||
'stripe_customer_id' => 'cus_orphan_test',
|
||||
'stripe_invoice_paid' => true,
|
||||
]);
|
||||
|
||||
$event = [
|
||||
'type' => 'customer.subscription.updated',
|
||||
'data' => [
|
||||
'object' => [
|
||||
'customer' => 'cus_orphan_test',
|
||||
'id' => 'sub_orphan',
|
||||
'status' => 'active',
|
||||
'metadata' => [
|
||||
'team_id' => null,
|
||||
'user_id' => null,
|
||||
],
|
||||
'items' => [
|
||||
'data' => [[
|
||||
'subscription' => 'sub_orphan',
|
||||
'plan' => ['id' => 'price_dynamic_monthly'],
|
||||
'price' => ['lookup_key' => 'dynamic_monthly'],
|
||||
'quantity' => 5,
|
||||
]],
|
||||
],
|
||||
'cancel_at_period_end' => false,
|
||||
'cancellation_details' => ['feedback' => null, 'comment' => null],
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$job = new StripeProcessJob($event);
|
||||
$job->handle();
|
||||
|
||||
Queue::assertNotPushed(ServerLimitCheckJob::class);
|
||||
});
|
||||
});
|
||||
16
tests/Feature/Subscription/TeamSubscriptionEndedTest.php
Normal file
16
tests/Feature/Subscription/TeamSubscriptionEndedTest.php
Normal file
|
|
@ -0,0 +1,16 @@
|
|||
<?php
|
||||
|
||||
use App\Models\Team;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
|
||||
uses(RefreshDatabase::class);
|
||||
|
||||
test('subscriptionEnded does not throw when team has no subscription', function () {
|
||||
$team = Team::factory()->create();
|
||||
|
||||
// Should return early without error — no NPE
|
||||
$team->subscriptionEnded();
|
||||
|
||||
// If we reach here, no exception was thrown
|
||||
expect(true)->toBeTrue();
|
||||
});
|
||||
|
|
@ -0,0 +1,102 @@
|
|||
<?php
|
||||
|
||||
use App\Jobs\VerifyStripeSubscriptionStatusJob;
|
||||
use App\Models\Server;
|
||||
use App\Models\Subscription;
|
||||
use App\Models\Team;
|
||||
use App\Models\User;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Stripe\Service\SubscriptionService;
|
||||
use Stripe\StripeClient;
|
||||
|
||||
uses(RefreshDatabase::class);
|
||||
|
||||
beforeEach(function () {
|
||||
config()->set('constants.coolify.self_hosted', false);
|
||||
config()->set('subscription.provider', 'stripe');
|
||||
config()->set('subscription.stripe_api_key', 'sk_test_fake');
|
||||
|
||||
$this->team = Team::factory()->create();
|
||||
$this->user = User::factory()->create();
|
||||
$this->team->members()->attach($this->user->id, ['role' => 'owner']);
|
||||
|
||||
$this->subscription = Subscription::create([
|
||||
'team_id' => $this->team->id,
|
||||
'stripe_subscription_id' => 'sub_verify_123',
|
||||
'stripe_customer_id' => 'cus_verify_123',
|
||||
'stripe_invoice_paid' => true,
|
||||
'stripe_past_due' => false,
|
||||
]);
|
||||
});
|
||||
|
||||
test('subscriptionEnded is called for unpaid status', function () {
|
||||
$mockStripe = Mockery::mock(StripeClient::class);
|
||||
$mockSubscriptions = Mockery::mock(SubscriptionService::class);
|
||||
$mockStripe->subscriptions = $mockSubscriptions;
|
||||
|
||||
$mockSubscriptions
|
||||
->shouldReceive('retrieve')
|
||||
->with('sub_verify_123')
|
||||
->andReturn((object) [
|
||||
'status' => 'unpaid',
|
||||
'cancel_at_period_end' => false,
|
||||
]);
|
||||
|
||||
app()->bind(StripeClient::class, fn () => $mockStripe);
|
||||
|
||||
// Create a server to verify it gets disabled
|
||||
$server = Server::factory()->create(['team_id' => $this->team->id]);
|
||||
|
||||
$job = new VerifyStripeSubscriptionStatusJob($this->subscription);
|
||||
$job->handle();
|
||||
|
||||
$this->subscription->refresh();
|
||||
expect($this->subscription->stripe_invoice_paid)->toBeFalsy();
|
||||
expect($this->subscription->stripe_subscription_id)->toBeNull();
|
||||
});
|
||||
|
||||
test('subscriptionEnded is called for incomplete_expired status', function () {
|
||||
$mockStripe = Mockery::mock(StripeClient::class);
|
||||
$mockSubscriptions = Mockery::mock(SubscriptionService::class);
|
||||
$mockStripe->subscriptions = $mockSubscriptions;
|
||||
|
||||
$mockSubscriptions
|
||||
->shouldReceive('retrieve')
|
||||
->with('sub_verify_123')
|
||||
->andReturn((object) [
|
||||
'status' => 'incomplete_expired',
|
||||
'cancel_at_period_end' => false,
|
||||
]);
|
||||
|
||||
app()->bind(StripeClient::class, fn () => $mockStripe);
|
||||
|
||||
$job = new VerifyStripeSubscriptionStatusJob($this->subscription);
|
||||
$job->handle();
|
||||
|
||||
$this->subscription->refresh();
|
||||
expect($this->subscription->stripe_invoice_paid)->toBeFalsy();
|
||||
expect($this->subscription->stripe_subscription_id)->toBeNull();
|
||||
});
|
||||
|
||||
test('subscriptionEnded is called for canceled status', function () {
|
||||
$mockStripe = Mockery::mock(StripeClient::class);
|
||||
$mockSubscriptions = Mockery::mock(SubscriptionService::class);
|
||||
$mockStripe->subscriptions = $mockSubscriptions;
|
||||
|
||||
$mockSubscriptions
|
||||
->shouldReceive('retrieve')
|
||||
->with('sub_verify_123')
|
||||
->andReturn((object) [
|
||||
'status' => 'canceled',
|
||||
'cancel_at_period_end' => false,
|
||||
]);
|
||||
|
||||
app()->bind(StripeClient::class, fn () => $mockStripe);
|
||||
|
||||
$job = new VerifyStripeSubscriptionStatusJob($this->subscription);
|
||||
$job->handle();
|
||||
|
||||
$this->subscription->refresh();
|
||||
expect($this->subscription->stripe_invoice_paid)->toBeFalsy();
|
||||
expect($this->subscription->stripe_subscription_id)->toBeNull();
|
||||
});
|
||||
Loading…
Reference in a new issue