GH-971 Migrate jail feature from Position to Location with LocationPersister. Rename config section. by igoyek · Pull Request #1141 · EternalCodeTeam/EternalCore

igoyek · 2025-08-21T09:06:28Z

The entire prison system has been improved and fixes from #971 have been applied.
All game changes and migrations have been tested.

Introduced JailCommandRestrictionType enum to support both whitelist and blacklist command restrictions for jailed players. Updated JailConfig and JailSettings to use 'restrictedCommands' and 'restrictionType' instead of 'allowedCommands'. Modified JailController logic to handle both restriction types. Took 12 minutes

Added a record of the player's previous location. Configuration values and messages have been migrated. Added constant permission values and used static imports for better appearance. Other cosmetic or quality improvements have been made. Took 49 minutes

gemini-code-assist

Code Review

This pull request refactors the jail system, introducing features like storing a player's last location before being jailed and a more flexible command restriction system (whitelist/blacklist). The refactoring also includes centralizing permission constants and cleaning up configuration keys and message structures. My review has identified a couple of issues: a critical bug where a new configuration migration is not registered, which could lead to data loss for users, and a logical flaw in the releaseAllPlayers method where event cancellations are not fully respected. I've also suggested a minor improvement to a configuration comment for better clarity. Overall, this is a solid refactoring with significant improvements, and addressing these points will make it even better.

eternalcore-core/src/main/java/com/eternalcode/core/configuration/migrations/Migrations.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailServiceImpl.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailConfig.java

P1otrulla

Good job, follow gemini suggestions 🙌

P1otrulla · 2025-08-21T09:18:00Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/jail/JailedPlayer.java

    private final Duration prisonTime;
    private final String detainedBy;
+    @Nullable
+    private final Location lastLocation;


Location -> Position, We don't keep the location because it creates memory leaks and has a lot of world dependencies

Ok but I think I will be obliged to use another serializer for Position class in database?

Jakubk15

Other than that, please apply Gemini's suggestions. It gave valuable feedback

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/messages/JailMessages.java

Refactored JailServiceImpl.releaseAllPlayers to avoid clearing all jailed players and prisoners at once, now only releasing non-cancelled events and handling null players. Updated JailConfig comments for clarity and registered new migration for jail section in Migrations. Took 8 minutes

Took 15 minutes

releasePrivate -> released

CitralFlo

Nice

Replaced custom Position class with Bukkit's Location for storing and handling player locations in jail-related classes. Updated constructors, field names, and database persister to reflect this change, improving compatibility and simplifying location management. Took 36 minutes

vLuckyyy · 2025-08-21T22:19:50Z

