#482 Provide better default messages and merge MessagesManager

- Return message from the JAR's messages_en.yml file instead of an unusable error for the end user
- Merge MessagesManager into Messages

src/main/java/fr/xephi/authme/AuthMe.java

@@ -231,7 +231,7 @@ public class AuthMe extends JavaPlugin {
             return;
         }
 
-        messages = new Messages(newSettings.getMessagesFile());
+        messages = new Messages(newSettings.getMessagesFile(), newSettings.getDefaultMessagesFile());
 
         // Connect to the database and setup tables
         try {

src/main/java/fr/xephi/authme/output/Messages.java

@@ -1,7 +1,11 @@
 package fr.xephi.authme.output;
 
+import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.util.StringUtils;
+import org.bukkit.ChatColor;
 import org.bukkit.command.CommandSender;
+import org.bukkit.configuration.file.FileConfiguration;
+import org.bukkit.configuration.file.YamlConfiguration;
 
 import java.io.File;
 
@@ -12,15 +16,19 @@ import java.io.File;
  */
 public class Messages {
 
-    private MessagesManager manager;
+    private FileConfiguration configuration;
+    private String fileName;
+    private File defaultFile;
+    private FileConfiguration defaultConfiguration;
 
     /**
      * Constructor.
      *
      * @param messageFile The messages file to use
      */
-    public Messages(File messageFile) {
-        manager = new MessagesManager(messageFile);
+    public Messages(File messageFile, File defaultFile) {
+        initializeFile(messageFile);
+        this.defaultFile = defaultFile;
     }
 
     /**
@@ -30,7 +38,7 @@ public class Messages {
      * @param key The key of the message to send
      */
     public void send(CommandSender sender, MessageKey key) {
-        String[] lines = manager.retrieve(key.getKey());
+        String[] lines = retrieve(key);
         for (String line : lines) {
             sender.sendMessage(line);
         }
@@ -38,7 +46,7 @@ public class Messages {
 
     /**
      * Send the given message code to the player with the given tag replacements. Note that this method
-     * issues an exception if the number of supplied replacements doesn't correspond to the number of tags
+     * logs an error if the number of supplied replacements doesn't correspond to the number of tags
      * the message key contains.
      *
      * @param sender The entity to send the message to
@@ -48,13 +56,13 @@ public class Messages {
     public void send(CommandSender sender, MessageKey key, String... replacements) {
         String message = retrieveSingle(key);
         String[] tags = key.getTags();
-        if (replacements.length != tags.length) {
-            throw new IllegalStateException(
-                "Given replacement size does not match the tags in message key '" + key + "'");
-        }
-
-        for (int i = 0; i < tags.length; ++i) {
-            message = message.replace(tags[i], replacements[i]);
+        if (replacements.length == tags.length) {
+            for (int i = 0; i < tags.length; ++i) {
+                message = message.replace(tags[i], replacements[i]);
+            }
+        } else {
+            ConsoleLogger.showError("Invalid number of replacements for message key '" + key + "'");
+            send(sender, key);
         }
 
         for (String line : message.split("\n")) {
@@ -66,11 +74,18 @@ public class Messages {
      * Retrieve the message from the text file and return it split by new line as an array.
      *
      * @param key The message key to retrieve
-     *
      * @return The message split by new lines
      */
     public String[] retrieve(MessageKey key) {
-        return manager.retrieve(key.getKey());
+        final String code = key.getKey();
+        String message = configuration.getString(code);
+
+        if (message == null) {
+            ConsoleLogger.showError("Error getting message with key '" + code + "'. "
+                + "Please verify your config file at '" + fileName + "'");
+            return formatMessage(getDefault(code));
+        }
+        return formatMessage(message);
     }
 
     /**
@@ -88,7 +103,38 @@ public class Messages {
      * Reload the messages manager.
      */
     public void reload(File messagesFile) {
-        manager = new MessagesManager(messagesFile);
+        initializeFile(messagesFile);
+    }
+
+    private void initializeFile(File messageFile) {
+        this.configuration = YamlConfiguration.loadConfiguration(messageFile);
+        this.fileName = messageFile.getName();
+    }
+
+    private String getDefault(String code) {
+        if (defaultFile == null) {
+            return getDefaultErrorMessage(code);
+        }
+
+        if (defaultConfiguration == null) {
+            defaultConfiguration = YamlConfiguration.loadConfiguration(defaultFile);
+        }
+        String message = defaultConfiguration.getString(code);
+        return (message == null)
+            ? "Error retrieving message '" + code + "'"
+            : message;
+    }
+
+    private static String getDefaultErrorMessage(String code) {
+        return "Error retrieving message '" + code + "'";
+    }
+
+    private static String[] formatMessage(String message) {
+        String[] lines = message.split("&n");
+        for (int i = 0; i < lines.length; ++i) {
+            lines[i] = ChatColor.translateAlternateColorCodes('&', lines[i]);
+        }
+        return lines;
     }
 
 }

src/main/java/fr/xephi/authme/output/MessagesManager.java

@@ -1,58 +0,0 @@
-package fr.xephi.authme.output;
-
-import fr.xephi.authme.ConsoleLogger;
-import org.bukkit.ChatColor;
-import org.bukkit.configuration.file.YamlConfiguration;
-
-import java.io.File;
-
-/**
- * Class responsible for reading messages from a file and formatting them for Minecraft.
- * <p>
- * This class is used within {@link Messages}, which offers a high-level interface for accessing
- * or sending messages from a properties file.
- */
-class MessagesManager {
-
-    private final YamlConfiguration configuration;
-    private final String fileName;
-
-    /**
-     * Constructor for Messages.
-     *
-     * @param file the configuration file
-     */
-    MessagesManager(File file) {
-        this.fileName = file.getName();
-        this.configuration = YamlConfiguration.loadConfiguration(file);
-    }
-
-    /**
-     * Retrieve the message from the configuration file.
-     *
-     * @param key The key to retrieve
-     *
-     * @return The message
-     */
-    public String[] retrieve(String key) {
-        String message = configuration.getString(key);
-        if (message != null) {
-            return formatMessage(message);
-        }
-
-        // Message is null: log key not being found and send error back as message
-        String retrievalError = "Error getting message with key '" + key + "'. ";
-        ConsoleLogger.showError(retrievalError + "Please verify your config file at '" + fileName + "'");
-        return new String[]{
-            retrievalError + "Please contact the admin to verify or update the AuthMe messages file."};
-    }
-
-    private static String[] formatMessage(String message) {
-        String[] lines = message.split("&n");
-        for (int i = 0; i < lines.length; ++i) {
-            lines[i] = ChatColor.translateAlternateColorCodes('&', lines[i]);
-        }
-        return lines;
-    }
-
-}

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

@@ -18,6 +18,7 @@ import org.yaml.snakeyaml.Yaml;
 import java.io.File;
 import java.io.FileWriter;
 import java.io.IOException;
+import java.net.URL;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -108,6 +109,21 @@ public class NewSetting {
         return messagesFile;
     }
 
+    /**
+     * Return the default messages file within the JAR that should contain all messages.
+     *
+     * @return The default messages file, or {@code null} if it could not be retrieved
+     */
+    public File getDefaultMessagesFile() {
+        String defaultFilePath = "/messages/messages_en.yml";
+        URL url = NewSetting.class.getResource(defaultFilePath);
+        if (url == null) {
+            return null;
+        }
+        File file = new File(url.getFile());
+        return file.exists() ? file : null;
+    }
+
     public String getEmailMessage() {
         return emailMessage;
     }

src/test/java/fr/xephi/authme/TestHelper.java

@@ -0,0 +1,28 @@
+package fr.xephi.authme;
+
+import java.io.File;
+import java.net.URL;
+
+/**
+ * AuthMe test utilities.
+ */
+public final class TestHelper {
+
+    private TestHelper() {
+    }
+
+    /**
+     * Return a {@link File} to an existing file from the main (non-test) resources folder.
+     *
+     * @param path The absolute path to the file
+     * @return The project file
+     */
+    public static File getJarFile(String path) {
+        URL url = TestHelper.class.getResource(path);
+        if (url == null) {
+            throw new IllegalStateException("File '" + path + "' could not be loaded");
+        }
+        return new File(url.getFile());
+    }
+
+}

src/test/java/fr/xephi/authme/output/MessagesFileConsistencyTest.java

@@ -0,0 +1,43 @@
+package fr.xephi.authme.output;
+
+import fr.xephi.authme.TestHelper;
+import fr.xephi.authme.util.StringUtils;
+import org.bukkit.configuration.file.FileConfiguration;
+import org.bukkit.configuration.file.YamlConfiguration;
+import org.junit.Test;
+
+import java.io.File;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Tests that the project's default language file contains a text for all message keys.
+ */
+public class MessagesFileConsistencyTest {
+
+    private static final String DEFAULT_MESSAGES_FILE = "/messages/messages_en.yml";
+
+    @Test
+    public void shouldHaveAllMessages() {
+        File file = TestHelper.getJarFile(DEFAULT_MESSAGES_FILE);
+        FileConfiguration configuration = YamlConfiguration.loadConfiguration(file);
+        for (MessageKey messageKey : MessageKey.values()) {
+            verifyHasMessage(messageKey, configuration);
+        }
+    }
+
+    private static void verifyHasMessage(MessageKey messageKey, FileConfiguration configuration) {
+        final String key = messageKey.getKey();
+        final String message = configuration.getString(key);
+
+        assertThat("Default messages file should have message for key '" + key + "'",
+            StringUtils.isEmpty(message), equalTo(false));
+
+
+        for (String tag : messageKey.getTags()) {
+            assertThat("The message for key '" + key + "' contains the tag '" + tag + "' in the default messages file",
+                message.contains(tag), equalTo(true));
+        }
+    }
+}

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

@@ -1,6 +1,8 @@
 package fr.xephi.authme.output;
 
+import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.ConsoleLoggerTestInitializer;
+import fr.xephi.authme.TestHelper;
 import fr.xephi.authme.util.WrapperMock;
 import org.bukkit.command.CommandSender;
 import org.bukkit.entity.Player;
@@ -11,11 +13,11 @@ import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
 
 import java.io.File;
-import java.net.URL;
+import java.util.logging.Logger;
 
 import static org.hamcrest.Matchers.arrayWithSize;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.startsWith;
 import static org.junit.Assert.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
@@ -26,7 +28,8 @@ import static org.mockito.Mockito.verify;
  */
 public class MessagesIntegrationTest {
 
-    private static final String YML_TEST_FILE = "messages_test.yml";
+    private static final String YML_TEST_FILE = "/messages_test.yml";
+    private static final String YML_DEFAULT_TEST_FILE = "/messages_default.yml";
     private Messages messages;
 
     @BeforeClass
@@ -39,15 +42,15 @@ public class MessagesIntegrationTest {
      * Loads the messages in the file {@code messages_test.yml} in the test resources folder.
      * The file does not contain all messages defined in {@link MessageKey} and its contents
      * reflect various test cases -- not what the keys stand for.
+     * <p>
+     * Similarly, the {@code messages_default.yml} from the test resources represents a default
+     * file that should contain all messages, but again, for testing, it just contains a few.
      */
     @Before
     public void setUpMessages() {
-        URL url = getClass().getClassLoader().getResource(YML_TEST_FILE);
-        if (url == null) {
-            throw new RuntimeException("File '" + YML_TEST_FILE + "' could not be loaded");
-        }
-
-        messages = new Messages(new File(url.getFile()));
+        File testFile = TestHelper.getJarFile(YML_TEST_FILE);
+        File defaultFile = TestHelper.getJarFile(YML_DEFAULT_TEST_FILE);
+        messages = new Messages(testFile, defaultFile);
     }
 
     @Test
@@ -101,20 +104,6 @@ public class MessagesIntegrationTest {
         assertThat(message[0], equalTo("Apostrophes ' should be loaded correctly, don't you think?"));
     }
 
-    @Test
-    public void shouldReturnErrorForUnknownCode() {
-        // given
-        // The following is a key that is not defined in the test file
-        MessageKey key = MessageKey.UNREGISTERED_SUCCESS;
-
-        // when
-        String[] message = messages.retrieve(key);
-
-        // then
-        assertThat(message, arrayWithSize(1));
-        assertThat(message[0], startsWith("Error getting message with key '"));
-    }
-
     @Test
     public void shouldSendMessageToPlayer() {
         // given
@@ -176,25 +165,88 @@ public class MessagesIntegrationTest {
         assertThat(message, equalTo("Use /captcha THE_CAPTCHA to solve the captcha"));
     }
 
-    @Test(expected = RuntimeException.class)
-    public void shouldThrowForInvalidReplacementCount() {
+    @Test
+    public void shouldLogErrorForInvalidReplacementCount() {
         // given
+        Logger logger = mock(Logger.class);
+        ConsoleLogger.setLogger(logger);
         MessageKey key = MessageKey.CAPTCHA_WRONG_ERROR;
 
         // when
         messages.send(mock(CommandSender.class), key, "rep", "rep2");
 
-        // then - expect exception
+        // then
+        ArgumentCaptor<String> captor = ArgumentCaptor.forClass(String.class);
+        verify(logger).warning(captor.capture());
+        assertThat(captor.getValue(), containsString("Invalid number of replacements"));
     }
 
-    @Test(expected = RuntimeException.class)
+    @Test
     public void shouldThrowForReplacementsOnKeyWithNoTags() {
         // given
+        Logger logger = mock(Logger.class);
+        ConsoleLogger.setLogger(logger);
         MessageKey key = MessageKey.UNKNOWN_USER;
 
         // when
         messages.send(mock(CommandSender.class), key, "Replacement");
 
-        // then - expect exception
+        // then
+        ArgumentCaptor<String> captor = ArgumentCaptor.forClass(String.class);
+        verify(logger).warning(captor.capture());
+        assertThat(captor.getValue(), containsString("Invalid number of replacements"));
+    }
+
+    @Test
+    public void shouldGetMessageFromDefaultFile() {
+        // given
+        // Key is only present in default file
+        MessageKey key = MessageKey.MUST_REGISTER_MESSAGE;
+
+        // when
+        String message = messages.retrieveSingle(key);
+
+        // then
+        assertThat(message, equalTo("Message from default file"));
+    }
+
+    @Test
+    public void shouldNotUseMessageFromDefaultFile() {
+        // given
+        // Key is present in both files
+        MessageKey key = MessageKey.WRONG_PASSWORD;
+
+        // when
+        String message = messages.retrieveSingle(key);
+
+        // then
+        assertThat(message, equalTo("§cWrong password!"));
+    }
+
+    @Test
+    public void shouldReturnErrorForMissingMessage() {
+        // given
+        // Key is not present in test file or default file
+        MessageKey key = MessageKey.TWO_FACTOR_CREATE;
+
+        // when
+        String message = messages.retrieveSingle(key);
+
+        // then
+        assertThat(message, containsString("Error retrieving message"));
+    }
+
+    @Test
+    public void shouldAllowNullAsDefaultFile() {
+        // given
+        Messages testMessages = new Messages(TestHelper.getJarFile(YML_TEST_FILE), null);
+        // Key not present in test file
+        MessageKey key = MessageKey.TWO_FACTOR_CREATE;
+
+        // when
+        String message = testMessages.retrieveSingle(key);
+
+        // then
+        assertThat(message, containsString("Error retrieving message"));
     }
 }

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

@@ -1,5 +1,6 @@
 package fr.xephi.authme.settings;
 
+import fr.xephi.authme.TestHelper;
 import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.properties.SettingsFieldRetriever;
 import fr.xephi.authme.settings.propertymap.PropertyMap;
@@ -32,8 +33,7 @@ public class ConfigFileConsistencyTest {
     @Test
     public void shouldHaveAllConfigs() throws IOException {
         // given
-        URL url = this.getClass().getResource(CONFIG_FILE);
-        File configFile = new File(url.getFile());
+        File configFile = TestHelper.getJarFile(CONFIG_FILE);
         FileConfiguration configuration = YamlConfiguration.loadConfiguration(configFile);
 
         // when

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

@@ -12,12 +12,12 @@ import org.junit.Test;
 
 import java.io.File;
 import java.lang.reflect.Field;
-import java.net.URL;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
+import static fr.xephi.authme.TestHelper.getJarFile;
 import static fr.xephi.authme.settings.domain.Property.newProperty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
@@ -29,18 +29,18 @@ import static org.junit.Assume.assumeThat;
 public class NewSettingIntegrationTest {
 
     /** File name of the sample config including all {@link TestConfiguration} values. */
-    private static final String COMPLETE_FILE = "config-sample-values.yml";
+    private static final String COMPLETE_FILE = "/config-sample-values.yml";
     /** File name of the sample config missing certain {@link TestConfiguration} values. */
-    private static final String INCOMPLETE_FILE = "config-incomplete-sample.yml";
+    private static final String INCOMPLETE_FILE = "/config-incomplete-sample.yml";
     /** File name for testing difficult values. */
-    private static final String DIFFICULT_FILE = "config-difficult-values.yml";
+    private static final String DIFFICULT_FILE = "/config-difficult-values.yml";
 
     private static PropertyMap propertyMap = generatePropertyMap();
 
     @Test
     public void shouldLoadAndReadAllProperties() {
         // given
-        YamlConfiguration configuration = YamlConfiguration.loadConfiguration(getConfigFile(COMPLETE_FILE));
+        YamlConfiguration configuration = YamlConfiguration.loadConfiguration(getJarFile(COMPLETE_FILE));
         File file = new File("unused");
 
         // when / then
@@ -67,7 +67,7 @@ public class NewSettingIntegrationTest {
     @Test
     public void shouldWriteMissingProperties() {
         // given/when
-        File file = getConfigFile(INCOMPLETE_FILE);
+        File file = getJarFile(INCOMPLETE_FILE);
         YamlConfiguration configuration = YamlConfiguration.loadConfiguration(file);
         assumeThat(configuration.contains(TestConfiguration.BORING_COLORS.getPath()), equalTo(false));
         // Expectation: File is rewritten to since it does not have all configurations
@@ -100,7 +100,7 @@ public class NewSettingIntegrationTest {
     @Test
     public void shouldProperlyExportAnyValues() {
         // given
-        File file = getConfigFile(DIFFICULT_FILE);
+        File file = getJarFile(DIFFICULT_FILE);
         YamlConfiguration configuration = YamlConfiguration.loadConfiguration(file);
         assumeThat(configuration.contains(TestConfiguration.DUST_LEVEL.getPath()), equalTo(false));
 
@@ -147,19 +147,6 @@ public class NewSettingIntegrationTest {
         }
     }
 
-    /**
-     * Return a {@link File} instance to an existing file in the target/test-classes folder.
-     *
-     * @return The generated File
-     */
-    private File getConfigFile(String file) {
-        URL url = getClass().getClassLoader().getResource(file);
-        if (url == null) {
-            throw new IllegalStateException("File '" + file + "' could not be loaded");
-        }
-        return new File(url.getFile());
-    }
-
     /**
      * Generate a property map with all properties in {@link TestConfiguration}.
      *

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

@@ -3,7 +3,6 @@ package fr.xephi.authme.settings;
 import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.properties.TestConfiguration;
 import fr.xephi.authme.settings.properties.TestEnum;
-import fr.xephi.authme.settings.propertymap.PropertyMap;
 import org.bukkit.configuration.file.YamlConfiguration;
 import org.junit.Test;
 import org.mockito.invocation.InvocationOnMock;
@@ -13,6 +12,8 @@ import java.io.File;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Matchers.anyBoolean;
 import static org.mockito.Matchers.anyDouble;
@@ -30,19 +31,19 @@ public class NewSettingTest {
     @Test
     public void shouldLoadAllConfigs() {
         // given
-        YamlConfiguration file = mock(YamlConfiguration.class);
-        given(file.getString(anyString(), anyString())).willAnswer(withDefaultArgument());
-        given(file.getBoolean(anyString(), anyBoolean())).willAnswer(withDefaultArgument());
-        given(file.getDouble(anyString(), anyDouble())).willAnswer(withDefaultArgument());
-        given(file.getInt(anyString(), anyInt())).willAnswer(withDefaultArgument());
+        YamlConfiguration configuration = mock(YamlConfiguration.class);
+        given(configuration.getString(anyString(), anyString())).willAnswer(withDefaultArgument());
+        given(configuration.getBoolean(anyString(), anyBoolean())).willAnswer(withDefaultArgument());
+        given(configuration.getDouble(anyString(), anyDouble())).willAnswer(withDefaultArgument());
+        given(configuration.getInt(anyString(), anyInt())).willAnswer(withDefaultArgument());
 
-        setReturnValue(file, TestConfiguration.VERSION_NUMBER, 20);
-        setReturnValue(file, TestConfiguration.SKIP_BORING_FEATURES, true);
-        setReturnValue(file, TestConfiguration.RATIO_ORDER, TestEnum.THIRD);
-        setReturnValue(file, TestConfiguration.SYSTEM_NAME, "myTestSys");
+        setReturnValue(configuration, TestConfiguration.VERSION_NUMBER, 20);
+        setReturnValue(configuration, TestConfiguration.SKIP_BORING_FEATURES, true);
+        setReturnValue(configuration, TestConfiguration.RATIO_ORDER, TestEnum.THIRD);
+        setReturnValue(configuration, TestConfiguration.SYSTEM_NAME, "myTestSys");
 
         // when / then
-        NewSetting settings = new NewSetting(file, new File("conf.txt"), (PropertyMap) null);
+        NewSetting settings = new NewSetting(configuration, null, null);
 
         assertThat(settings.getProperty(TestConfiguration.VERSION_NUMBER), equalTo(20));
         assertThat(settings.getProperty(TestConfiguration.SKIP_BORING_FEATURES), equalTo(true));
@@ -54,6 +55,20 @@ public class NewSettingTest {
         assertDefaultValue(TestConfiguration.COOL_OPTIONS, settings);
     }
 
+    @Test
+    public void shouldReturnDefaultFile() {
+        // given
+        YamlConfiguration configuration = mock(YamlConfiguration.class);
+        NewSetting settings = new NewSetting(configuration, null, null);
+
+        // when
+        File defaultFile = settings.getDefaultMessagesFile();
+
+        // then
+        assertThat(defaultFile, not(nullValue()));
+        assertThat(defaultFile.exists(), equalTo(true));
+    }
+
     private static <T> void setReturnValue(YamlConfiguration config, Property<T> property, T value) {
         if (value instanceof String) {
             when(config.getString(eq(property.getPath()), anyString())).thenReturn((String) value);

src/test/resources/messages_default.yml

@@ -0,0 +1,5 @@
+# Simulates a default file
+
+wrong_pwd: 'This message is overridden in messages_test.yml'
+reg_only: 'Message from default file'
+logged_in: '&cYou''re already logged in!'

src/test/resources/messages_test.yml

@@ -1,3 +1,5 @@
+# Sample messages file
+
 unknown_user: 'This test message&nincludes&nsome new lines'
 unsafe_spawn: '&cHere we have&bdefined some colors &dand some other &lthings'
 not_logged_in: 'Apostrophes '' should be loaded correctly, don''t you think?'