Skip to content

GH-971 Migrate jail feature from Position to Location with LocationPersister. Rename config section. #1141

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

GH-971 Migrate jail feature from Position to Location with LocationPersister. Rename config section. #1141

wants to merge 18 commits into from

Conversation

@igoyek
Copy link
Contributor

@igoyek igoyek commented Aug 21, 2025

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

igoyek added 2 commits August 21, 2025 09:31
@igoyek
Add jail command restriction type and refactor config
4a1734c 
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
@igoyek
The prison system has been improved
ad0b8c2 
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
@igoyek igoyek self-assigned this Aug 21, 2025
@igoyek igoyek requested a review from a team as a code owner August 21, 2025 09:06
@igoyek igoyek added 🆕 feature New feature or request 🔧 enhancement Enhances existing function labels Aug 21, 2025
@github-project-automation github-project-automation bot moved this to 📋 Backlog in EternalCore Aug 21, 2025
@igoyek igoyek moved this from 📋 Backlog to 👀 In review in EternalCore Aug 21, 2025
@igoyek 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]
gemini-code-assist bot reviewed Aug 21, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

P1otrulla
P1otrulla approved these changes Aug 21, 2025
View reviewed changes
Copy link
Member

@P1otrulla P1otrulla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, follow gemini suggestions 🙌

eternalcore-api/src/main/java/com/eternalcode/core/feature/jail/JailedPlayer.java Outdated
private final Duration prisonTime;
private final String detainedBy;
@Nullable
private final Location lastLocation;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Jakubk15
Jakubk15 reviewed Aug 21, 2025
View reviewed changes
Copy link
Member

@Jakubk15 Jakubk15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

igoyek and others added 3 commits August 21, 2025 13:50
@igoyek
Refactor jail release logic and update config comments
656b7be 
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
@igoyek
Need tests
5bf5d52 
Took 15 minutes
@Jakubk15
detainPrivate -> detained
526e344 
releasePrivate -> released
CitralFlo
CitralFlo approved these changes Aug 21, 2025
View reviewed changes
Copy link
Member

@CitralFlo CitralFlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@igoyek
Refactor jail feature to use Bukkit Location
67c0735 
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
@igoyek igoyek requested a review from Jakubk15 August 21, 2025 20:57
vLuckyyy
vLuckyyy approved these changes Aug 21, 2025
View reviewed changes
eternalcore-api/build.gradle.kts Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Position usage in API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

igoyek added 3 commits August 22, 2025 14:05
@igoyek
Location -> Position refactor
ef0a5cb 
Took 58 minutes
@igoyek
Remove PositionArgument and related messages
c3bb59a 
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
@igoyek
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 P1otrulla self-requested a review August 22, 2025 13:07
igoyek added 2 commits August 22, 2025 15:20
@igoyek
Refactor jail section migration and improve Position handling
e09ca18 
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
@igoyek
Remove unused import
6c38f6f 
Took 2 minutes
@P1otrulla P1otrulla removed their request for review August 23, 2025 22:22
@P1otrulla P1otrulla removed their assignment Aug 23, 2025
Rollczi
Rollczi requested changes Aug 26, 2025
View reviewed changes
Copy link
Member

@Rollczi Rollczi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@github-project-automation github-project-automation bot moved this from 👀 In review to 🏗 In progress in EternalCore Aug 26, 2025
@igoyek igoyek requested a review from Rollczi October 8, 2025 12:19
@vLuckyyy
Merge branch 'master' into jail-system-refactor
9015129 
# 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
@vLuckyyy
Merge.
b88917d
@vLuckyyy
Remove useless migration.
5da106e
@vLuckyyy
Revert not-related to issue changes.
feb4c9e
@vLuckyyy
Review.
e31948e
@vLuckyyy 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vLuckyyy vLuckyyy vLuckyyy approved these changes

@Jakubk15 Jakubk15 Jakubk15 approved these changes

@P1otrulla P1otrulla P1otrulla approved these changes

@CitralFlo CitralFlo CitralFlo approved these changes

@Rollczi Rollczi Awaiting requested review from Rollczi

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

@igoyek igoyek

Labels

🔧 enhancement Enhances existing function 🆕 feature New feature or request

Projects

EternalCore
Status: 🏗 In progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor jail system

6 participants

@igoyek @Rollczi @vLuckyyy @Jakubk15 @P1otrulla @CitralFlo