Skip to content
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

Fix & Enhance Whitelist #5422

Merged
merged 22 commits into from
May 8, 2024
Merged

Fix & Enhance Whitelist #5422

merged 22 commits into from
May 8, 2024

Conversation

NotSoDelayed
Copy link
Contributor

@NotSoDelayed NotSoDelayed commented Feb 2, 2023 •
edited by AyhamAl-Ali
Loading

Description

This PR rewrites partial of whitelist related code in Skript, which fixes:

  • CondIsWhitelisted returns incorrect value when used on player lists
  • CondIsWhitelisted returns inverted results of whitelist state

The following were added/enhanced:

  • Changed type from Player to OfflinePlayer, which the method originally supports
  • Added whitelist enforcement support

Target Minecraft Versions: Minecraft 1.17+ (for whitelist enforcement)
Requirements: none
Related Issues: #6300

@AyhamAl-Ali AyhamAl-Ali added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. labels Feb 2, 2023
APickledWalrus
APickledWalrus requested changes Feb 2, 2023
View reviewed changes
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprWhitelist.java Outdated Show resolved Hide resolved
@NotSoDelayed
Copy link
Contributor Author

I forgot to mention:
When after modifying whitelisted players, usually via vanilla commands the non-whitelisted will be kicked immediately, but after my tests with Bukkit method, it does not do that. Hence my reloadWhitelist() method was added as a workaround.

AyhamAl-Ali
AyhamAl-Ali requested changes Feb 2, 2023
View reviewed changes
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Nice PR ⚡

src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprWhitelist.java Outdated Show resolved Hide resolved
@AyhamAl-Ali
Copy link
Member

AyhamAl-Ali commented Feb 2, 2023 •
edited
Loading

I forgot to mention:
When after modifying whitelisted players, usually via vanilla commands the non-whitelisted will be kicked immediately, but after my tests with Bukkit method, it does not do that. Hence my reloadWhitelist() method was added as a workaround.

If without reloadWhitelist() it still updates the list so that if they rejoin they can't then we maybe can add to the syntax something like without kicking players which gives the user the ability to choose

@NotSoDelayed
Copy link
Contributor Author

If without reloadWhitelist() it still updates the list so that if they rejoin they can't then we maybe can add to the syntax something like without kicking players which gives the user the ability to choose

Hm? Enforcing the whitelist will kick all non-whitelisted in the server. You sure adding an option to not kick is valid here?

@AyhamAl-Ali
Copy link
Member

Hm? Enforcing the whitelist will kick all non-whitelisted in the server. You sure adding an option to not kick is valid here?

I've the got enforcement option wrong then, nvm me :)

@NotSoDelayed NotSoDelayed requested review from AyhamAl-Ali and removed request for APickledWalrus February 11, 2023 13:34
@NotSoDelayed NotSoDelayed requested review from UnderscoreTud and removed request for APickledWalrus February 19, 2023 05:37
@NotSoDelayed NotSoDelayed requested review from APickledWalrus and removed request for UnderscoreTud February 26, 2023 20:07
@Moderocky Moderocky force-pushed the master branch 2 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 10:12
@Moderocky Moderocky added patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. and removed patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. labels Sep 18, 2023
@sovdeeth sovdeeth added the 2.8 Targeting a 2.8.X version release label Dec 30, 2023
sovdeeth
sovdeeth requested changes Dec 30, 2023
View reviewed changes
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

looks great otherwise!

src/main/java/ch/njol/skript/conditions/CondIsWhitelisted.java Outdated Show resolved Hide resolved
@sovdeeth sovdeeth removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Dec 31, 2023
@NotSoDelayed
Copy link
Contributor Author

Tests will be added for this PR today and only then I’ll request review from the reviewers who have yet to approve the change.

APickledWalrus
APickledWalrus approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

a few minor suggestions

src/test/skript/tests/syntaxes/expressions/ExprWhitelist.sk Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprWhitelist.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffEnforceWhitelist.java Show resolved Hide resolved
@sovdeeth sovdeeth added feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. 2.9 Targeting a 2.9.X version release and removed 2.8 Targeting a 2.8.X version release labels Mar 19, 2024
@NotSoDelayed NotSoDelayed mentioned this pull request Apr 23, 2024
Closed
@sovdeeth sovdeeth merged commit 96ea3d5 into SkriptLang:dev/feature May 8, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AyhamAl-Ali AyhamAl-Ali AyhamAl-Ali approved these changes

@APickledWalrus APickledWalrus APickledWalrus approved these changes

@TheLimeGlass TheLimeGlass TheLimeGlass approved these changes

@sovdeeth sovdeeth sovdeeth approved these changes

@UnderscoreTud UnderscoreTud Awaiting requested review from UnderscoreTud

Assignees
No one assigned
Labels
2.9 Targeting a 2.9.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@NotSoDelayed @AyhamAl-Ali @sovdeeth @TheLimeGlass @APickledWalrus @UnderscoreTud @Moderocky