From fe844e652c132da5f335b8bc7270007e15054315 Mon Sep 17 00:00:00 2001 From: DariusIII Date: Wed, 11 Mar 2026 17:06:11 +0100 Subject: [PATCH] Fix inactive account removal logic and checks --- app/Jobs/RemoveInactiveAccounts.php | 33 +++-- app/Models/User.php | 6 +- app/Services/UserInactivityEvaluator.php | 52 ++++++++ ...120000_add_lastdownload_to_users_table.php | 28 +++++ tests/Feature/ReleaseReportControllerTest.php | 113 ++++++++++++++++++ .../Services/UserInactivityEvaluatorTest.php | 65 ++++++++++ 6 files changed, 283 insertions(+), 14 deletions(-) create mode 100644 app/Services/UserInactivityEvaluator.php create mode 100644 database/migrations/2026_03_11_120000_add_lastdownload_to_users_table.php create mode 100644 tests/Feature/ReleaseReportControllerTest.php create mode 100644 tests/Unit/Services/UserInactivityEvaluatorTest.php diff --git a/app/Jobs/RemoveInactiveAccounts.php b/app/Jobs/RemoveInactiveAccounts.php index f3a1181ad..c7b001454 100644 --- a/app/Jobs/RemoveInactiveAccounts.php +++ b/app/Jobs/RemoveInactiveAccounts.php @@ -4,7 +4,9 @@ declare(strict_types=1); namespace App\Jobs; +use App\Enums\UserRole; use App\Models\User; +use App\Services\UserInactivityEvaluator; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; @@ -32,20 +34,25 @@ class RemoveInactiveAccounts implements ShouldQueue { $purgeDays = (int) config('nntmux.purge_inactive_users_days'); $threshold = now()->subDays($purgeDays); + $evaluator = new UserInactivityEvaluator; - User::query()->where('roles_id', 1)->where(function ($q) use ($threshold) { - $q->where(function ($qq) use ($threshold) { - $qq->whereNotNull('lastlogin')->where('lastlogin', '<', $threshold); - })->orWhere(function ($qq) use ($threshold) { - // Only treat null lastlogin as inactive if the account is older than threshold - $qq->whereNull('lastlogin')->where('created_at', '<', $threshold); + User::query() + ->where('roles_id', UserRole::USER->value) + ->where('created_at', '<', $threshold) + ->chunkById(100, static function ($users) use ($evaluator, $threshold): void { + $users + ->filter( + static fn (User $user): bool => $evaluator->shouldPurge( + createdAt: $user->created_at, + updatedAt: $user->updated_at, + lastLoginAt: $user->lastlogin, + apiAccessAt: $user->apiaccess, + lastDownloadAt: $user->lastdownload, + grabs: $user->grabs, + threshold: $threshold, + ) + ) + ->each(static fn (User $user) => $user->delete()); }); - })->where(function ($q) use ($threshold) { - $q->where(function ($qq) use ($threshold) { - $qq->whereNotNull('apiaccess')->where('apiaccess', '<', $threshold); - })->orWhere(function ($qq) use ($threshold) { - $qq->whereNull('apiaccess')->where('created_at', '<', $threshold); - }); - }); } } diff --git a/app/Models/User.php b/app/Models/User.php index 71a0fcd26..e0256b4c4 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -52,6 +52,7 @@ use Spatie\Permission\Traits\HasRoles; * @property string|null $resetguid * @property Carbon|null $lastlogin * @property Carbon|null $apiaccess + * @property Carbon|null $lastdownload * @property int $invites * @property int|null $invitedby * @property bool $movieview @@ -166,6 +167,7 @@ final class User extends Authenticatable 'email_verified_at' => 'datetime', 'lastlogin' => 'datetime', 'apiaccess' => 'datetime', + 'lastdownload' => 'datetime', 'rolechangedate' => 'datetime', 'pending_role_start_date' => 'datetime', 'created_at' => 'datetime', @@ -1411,7 +1413,9 @@ final class User extends Authenticatable */ public static function incrementGrabs(int $id, int $num = 1): void { - static::find($id)?->increment('grabs', $num); + static::query() + ->whereKey($id) + ->increment('grabs', $num, ['lastdownload' => now()]); } // ===== Validation Methods ===== diff --git a/app/Services/UserInactivityEvaluator.php b/app/Services/UserInactivityEvaluator.php new file mode 100644 index 000000000..8b3ae6079 --- /dev/null +++ b/app/Services/UserInactivityEvaluator.php @@ -0,0 +1,52 @@ +isActivityStale($lastLoginAt, $createdAt, $threshold) + && $this->isActivityStale($apiAccessAt, $createdAt, $threshold) + && $this->isDownloadActivityStale($createdAt, $updatedAt, $lastDownloadAt, $grabs, $threshold); + } + + private function isActivityStale( + ?CarbonInterface $activityAt, + ?CarbonInterface $fallbackAt, + CarbonInterface $threshold + ): bool { + $comparisonDate = $activityAt ?? $fallbackAt; + + return $comparisonDate !== null && $comparisonDate->lt($threshold); + } + + private function isDownloadActivityStale( + ?CarbonInterface $createdAt, + ?CarbonInterface $updatedAt, + ?CarbonInterface $lastDownloadAt, + int $grabs, + CarbonInterface $threshold + ): bool { + if ($lastDownloadAt !== null) { + return $lastDownloadAt->lt($threshold); + } + + if ($grabs > 0) { + return $updatedAt !== null && $updatedAt->lt($threshold); + } + + return $createdAt !== null && $createdAt->lt($threshold); + } +} diff --git a/database/migrations/2026_03_11_120000_add_lastdownload_to_users_table.php b/database/migrations/2026_03_11_120000_add_lastdownload_to_users_table.php new file mode 100644 index 000000000..8278ac1e5 --- /dev/null +++ b/database/migrations/2026_03_11_120000_add_lastdownload_to_users_table.php @@ -0,0 +1,28 @@ +dateTime('lastdownload')->nullable()->after('apiaccess'); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('users', function (Blueprint $table) { + $table->dropColumn('lastdownload'); + }); + } +}; diff --git a/tests/Feature/ReleaseReportControllerTest.php b/tests/Feature/ReleaseReportControllerTest.php new file mode 100644 index 000000000..86a7a1882 --- /dev/null +++ b/tests/Feature/ReleaseReportControllerTest.php @@ -0,0 +1,113 @@ +withoutMiddleware(); + Cache::flush(); + + Schema::dropIfExists('settings'); + Schema::dropIfExists('release_reports'); + Schema::dropIfExists('releases'); + + Schema::create('settings', function (Blueprint $table): void { + $table->string('name')->primary(); + $table->text('value')->nullable(); + }); + + Schema::create('releases', function (Blueprint $table): void { + $table->increments('id'); + }); + + Schema::create('release_reports', function (Blueprint $table): void { + $table->increments('id'); + $table->unsignedInteger('releases_id'); + $table->unsignedInteger('users_id'); + $table->string('reason', 255); + $table->text('description')->nullable(); + $table->string('status', 255); + $table->unsignedInteger('reviewed_by')->nullable(); + $table->timestamp('reviewed_at')->nullable(); + $table->timestamps(); + }); + } + + public function test_store_accepts_a_numeric_string_release_id(): void + { + $this->mockAuthenticatedUserId(); + $releaseId = $this->createRelease(); + + $response = $this->postJson(route('release-report.store'), [ + 'release_id' => (string) $releaseId, + 'reason' => 'spam', + 'description' => 'Runtime regression coverage for numeric string ids.', + ]); + + $response + ->assertOk() + ->assertJson([ + 'success' => true, + ]); + + $this->assertDatabaseHas('release_reports', [ + 'releases_id' => $releaseId, + 'users_id' => 1, + 'reason' => 'spam', + 'status' => 'pending', + ]); + } + + public function test_check_reported_accepts_a_numeric_string_release_id(): void + { + $this->mockAuthenticatedUserId(); + $releaseId = $this->createRelease(); + + ReleaseReport::query()->create([ + 'releases_id' => $releaseId, + 'users_id' => 1, + 'reason' => 'duplicate', + 'description' => null, + 'status' => 'pending', + ]); + + $response = $this->getJson(route('release-report.check', [ + 'release_id' => (string) $releaseId, + ])); + + $response + ->assertOk() + ->assertJson([ + 'has_reported' => true, + ]); + } + + private function mockAuthenticatedUserId(int $userId = 1): void + { + Auth::shouldReceive('check')->andReturn(false); + Auth::shouldReceive('id')->andReturn($userId); + } + + private function createRelease(int $id = 1): int + { + DB::table('releases')->insert([ + 'id' => $id, + ]); + + return $id; + } +} diff --git a/tests/Unit/Services/UserInactivityEvaluatorTest.php b/tests/Unit/Services/UserInactivityEvaluatorTest.php new file mode 100644 index 000000000..06d5950e2 --- /dev/null +++ b/tests/Unit/Services/UserInactivityEvaluatorTest.php @@ -0,0 +1,65 @@ +subDays(180); + $staleDate = $threshold->copy()->subDays(181); + + $this->assertTrue($evaluator->shouldPurge( + createdAt: $staleDate, + updatedAt: $staleDate, + lastLoginAt: null, + apiAccessAt: null, + lastDownloadAt: null, + grabs: 0, + threshold: $threshold, + )); + } + + public function test_it_keeps_users_with_recent_download_activity(): void + { + $evaluator = new UserInactivityEvaluator; + $now = Carbon::parse('2026-03-11 12:00:00'); + $threshold = $now->copy()->subDays(180); + $staleDate = $threshold->copy()->subDays(181); + + $this->assertFalse($evaluator->shouldPurge( + createdAt: $staleDate, + updatedAt: $now->copy()->subHours(2), + lastLoginAt: $staleDate, + apiAccessAt: $staleDate, + lastDownloadAt: $now->copy()->subHours(2), + grabs: 12, + threshold: $threshold, + )); + } + + public function test_it_keeps_legacy_downloaders_while_lastdownload_is_still_null(): void + { + $evaluator = new UserInactivityEvaluator; + $now = Carbon::parse('2026-03-11 12:00:00'); + $threshold = $now->copy()->subDays(180); + $staleDate = $threshold->copy()->subDays(181); + + $this->assertFalse($evaluator->shouldPurge( + createdAt: $staleDate, + updatedAt: $now->copy()->subHours(6), + lastLoginAt: $staleDate, + apiAccessAt: $staleDate, + lastDownloadAt: null, + grabs: 15, + threshold: $threshold, + )); + } +}