diff --git a/src/app/Console/Commands/Job/WalletCharge.php b/src/app/Console/Commands/Job/WalletCharge.php --- a/src/app/Console/Commands/Job/WalletCharge.php +++ b/src/app/Console/Commands/Job/WalletCharge.php @@ -34,7 +34,7 @@ return 1; } - $job = new \App\Jobs\WalletCharge($wallet); + $job = new \App\Jobs\WalletCharge($wallet->id); $job->handle(); } } diff --git a/src/app/Console/Commands/Job/WalletCheck.php b/src/app/Console/Commands/Job/WalletCheck.php --- a/src/app/Console/Commands/Job/WalletCheck.php +++ b/src/app/Console/Commands/Job/WalletCheck.php @@ -34,7 +34,7 @@ return 1; } - $job = new \App\Jobs\WalletCheck($wallet); + $job = new \App\Jobs\WalletCheck($wallet->id); $job->handle(); } } diff --git a/src/app/Console/Commands/Wallet/ChargeCommand.php b/src/app/Console/Commands/Wallet/ChargeCommand.php --- a/src/app/Console/Commands/Wallet/ChargeCommand.php +++ b/src/app/Console/Commands/Wallet/ChargeCommand.php @@ -51,30 +51,7 @@ } foreach ($wallets as $wallet) { - // This is a long-running process. Because another process might have modified - // the wallet balance in meantime we have to refresh it. - // Note: This is needed despite the use of cursor() above. - $wallet->refresh(); - - // Sanity check after refresh (owner deleted in meantime) - if (!$wallet->owner) { - continue; - } - - $charge = $wallet->chargeEntitlements(); - - if ($charge > 0) { - $this->info("Charged wallet {$wallet->id} for user {$wallet->owner->email} with {$charge}"); - - // Top-up the wallet if auto-payment enabled for the wallet - \App\Jobs\WalletCharge::dispatch($wallet); - } - - if ($wallet->balance < 0) { - // Check the account balance, send notifications, (suspend, delete,) degrade - // Also sends reminders to the degraded account owners - \App\Jobs\WalletCheck::dispatch($wallet); - } + \App\Jobs\WalletCheck::dispatch($wallet->id); } } } diff --git a/src/app/Http/Controllers/API/V4/PaymentsController.php b/src/app/Http/Controllers/API/V4/PaymentsController.php --- a/src/app/Http/Controllers/API/V4/PaymentsController.php +++ b/src/app/Http/Controllers/API/V4/PaymentsController.php @@ -132,7 +132,7 @@ // Trigger auto-payment if the balance is below the threshold if ($wallet->balance < round($request->balance * 100)) { - \App\Jobs\WalletCharge::dispatch($wallet); + \App\Jobs\WalletCharge::dispatch($wallet->id); } $result = self::walletMandate($wallet); diff --git a/src/app/Jobs/WalletCharge.php b/src/app/Jobs/WalletCharge.php --- a/src/app/Jobs/WalletCharge.php +++ b/src/app/Jobs/WalletCharge.php @@ -8,17 +8,12 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; -use Illuminate\Queue\SerializesModels; class WalletCharge implements ShouldQueue { use Dispatchable; use InteractsWithQueue; use Queueable; - use SerializesModels; - - /** @var \App\Wallet A wallet object */ - protected $wallet; /** @var int The number of seconds to wait before retrying the job. */ public $backoff = 10; @@ -29,17 +24,19 @@ /** @var bool Delete the job if the wallet no longer exist. */ public $deleteWhenMissingModels = true; + /** @var string A wallet identifier */ + protected $walletId; /** * Create a new job instance. * - * @param \App\Wallet $wallet The wallet that has been charged. + * @param string $walletId The wallet that has been charged. * * @return void */ - public function __construct(Wallet $wallet) + public function __construct(string $walletId) { - $this->wallet = $wallet; + $this->walletId = $walletId; } /** @@ -49,6 +46,8 @@ */ public function handle() { - PaymentsController::topUpWallet($this->wallet); + if ($wallet = Wallet::find($this->walletId)) { + PaymentsController::topUpWallet($wallet); + } } } diff --git a/src/app/Jobs/WalletCheck.php b/src/app/Jobs/WalletCheck.php --- a/src/app/Jobs/WalletCheck.php +++ b/src/app/Jobs/WalletCheck.php @@ -9,14 +9,12 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; -use Illuminate\Queue\SerializesModels; class WalletCheck implements ShouldQueue { use Dispatchable; use InteractsWithQueue; use Queueable; - use SerializesModels; public const THRESHOLD_DEGRADE = 'degrade'; public const THRESHOLD_DEGRADE_REMINDER = 'degrade-reminder'; @@ -34,20 +32,23 @@ /** @var bool Delete the job if the wallet no longer exist. */ public $deleteWhenMissingModels = true; - /** @var \App\Wallet A wallet object */ + /** @var ?Wallet A wallet object */ protected $wallet; + /** @var string A wallet identifier */ + protected $walletId; + /** * Create a new job instance. * - * @param \App\Wallet $wallet The wallet that has been charged. + * @param string $walletId The wallet that has been charged. * * @return void */ - public function __construct(Wallet $wallet) + public function __construct(string $walletId) { - $this->wallet = $wallet; + $this->walletId = $walletId; } /** @@ -57,6 +58,25 @@ */ public function handle() { + $this->wallet = Wallet::find($this->walletId); + + // Sanity check (owner deleted in meantime) + if (!$this->wallet || !$this->wallet->owner) { + return null; + } + + if ($this->wallet->chargeEntitlements() > 0) { + // We make a payment when there's a charge. If for some reason the + // payment failed we can't just throw here, as another execution of this job + // will not re-try the payment. So, we attempt a payment in a separate job. + try { + $this->topUpWallet(); + } catch (\Exception $e) { + \Log::error("Failed to top-up wallet {$this->walletId}: " . $e->getMessage()); + WalletCharge::dispatch($this->wallet->id); + } + } + if ($this->wallet->balance >= 0) { return null; } diff --git a/src/app/Providers/Payment/Stripe.php b/src/app/Providers/Payment/Stripe.php --- a/src/app/Providers/Payment/Stripe.php +++ b/src/app/Providers/Payment/Stripe.php @@ -375,7 +375,7 @@ // Top-up the wallet if balance is below the threshold if ($payment->wallet->balance < $threshold && $payment->status != Payment::STATUS_PAID) { - \App\Jobs\WalletCharge::dispatch($payment->wallet); + \App\Jobs\WalletCharge::dispatch($payment->wallet->id); } } diff --git a/src/tests/Feature/Console/Wallet/ChargeTest.php b/src/tests/Feature/Console/Wallet/ChargeTest.php --- a/src/tests/Feature/Console/Wallet/ChargeTest.php +++ b/src/tests/Feature/Console/Wallet/ChargeTest.php @@ -50,57 +50,10 @@ $this->artisan('wallet:charge ' . $wallet->id) ->assertExitCode(0); - Queue::assertNothingPushed(); - - // The wallet has no entitlements, but has negative balance - $wallet->balance = -100; - $wallet->save(); - - $this->artisan('wallet:charge ' . $wallet->id) - ->assertExitCode(0); - - Queue::assertPushed(\App\Jobs\WalletCharge::class, 0); - Queue::assertPushed(\App\Jobs\WalletCheck::class, 1); - Queue::assertPushed(\App\Jobs\WalletCheck::class, function ($job) use ($wallet) { - $job_wallet = TestCase::getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; - }); - - Queue::fake(); - - // The wallet has entitlements to charge, and negative balance - $sku = \App\Sku::withObjectTenantContext($user)->where('title', 'mailbox')->first(); - $entitlement = \App\Entitlement::create([ - 'wallet_id' => $wallet->id, - 'sku_id' => $sku->id, - 'cost' => 100, - 'entitleable_id' => $user->id, - 'entitleable_type' => \App\User::class, - ]); - \App\Entitlement::where('id', $entitlement->id)->update([ - 'created_at' => \Carbon\Carbon::now()->subMonthsNoOverflow(1), - 'updated_at' => \Carbon\Carbon::now()->subMonthsNoOverflow(1), - ]); - \App\User::where('id', $user->id)->update([ - 'created_at' => \Carbon\Carbon::now()->subMonthsNoOverflow(1), - 'updated_at' => \Carbon\Carbon::now()->subMonthsNoOverflow(1), - ]); - - $this->assertSame(100, $wallet->fresh()->chargeEntitlements(false)); - - $this->artisan('wallet:charge ' . $wallet->id) - ->assertExitCode(0); - - Queue::assertPushed(\App\Jobs\WalletCharge::class, 1); - Queue::assertPushed(\App\Jobs\WalletCharge::class, function ($job) use ($wallet) { - $job_wallet = TestCase::getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; - }); - Queue::assertPushed(\App\Jobs\WalletCheck::class, 1); Queue::assertPushed(\App\Jobs\WalletCheck::class, function ($job) use ($wallet) { - $job_wallet = TestCase::getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; + $job_wallet_id = TestCase::getObjectProperty($job, 'walletId'); + return $job_wallet_id === $wallet->id; }); } @@ -109,39 +62,29 @@ */ public function testHandleAll(): void { - $user = $this->getTestUser('john@kolab.org'); - $wallet = $user->wallets()->first(); - $wallet->balance = 0; - $wallet->save(); - - // backdate john's entitlements and set balance=0 for all wallets - $this->backdateEntitlements($user->entitlements, \Carbon\Carbon::now()->subWeeks(5)); - \App\Wallet::where('balance', '<', '0')->update(['balance' => 0]); + $user1 = $this->getTestUser('john@kolab.org'); + $wallet1 = $user1->wallets()->first(); $user2 = $this->getTestUser('wallet-charge@kolabnow.com'); $wallet2 = $user2->wallets()->first(); - $wallet2->balance = -100; - $wallet2->save(); + + $count = \App\Wallet::join('users', 'users.id', '=', 'wallets.user_id') + ->withEnvTenantContext('users') + ->whereNull('users.deleted_at') + ->count(); Queue::fake(); - // Non-existing wallet ID $this->artisan('wallet:charge')->assertExitCode(0); - Queue::assertPushed(\App\Jobs\WalletCheck::class, 2); - Queue::assertPushed(\App\Jobs\WalletCheck::class, function ($job) use ($wallet) { - $job_wallet = TestCase::getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; + Queue::assertPushed(\App\Jobs\WalletCheck::class, $count); + Queue::assertPushed(\App\Jobs\WalletCheck::class, function ($job) use ($wallet1) { + $job_wallet_id = TestCase::getObjectProperty($job, 'walletId'); + return $job_wallet_id === $wallet1->id; }); Queue::assertPushed(\App\Jobs\WalletCheck::class, function ($job) use ($wallet2) { - $job_wallet = TestCase::getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet2->id; - }); - - Queue::assertPushed(\App\Jobs\WalletCharge::class, 1); - Queue::assertPushed(\App\Jobs\WalletCharge::class, function ($job) use ($wallet) { - $job_wallet = TestCase::getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; + $job_wallet_id = TestCase::getObjectProperty($job, 'walletId'); + return $job_wallet_id === $wallet2->id; }); } } diff --git a/src/tests/Feature/Controller/PaymentsMollieEuroTest.php b/src/tests/Feature/Controller/PaymentsMollieEuroTest.php --- a/src/tests/Feature/Controller/PaymentsMollieEuroTest.php +++ b/src/tests/Feature/Controller/PaymentsMollieEuroTest.php @@ -256,8 +256,8 @@ Bus::assertDispatchedTimes(\App\Jobs\WalletCharge::class, 1); Bus::assertDispatched(\App\Jobs\WalletCharge::class, function ($job) use ($wallet) { - $job_wallet = $this->getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; + $job_wallet_id = $this->getObjectProperty($job, 'walletId'); + return $job_wallet_id === $wallet->id; }); $this->unmockMollie(); diff --git a/src/tests/Feature/Controller/PaymentsMollieTest.php b/src/tests/Feature/Controller/PaymentsMollieTest.php --- a/src/tests/Feature/Controller/PaymentsMollieTest.php +++ b/src/tests/Feature/Controller/PaymentsMollieTest.php @@ -290,8 +290,8 @@ Bus::assertDispatchedTimes(\App\Jobs\WalletCharge::class, 1); Bus::assertDispatched(\App\Jobs\WalletCharge::class, function ($job) use ($wallet) { - $job_wallet = $this->getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; + $job_wallet_id = $this->getObjectProperty($job, 'walletId'); + return $job_wallet_id === $wallet->id; }); $this->unmockMollie(); diff --git a/src/tests/Feature/Controller/PaymentsStripeTest.php b/src/tests/Feature/Controller/PaymentsStripeTest.php --- a/src/tests/Feature/Controller/PaymentsStripeTest.php +++ b/src/tests/Feature/Controller/PaymentsStripeTest.php @@ -280,8 +280,8 @@ Bus::assertDispatchedTimes(\App\Jobs\WalletCharge::class, 1); Bus::assertDispatched(\App\Jobs\WalletCharge::class, function ($job) use ($wallet) { - $job_wallet = $this->getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; + $job_wallet_id = $this->getObjectProperty($job, 'walletId'); + return $job_wallet_id === $wallet->id; }); $this->unmockStripe(); @@ -513,8 +513,8 @@ // Expect a WalletCharge job if the balance is negative Bus::assertDispatchedTimes(\App\Jobs\WalletCharge::class, 1); Bus::assertDispatched(\App\Jobs\WalletCharge::class, function ($job) use ($wallet) { - $job_wallet = TestCase::getObjectProperty($job, 'wallet'); - return $job_wallet->id === $wallet->id; + $job_wallet_id = TestCase::getObjectProperty($job, 'walletId'); + return $job_wallet_id === $wallet->id; }); // TODO: test other setup_intent.* events diff --git a/src/tests/Feature/Jobs/WalletCheckTest.php b/src/tests/Feature/Jobs/WalletCheckTest.php --- a/src/tests/Feature/Jobs/WalletCheckTest.php +++ b/src/tests/Feature/Jobs/WalletCheckTest.php @@ -7,6 +7,7 @@ use App\Wallet; use Carbon\Carbon; use Illuminate\Support\Facades\Mail; +use Illuminate\Support\Facades\Queue; use Tests\TestCase; class WalletCheckTest extends TestCase @@ -45,7 +46,7 @@ $wallet->balance = 0; $wallet->save(); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); Mail::assertNothingSent(); @@ -54,7 +55,7 @@ $wallet->balance = -100; $wallet->save(); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); Mail::assertNothingSent(); @@ -63,7 +64,7 @@ $wallet->setSetting('balance_negative_since', $now->subHours(2)->toDateTimeString()); $wallet->setSetting('balance_warning_initial', null); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); // Assert the mail was sent to the user's email, but not to his external email @@ -74,7 +75,7 @@ // Run the job again to make sure the notification is not sent again Mail::fake(); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); Mail::assertNothingSent(); @@ -84,7 +85,7 @@ $wallet->setSetting('balance_negative_since', null); $wallet->setSetting('balance_warning_initial', null); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); // Assert the mail was sent to the user's email, but not to his external email @@ -103,12 +104,37 @@ $wallet->owner->suspend(); $wallet->setSetting('balance_warning_initial', null); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); Mail::assertNothingSent(); } + /** + * Test job handle, wallet charge and top-up + */ + public function testHandleInitialCharge(): void + { + Mail::fake(); + Queue::fake(); + + $user = $this->prepareTestUser($wallet); + $wallet->balance = 0; + $wallet->save(); + $this->backdateEntitlements($wallet->entitlements, Carbon::now()->subWeeks(5)); + + $job = new WalletCheck($wallet->id); + $job->handle(); + + $wallet->refresh(); + + $this->assertTrue($wallet->balance < 0); // @phpstan-ignore-line + + // TODO: How to mock PaymentsProvider::topUpWallet() to make sure it was called? + // We should probably move this method to the App\Wallet class to make it possible. + $this->markTestIncomplete(); + } + /** * Test job handle, top-up before reminder notification * @@ -124,7 +150,7 @@ // Balance turned negative 7-1 days ago $wallet->setSetting('balance_negative_since', $now->subDays(7 - 1)->toDateTimeString()); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $res = $job->handle(); Mail::assertNothingSent(); @@ -148,7 +174,7 @@ // Balance turned negative 7+1 days ago, expect mail sent $wallet->setSetting('balance_negative_since', $now->subDays(7 + 1)->toDateTimeString()); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); // Assert the mail was sent to the user's email and to his external email @@ -159,7 +185,7 @@ // Run the job again to make sure the notification is not sent again Mail::fake(); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); Mail::assertNothingSent(); @@ -169,7 +195,7 @@ $wallet->owner->suspend(); $wallet->setSetting('balance_warning_reminder', null); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); Mail::assertNothingSent(); @@ -193,7 +219,7 @@ $days = 7 + 7 + 1; $wallet->setSetting('balance_negative_since', $now->subDays($days)->toDateTimeString()); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); // Assert the mail was sent to the user's email, and his external email @@ -210,7 +236,7 @@ $wallet->owner->suspend(); $wallet->owner->undegrade(); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); Mail::assertNothingSent(); @@ -234,7 +260,7 @@ // Test degraded_last_reminder not set $wallet->setSetting('degraded_last_reminder', null); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $res = $job->handle(); Mail::assertNothingSent(); @@ -247,7 +273,7 @@ $last = $now->copy()->subDays(10); $wallet->setSetting('degraded_last_reminder', $last->toDateTimeString()); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $res = $job->handle(); Mail::assertNothingSent(); @@ -259,7 +285,7 @@ // Test degraded_last_reminder set, and 14 days passed $wallet->setSetting('degraded_last_reminder', $now->copy()->subDays(14)->setSeconds(0)); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $res = $job->handle(); // Assert the mail was sent to the user's email, and his external email @@ -278,7 +304,7 @@ $wallet->owner->undegrade(); $wallet->setSetting('degraded_last_reminder', null); - $job = new WalletCheck($wallet); + $job = new WalletCheck($wallet->id); $job->handle(); Mail::assertNothingSent();