-
Notifications
You must be signed in to change notification settings - Fork 115
Feature/922 delete inactive players #1490
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| <?php | ||
|
|
||
| namespace OGame\Console\Commands\Scheduler; | ||
|
|
||
| use Illuminate\Console\Attributes\Description; | ||
| use Illuminate\Console\Attributes\Signature; | ||
| use Illuminate\Console\Command; | ||
| use OGame\Factories\PlayerServiceFactory; | ||
| use OGame\Models\User; | ||
| use OGame\Services\SettingsService; | ||
| use Throwable; | ||
|
|
||
| #[Description('Permanently deletes players who have been inactive for the configured number of days.')] | ||
| #[Signature('ogamex:scheduler:delete-inactive-players')] | ||
| class DeleteInactivePlayers extends Command | ||
| { | ||
| /** | ||
| * Execute the console command. | ||
| */ | ||
| public function handle(SettingsService $settings, PlayerServiceFactory $playerServiceFactory): int | ||
| { | ||
| $days = $settings->inactivePlayerDeletionDays(); | ||
|
|
||
| // 0 = feature disabled (this is the default). | ||
| if ($days <= 0) { | ||
| $this->info('Inactive player deletion is disabled (inactive_player_deletion_days = 0).'); | ||
|
|
||
| return Command::SUCCESS; | ||
| } | ||
|
|
||
| $cutoff = now()->subDays($days)->timestamp; | ||
| $deletedCount = 0; | ||
| $failedCount = 0; | ||
|
|
||
| // Admins are excluded to protect the server operator and system accounts, mirroring the | ||
| // existing "administrators cannot be banned" rule. Vacation mode does NOT exempt a player. | ||
| // The users.time column holds the last-activity UNIX timestamp. | ||
| User::withoutRole('admin') | ||
| ->whereNotNull('time') | ||
| ->where('time', '<', $cutoff) | ||
| ->chunkById(200, function ($users) use ($playerServiceFactory, &$deletedCount, &$failedCount): void { | ||
| foreach ($users as $user) { | ||
| try { | ||
| // Load with a fresh cache so the deletion acts on the player's current | ||
| // planets, moons and missions rather than any stale cached state. | ||
| $playerServiceFactory->make($user->id, true)->deleteInactiveAccount(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The player and planet factories are shared singletons with no way to clear them, so everything built for each deleted account stays in memory for the whole run. The batching limits query size but not memory. Likely a non-issue for nightly runs; I'd mainly want to know it's been tried against a large backlog, since that first run could be thousands of accounts. Most of the deployments will have only small groups of people, but something main.ogamex.dev with its many inactive accounts could be seriously in trouble. |
||
| $deletedCount++; | ||
| } catch (Throwable $e) { | ||
| $failedCount++; | ||
| $this->error("Failed to delete inactive player #{$user->id}: " . $e->getMessage()); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same pattern as |
||
| } | ||
| } | ||
| }); | ||
|
|
||
| $this->info("Deleted {$deletedCount} inactive player(s) with no activity in the last {$days} day(s)."); | ||
|
|
||
| if ($failedCount > 0) { | ||
| $this->warn("{$failedCount} player(s) could not be deleted; see errors above."); | ||
| } | ||
|
|
||
| return $failedCount > 0 ? Command::FAILURE : Command::SUCCESS; | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this setting permanently deletes accounts with no undo, could we put a floor on it? Right now 1 is accepted and would wipe everyone inactive for a day on the next run and it's an easy slip. I'd be much more comfortable if there was an dialogue box prompting the user to confirm the setting. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -916,6 +916,56 @@ public function delete(): void | |
| $this->user->delete(); | ||
| } | ||
|
|
||
| /** | ||
| * Permanently delete an inactive player: abandon all of their planets (and moons) and remove | ||
| * the account with all associated records. | ||
| * | ||
| * Unlike delete(), planets are removed through the abandonment flow (PlanetService::abandonPlanet()) | ||
| * so galaxy slots are freed and incoming missions from other players are handled gracefully. The | ||
| * abandonment guards (last remaining planet / active fleet missions) are bypassed because the whole | ||
| * account is being removed. Careful: this action is irreversible! | ||
| * | ||
| * @return void | ||
| */ | ||
| public function deleteInactiveAccount(): void | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This overlaps |
||
| { | ||
| DB::transaction(function () { | ||
| // Delete the player's own fleet missions. Child (return) missions reference their parent | ||
| // via parent_id, so remove those first to satisfy the self-referencing foreign key. | ||
| $missions = FleetMission::where('user_id', $this->getId())->get(); | ||
| foreach ($missions as $mission) { | ||
| FleetMission::where('parent_id', $mission->id)->delete(); | ||
| $mission->delete(); | ||
| } | ||
|
|
||
| // Abandon every planet; each call cascades to its moon. Force bypasses the user-facing | ||
| // guards which do not apply during full account deletion. Abandoning frees the galaxy slot | ||
| // and nulls out any remaining incoming missions from other players. | ||
| foreach ($this->planets->allPlanets() as $planet) { | ||
| $planet->abandonPlanet(true); | ||
| } | ||
|
|
||
| // Delete remaining user-scoped records that no database cascade covers. | ||
| Message::where('user_id', $this->getId())->delete(); | ||
| Highscore::where('player_id', $this->getId())->delete(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The highscores table already deletes these rows automatically when the user goes, so this line does nothing. |
||
| UserTech::where('user_id', $this->getId())->delete(); | ||
|
|
||
| // Remove any lingering login sessions for this user (the sessions table has no foreign key). | ||
| DB::table('sessions')->where('user_id', $this->getId())->delete(); | ||
|
|
||
| // Note: battle and espionage reports are intentionally kept. Their planet_user_id foreign | ||
| // key is ON DELETE SET NULL, so the reports are preserved for the other party involved. | ||
|
|
||
| // Clear the current planet reference before deleting the user (foreign key constraint). | ||
| $this->user->planet_current = null; | ||
| $this->user->save(); | ||
|
|
||
| // Delete the user. Cascade deletes handle notes, buddies, alliance membership, chat | ||
| // messages, dark matter transactions, bans, etc. | ||
| $this->user->delete(); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Get is the player building the object or not | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <?php | ||
|
|
||
| use Illuminate\Database\Migrations\Migration; | ||
| use Illuminate\Support\Facades\DB; | ||
|
|
||
| return new class () extends Migration { | ||
| /** | ||
| * Run the migrations. | ||
| * | ||
| * @return void | ||
| */ | ||
| public function up(): void | ||
| { | ||
| // Seed the inactive player deletion threshold. 0 = feature disabled (default). | ||
| DB::table('settings')->updateOrInsert( | ||
| ['key' => 'inactive_player_deletion_days'], | ||
| ['value' => '0', 'created_at' => now(), 'updated_at' => now()] | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Reverse the migrations. | ||
| * | ||
| * @return void | ||
| */ | ||
| public function down(): void | ||
| { | ||
| DB::table('settings')->where('key', 'inactive_player_deletion_days')->delete(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is more for @lanedirt: Should moderators be excluded alongside admin? It looks like a non-issue today since that role doesn't grant any permissions yet, but if it ever becomes a real staff role those accounts are exactly the infrequent-login users this targets. Cheap to add now: withoutRole(['admin', 'moderator']). There is only a handful of mentions of the role in the codebase and it currently doesn't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Geda173 Yes I agree, since this is being touched already, adding the moderator role exclusion too here is a good idea (even if the role itself is not actually implemented yet as of this moment).