diff --git a/src/app/Jobs/User/UpdateJob.php b/src/app/Jobs/User/UpdateJob.php --- a/src/app/Jobs/User/UpdateJob.php +++ b/src/app/Jobs/User/UpdateJob.php @@ -3,9 +3,13 @@ namespace App\Jobs\User; use App\Jobs\UserJob; +use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing; -class UpdateJob extends UserJob +class UpdateJob extends UserJob implements ShouldBeUniqueUntilProcessing { + /** @var int The number of seconds after which the job's unique lock will be released. */ + public $uniqueFor = 60; + /** * Execute the job. * @@ -34,4 +38,16 @@ } } } + + /** + * Get the unique ID for the job. + * + * This together with ShouldBeUniqueUntilProcessing makes sure there's only one update job + * for the same user at the same time. E.g. if you delete 5 storage entitlements in one action, + * we'll reach to LDAP/IMAP backend only once (instead of five). + */ + public function uniqueId(): string + { + return (string) $this->userId; + } } diff --git a/src/config/queue.php b/src/config/queue.php --- a/src/config/queue.php +++ b/src/config/queue.php @@ -68,7 +68,7 @@ 'queue' => env('REDIS_QUEUE', 'default'), 'retry_after' => 90, 'block_for' => null, - 'after_commit' => false, + 'after_commit' => true, ], ], diff --git a/src/tests/Feature/BillingTest.php b/src/tests/Feature/BillingTest.php --- a/src/tests/Feature/BillingTest.php +++ b/src/tests/Feature/BillingTest.php @@ -3,7 +3,6 @@ namespace Tests\Feature; use Carbon\Carbon; -use Illuminate\Support\Facades\Queue; use Tests\TestCase; class BillingTest extends TestCase diff --git a/src/tests/Feature/EntitlementTest.php b/src/tests/Feature/EntitlementTest.php --- a/src/tests/Feature/EntitlementTest.php +++ b/src/tests/Feature/EntitlementTest.php @@ -52,11 +52,11 @@ $wallet = $user->wallets->first(); // Test dispatching update jobs for the user, on quota update - Queue::fake(); + $this->fakeQueueReset(); $user->assignSku($skuMailbox, 1, $wallet); Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 0); - Queue::fake(); + $this->fakeQueueReset(); $user->assignSku($skuStorage, 1, $wallet); Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); Queue::assertPushed( @@ -66,12 +66,12 @@ } ); - Queue::fake(); + $this->fakeQueueReset(); $user->entitlements()->where('sku_id', $skuMailbox->id)->first()->delete(); //FIXME this sometimes gives 1? Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 0); - Queue::fake(); + $this->fakeQueueReset(); $user->entitlements()->where('sku_id', $skuStorage->id)->first()->delete(); //FIXME this sometimes gives 2? Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); @@ -134,8 +134,6 @@ */ public function testEntitleableTitle(): void { - Queue::fake(); - $packageDomain = Package::withEnvTenantContext()->where('title', 'domain-hosting')->first(); $packageKolab = Package::withEnvTenantContext()->where('title', 'kolab')->first(); $user = $this->getTestUser('entitled-user@custom-domain.com'); diff --git a/src/tests/Feature/UserTest.php b/src/tests/Feature/UserTest.php --- a/src/tests/Feature/UserTest.php +++ b/src/tests/Feature/UserTest.php @@ -628,7 +628,7 @@ */ public function testDegradeAndUndegrade(): void { - Queue::fake(); + $this->fakeQueueReset(); // Test an account with users, domain $userA = $this->getTestUser('UserAccountA@UserAccount.com'); @@ -660,7 +660,7 @@ $this->assertSame(7, $entitlementsB->whereDate('updated_at', $yesterday->toDateString())->count()); $this->assertSame(0, $wallet->balance); - Queue::fake(); // reset queue state + $this->fakeQueueReset(); // Degrade the account/wallet owner $userA->degrade(); @@ -696,7 +696,7 @@ $this->backdateEntitlements($entitlementsA->get(), $yesterday, Carbon::now()->subMonthsWithoutOverflow(1)); $this->backdateEntitlements($entitlementsB->get(), $yesterday, Carbon::now()->subMonthsWithoutOverflow(1)); - Queue::fake(); // reset queue state + $this->fakeQueueReset(); $userA->undegrade(); @@ -1136,7 +1136,7 @@ */ public function testRestore(): void { - Queue::fake(); + $this->fakeQueueReset(); // Test an account with users and domain $userA = $this->getTestUser('UserAccountA@UserAccount.com', [ @@ -1197,7 +1197,7 @@ \App\Entitlement::withTrashed()->where('wallet_id', $wallet_id) ->update(['updated_at' => $now->subMinutes(10)]); - Queue::fake(); + $this->fakeQueueReset(); // Then restore it $userA->restore(); @@ -1244,7 +1244,7 @@ */ public function testRestrictAndUnrestrict(): void { - Queue::fake(); + $this->fakeQueueReset(); // Test an account with users, domain $user = $this->getTestUser('UserAccountA@UserAccount.com'); @@ -1277,7 +1277,7 @@ $userB->restrict(); $this->assertTrue($userB->fresh()->isRestricted()); - Queue::fake(); // reset queue state + $this->fakeQueueReset(); $user->refresh(); $user->unrestrict(); @@ -1292,7 +1292,7 @@ } ); - Queue::fake(); // reset queue state + $this->fakeQueueReset(); $user->unrestrict(true); @@ -1312,8 +1312,7 @@ */ public function testSetAliases(): void { - Queue::fake(); - Queue::assertNothingPushed(); + $this->fakeQueueReset(); $user = $this->getTestUser('UserAccountA@UserAccount.com'); $domain = $this->getTestDomain('UserAccount.com', [ @@ -1338,10 +1337,11 @@ $this->assertSame('useralias1@useraccount.com', $aliases[0]['alias']); // Add another alias + $this->fakeQueueReset(); $user->setAliases(['UserAlias1@UserAccount.com', 'UserAlias2@UserAccount.com']); - Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 2); - Queue::assertPushed(\App\Jobs\PGP\KeyCreateJob::class, 1); + Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); + Queue::assertPushed(\App\Jobs\PGP\KeyCreateJob::class, 0); $aliases = $user->aliases()->orderBy('alias')->get(); $this->assertCount(2, $aliases); @@ -1351,11 +1351,12 @@ $user->tenant->setSetting('pgp.enable', 1); // Remove an alias + $this->fakeQueueReset(); $user->setAliases(['UserAlias1@UserAccount.com']); $user->tenant->setSetting('pgp.enable', 0); - Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 3); + Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); Queue::assertPushed(\App\Jobs\PGP\KeyDeleteJob::class, 1); Queue::assertPushed( \App\Jobs\PGP\KeyDeleteJob::class, @@ -1371,9 +1372,10 @@ $this->assertSame('useralias1@useraccount.com', $aliases[0]['alias']); // Remove all aliases + $this->fakeQueueReset(); $user->setAliases([]); - Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 4); + Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); $this->assertCount(0, $user->aliases()->get()); } @@ -1426,8 +1428,7 @@ */ public function testUserSettings(): void { - Queue::fake(); - Queue::assertNothingPushed(); + $this->fakeQueueReset(); $user = $this->getTestUser('UserAccountA@UserAccount.com'); @@ -1455,10 +1456,11 @@ $this->assertSame('Firstname', $user->fresh()->getSetting('first_name')); // Update a setting + $this->fakeQueueReset(); $user->setSetting('first_name', 'Firstname1'); if (\config('app.with_ldap')) { - Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 2); + Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); } // Note: We test both current user as well as fresh user object @@ -1467,10 +1469,11 @@ $this->assertSame('Firstname1', $user->fresh()->getSetting('first_name')); // Delete a setting (null) + $this->fakeQueueReset(); $user->setSetting('first_name', null); if (\config('app.with_ldap')) { - Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 3); + Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); } // Note: We test both current user as well as fresh user object @@ -1479,11 +1482,12 @@ $this->assertSame(null, $user->fresh()->getSetting('first_name')); // Delete a setting (empty string) + $this->fakeQueueReset(); $user->setSetting('first_name', 'Firstname1'); $user->setSetting('first_name', ''); if (\config('app.with_ldap')) { - Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 5); + Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); } // Note: We test both current user as well as fresh user object @@ -1492,15 +1496,16 @@ $this->assertSame(null, $user->fresh()->getSetting('first_name')); // Set multiple settings at once + $this->fakeQueueReset(); $user->setSettings([ 'first_name' => 'Firstname2', 'last_name' => 'Lastname2', 'country' => null, ]); - // TODO: This really should create a single UserUpdate job, not 3 + // Thanks to job locking it creates a single UserUpdate job if (\config('app.with_ldap')) { - Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 7); + Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); } // Note: We test both current user as well as fresh user object @@ -1512,18 +1517,21 @@ $this->assertSame(null, $user->getSetting('country')); $this->assertSame(null, $user->fresh()->getSetting('country')); - $all_settings = $user->settings()->orderBy('key')->get(); - $this->assertCount(3, $all_settings); + $expected = [ + 'currency' => 'CHF', + 'first_name' => 'Firstname2', + 'last_name' => 'Lastname2', + ]; - // Test getSettings() method - $this->assertSame( - [ - 'first_name' => 'Firstname2', - 'last_name' => 'Lastname2', - 'unknown' => null, - ], - $user->getSettings(['first_name', 'last_name', 'unknown']) - ); + $this->assertSame($expected, $user->settings()->orderBy('key')->get()->pluck('value', 'key')->all()); + + $expected = [ + 'first_name' => 'Firstname2', + 'last_name' => 'Lastname2', + 'unknown' => null, + ]; + + $this->assertSame($expected, $user->getSettings(['first_name', 'last_name', 'unknown'])); } /** diff --git a/src/tests/Feature/WalletTest.php b/src/tests/Feature/WalletTest.php --- a/src/tests/Feature/WalletTest.php +++ b/src/tests/Feature/WalletTest.php @@ -113,7 +113,7 @@ $this->assertTrue($user1->isRestricted()); $this->assertTrue($user2->isRestricted()); - Queue::fake(); + $this->fakeQueueReset(); $wallet->balance = 100; $wallet->save(); diff --git a/src/tests/TestCaseTrait.php b/src/tests/TestCaseTrait.php --- a/src/tests/TestCaseTrait.php +++ b/src/tests/TestCaseTrait.php @@ -620,6 +620,24 @@ return $method->invokeArgs($object, $parameters); } + /** + * Init fake queue. Release unique job locks. + */ + protected function fakeQueueReset() + { + // Release all locks for ShouldBeUnique jobs. Works only with Redis cache. + $db = \cache()->getStore()->lockConnection(); + $prefix = $db->getOptions()->prefix?->getPrefix(); + + foreach ($db->keys('*') as $key) { + if (strpos($key, 'laravel_unique_job') !== false) { + $db->del($prefix ? substr($key, strlen($prefix)) : $key); + } + } + + Queue::fake(); + } + /** * Extract content of an email message. *