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.

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

GH-835 Add permission-based access to warps #856

Merged
merged 24 commits into from
Jan 12, 2025
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fca57f8
Add possibility to manage warp permissions
eripe14 Oct 1, 2024
65e7564
Change warp permission feature
eripe14 Oct 8, 2024
374fc89
Add config description
eripe14 Oct 8, 2024
f1a6ada
Resolve CitralFlo suggestion
eripe14 Oct 8, 2024
7eb3bfc
Merge branch 'master' into add-possibility-to-set-warp-permissions
vLuckyyy Jan 10, 2025
1dbc78f
Merge branch 'master' into add-possibility-to-set-warp-permissions
vLuckyyy Jan 12, 2025
2e80db7
Refactor and enhance warp permission handling
vLuckyyy Jan 12, 2025
0aaf226
Update eternalcore-core/src/main/java/com/eternalcode/core/translatio…
vLuckyyy Jan 12, 2025
bf890c7
Update eternalcore-core/src/main/java/com/eternalcode/core/translatio…
vLuckyyy Jan 12, 2025
d11676f
Update eternalcore-core/src/main/java/com/eternalcode/core/translatio…
vLuckyyy Jan 12, 2025
2804b53
Update eternalcore-core/src/main/java/com/eternalcode/core/feature/wa…
vLuckyyy Jan 12, 2025
a8ac2e2
Update eternalcore-core/src/main/java/com/eternalcode/core/feature/wa…
vLuckyyy Jan 12, 2025
708224f
Update eternalcore-core/src/main/java/com/eternalcode/core/feature/wa…
vLuckyyy Jan 12, 2025
2a67403
Fix repository name, check if permission already exist.
vLuckyyy Jan 12, 2025
95f8ed3
Follow mr. @Rollczi review.
vLuckyyy Jan 12, 2025
60b4c11
Follow mr. @Rollczi review. v2
vLuckyyy Jan 12, 2025
f86cc47
Fix synchronized
Rollczi Jan 12, 2025
a84633d
Merge remote-tracking branch 'origin/add-possibility-to-set-warp-perm…
Rollczi Jan 12, 2025
e68afcc
Fix merge
Rollczi Jan 12, 2025
a67693c
Fix events life cycle
Rollczi Jan 12, 2025
c9d7daf
Fix gui permissions
Rollczi Jan 12, 2025
c6115be
Fix cdn
Rollczi Jan 12, 2025
cec9d90
Fix cdn
Rollczi Jan 12, 2025
72feb45
Fix cdn.
vLuckyyy Jan 12, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,26 @@

import org.bukkit.Location;

import java.util.List;
import org.bukkit.permissions.Permissible;

