From 6cf8789fe0888a439b9a8975e2f180b192c896b8 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Thu, 5 Jan 2017 20:47:14 +0100 Subject: [PATCH] #758 Show login usage message on invalid arguments, #1053 send translatable "no permission" message - Add default method to ExecutableCommand interface that allows to define the message key to show if a command's arguments are invalid. If not defined the behavior is as before: show the output of / help - Use translatable "no permission" message instead of hardcoded one --- .../xephi/authme/command/CommandHandler.java | 29 ++++++++----- .../authme/command/ExecutableCommand.java | 11 +++++ .../changepassword/ChangePasswordCommand.java | 5 +++ .../executable/login/LoginCommand.java | 11 +++++ .../executable/register/RegisterCommand.java | 5 +++ .../authme/command/CommandHandlerTest.java | 41 +++++++++++++++---- .../ChangePasswordCommandTest.java | 2 +- .../executable/login/LoginCommandTest.java | 5 +-- 8 files changed, 85 insertions(+), 24 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/CommandHandler.java b/src/main/java/fr/xephi/authme/command/CommandHandler.java index 743b01e8..394b6f14 100644 --- a/src/main/java/fr/xephi/authme/command/CommandHandler.java +++ b/src/main/java/fr/xephi/authme/command/CommandHandler.java @@ -3,6 +3,8 @@ package fr.xephi.authme.command; import ch.jalu.injector.Injector; import fr.xephi.authme.AuthMe; import fr.xephi.authme.command.help.HelpProvider; +import fr.xephi.authme.message.MessageKey; +import fr.xephi.authme.message.Messages; import fr.xephi.authme.permission.PermissionsManager; import fr.xephi.authme.util.StringUtils; import org.bukkit.ChatColor; @@ -29,6 +31,7 @@ public class CommandHandler { private final CommandMapper commandMapper; private final PermissionsManager permissionsManager; + private final Messages messages; private final HelpProvider helpProvider; /** @@ -37,10 +40,11 @@ public class CommandHandler { private Map, ExecutableCommand> commands = new HashMap<>(); @Inject - public CommandHandler(Injector injector, CommandMapper commandMapper, - PermissionsManager permissionsManager, HelpProvider helpProvider) { + CommandHandler(Injector injector, CommandMapper commandMapper, PermissionsManager permissionsManager, + Messages messages, HelpProvider helpProvider) { this.commandMapper = commandMapper; this.permissionsManager = permissionsManager; + this.messages = messages; this.helpProvider = helpProvider; initializeCommands(injector, commandMapper.getCommandClasses()); } @@ -80,7 +84,7 @@ public class CommandHandler { sendUnknownCommandMessage(sender, result); break; case NO_PERMISSION: - sendPermissionDeniedError(sender); + messages.send(sender, MessageKey.NO_PERMISSION); break; default: throw new IllegalStateException("Unknown result status '" + result.getResultStatus() + "'"); @@ -151,11 +155,20 @@ public class CommandHandler { private void sendImproperArgumentsMessage(CommandSender sender, FoundCommandResult result) { CommandDescription command = result.getCommandDescription(); if (!permissionsManager.hasPermission(sender, command.getPermission())) { - sendPermissionDeniedError(sender); + messages.send(sender, MessageKey.NO_PERMISSION); return; } - // Show the command argument help + ExecutableCommand executableCommand = commands.get(command.getExecutableCommand()); + MessageKey usageMessage = executableCommand.getArgumentsMismatchMessage(); + if (usageMessage == null) { + showHelpForCommand(sender, result); + } else { + messages.send(sender, usageMessage); + } + } + + private void showHelpForCommand(CommandSender sender, FoundCommandResult result) { sender.sendMessage(ChatColor.DARK_RED + "Incorrect command arguments!"); helpProvider.outputHelp(sender, result, HelpProvider.SHOW_ARGUMENTS); @@ -164,10 +177,4 @@ public class CommandHandler { sender.sendMessage(ChatColor.GOLD + "Detailed help: " + ChatColor.WHITE + "/" + labels.get(0) + " help " + childLabel); } - - // TODO ljacqu 20151212: Remove me once I am a MessageKey - private static void sendPermissionDeniedError(CommandSender sender) { - sender.sendMessage(ChatColor.DARK_RED + "You don't have permission to use this command!"); - } - } diff --git a/src/main/java/fr/xephi/authme/command/ExecutableCommand.java b/src/main/java/fr/xephi/authme/command/ExecutableCommand.java index ceda12ef..6dd81ca8 100644 --- a/src/main/java/fr/xephi/authme/command/ExecutableCommand.java +++ b/src/main/java/fr/xephi/authme/command/ExecutableCommand.java @@ -1,5 +1,6 @@ package fr.xephi.authme.command; +import fr.xephi.authme.message.MessageKey; import org.bukkit.command.CommandSender; import java.util.List; @@ -17,4 +18,14 @@ public interface ExecutableCommand { */ void executeCommand(CommandSender sender, List arguments); + /** + * Returns the message to show to the user if the command is used with the wrong commands. + * If null is returned, the standard help (/command help) output is shown. + * + * @return the message explaining the command's usage, or {@code null} for default behavior + */ + default MessageKey getArgumentsMismatchMessage() { + return null; + } + } diff --git a/src/main/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommand.java b/src/main/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommand.java index 4403c982..7c5e7d9d 100644 --- a/src/main/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommand.java @@ -49,4 +49,9 @@ public class ChangePasswordCommand extends PlayerCommand { management.performPasswordChange(player, oldPassword, newPassword); } + + @Override + protected String getAlternativeCommand() { + return "/authme password "; + } } diff --git a/src/main/java/fr/xephi/authme/command/executable/login/LoginCommand.java b/src/main/java/fr/xephi/authme/command/executable/login/LoginCommand.java index a5d25abf..32a4d87d 100644 --- a/src/main/java/fr/xephi/authme/command/executable/login/LoginCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/login/LoginCommand.java @@ -1,6 +1,7 @@ package fr.xephi.authme.command.executable.login; import fr.xephi.authme.command.PlayerCommand; +import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.process.Management; import org.bukkit.entity.Player; @@ -20,4 +21,14 @@ public class LoginCommand extends PlayerCommand { final String password = arguments.get(0); management.performLogin(player, password); } + + @Override + public MessageKey getArgumentsMismatchMessage() { + return MessageKey.USAGE_LOGIN; + } + + @Override + protected String getAlternativeCommand() { + return "/authme forcelogin "; + } } diff --git a/src/main/java/fr/xephi/authme/command/executable/register/RegisterCommand.java b/src/main/java/fr/xephi/authme/command/executable/register/RegisterCommand.java index 301baf65..ac4bb4bd 100644 --- a/src/main/java/fr/xephi/authme/command/executable/register/RegisterCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/register/RegisterCommand.java @@ -72,6 +72,11 @@ public class RegisterCommand extends PlayerCommand { return "/authme register "; } + @Override + public MessageKey getArgumentsMismatchMessage() { + return MessageKey.USAGE_LOGIN; + } + private void handlePasswordRegistration(Player player, List arguments) { if (isSecondArgValidForPasswordRegistration(player, arguments)) { final String password = arguments.get(0); diff --git a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java b/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java index 4e0c43af..1166cef2 100644 --- a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java +++ b/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java @@ -6,6 +6,8 @@ import fr.xephi.authme.command.TestCommandsUtil.TestLoginCommand; import fr.xephi.authme.command.TestCommandsUtil.TestRegisterCommand; import fr.xephi.authme.command.TestCommandsUtil.TestUnregisterCommand; import fr.xephi.authme.command.help.HelpProvider; +import fr.xephi.authme.message.MessageKey; +import fr.xephi.authme.message.Messages; import fr.xephi.authme.permission.PermissionsManager; import org.bukkit.command.CommandSender; import org.junit.Before; @@ -60,6 +62,8 @@ public class CommandHandlerTest { @Mock private PermissionsManager permissionsManager; @Mock + private Messages messages; + @Mock private HelpProvider helpProvider; private Map, ExecutableCommand> mockedCommands = new HashMap<>(); @@ -71,7 +75,7 @@ public class CommandHandlerTest { ExecutableCommand.class, TestLoginCommand.class, TestRegisterCommand.class, TestUnregisterCommand.class)); setInjectorToMockExecutableCommandClasses(); - handler = new CommandHandler(injector, commandMapper, permissionsManager, helpProvider); + handler = new CommandHandler(injector, commandMapper, permissionsManager, messages, helpProvider); } /** @@ -138,7 +142,7 @@ public class CommandHandlerTest { // then verify(commandMapper).mapPartsToCommand(sender, asList("unreg", "testPlayer")); verify(command, never()).getExecutableCommand(); - verify(sender).sendMessage(argThat(containsString("don't have permission"))); + verify(messages).send(sender, MessageKey.NO_PERMISSION); } @Test @@ -148,6 +152,7 @@ public class CommandHandlerTest { String[] bukkitArgs = {"testPlayer"}; CommandSender sender = mock(CommandSender.class); CommandDescription command = mock(CommandDescription.class); + given(command.getExecutableCommand()).willReturn((Class) TestUnregisterCommand.class); given(commandMapper.mapPartsToCommand(any(CommandSender.class), anyList())).willReturn( new FoundCommandResult(command, asList("unreg"), asList("testPlayer"), 0.0, INCORRECT_ARGUMENTS)); given(permissionsManager.hasPermission(sender, command.getPermission())).willReturn(true); @@ -157,10 +162,30 @@ public class CommandHandlerTest { // then verify(commandMapper).mapPartsToCommand(sender, asList("unreg", "testPlayer")); - verify(command, never()).getExecutableCommand(); - ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); - verify(sender, atLeastOnce()).sendMessage(captor.capture()); - assertThat(captor.getAllValues().get(0), containsString("Incorrect command arguments")); + verify(sender, atLeastOnce()).sendMessage(argThat(containsString("Incorrect command arguments"))); + } + + @Test + public void shouldUseCustomMessageUponArgumentMismatch() { + // given + String bukkitLabel = "unreg"; + String[] bukkitArgs = {"testPlayer"}; + CommandSender sender = mock(CommandSender.class); + CommandDescription command = mock(CommandDescription.class); + given(command.getExecutableCommand()).willReturn((Class) TestUnregisterCommand.class); + given(mockedCommands.get(TestUnregisterCommand.class).getArgumentsMismatchMessage()) + .willReturn(MessageKey.USAGE_RECOVER_EMAIL); + given(commandMapper.mapPartsToCommand(any(CommandSender.class), anyList())).willReturn( + new FoundCommandResult(command, asList("unreg"), asList("testPlayer"), 0.0, INCORRECT_ARGUMENTS)); + given(permissionsManager.hasPermission(sender, command.getPermission())).willReturn(true); + + // when + handler.processCommand(sender, bukkitLabel, bukkitArgs); + + // then + verify(commandMapper).mapPartsToCommand(sender, asList("unreg", "testPlayer")); + verify(messages).send(sender, MessageKey.USAGE_RECOVER_EMAIL); + verify(sender, never()).sendMessage(anyString()); } @Test @@ -180,9 +205,7 @@ public class CommandHandlerTest { // then verify(commandMapper).mapPartsToCommand(sender, asList("unreg", "testPlayer")); verify(command, never()).getExecutableCommand(); - ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); - verify(sender).sendMessage(captor.capture()); - assertThat(captor.getValue(), containsString("You don't have permission")); + verify(messages).send(sender, MessageKey.NO_PERMISSION); } @Test diff --git a/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java index d306ff06..e43566f2 100644 --- a/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java @@ -58,7 +58,7 @@ public class ChangePasswordCommandTest { command.executeCommand(sender, Collections.emptyList()); // then - verify(sender).sendMessage(argThat(containsString("only for players"))); + verify(sender).sendMessage(argThat(containsString("use /authme password instead"))); } @Test diff --git a/src/test/java/fr/xephi/authme/command/executable/login/LoginCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/login/LoginCommandTest.java index 2b6a5c19..df6eb7d8 100644 --- a/src/test/java/fr/xephi/authme/command/executable/login/LoginCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/login/LoginCommandTest.java @@ -10,7 +10,6 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; -import java.util.ArrayList; import java.util.Collections; import static org.hamcrest.Matchers.containsString; @@ -39,11 +38,11 @@ public class LoginCommandTest { CommandSender sender = mock(BlockCommandSender.class); // when - command.executeCommand(sender, new ArrayList()); + command.executeCommand(sender, Collections.emptyList()); // then verifyZeroInteractions(management); - verify(sender).sendMessage(argThat(containsString("only for players"))); + verify(sender).sendMessage(argThat(containsString("/authme forcelogin "))); } @Test