eternalcore-api/build.gradle.kts


 dependencies {
    compileOnly("org.spigotmc:spigot-api:${Versions.SPIGOT_API}")
+    compileOnly("com.eternalcode:eternalcode-commons-bukkit:${Versions.ETERNALCODE_COMMONS}")


Position usage in API

trochę słabo będzie wystawiać API które jest relokowane

...eternalcode/core/configuration/migrations/Migration_0010_Move_jail_to_dedicated_section.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailPermissionConstant.java

Took 58 minutes

Deleted PositionArgument class and removed references to invalidPosition messages in ArgumentMessages, ENArgumentMessages, and PLArgumentMessages. Updated HomeAdminCommand to use Location instead of Position for home setting, simplifying argument handling. Took 25 minutes

When setting a home for a user, the location now explicitly uses the sender's world, yaw, and pitch to avoid inconsistencies if a custom location is provided without these values. Took 8 minutes

Renamed migration class and references from 'Move_jail_to_dedicated_section' to 'Rename_jail_section' for clarity. Updated PositionPersister to use Position's toString method and added type safety. Minor import and formatting adjustments in JailedPlayer and PrisonerTable. Took 35 minutes

Took 2 minutes

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/HomeCreateEvent.java

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/HomeOverrideEvent.java

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/PreHomeTeleportEvent.java

eternalcore-api/src/main/java/com/eternalcode/core/feature/jail/JailedPlayer.java

Rollczi

Zmiany w home do wywalenia, migracje były testowane? I czy ten persister do DB był testowany? I czy API z jakrą było testowane? Nie mówię, że nie było albo że trzeba wszystko testować ale widzę parę miejsc które słabo rokują

szczególnie te zmiany w wystawianiu libki jako API #1141 (comment)

...core/configuration/migrations/Migration_0009_Rename_allowed_to_restricted_jail_commands.java

eternalcore-core/src/main/java/com/eternalcode/core/database/persister/PositionPersister.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomeImpl.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomeManager.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/SetHomeCommand.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/database/HomeTable.java

...nalcore-core/src/main/java/com/eternalcode/core/feature/home/homeadmin/HomeAdminCommand.java

Took 38 minutes

# Conflicts: # eternalcore-core/src/main/java/com/eternalcode/core/configuration/migrations/Migrations.java # eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java # eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java

Rollczi · 2025-11-30T22:43:34Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/jail/JailedPlayer.java

+     * Gets the player's UUID.
+     * @return player's UUID.
+     */
+    UUID playerUniqueId();


raczej nie w konwencji naszej i po co rozwalać API żeby usuwać get

Rollczi · 2025-11-30T22:53:44Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailServiceImpl.java


    @Override
    public boolean releasePlayer(Player player) {
+        if (player == null) {


trochę przesada z tymi niektórymi nullcheckami

Rollczi · 2025-11-30T22:55:36Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailServiceImpl.java

+        JailedPlayer jailedPlayer = this.jailedPlayers.get(player.getUniqueId());
+
        this.prisonerRepository.deletePrisoner(player.getUniqueId());
        this.jailedPlayers.remove(player.getUniqueId());


JailedPlayer jailedPlayer = this.jailedPlayers.remove(player.getUniqueId()); this.prisonerRepository.deletePrisoner(player.getUniqueId());

:P

Rollczi · 2025-11-30T22:57:57Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailServiceImpl.java

-        if (jailedPlayer == null || jailedPlayer.isPrisonExpired()) {
-            return false;
+        if (player == null) {
+            throw new IllegalArgumentException("Player UUID cannot be null");


Rollczi · 2025-11-30T22:58:17Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailServiceImpl.java

    @Blocking
    public void setupJailArea(Location jailLocation) {
+        if (jailLocation == null) {
+            throw new IllegalArgumentException("Jail location cannot be null");


igoyek added 2 commits August 21, 2025 09:31

igoyek requested review from CitralFlo, Jakubk15, P1otrulla and vLuckyyy August 21, 2025 09:06

igoyek self-assigned this Aug 21, 2025

igoyek requested a review from a team as a code owner August 21, 2025 09:06

igoyek added this to EternalCore Aug 21, 2025

igoyek added 🆕 feature New feature or request 🔧 enhancement Enhances existing function labels Aug 21, 2025

github-project-automation bot moved this to 📋 Backlog in EternalCore Aug 21, 2025

igoyek moved this from 📋 Backlog to 👀 In review in EternalCore Aug 21, 2025

igoyek linked an issue Aug 21, 2025 that may be closed by this pull request

Refactor jail system #971

Open

4 tasks

gemini-code-assist bot reviewed Aug 21, 2025

View reviewed changes

P1otrulla approved these changes Aug 21, 2025

View reviewed changes

Jakubk15 reviewed Aug 21, 2025

View reviewed changes

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/messages/JailMessages.java Outdated Show resolved Hide resolved

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/messages/JailMessages.java Outdated Show resolved Hide resolved

igoyek and others added 3 commits August 21, 2025 13:50

Need tests

5bf5d52

Took 15 minutes

detainPrivate -> detained

526e344

releasePrivate -> released

CitralFlo approved these changes Aug 21, 2025

View reviewed changes

igoyek requested a review from Jakubk15 August 21, 2025 20:57

vLuckyyy approved these changes Aug 21, 2025

View reviewed changes

igoyek added 3 commits August 22, 2025 14:05

Location -> Position refactor

ef0a5cb

Took 58 minutes

Ensure home location uses sender's world and orientation

d640f4c

When setting a home for a user, the location now explicitly uses the sender's world, yaw, and pitch to avoid inconsistencies if a custom location is provided without these values. Took 8 minutes

P1otrulla self-requested a review August 22, 2025 13:07

igoyek added 2 commits August 22, 2025 15:20

Remove unused import

6c38f6f

Took 2 minutes

igoyek assigned P1otrulla Aug 22, 2025

Jakubk15 approved these changes Aug 23, 2025

View reviewed changes

P1otrulla removed their request for review August 23, 2025 22:22

P1otrulla removed their assignment Aug 23, 2025

Rollczi reviewed Aug 26, 2025

View reviewed changes

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/HomeCreateEvent.java Outdated Show resolved Hide resolved

Rollczi reviewed Aug 26, 2025

View reviewed changes

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/HomeCreateEvent.java Outdated Show resolved Hide resolved

Rollczi reviewed Aug 26, 2025

View reviewed changes

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/HomeOverrideEvent.java Outdated Show resolved Hide resolved

Rollczi reviewed Aug 26, 2025

View reviewed changes

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/PreHomeTeleportEvent.java Outdated Show resolved Hide resolved

Rollczi reviewed Aug 26, 2025

View reviewed changes

eternalcore-api/src/main/java/com/eternalcode/core/feature/jail/JailedPlayer.java Outdated Show resolved Hide resolved

Rollczi requested changes Aug 26, 2025

View reviewed changes

github-project-automation bot moved this from 👀 In review to 🏗 In progress in EternalCore Aug 26, 2025

igoyek added 2 commits October 8, 2025 14:17

Follow instructions

c2768cc

Took 38 minutes

Merge branch 'master' into jail-system-refactor

5a76360

igoyek requested a review from Rollczi October 8, 2025 12:19

vLuckyyy added 5 commits October 18, 2025 15:33

Merge.

b88917d

Remove useless migration.

5da106e

Revert not-related to issue changes.

feb4c9e

Review.

e31948e

vLuckyyy changed the title ~~GH-971 Jail system refactor~~ GH-971 Migrate jail feature from Position to Location with LocationPersister. Rename config section. Oct 18, 2025

vLuckyyy approved these changes Oct 18, 2025

View reviewed changes

Rollczi reviewed Nov 30, 2025

View reviewed changes

Uh oh!

Conversation

igoyek commented Aug 21, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

P1otrulla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jakubk15 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CitralFlo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rollczi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Rollczi left a comment •

edited

Loading