Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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,6 +2,7 @@

import com.eternalcode.core.feature.afk.AfkService;
import com.eternalcode.core.feature.catboy.CatboyService;
import com.eternalcode.core.feature.helpop.HelpOpService;
import com.eternalcode.core.feature.home.HomeService;
import com.eternalcode.core.feature.jail.JailService;
import com.eternalcode.core.feature.msg.MsgService;
Expand All @@ -19,6 +20,8 @@ public interface EternalCoreApi {

IgnoreService getIgnoreService();

HelpOpService getHelpOpService();

HomeService getHomeService();

JailService getJailService();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.eternalcode.core.feature.helpop;

import java.util.UUID;
import org.jetbrains.annotations.NotNull;

/**
* Service responsible for managing helpop functionality.
*/
public interface HelpOpService {

/**
* Marks a player as having sent a helpop request.
*
* <p>This allows administrators to reply to the player's request
* using the helpop reply command.
*
* @param playerUuid the UUID of the player who sent the helpop request
*/
void markSender(@NotNull UUID playerUuid);
Copy link
Contributor

Choose a reason for hiding this comment

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

markHelpOp


/**
* Checks if a player has sent a helpop request.
*
* <p>Helpop requests expire after a configured duration (default: 1 hour).
*
* @param playerUuid the UUID of the player to check
* @return {@code true} if the player has an active helpop request, {@code false} otherwise
*/
boolean hasSentHelpOp(@NotNull UUID playerUuid);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package com.eternalcode.core.feature.helpop.event;

import org.bukkit.entity.Player;
import org.bukkit.event.Cancellable;
import org.bukkit.event.Event;
import org.bukkit.event.HandlerList;
import org.jetbrains.annotations.NotNull;

/**
* Event called when an administrator replies to a player's helpop request.
*
* <p>This event is triggered before the reply message is actually sent to the player.
* Plugins can cancel this event to prevent the reply from being sent, or modify
* the reply content before it's delivered.
*/
public class HelpOpReplyEvent extends Event implements Cancellable {

private static final HandlerList HANDLER_LIST = new HandlerList();

private final Player admin;
private final Player target;
private String content;
private boolean cancelled;

public HelpOpReplyEvent(@NotNull Player admin, @NotNull Player target, @NotNull String content) {
Copy link
Member

Choose a reason for hiding this comment

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

why not the Jspecify?????????????????????

Copy link
Member

Choose a reason for hiding this comment

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

applies for the whole class

super(false);

this.admin = admin;
this.target = target;
this.content = content;
}

/**
* Gets the administrator who is replying.
*
* @return the admin player
*/
@NotNull
public Player getAdmin() {
return this.admin;
}

/**
* Gets the player who originally sent the helpop request.
*
* @return the target player
*/
@NotNull
public Player getTarget() {
return this.target;
}

/**
* Gets the reply message content.
*
* @return the reply content
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return the reply content
* @return the reply content as raw string

I think it would be good to specify this in Javadoc, as the tag-escaping process is done after calling the event: https://github.com/EternalCodeTeam/EternalCore/pull/1296/changes#diff-2c33053246b78aac204d193999e57efd64d38f95eeb1bd6cc571be001f923810R62

*/
@NotNull
public String getContent() {
return this.content;
}

/**
* Sets the reply message content.
*
* @param content the new reply content
*/
public void setContent(@NotNull String content) {
this.content = content;
}

@Override
public boolean isCancelled() {
return this.cancelled;
}

@Override
public void setCancelled(boolean cancel) {
this.cancelled = cancel;
}

@Override
@NotNull
public HandlerList getHandlers() {
return HANDLER_LIST;
}

@NotNull
public static HandlerList getHandlerList() {
return HANDLER_LIST;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.eternalcode.core.feature.afk.AfkService;
import com.eternalcode.core.feature.catboy.CatboyService;
import com.eternalcode.core.feature.helpop.HelpOpService;
import com.eternalcode.core.feature.home.HomeService;
import com.eternalcode.core.feature.ignore.IgnoreService;
import com.eternalcode.core.feature.jail.JailService;
Expand Down Expand Up @@ -35,6 +36,11 @@ public IgnoreService getIgnoreService() {
return this.dependencyProvider.getDependency(IgnoreService.class);
}

@Override
public HelpOpService getHelpOpService() {
return this.dependencyProvider.getDependency(HelpOpService.class);
}

@Override
public HomeService getHomeService() {
return this.dependencyProvider.getDependency(HomeService.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,22 @@ class HelpOpCommand {

private final NoticeService noticeService;
private final HelpOpSettings helpOpSettings;
private final HelpOpService helpOpService;
private final EventCaller eventCaller;
private final Server server;
private final Delay<UUID> delay;

@Inject
HelpOpCommand(NoticeService noticeService, HelpOpSettings helpOpSettings, EventCaller eventCaller, Server server) {
HelpOpCommand(
NoticeService noticeService,
HelpOpSettings helpOpSettings,
HelpOpService helpOpService,
EventCaller eventCaller,
Server server
) {
this.noticeService = noticeService;
this.helpOpSettings = helpOpSettings;
this.helpOpService = helpOpService;
this.eventCaller = eventCaller;
this.server = server;
this.delay = Delay.withDefault(() -> this.helpOpSettings.helpOpDelay());
Expand Down Expand Up @@ -93,5 +101,6 @@ void execute(@Sender Player player, @Join String message) {
.send();

this.delay.markDelay(uuid);
this.helpOpService.markSender(uuid);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package com.eternalcode.core.feature.helpop;

import com.eternalcode.annotations.scan.command.DescriptionDocs;
import com.eternalcode.annotations.scan.permission.PermissionDocs;
import com.eternalcode.core.event.EventCaller;
import com.eternalcode.core.feature.helpop.event.HelpOpReplyEvent;
import com.eternalcode.core.injector.annotations.Inject;
import com.eternalcode.core.notice.NoticeService;
import com.eternalcode.multification.notice.NoticeBroadcast;
import dev.rollczi.litecommands.annotations.argument.Arg;
import dev.rollczi.litecommands.annotations.command.Command;
import dev.rollczi.litecommands.annotations.context.Sender;
import dev.rollczi.litecommands.annotations.execute.Execute;
import dev.rollczi.litecommands.annotations.join.Join;
import dev.rollczi.litecommands.annotations.permission.Permission;
import net.kyori.adventure.text.minimessage.MiniMessage;
import org.bukkit.Server;
import org.bukkit.entity.Player;

@Command(name = "helpopreply")
Comment on lines +19 to +20
Copy link
Member

Choose a reason for hiding this comment

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

maybe add alias "hr"

@Permission("eternalcore.helpop.reply")
@PermissionDocs(
name = "HelpOp Reply",
description = "Allows administrator to reply to a player's helpop request",
permission = "eternalcore.helpop.reply"
)
class HelpOpReplyCommand {

private final NoticeService noticeService;
private final HelpOpService helpOpService;
private final EventCaller eventCaller;
private final Server server;

@Inject
HelpOpReplyCommand(NoticeService noticeService, HelpOpService helpOpService, EventCaller eventCaller, Server server) {
this.noticeService = noticeService;
this.helpOpService = helpOpService;
this.eventCaller = eventCaller;
this.server = server;
}

Copy link
Member

Choose a reason for hiding this comment

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

maybe add default execute - when player is not provided - then admin replays to the last helpop message

@Execute
@DescriptionDocs(description = "Reply to a player's helpop request", arguments = "<player> <message>")
void execute(@Sender Player admin, @Arg Player target, @Join String message) {
if (!this.helpOpService.hasSentHelpOp(target.getUniqueId())) {
this.noticeService.create()
.player(admin.getUniqueId())
.notice(translation -> translation.helpOp().playerNotSentHelpOp())
.placeholder("{PLAYER}", target.getName())
.send();

return;
}

HelpOpReplyEvent event = new HelpOpReplyEvent(admin, target, message);
this.eventCaller.callEvent(event);

if (event.isCancelled()) {
return;
}

String escapedMessage = MiniMessage.miniMessage().escapeTags(event.getContent());

NoticeBroadcast notice = this.noticeService.create()
.console()
.player(target.getUniqueId())
.notice(translation -> translation.helpOp().adminReply())
.placeholder("{ADMIN}", admin.getName())
.placeholder("{PLAYER}", target.getName())
.placeholder("{TEXT}", escapedMessage);

for (Player onlinePlayer : this.server.getOnlinePlayers()) {
if (!onlinePlayer.hasPermission(HelpOpCommand.HELPOP_SPY)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The HELPOP_SPY constant is referenced from HelpOpCommand. This creates a tight coupling between two command classes. It would be better to define this permission constant in a more central and accessible place, for example, the HelpOpService interface. This would improve modularity and make the code easier to maintain.

For example, you could add String HELPOP_SPY_PERMISSION = "eternalcore.helpop.spy"; to HelpOpService and use it in both command classes.

Copy link
Member

Choose a reason for hiding this comment

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

I like the concept, but in a few hundred player servers with players spamming the helpop and admins replying, it will basically also send the reply to all other administrators with the permission, and they have absolutely no way to toggle it.

A /helpoptoggle <all/reply/requests> with a simple cache should suffice IMO. Up for discussion with other reviewers.

continue;
}

notice = notice.player(onlinePlayer.getUniqueId());
}
Comment on lines +72 to +78
Copy link
Member

Choose a reason for hiding this comment

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


notice.send();

this.noticeService.create()
.player(admin.getUniqueId())
.notice(translation -> translation.helpOp().adminReplySend())
.placeholder("{PLAYER}", target.getName())
.send();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.eternalcode.core.feature.helpop;

import com.eternalcode.core.injector.annotations.component.Service;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import java.time.Duration;
import java.util.UUID;
import org.jspecify.annotations.NonNull;

@Service
class HelpOpServiceImpl implements HelpOpService {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class HelpOpServiceImpl implements HelpOpService {
class CachedHelpOpService implements HelpOpService {

Copy link
Member

Choose a reason for hiding this comment

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

why though?? So we should add the Cached prefix to everything that has cache? I think we have other services and they don't have that prefix, so we need to be consistent.


private final Cache<UUID, Boolean> helpOpSenders = CacheBuilder.newBuilder()
Copy link
Member

Choose a reason for hiding this comment

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

Question, do we plan to manually invalidate this cache? The current implementation just allows the administrators to reply to a single HelpOp request infinite times in the given time period.

.expireAfterWrite(Duration.ofHours(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The cache expiration is hardcoded to 1 hour. The Javadoc for HelpOpService#hasSentHelpOp states that 'Helpop requests expire after a configured duration (default: 1 hour)', which implies this value should be configurable. Consider adding a setting for this in HelpOpSettings and using it here to allow for configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with Gemini here; it won't hurt us if we make this configurable.

Copy link
Contributor

Choose a reason for hiding this comment

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

imo too much

Suggested change
.expireAfterWrite(Duration.ofHours(1))
.expireAfterWrite(Duration.ofMinutes(10))

Copy link
Member

Choose a reason for hiding this comment

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

Let's make this configurable

.build();

@Override
public void markSender(@NonNull UUID playerUuid) {
this.helpOpSenders.put(playerUuid, true);
}

@Override
public boolean hasSentHelpOp(@NonNull UUID playerUuid) {
return this.helpOpSenders.getIfPresent(playerUuid) != null;
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,34 @@
package com.eternalcode.core.feature.helpop.messages;

import com.eternalcode.multification.bukkit.notice.BukkitNotice;
import com.eternalcode.multification.notice.Notice;
import eu.okaeri.configs.OkaeriConfig;
import eu.okaeri.configs.annotation.Comment;
import lombok.Getter;
import lombok.experimental.Accessors;
import org.bukkit.Sound;

@Getter
@Accessors(fluent = true)
public class ENHelpOpMessages extends OkaeriConfig implements HelpOpSection {
@Comment("# {PLAYER} - Player who send message on /helpop, {TEXT} - message")
Notice format = Notice.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{PLAYER}<dark_gray>: <white>{TEXT}");

@Comment(" ")
Notice send = Notice.chat("<color:#9d6eef>► <white>This message has been successfully sent to administration");

@Comment("# {TIME} - Time to next use (cooldown)")
Notice helpOpDelay = Notice.chat("<red>✘ <dark_red>You can use this command for: <red>{TIME}");

@Comment("# {ADMIN} - Admin who replied, {PLAYER} - Player who received the reply, {TEXT} - Reply message")
Notice adminReply = BukkitNotice.builder()
.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray>-> <white>{PLAYER}<dark_gray>: <white>{TEXT}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray>-> <white>{PLAYER}<dark_gray>: <white>{TEXT}")
.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray> <white>{PLAYER}<dark_gray>: <white>{TEXT}")

.sound(Sound.ENTITY_EXPERIENCE_ORB_PICKUP, 1.0f, 1.0f)
.build();

@Comment(" ")
Notice adminReplySend = Notice.chat("<color:#9d6eef>► <white>Reply has been sent to player <color:#9d6eef>{PLAYER}");

@Comment("# {PLAYER} - Player name")
Notice playerNotSentHelpOp = Notice.chat("<red>✘ <dark_red>Player <red>{PLAYER} <dark_red>has not sent any helpop request");
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,7 @@ public interface HelpOpSection {
Notice format();
Notice send();
Notice helpOpDelay();
Notice adminReply();
Notice adminReplySend();
Notice playerNotSentHelpOp();
}
Original file line number Diff line number Diff line change
@@ -1,19 +1,35 @@
package com.eternalcode.core.feature.helpop.messages;

import com.eternalcode.multification.bukkit.notice.BukkitNotice;
import com.eternalcode.multification.notice.Notice;
import eu.okaeri.configs.OkaeriConfig;
import eu.okaeri.configs.annotation.Comment;
import lombok.Getter;
import lombok.experimental.Accessors;
import org.bukkit.Sound;

@Getter
@Accessors(fluent = true)
public class PLHelpOpMessages extends OkaeriConfig implements HelpOpSection {
@Comment({ "# {PLAYER} - Gracz który wysłał wiadomość na helpop, {TEXT} - Treść wysłanej wiadomości" })
Notice format = Notice
.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{PLAYER}<dark_gray>: <white>{TEXT}");

@Comment(" ")
Notice send = Notice.chat("<color:#9d6eef>► <white>Wiadomość została wysłana do administracji");

@Comment("# {TIME} - Czas do końca blokady (cooldown)")
Notice helpOpDelay = Notice.chat("<red>✘ <dark_red>Możesz użyć tej komendy dopiero za <red>{TIME}<dark_red>!");

@Comment("# {ADMIN} - Admin który odpowiedział, {PLAYER} - Gracz który otrzymał odpowiedź, {TEXT} - Treść odpowiedzi")
Notice adminReply = BukkitNotice.builder()
.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray>-> <white>{PLAYER}<dark_gray>: <white>{TEXT}")
.sound(Sound.ENTITY_EXPERIENCE_ORB_PICKUP, 1.0f, 1.0f)
.build();

@Comment(" ")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Comment(" ")
@Comment("# {PLAYER} - Nazwa gracza")

Notice adminReplySend = Notice.chat("<color:#9d6eef>► <white>Odpowiedź została wysłana do gracza <color:#9d6eef>{PLAYER}");

@Comment("# {PLAYER} - Nazwa gracza")
Notice playerNotSentHelpOp = Notice.chat("<red>✘ <dark_red>Gracz <red>{PLAYER} <dark_red>nie wysłał żadnego zapytania na helpop");
}