public interface Warp {

Location getLocation();

String getName();

List<String> getPermissions();
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider returning an unmodifiable list of permissions

To prevent accidental modifications, wrap the permissions list with Collections.unmodifiableList().


default boolean hasPermissions(Permissible permissible) {
List<String> permissions = this.getPermissions();
if (permissions.isEmpty()) {
return true;
}

return permissions
.stream()
.anyMatch(permission -> permissible.hasPermission(permission));
}
Comment on lines +16 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider using anyMatch directly instead of stream

The current implementation works well, but for better performance you could use:

-        return permissions
-            .stream()
-            .anyMatch(permission -> permissible.hasPermission(permission));
+        return permissions.stream().anyMatch(permissible::hasPermission);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
default boolean hasPermissions(Permissible permissible) {
List<String> permissions = this.getPermissions();
if (permissions.isEmpty()) {
return true;
}
return permissions
.stream()
.anyMatch(permission -> permissible.hasPermission(permission));
}
default boolean hasPermissions(Permissible permissible) {
List<String> permissions = this.getPermissions();
if (permissions.isEmpty()) {
return true;
}
return permissions.stream().anyMatch(permissible::hasPermission);
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,23 @@

import java.util.Collection;
import java.util.Optional;
import org.jetbrains.annotations.ApiStatus.Experimental;

public interface WarpService {

Warp createWarp(String name, Location location);

void removeWarp(String warp);

boolean warpExists(String name);
@Experimental
Warp addPermissions(String warp, String... permissions);

Optional<Warp> findWarp(String name);
@Experimental
Warp removePermissions(String warp, String... permissions);

vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved
boolean isExist(String name);
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider renaming isExist to exists for clarity.

This name is more conventional and readable.


Collection<String> getNamesOfWarps();
Optional<Warp> findWarp(String name);

boolean hasWarps();
Collection<Warp> getWarps();
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package com.eternalcode.core.feature.warp.event;

import com.eternalcode.core.feature.warp.Warp;
import com.google.common.base.Preconditions;
import java.time.Duration;
import org.bukkit.Location;
import org.bukkit.entity.Player;
import org.bukkit.event.Cancellable;
import org.bukkit.event.Event;
import org.bukkit.event.HandlerList;
import org.jetbrains.annotations.Nullable;

/**
* Called before teleportation to warp.
Expand All @@ -16,12 +20,15 @@ public class PreWarpTeleportEvent extends Event implements Cancellable {
private final Player player;
private Warp warp;
private boolean cancelled;
private Duration teleportTime;
private @Nullable Location destination;
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider making teleportTime final.

Since teleportTime is a required parameter in the constructor and represents a core property, consider making it final for better immutability.

-    private Duration teleportTime;
+    private final Duration teleportTime;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private Duration teleportTime;
private @Nullable Location destination;
private final Duration teleportTime;
private @Nullable Location destination;


public PreWarpTeleportEvent(Player player, Warp warp) {
public PreWarpTeleportEvent(Player player, Warp warp, Duration teleportTime) {
super(false);

this.player = player;
this.warp = warp;
this.teleportTime = teleportTime;
}

public Player getPlayer() {
Expand All @@ -33,9 +40,28 @@ public Warp getWarp() {
}

public void setWarp(Warp warp) {
Preconditions.checkNotNull(warp, "Warp cannot be null");
this.warp = warp;
}

public Duration getTeleportTime() {
return this.teleportTime;
}

public void setTeleportTime(Duration teleportTime) {
Preconditions.checkNotNull(teleportTime, "Teleport time cannot be null");
this.teleportTime = teleportTime;
}
Comment on lines +47 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider removing setTeleportTime if field becomes final.

If you make teleportTime final as suggested earlier, this setter would need to be removed. The getter can stay.


public Location getDestination() {
return this.destination != null ? this.destination : this.warp.getLocation();
}

public void setDestination(Location destination) {
Preconditions.checkNotNull(destination, "Destination cannot be null");
this.destination = destination;
}
Comment on lines +56 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Smart fallback in getDestination.

Good implementation of the destination getter with fallback to warp location. The null check in the setter is also appropriate.

However, consider documenting the fallback behavior in a Javadoc comment:

/**
 * Gets the destination location for this teleport.
 * @return the custom destination if set, otherwise falls back to the warp's location
 */


@Override
public boolean isCancelled() {
return this.cancelled;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.eternalcode.core.feature.warp.event;

import com.eternalcode.core.feature.warp.Warp;
import org.bukkit.Location;
import org.bukkit.entity.Player;
import org.bukkit.event.Cancellable;
import org.bukkit.event.Event;
import org.bukkit.event.HandlerList;

Expand All @@ -15,12 +15,14 @@ public class WarpTeleportEvent extends Event {

private final Player player;
private final Warp warp;
private final Location destination;

public WarpTeleportEvent(Player player, Warp warp) {
public WarpTeleportEvent(Player player, Warp warp, Location destination) {
super(false);

this.player = player;
this.warp = warp;
this.destination = destination;
}

public Player getPlayer() {
Expand All @@ -31,6 +33,10 @@ public Warp getWarp() {
return this.warp;
}

public Location getDestination() {
return this.destination;
}

@Override
public HandlerList getHandlers() {
return HANDLER_LIST;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.eternalcode.core.configuration.implementation;

import com.eternalcode.commons.bukkit.position.Position;
import com.eternalcode.core.configuration.ReloadableConfig;
import com.eternalcode.core.injector.annotations.component.ConfigurationFile;
import com.eternalcode.commons.bukkit.position.Position;
import net.dzikoysk.cdn.entity.Description;
import net.dzikoysk.cdn.entity.Exclude;
import net.dzikoysk.cdn.source.Resource;
Expand All @@ -21,7 +21,8 @@ public class LocationsConfiguration implements ReloadableConfig {
@Description("# This is spawn location, for your own safety, please don't touch it.")
public Position spawn = EMPTY_POSITION;

@Description("# These are warp locations, for your own safety, please don't touch it.")
@Description("# Warps now are stored in warps.yml. This is deprecated.")
@Deprecated
vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved
public Map<String, Position> warps = new HashMap<>();
vLuckyyy marked this conversation as resolved.
Show resolved Hide resolved

@Description("# This is jail location, for your own safety, please don't touch it.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@
import com.eternalcode.core.feature.afk.AfkSettings;
import com.eternalcode.core.feature.automessage.AutoMessageSettings;
import com.eternalcode.core.feature.chat.ChatSettings;
import com.eternalcode.core.feature.jail.JailSettings;
import com.eternalcode.core.feature.helpop.HelpOpSettings;
import com.eternalcode.core.feature.jail.JailSettings;
import com.eternalcode.core.feature.randomteleport.RandomTeleportSettingsImpl;
import com.eternalcode.core.feature.spawn.SpawnSettings;
import com.eternalcode.core.injector.annotations.Bean;
import com.eternalcode.core.injector.annotations.component.ConfigurationFile;
import com.eternalcode.core.feature.teleportrequest.TeleportRequestSettings;
import java.util.LinkedHashMap;
import java.util.Set;
import net.dzikoysk.cdn.entity.Contextual;
import net.dzikoysk.cdn.entity.Description;
import net.dzikoysk.cdn.entity.Exclude;
Expand All @@ -24,7 +22,9 @@

import java.io.File;
import java.time.Duration;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Set;

@ConfigurationFile
public class PluginConfiguration implements ReloadableConfig {
Expand Down Expand Up @@ -352,7 +352,6 @@ public static class Warp {

@Description("# Texture of the item (only for PLAYER_HEAD material)")
public String itemTexture = "eyJ0ZXh0dXJlcyI6eyJTS0lOIjp7InVybCI6Imh0dHA6Ly90ZXh0dXJlcy5taW5lY3JhZnQubmV0L3RleHR1cmUvNzk4ODVlODIzZmYxNTkyNjdjYmU4MDkwOTNlMzNhNDc2ZTI3NDliNjU5OGNhNGEyYTgxZWU2OTczODAzZmI2NiJ9fX0=";

}

@Description({ " ", "# Butcher" })
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,22 @@

import com.eternalcode.commons.bukkit.position.Position;
import com.eternalcode.commons.bukkit.position.PositionAdapter;
import java.util.ArrayList;
import org.bukkit.Location;

class WarpImpl implements Warp {
import java.util.Collections;
import java.util.List;

public class WarpImpl implements Warp {

private final String name;
private final Position position;
private final List<String> permissions;

WarpImpl(String name, Position position) {
public WarpImpl(String name, Position position, List<String> permissions) {
this.name = name;
this.position = position;
this.permissions = new ArrayList<>(permissions);
Comment on lines +17 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add a null check for permissions in the constructor.

This helps prevent potential NullPointerException.

}

@Override
Expand All @@ -23,4 +29,10 @@ public String getName() {
public Location getLocation() {
return PositionAdapter.convert(this.position);
}

@Override
public List<String> getPermissions() {
return Collections.unmodifiableList(this.permissions);
}

}
Loading
Loading