#411 Improve command migration handling, write tests

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

@@ -2,11 +2,9 @@ package fr.xephi.authme.settings.commandconfig;
 
 import com.github.authme.configme.SettingsManager;
 import com.github.authme.configme.resource.YamlFileResource;
-import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.initialization.DataFolder;
 import fr.xephi.authme.initialization.Reloadable;
 import fr.xephi.authme.service.BukkitService;
-import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.util.FileUtils;
 import org.bukkit.entity.Player;
 
@@ -22,17 +20,14 @@ public class CommandManager implements Reloadable {
     private final File dataFolder;
     private final BukkitService bukkitService;
     private final CommandsMigrater commandsMigrater;
-    private final Settings settings;
 
     private CommandConfig commandConfig;
 
     @Inject
-    CommandManager(@DataFolder File dataFolder, BukkitService bukkitService,
+    CommandManager(@DataFolder File dataFolder, BukkitService bukkitService, CommandsMigrater commandsMigrater) {
-                   CommandsMigrater commandsMigrater, Settings settings) {
         this.dataFolder = dataFolder;
         this.bukkitService = bukkitService;
         this.commandsMigrater = commandsMigrater;
-        this.settings = settings;
         reload();
     }
 
@@ -65,21 +60,8 @@ public class CommandManager implements Reloadable {
         FileUtils.copyFileFromResource(file, "commands.yml");
 
         SettingsManager settingsManager = new SettingsManager(
-            new YamlFileResource(file), null, CommandSettingsHolder.class);
+            new YamlFileResource(file), commandsMigrater, CommandSettingsHolder.class);
-        CommandConfig commandConfig = settingsManager.getProperty(CommandSettingsHolder.COMMANDS);
+        commandConfig = settingsManager.getProperty(CommandSettingsHolder.COMMANDS);
-
-        if (commandsMigrater.transformOldCommands(commandConfig)) {
-            ConsoleLogger.warning("Old setting properties (such as settings.forceCommands) were found. "
-                + "They have been moved to commands.yml");
-            settingsManager.setProperty(CommandSettingsHolder.COMMANDS, commandConfig);
-            settingsManager.save();
-            settingsManager.reload();
-            settings.save();
-            settings.reload();
-
-            commandConfig = settingsManager.getProperty(CommandSettingsHolder.COMMANDS);
-        }
-        this.commandConfig = commandConfig;
     }
 
 

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

@@ -25,7 +25,7 @@ public final class CommandSettingsHolder implements SettingsHolder {
         String[] comments = {
             "This configuration file allows you to execute commands on various events.",
             "%p in commands will be replaced with the player name.",
-            "For example, if you want to message a welcome message to a player who just registered:",
+            "For example, if you want to send a welcome message to a player who just registered:",
             "onRegister:",
             "  welcome:",
             "    command: 'msg %p Welcome to the server!'",
@@ -33,15 +33,17 @@ public final class CommandSettingsHolder implements SettingsHolder {
             "",
             "This will make the console execute the msg command to the player.",
             "Each command under an event has a name you can choose freely (e.g. 'welcome' as above),",
-            "after which a mandatory 'command' field defines the command to run, ",
+            "after which a mandatory 'command' field defines the command to run,",
             "and 'as' defines who will run the command (either PLAYER or CONSOLE). Longer example:",
             "onLogin:",
             "  welcome:",
             "    command: 'msg %p Welcome back!'",
-            "    # as: PLAYER  # player is the default, you can leave this out if you want",
+            "    as: PLAYER",
             "  broadcast:",
             "    command: 'broadcast %p has joined, welcome back!'",
-            "    as: CONSOLE"
+            "    as: CONSOLE",
+            "",
+            "Supported command events: onLogin, onJoin, onRegister"
         };
         Map<String, String[]> commentMap = new HashMap<>();
         commentMap.put("onLogin", comments);

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

@@ -1,5 +1,9 @@
 package fr.xephi.authme.settings.commandconfig;
 
+import com.github.authme.configme.migration.MigrationService;
+import com.github.authme.configme.properties.Property;
+import com.github.authme.configme.resource.PropertyResource;
+import com.google.common.annotations.VisibleForTesting;
 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.settings.SettingsMigrationService;
 import fr.xephi.authme.util.RandomStringUtils;
@@ -13,7 +17,7 @@ import java.util.stream.Collectors;
 /**
  * Migrates the commands from their old location, in config.yml, to the dedicated commands configuration file.
  */
-class CommandsMigrater {
+class CommandsMigrater implements MigrationService {
 
     @Inject
     private SettingsMigrationService settingsMigrationService;
@@ -21,12 +25,25 @@ class CommandsMigrater {
     CommandsMigrater() {
     }
 
+    @Override
+    public boolean checkAndMigrate(PropertyResource resource, List<Property<?>> properties) {
+        final CommandConfig commandConfig = CommandSettingsHolder.COMMANDS.getValue(resource);
+        final boolean didMoveCommands = transformOldCommands(commandConfig);
+
+        if (didMoveCommands) { // TODO ConfigMe/#29: Check that loaded file isn't completely empty
+            resource.setValue("", commandConfig);
+            return true;
+        }
+        return false;
+    }
+
     /**
      * Adds command settings from their old location (in config.yml) to the given command configuration object.
      *
      * @param commandConfig the command config object to move old commands to
      * @return true if commands have been moved, false if no migration was necessary
      */
+    @VisibleForTesting
     boolean transformOldCommands(CommandConfig commandConfig) {
         boolean didMoveCommands = false;
         for (MigratableCommandSection section : MigratableCommandSection.values()) {
@@ -97,7 +114,8 @@ class CommandsMigrater {
             }
             Map<String, Command> commandMap = commandMapGetter.apply(commandConfig);
             commands.forEach(cmd -> commandMap.put(RandomStringUtils.generate(10), cmd));
-            ConsoleLogger.info("Migrated " + commands.size() + " commands of type " + this);
+            ConsoleLogger.info("Moving " + commands.size() + " commands of type " + this
+                + " from config.yml to commands.yml");
             return true;
         }
     }

src/main/java/fr/xephi/authme/settings/properties/AuthMeSettingsRetriever.java

@@ -6,8 +6,7 @@ import com.github.authme.configme.knownproperties.ConfigurationDataBuilder;
 import com.github.authme.configme.properties.Property;
 
 /**
- * Utility class responsible for retrieving all {@link Property} fields
+ * Utility class responsible for retrieving all {@link Property} fields from {@link SettingsHolder} classes.
- * from {@link SettingsHolder} implementations via reflection.
  */
 public final class AuthMeSettingsRetriever {
 

src/main/resources/commands.yml

@@ -1,24 +1,26 @@
 
 # This configuration file allows you to execute commands on various events.
 # %p in commands will be replaced with the player name.
-# For example, if you want to message a welcome message to a player who just registered:
+# For example, if you want to send a welcome message to a player who just registered:
 # onRegister:
 #   welcome:
 #     command: 'msg %p Welcome to the server!'
 #     as: CONSOLE
-# 
+#
 # This will make the console execute the msg command to the player.
 # Each command under an event has a name you can choose freely (e.g. 'welcome' as above),
-# after which a mandatory 'command' field defines the command to run, 
+# after which a mandatory 'command' field defines the command to run,
 # and 'as' defines who will run the command (either PLAYER or CONSOLE). Longer example:
 # onLogin:
 #   welcome:
 #     command: 'msg %p Welcome back!'
-#     # as: PLAYER  # player is the default, you can leave this out if you want
+#     as: PLAYER
 #   broadcast:
 #     command: 'broadcast %p has joined, welcome back!'
 #     as: CONSOLE
+#
+# Supported command events: onLogin, onJoin, onRegister
 onLogin:
     welcome:
         command: 'msg %p Welcome back!'
-        executor: 'PLAYER'
+        executor: 'PLAYER'

src/test/java/fr/xephi/authme/command/executable/authme/ForceLoginCommandTest.java

@@ -15,7 +15,6 @@ import org.mockito.runners.MockitoJUnitRunner;
 import java.util.Collections;
 
 import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.equalTo;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -53,7 +52,7 @@ public class ForceLoginCommandTest {
 
         // then
         verify(bukkitService).getPlayerExact(playerName);
-        verify(sender).sendMessage(argThat(equalTo("Player needs to be online!")));
+        verify(sender).sendMessage("Player needs to be online!");
         verifyZeroInteractions(management);
     }
 
@@ -69,7 +68,7 @@ public class ForceLoginCommandTest {
 
         // then
         verify(bukkitService).getPlayerExact(playerName);
-        verify(sender).sendMessage(argThat(equalTo("Player needs to be online!")));
+        verify(sender).sendMessage("Player needs to be online!");
         verifyZeroInteractions(management);
     }
 

src/test/java/fr/xephi/authme/message/MessagesIntegrationTest.java

@@ -160,7 +160,7 @@ public class MessagesIntegrationTest {
         messages.send(sender, key);
 
         // then
-        verify(sender).sendMessage(argThat(equalTo("Use /captcha THE_CAPTCHA to solve the captcha")));
+        verify(sender).sendMessage("Use /captcha THE_CAPTCHA to solve the captcha");
     }
 
     @Test

src/test/java/fr/xephi/authme/settings/SettingsTest.java

@@ -83,6 +83,24 @@ public class SettingsTest {
         assertThat(result, equalTo(emailMessage));
     }
 
+    @Test
+    public void shouldLoadRecoveryCodeMessage() throws IOException {
+        // given
+        String emailMessage = "Your recovery code is %code.";
+        File emailFile = new File(testPluginFolder, "recovery_code_email.html");
+        createFile(emailFile);
+        Files.write(emailFile.toPath(), emailMessage.getBytes());
+
+        PropertyResource resource = mock(PropertyResource.class);
+        Settings settings = new Settings(testPluginFolder, resource, null, CONFIG_DATA);
+
+        // when
+        String result = settings.getRecoveryCodeEmailMessage();
+
+        // then
+        assertThat(result, equalTo(emailMessage));
+    }
+
     private static void createFile(File file) {
         try {
             file.getParentFile().mkdirs();

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

@@ -19,7 +19,7 @@ final class CommandConfigTestHelper {
      * @param executor the expected executor
      * @return the matcher
      */
-    public static Matcher<Command> isCommand(String cmd, Executor executor) {
+    static Matcher<Command> isCommand(String cmd, Executor executor) {
         return new TypeSafeMatcher<Command>() {
             @Override
             protected boolean matchesSafely(Command item) {

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

@@ -4,26 +4,33 @@ 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.settings.Settings;
+import fr.xephi.authme.settings.SettingsMigrationService;
-import org.bukkit.configuration.file.FileConfiguration;
+import org.bukkit.entity.Player;
-import org.bukkit.configuration.file.YamlConfiguration;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.runners.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.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.only;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 /**
  * Test for {@link CommandManager}.
@@ -33,16 +40,18 @@ public class CommandManagerTest {
 
     private static final String TEST_FILES_FOLDER = "/fr/xephi/authme/settings/commandconfig/";
 
+    private CommandManager manager;
+    @InjectMocks
+    private CommandsMigrater commandsMigrater;
+
     @Rule
     public TemporaryFolder temporaryFolder = new TemporaryFolder();
 
-    private CommandManager manager;
     @Mock
     private BukkitService bukkitService;
     @Mock
-    private Settings settings;
+    private SettingsMigrationService settingsMigrationService;
-    @Mock
+
-    private CommandsMigrater commandsMigrater;
     private File testFolder;
 
     @Before
@@ -75,10 +84,9 @@ public class CommandManagerTest {
     }
 
     @Test
-    @Ignore // TODO #411: Implement tested behavior
     public void shouldLoadIncompleteFile() {
         // given
-        File configFile = copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.incomplete.yml");
+        copyJarFileAsCommandsYml(TEST_FILES_FOLDER + "commands.incomplete.yml");
 
         // when
         initManager();
@@ -90,23 +98,74 @@ public class CommandManagerTest {
             isCommand("msg %p Welcome back", Executor.CONSOLE),
             isCommand("list", Executor.PLAYER)));
         assertThat(commandConfig.getOnRegister(), anEmptyMap());
+    }
 
-        // verify that we have rewritten the file with the proper sections
+    @Test
-        FileConfiguration configuration = YamlConfiguration.loadConfiguration(configFile);
+    public void shouldExecuteCommandsOnJoin() {
-        assertThat(configuration.contains("onRegister"), equalTo(true));
+        // given
-        assertThat(configuration.contains("doesNotExist"), equalTo(false));
+        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";
+
+        // when
+        testCommandExecution(name, CommandManager::runCommandsOnLogin);
+
+        // then
+        verify(bukkitService).dispatchConsoleCommand(format("msg %s Welcome back", name));
+        verify(bukkitService).dispatchCommand(any(Player.class), eq("motd"));
+        verify(bukkitService).dispatchCommand(any(Player.class), eq("list"));
+        verifyNoMoreInteractions(bukkitService);
+    }
+
+    @Test
+    public void shouldHaveHiddenConstructorInSettingsHolderClass() {
+        // given / when / then
+        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, commandsMigrater, settings);
+        manager = new CommandManager(testFolder, bukkitService, commandsMigrater);
     }
 
-    private File copyJarFileAsCommandsYml(String path) {
+    private void copyJarFileAsCommandsYml(String path) {
         File source = TestHelper.getJarFile(path);
         File destination = new File(testFolder, "commands.yml");
         try {
             Files.copy(source, destination);
-            return destination;
         } catch (IOException e) {
             throw new IllegalStateException(e);
         }

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

@@ -0,0 +1,60 @@
+package fr.xephi.authme.settings.commandconfig;
+
+import com.github.authme.configme.knownproperties.ConfigurationDataBuilder;
+import com.github.authme.configme.resource.PropertyResource;
+import com.github.authme.configme.resource.YamlFileResource;
+import fr.xephi.authme.TestHelper;
+import fr.xephi.authme.settings.SettingsMigrationService;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import java.io.File;
+import java.util.Collections;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
+import static org.mockito.BDDMockito.given;
+
+/**
+ * Tests that commands.yml is well-formed.
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class CommandYmlConsistencyTest {
+
+    @InjectMocks
+    private CommandsMigrater commandsMigrater;
+
+    @Mock
+    private SettingsMigrationService settingsMigrationService;
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+    @Before
+    public void setUpSettingsMigrationService() {
+        given(settingsMigrationService.getOnLoginCommands()).willReturn(Collections.emptyList());
+        given(settingsMigrationService.getOnLoginConsoleCommands()).willReturn(Collections.emptyList());
+        given(settingsMigrationService.getOnRegisterCommands()).willReturn(Collections.emptyList());
+        given(settingsMigrationService.getOnRegisterConsoleCommands()).willReturn(Collections.emptyList());
+    }
+
+    @Test
+    public void shouldLoadWithNoMigrations() {
+        // given
+        File commandFile = TestHelper.getJarFile("/commands.yml");
+        PropertyResource resource = new YamlFileResource(commandFile);
+
+        // when
+        boolean result = commandsMigrater.checkAndMigrate(
+            resource, ConfigurationDataBuilder.collectData(CommandSettingsHolder.class).getProperties());
+
+        // then
+        assertThat(result, equalTo(false));
+    }
+}

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

@@ -1,14 +1,19 @@
 package fr.xephi.authme.settings.commandconfig;
 
+import com.github.authme.configme.knownproperties.ConfigurationDataBuilder;
+import com.github.authme.configme.resource.PropertyResource;
+import com.github.authme.configme.resource.YamlFileResource;
 import fr.xephi.authme.TestHelper;
 import fr.xephi.authme.settings.SettingsMigrationService;
 import org.junit.BeforeClass;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
 
+import java.io.File;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -108,4 +113,19 @@ public class CommandsMigraterTest {
             isCommand("log %p registered", Executor.CONSOLE),
             isCommand("whois %p", Executor.CONSOLE)));
     }
+
+    @Test
+    @Ignore // TODO ConfigMe/#29: Create PropertyResource#getKeys
+    public void shouldRewriteForEmptyFile() {
+        // given
+        File commandFile = TestHelper.getJarFile("/fr/xephi/authme/settings/commandconfig/commands.empty.yml");
+        PropertyResource resource = new YamlFileResource(commandFile);
+
+        // when
+        boolean result = commandsMigrater.checkAndMigrate(
+            resource, ConfigurationDataBuilder.collectData(CommandSettingsHolder.class).getProperties());
+
+        // then
+        assertThat(result, equalTo(true));
+    }
 }

src/test/java/fr/xephi/authme/settings/properties/AuthMeSettingsRetrieverTest.java

@@ -0,0 +1,32 @@
+package fr.xephi.authme.settings.properties;
+
+import com.github.authme.configme.knownproperties.ConfigurationData;
+import fr.xephi.authme.TestHelper;
+import org.junit.Test;
+
+import static org.hamcrest.Matchers.closeTo;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Test for {@link AuthMeSettingsRetriever}.
+ */
+public class AuthMeSettingsRetrieverTest {
+
+    @Test
+    public void shouldReturnAllProperties() {
+        // given / when
+        ConfigurationData configurationData = AuthMeSettingsRetriever.buildConfigurationData();
+
+        // then
+        // Note ljacqu 20161123: Check that the number of properties returned corresponds to what we expect with
+        // an error margin of 10: this prevents us from having to adjust the test every time the config is changed.
+        // If this test fails, replace the first argument in closeTo() with the new number of properties
+        assertThat((double) configurationData.getProperties().size(),
+            closeTo(150, 10));
+    }
+
+    @Test
+    public void shouldHaveHiddenConstructor() {
+        TestHelper.validateHasOnlyPrivateEmptyConstructor(AuthMeSettingsRetriever.class);
+    }
+}