#1026 Add more tags for forced commands (lazily replaced) (#214)

* #1026 Add more tags for forced commands (lazily replaced)
- Extract lazy replacement of tags to its own class
- Implement wrapper to replace a String property within an object
  - Use wrapper in command manager and add new tags

* Make argument type generic in lazy tags util

src/main/java/fr/xephi/authme/settings/WelcomeMessageConfiguration.java

@@ -8,6 +8,7 @@ import fr.xephi.authme.service.BukkitService;
 import fr.xephi.authme.service.GeoIpService;
 import fr.xephi.authme.util.PlayerUtils;
 import fr.xephi.authme.util.lazytags.Tag;
+import fr.xephi.authme.util.lazytags.TagReplacer;
 import org.bukkit.Server;
 import org.bukkit.entity.Player;
 
@@ -19,9 +20,7 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.LinkedList;
 import java.util.List;
-import java.util.stream.Collectors;
 
 import static fr.xephi.authme.util.FileUtils.copyFileFromResource;
 import static fr.xephi.authme.util.lazytags.TagBuilder.createTag;
@@ -48,7 +47,7 @@ public class WelcomeMessageConfiguration implements Reloadable {
     private PlayerCache playerCache;
 
     /** List of all supported tags for the welcome message. */
-    private final List<Tag> availableTags = Arrays.asList(
+    private final List<Tag<Player>> availableTags = Arrays.asList(
         createTag("&",            () -> "\u00a7"),
         createTag("{PLAYER}",     pl -> pl.getName()),
         createTag("{ONLINE}",     () -> Integer.toString(bukkitService.getOnlinePlayers().size())),
@@ -60,16 +59,13 @@ public class WelcomeMessageConfiguration implements Reloadable {
         createTag("{VERSION}",    () -> server.getBukkitVersion()),
         createTag("{COUNTRY}",    pl -> geoIpService.getCountryName(PlayerUtils.getPlayerIp(pl))));
 
-    /** Welcome message, by lines. */
+    private TagReplacer<Player> messageSupplier;
-    private List<String> welcomeMessage;
-    /** Tags used in the welcome message. */
-    private List<Tag> usedTags;
 
     @PostConstruct
     @Override
     public void reload() {
-        welcomeMessage = readWelcomeFile();
+        List<String> welcomeMessage = readWelcomeFile();
-        usedTags = determineUsedTags(welcomeMessage);
+        messageSupplier = TagReplacer.newReplacer(availableTags, welcomeMessage);
     }
 
     /**
@@ -79,22 +75,7 @@ public class WelcomeMessageConfiguration implements Reloadable {
      * @return the welcome message
      */
     public List<String> getWelcomeMessage(Player player) {
-        // Note ljacqu 20170121: Using a Map might seem more natural here but we avoid doing so for performance
+        return messageSupplier.getAdaptedMessages(player);
-        // Although the performance gain here is probably minimal...
-        List<TagValue> tagValues = new LinkedList<>();
-        for (Tag tag : usedTags) {
-            tagValues.add(new TagValue(tag.getName(), tag.getValue(player)));
-        }
-
-        List<String> adaptedMessages = new LinkedList<>();
-        for (String line : welcomeMessage) {
-            String adaptedLine = line;
-            for (TagValue tagValue : tagValues) {
-                adaptedLine = adaptedLine.replace(tagValue.tag, tagValue.value);
-            }
-            adaptedMessages.add(adaptedLine);
-        }
-        return adaptedMessages;
     }
 
     /**
@@ -113,32 +94,4 @@ public class WelcomeMessageConfiguration implements Reloadable {
         }
         return Collections.emptyList();
     }
-
-    /**
-     * Determines which tags are used in the message.
-     *
-     * @param welcomeMessage the lines of the welcome message
-     * @return the tags
-     */
-    private List<Tag> determineUsedTags(List<String> welcomeMessage) {
-        return availableTags.stream()
-            .filter(tag -> welcomeMessage.stream().anyMatch(msg -> msg.contains(tag.getName())))
-            .collect(Collectors.toList());
-    }
-
-    private static final class TagValue {
-
-        private final String tag;
-        private final String value;
-
-        TagValue(String tag, String value) {
-            this.tag = tag;
-            this.value = value;
-        }
-
-        @Override
-        public String toString() {
-            return "TagValue[tag='" + tag + "', value='" + value + "']";
-        }
-    }
 }

src/main/java/fr/xephi/authme/settings/commandconfig/CommandManager.java

@@ -5,13 +5,21 @@ import ch.jalu.configme.resource.YamlFileResource;
 import fr.xephi.authme.initialization.DataFolder;
 import fr.xephi.authme.initialization.Reloadable;
 import fr.xephi.authme.service.BukkitService;
+import fr.xephi.authme.service.GeoIpService;
 import fr.xephi.authme.util.FileUtils;
+import fr.xephi.authme.util.PlayerUtils;
+import fr.xephi.authme.util.lazytags.Tag;
+import fr.xephi.authme.util.lazytags.WrappedTagReplacer;
 import org.bukkit.entity.Player;
 
 import javax.inject.Inject;
 import java.io.File;
+import java.util.Arrays;
+import java.util.List;
 import java.util.Map;
 
+import static fr.xephi.authme.util.lazytags.TagBuilder.createTag;
+
 /**
  * Manages configurable commands to be run when various events occur.
  */
@@ -19,15 +27,20 @@ public class CommandManager implements Reloadable {
 
     private final File dataFolder;
     private final BukkitService bukkitService;
+    private final GeoIpService geoIpService;
     private final CommandMigrationService commandMigrationService;
+    private final List<Tag<Player>> availableTags = buildAvailableTags();
 
-    private CommandConfig commandConfig;
+    private WrappedTagReplacer<Command, Player> onJoinCommands;
+    private WrappedTagReplacer<Command, Player> onLoginCommands;
+    private WrappedTagReplacer<Command, Player> onRegisterCommands;
 
     @Inject
-    CommandManager(@DataFolder File dataFolder, BukkitService bukkitService,
+    CommandManager(@DataFolder File dataFolder, BukkitService bukkitService, GeoIpService geoIpService,
                    CommandMigrationService commandMigrationService) {
         this.dataFolder = dataFolder;
         this.bukkitService = bukkitService;
+        this.geoIpService = geoIpService;
         this.commandMigrationService = commandMigrationService;
         reload();
     }
@@ -38,7 +51,7 @@ public class CommandManager implements Reloadable {
      * @param player the joining player
      */
     public void runCommandsOnJoin(Player player) {
-        executeCommands(player, commandConfig.getOnJoin());
+        executeCommands(player, onJoinCommands.getAdaptedItems(player));
     }
 
     /**
@@ -47,7 +60,7 @@ public class CommandManager implements Reloadable {
      * @param player the player who has registered
      */
     public void runCommandsOnRegister(Player player) {
-        executeCommands(player, commandConfig.getOnRegister());
+        executeCommands(player, onRegisterCommands.getAdaptedItems(player));
     }
 
     /**
@@ -56,11 +69,11 @@ public class CommandManager implements Reloadable {
      * @param player the player that logged in
      */
     public void runCommandsOnLogin(Player player) {
-        executeCommands(player, commandConfig.getOnLogin());
+        executeCommands(player, onLoginCommands.getAdaptedItems(player));
     }
 
-    private void executeCommands(Player player, Map<String, Command> commands) {
+    private void executeCommands(Player player, List<Command> commands) {
-        for (Command command : commands.values()) {
+        for (Command command : commands) {
             final String execution = command.getCommand().replace("%p", player.getName());
             if (Executor.CONSOLE.equals(command.getExecutor())) {
                 bukkitService.dispatchConsoleCommand(execution);
@@ -77,8 +90,22 @@ public class CommandManager implements Reloadable {
 
         SettingsManager settingsManager = new SettingsManager(
             new YamlFileResource(file), commandMigrationService, CommandSettingsHolder.class);
-        commandConfig = settingsManager.getProperty(CommandSettingsHolder.COMMANDS);
+        CommandConfig commandConfig = settingsManager.getProperty(CommandSettingsHolder.COMMANDS);
+        onJoinCommands = newReplacer(commandConfig.getOnJoin());
+        onLoginCommands = newReplacer(commandConfig.getOnLogin());
+        onRegisterCommands = newReplacer(commandConfig.getOnRegister());
     }
 
+    private WrappedTagReplacer<Command, Player> newReplacer(Map<String, Command> commands) {
+        return new WrappedTagReplacer<>(availableTags, commands.values(), Command::getCommand,
+            (cmd, text) -> new Command(text, cmd.getExecutor()));
+    }
 
+    private List<Tag<Player>> buildAvailableTags() {
+        return Arrays.asList(
+            createTag("%p",       pl -> pl.getName()),
+            createTag("%nick",    pl -> pl.getDisplayName()),
+            createTag("%ip",      pl -> PlayerUtils.getPlayerIp(pl)),
+            createTag("%country", pl -> geoIpService.getCountryName(PlayerUtils.getPlayerIp(pl))));
+    }
 }

src/main/java/fr/xephi/authme/util/lazytags/DependentTag.java

@@ -1,16 +1,16 @@
 package fr.xephi.authme.util.lazytags;
 
-import org.bukkit.entity.Player;
-
 import java.util.function.Function;
 
 /**
- * Replaceable tag whose value depends on the player.
+ * Replaceable tag whose value depends on an argument.
+ *
+ * @param <A> the argument type
  */
-public class PlayerTag implements Tag {
+public class DependentTag<A> implements Tag<A> {
 
     private final String name;
-    private final Function<Player, String> replacementFunction;
+    private final Function<A, String> replacementFunction;
 
     /**
      * Constructor.
@@ -18,7 +18,7 @@ public class PlayerTag implements Tag {
      * @param name the tag (placeholder) that will be replaced
      * @param replacementFunction the function producing the replacement
      */
-    public PlayerTag(String name, Function<Player, String> replacementFunction) {
+    public DependentTag(String name, Function<A, String> replacementFunction) {
         this.name = name;
         this.replacementFunction = replacementFunction;
     }
@@ -29,7 +29,7 @@ public class PlayerTag implements Tag {
     }
 
     @Override
-    public String getValue(Player player) {
+    public String getValue(A argument) {
-        return replacementFunction.apply(player);
+        return replacementFunction.apply(argument);
     }
 }

src/main/java/fr/xephi/authme/util/lazytags/SimpleTag.java

@@ -1,13 +1,13 @@
 package fr.xephi.authme.util.lazytags;
 
-import org.bukkit.entity.Player;
-
 import java.util.function.Supplier;
 
 /**
- * Tag to be replaced that does not depend on the player.
+ * Tag to be replaced that does not depend on an argument.
+ *
+ * @param <A> type of the argument (not used in this implementation)
  */
-public class SimpleTag implements Tag {
+public class SimpleTag<A> implements Tag<A> {
 
     private final String name;
     private final Supplier<String> replacementFunction;
@@ -23,7 +23,7 @@ public class SimpleTag implements Tag {
     }
 
     @Override
-    public String getValue(Player player) {
+    public String getValue(A argument) {
         return replacementFunction.get();
     }
 }

src/main/java/fr/xephi/authme/util/lazytags/Tag.java

@@ -1,11 +1,11 @@
 package fr.xephi.authme.util.lazytags;
 
-import org.bukkit.entity.Player;
-
 /**
- * Represents a tag in a text to be replaced with a value (which may depend on the Player).
+ * Represents a tag in a text to be replaced with a value (which may depend on some argument).
+ *
+ * @param <A> argument type the replacement may depend on
  */
-public interface Tag {
+public interface Tag<A> {
 
     /**
      * @return the tag to replace
@@ -13,10 +13,10 @@ public interface Tag {
     String getName();
 
     /**
-     * Returns the value to replace the tag with for the given player.
+     * Returns the value to replace the tag with for the given argument.
      *
-     * @param player the player to evaluate the replacement for
+     * @param argument the argument to evaluate the replacement for
      * @return the replacement
      */
-    String getValue(Player player);
+    String getValue(A argument);
 }

src/main/java/fr/xephi/authme/util/lazytags/TagBuilder.java

@@ -1,7 +1,5 @@
 package fr.xephi.authme.util.lazytags;
 
-import org.bukkit.entity.Player;
-
 import java.util.function.Function;
 import java.util.function.Supplier;
 
@@ -13,11 +11,11 @@ public final class TagBuilder {
     private TagBuilder() {
     }
 
-    public static Tag createTag(String name, Function<Player, String> replacementFunction) {
+    public static <A> Tag<A> createTag(String name, Function<A, String> replacementFunction) {
-        return new PlayerTag(name, replacementFunction);
+        return new DependentTag<>(name, replacementFunction);
     }
 
-    public static Tag createTag(String name, Supplier<String> replacementFunction) {
+    public static <A> Tag<A> createTag(String name, Supplier<String> replacementFunction) {
-        return new SimpleTag(name, replacementFunction);
+        return new SimpleTag<>(name, replacementFunction);
     }
 }

src/main/java/fr/xephi/authme/util/lazytags/TagReplacer.java

@@ -0,0 +1,102 @@
+package fr.xephi.authme.util.lazytags;
+
+import java.util.Collection;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Replaces tags lazily by first determining which tags are being used
+ * and only applying those replacements afterwards.
+ *
+ * @param <A> the argument type
+ */
+public class TagReplacer<A> {
+
+    private final List<Tag<A>> tags;
+    private final Collection<String> messages;
+
+    /**
+     * Private constructor. Use {@link #newReplacer(Collection, Collection)}.
+     *
+     * @param tags the tags that are being used in the messages
+     * @param messages the messages
+     */
+    private TagReplacer(List<Tag<A>> tags, Collection<String> messages) {
+        this.tags = tags;
+        this.messages = messages;
+    }
+
+    /**
+     * Creates a new instance of this class, which will provide the given
+     * messages adapted with the provided tags.
+     *
+     * @param allTags all available tags
+     * @param messages the messages to use
+     * @param <A> the argument type
+     * @return new tag replacer instance
+     */
+    public static <A> TagReplacer<A> newReplacer(Collection<Tag<A>> allTags, Collection<String> messages) {
+        List<Tag<A>> usedTags = determineUsedTags(allTags, messages);
+        return new TagReplacer<>(usedTags, messages);
+    }
+
+    /**
+     * Returns the messages with the tags applied for the given argument.
+     *
+     * @param argument the argument to get the messages for
+     * @return the adapted messages
+     */
+    public List<String> getAdaptedMessages(A argument) {
+        // Note ljacqu 20170121: Using a Map might seem more natural here but we avoid doing so for performance
+        // Although the performance gain here is probably minimal...
+        List<TagValue> tagValues = new LinkedList<>();
+        for (Tag<A> tag : tags) {
+            tagValues.add(new TagValue(tag.getName(), tag.getValue(argument)));
+        }
+
+        List<String> adaptedMessages = new LinkedList<>();
+        for (String line : messages) {
+            String adaptedLine = line;
+            for (TagValue tagValue : tagValues) {
+                adaptedLine = adaptedLine.replace(tagValue.tag, tagValue.value);
+            }
+            adaptedMessages.add(adaptedLine);
+        }
+        return adaptedMessages;
+    }
+
+    /**
+     * Determines which tags are used somewhere in the given list of messages.
+     *
+     * @param allTags all available tags
+     * @param messages the messages
+     * @param <A> argument type
+     * @return tags used at least once
+     */
+    private static <A> List<Tag<A>> determineUsedTags(Collection<Tag<A>> allTags, Collection<String> messages) {
+        return allTags.stream()
+            .filter(tag -> messages.stream().anyMatch(msg -> msg.contains(tag.getName())))
+            .collect(Collectors.toList());
+    }
+
+    /** (Tag, value) pair. */
+    private static final class TagValue {
+
+        /** The tag to replace. */
+        private final String tag;
+        /** The value to replace with. */
+        private final String value;
+
+        TagValue(String tag, String value) {
+            this.tag = tag;
+            this.value = value;
+        }
+
+        @Override
+        public String toString() {
+            return "TagValue[tag='" + tag + "', value='" + value + "']";
+        }
+    }
+
+}

src/main/java/fr/xephi/authme/util/lazytags/WrappedTagReplacer.java

@@ -0,0 +1,61 @@
+package fr.xephi.authme.util.lazytags;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+/**
+ * Applies tags lazily to the String property of an item. This class wraps
+ * a {@link TagReplacer} with the extraction of the String property and
+ * the creation of new items with the adapted string property.
+ *
+ * @param <T> the item type
+ * @param <A> the argument type to evaluate the replacements
+ */
+public class WrappedTagReplacer<T, A> {
+
+    private final Collection<T> items;
+    private final BiFunction<T, String, ? extends T> itemCreator;
+    private final TagReplacer<A> tagReplacer;
+
+    /**
+     * Constructor.
+     *
+     * @param allTags all available tags
+     * @param items the items to apply the replacements on
+     * @param stringGetter getter of the String property to adapt on the items
+     * @param itemCreator a function of signature (T, String) -> T: the original item and the adapted String are passed
+     */
+    public WrappedTagReplacer(Collection<Tag<A>> allTags,
+                              Collection<T> items,
+                              Function<? super T, String> stringGetter,
+                              BiFunction<T, String, ? extends T> itemCreator) {
+        this.items = items;
+        this.itemCreator = itemCreator;
+
+        List<String> stringItems = items.stream().map(stringGetter).collect(Collectors.toList());
+        tagReplacer = TagReplacer.newReplacer(allTags, stringItems);
+    }
+
+    /**
+     * Creates adapted items for the given argument.
+     *
+     * @param argument the argument to adapt the items for
+     * @return the adapted items
+     */
+    public List<T> getAdaptedItems(A argument) {
+        List<String> adaptedStrings = tagReplacer.getAdaptedMessages(argument);
+        List<T> adaptedItems = new LinkedList<>();
+
+        Iterator<T> originalItemsIter = items.iterator();
+        Iterator<String> newStringsIter = adaptedStrings.iterator();
+        while (originalItemsIter.hasNext() && newStringsIter.hasNext()) {
+            adaptedItems.add(itemCreator.apply(originalItemsIter.next(), newStringsIter.next()));
+        }
+        return adaptedItems;
+    }
+}

src/test/java/fr/xephi/authme/settings/commandconfig/CommandManagerTest.java

@@ -1,9 +1,9 @@
 package fr.xephi.authme.settings.commandconfig;
 
 import com.google.common.io.Files;
-import fr.xephi.authme.ReflectionTestUtils;
 import fr.xephi.authme.TestHelper;
 import fr.xephi.authme.service.BukkitService;
+import fr.xephi.authme.service.GeoIpService;
 import fr.xephi.authme.settings.SettingsMigrationService;
 import org.bukkit.entity.Player;
 import org.junit.Before;
@@ -17,13 +17,7 @@ import org.mockito.junit.MockitoJUnitRunner;
 
 import java.io.File;
 import java.io.IOException;
-import java.util.function.BiConsumer;
 
-import static fr.xephi.authme.settings.commandconfig.CommandConfigTestHelper.isCommand;
-import static java.lang.String.format;
-import static org.hamcrest.Matchers.anEmptyMap;
-import static org.hamcrest.Matchers.contains;
-import static org.junit.Assert.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.BDDMockito.given;
@@ -31,6 +25,7 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.only;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.verifyZeroInteractions;
 
 /**
  * Test for {@link CommandManager}.
@@ -41,104 +36,116 @@ public class CommandManagerTest {
     private static final String TEST_FILES_FOLDER = "/fr/xephi/authme/settings/commandconfig/";
 
     private CommandManager manager;
+    private Player player;
+
     @InjectMocks
     private CommandMigrationService commandMigrationService;
 
-    @Rule
-    public TemporaryFolder temporaryFolder = new TemporaryFolder();
-
     @Mock
     private BukkitService bukkitService;
     @Mock
+    private GeoIpService geoIpService;
+    @Mock
     private SettingsMigrationService settingsMigrationService;
 
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
     private File testFolder;
 
     @Before
     public void setup() throws IOException {
         testFolder = temporaryFolder.newFolder();
-    }
+        player = mockPlayer();
-
-    @Test
-    @SuppressWarnings("unchecked")
-    public void shouldLoadCompleteFile() {
-        // given
-        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.complete.yml");
-
-        // when
-        initManager();
-
-        // then
-        CommandConfig commandConfig = ReflectionTestUtils.getFieldValue(CommandManager.class, manager, "commandConfig");
-        assertThat(commandConfig.getOnJoin().keySet(), contains("broadcast"));
-        assertThat(commandConfig.getOnJoin().values(), contains(isCommand("broadcast %p has joined", Executor.CONSOLE)));
-        assertThat(commandConfig.getOnRegister().keySet(), contains("announce", "notify"));
-        assertThat(commandConfig.getOnRegister().values(), contains(
-            isCommand("me I just registered", Executor.PLAYER),
-            isCommand("log %p registered", Executor.CONSOLE)));
-        assertThat(commandConfig.getOnLogin().keySet(), contains("welcome", "show_motd", "display_list"));
-        assertThat(commandConfig.getOnLogin().values(), contains(
-            isCommand("msg %p Welcome back", Executor.CONSOLE),
-            isCommand("motd", Executor.PLAYER),
-            isCommand("list", Executor.PLAYER)));
-    }
-
-    @Test
-    public void shouldLoadIncompleteFile() {
-        // given
-        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.incomplete.yml");
-
-        // when
-        initManager();
-
-        // then
-        CommandConfig commandConfig = ReflectionTestUtils.getFieldValue(CommandManager.class, manager, "commandConfig");
-        assertThat(commandConfig.getOnJoin().values(), contains(isCommand("broadcast %p has joined", Executor.CONSOLE)));
-        assertThat(commandConfig.getOnLogin().values(), contains(
-            isCommand("msg %p Welcome back", Executor.CONSOLE),
-            isCommand("list", Executor.PLAYER)));
-        assertThat(commandConfig.getOnRegister(), anEmptyMap());
-    }
-
-    @Test
-    public void shouldExecuteCommandsOnJoin() {
-        // given
-        String name = "Bobby1";
-
-        // when
-        testCommandExecution(name, CommandManager::runCommandsOnJoin);
-
-        // then
-        verify(bukkitService, only()).dispatchConsoleCommand(format("broadcast %s has joined", name));
-    }
-
-    @Test
-    public void shouldExecuteCommandsOnRegister() {
-        // given
-        String name = "luis";
-
-        // when
-        testCommandExecution(name, CommandManager::runCommandsOnRegister);
-
-        // then
-        verify(bukkitService).dispatchCommand(any(Player.class), eq("me I just registered"));
-        verify(bukkitService).dispatchConsoleCommand(format("log %s registered", name));
-        verifyNoMoreInteractions(bukkitService);
     }
 
     @Test
     public void shouldExecuteCommandsOnLogin() {
         // given
-        String name = "plaYer01";
+        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.complete.yml");
+        initManager();
 
         // when
-        testCommandExecution(name, CommandManager::runCommandsOnLogin);
+        manager.runCommandsOnLogin(player);
 
         // then
-        verify(bukkitService).dispatchConsoleCommand(format("msg %s Welcome back", name));
+        verify(bukkitService).dispatchConsoleCommand("msg Bobby Welcome back");
         verify(bukkitService).dispatchCommand(any(Player.class), eq("motd"));
         verify(bukkitService).dispatchCommand(any(Player.class), eq("list"));
         verifyNoMoreInteractions(bukkitService);
+        verifyZeroInteractions(geoIpService);
+    }
+
+    @Test
+    public void shouldExecuteCommandsOnLoginWithIncompleteConfig() {
+        // given
+        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.incomplete.yml");
+        initManager();
+
+        // when
+        manager.runCommandsOnLogin(player);
+
+        // then
+        verify(bukkitService).dispatchConsoleCommand("msg Bobby Welcome back, bob");
+        verify(bukkitService).dispatchCommand(any(Player.class), eq("list"));
+        verifyNoMoreInteractions(bukkitService);
+        verifyZeroInteractions(geoIpService);
+    }
+
+    @Test
+    public void shouldExecuteCommandsOnJoin() {
+        // given
+        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.complete.yml");
+        initManager();
+
+        // when
+        manager.runCommandsOnJoin(player);
+
+        // then
+        verify(bukkitService, only()).dispatchConsoleCommand("broadcast bob has joined");
+        verifyZeroInteractions(geoIpService);
+    }
+
+    @Test
+    public void shouldExecuteCommandsOnJoinWithIncompleteConfig() {
+        // given
+        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.incomplete.yml");
+        initManager();
+
+        // when
+        manager.runCommandsOnJoin(player);
+
+        // then
+        verify(bukkitService, only()).dispatchConsoleCommand("broadcast Bobby has joined");
+        verifyZeroInteractions(geoIpService);
+    }
+
+    @Test
+    public void shouldExecuteCommandsOnRegister() {
+        // given
+        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.complete.yml");
+        initManager();
+
+        // when
+        manager.runCommandsOnRegister(player);
+
+        // then
+        verify(bukkitService).dispatchCommand(any(Player.class), eq("me I just registered"));
+        verify(bukkitService).dispatchConsoleCommand("log Bobby (127.0.0.3, Syldavia) registered");
+        verifyNoMoreInteractions(bukkitService);
+    }
+
+    @Test
+    public void shouldExecuteCommandsOnRegisterWithIncompleteConfig() {
+        // given
+        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.incomplete.yml");
+        initManager();
+
+        // when
+        manager.runCommandsOnRegister(player);
+
+        // then
+        verifyZeroInteractions(bukkitService, geoIpService);
     }
 
     @Test
@@ -147,18 +154,8 @@ public class CommandManagerTest {
         TestHelper.validateHasOnlyPrivateEmptyConstructor(CommandSettingsHolder.class);
     }
 
-
-    private void testCommandExecution(String playerName, BiConsumer<CommandManager, Player> testMethod) {
-        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.complete.yml");
-        initManager();
-        Player player = mock(Player.class);
-        given(player.getName()).willReturn(playerName);
-
-        testMethod.accept(manager, player);
-    }
-
     private void initManager() {
-        manager = new CommandManager(testFolder, bukkitService, commandMigrationService);
+        manager = new CommandManager(testFolder, bukkitService, geoIpService, commandMigrationService);
     }
 
     private void copyJarFileAsCommandsYml(String path) {
@@ -171,4 +168,13 @@ public class CommandManagerTest {
         }
     }
 
+    private Player mockPlayer() {
+        Player player = mock(Player.class);
+        given(player.getName()).willReturn("Bobby");
+        given(player.getDisplayName()).willReturn("bob");
+        String ip = "127.0.0.3";
+        TestHelper.mockPlayerIp(player, ip);
+        given(geoIpService.getCountryName(ip)).willReturn("Syldavia");
+        return player;
+    }
 }

src/test/java/tools/filegeneration/GenerateCommandsYml.java

@@ -26,7 +26,7 @@ public class GenerateCommandsYml implements AutoToolTask {
         // Get default and add sample entry
         CommandConfig commandConfig = CommandSettingsHolder.COMMANDS.getDefaultValue();
         commandConfig.setOnLogin(
-            ImmutableMap.of("welcome", newCommand("msg %p Welcome back!", Executor.PLAYER)));
+            ImmutableMap.of("welcome", new Command("msg %p Welcome back!", Executor.PLAYER)));
 
         // Export the value to the file
         SettingsManager settingsManager = new SettingsManager(
@@ -41,11 +41,4 @@ public class GenerateCommandsYml implements AutoToolTask {
     public String getTaskName() {
         return "generateCommandsYml";
     }
-
-    private static Command newCommand(String commandLine, Executor executor) {
-        Command command = new Command();
-        command.setCommand(commandLine);
-        command.setExecutor(executor);
-        return command;
-    }
 }

src/test/resources/fr/xephi/authme/settings/commandconfig/commands.complete.yml

@@ -2,14 +2,14 @@
 
 onJoin:
     broadcast:
-        command: 'broadcast %p has joined'
+        command: 'broadcast %nick has joined'
         executor: CONSOLE
 onRegister:
     announce:
         command: 'me I just registered'
         executor: PLAYER
     notify:
-        command: 'log %p registered'
+        command: 'log %p (%ip, %country) registered'
         executor: CONSOLE
 onLogin:
     welcome:

src/test/resources/fr/xephi/authme/settings/commandconfig/commands.incomplete.yml

@@ -6,7 +6,7 @@ onJoin:
         executor: CONSOLE
 onLogin:
     welcome:
-        command: 'msg %p Welcome back'
+        command: 'msg %p Welcome back, %nick'
         executor: CONSOLE
     show_motd:
         # command: 'motd' <-- mandatory property, so entry should be ignored