Fix TeleportationService tests + rename methods

- Fix and supplement unit tests for TeleportationService
- Rename methods as to avoid confusion (login vs. LoginEvent when player joins)
- Add javadoc with note about Player#hasPlayedBefore always being false

src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java

@@ -194,7 +194,7 @@ public class AuthMePlayerListener implements Listener {
     @EventHandler(priority = EventPriority.LOW)
     public void onPlayerJoin(PlayerJoinEvent event) {
         final Player player = event.getPlayer();
-        teleportationService.teleportOnJoin(player);
+        teleportationService.teleportNewPlayerToFirstSpawn(player);
         management.performJoin(player);
     }
 
@@ -233,7 +233,7 @@ public class AuthMePlayerListener implements Listener {
         }
 
         antiBot.handlePlayerJoin(player);
-        teleportationService.teleportOnLoginEvent(player);
+        teleportationService.teleportOnJoin(player);
     }
 
     @EventHandler(priority = EventPriority.HIGHEST)

src/main/java/fr/xephi/authme/util/TeleportationService.java

@@ -44,10 +44,6 @@ public class TeleportationService implements Reloadable {
     TeleportationService() {
     }
 
-    private static boolean isEventValid(AbstractTeleportEvent event) {
-        return !event.isCancelled() && event.getTo() != null && event.getTo().getWorld() != null;
-    }
-
     @PostConstruct
     @Override
     public void reload() {
@@ -55,7 +51,18 @@ public class TeleportationService implements Reloadable {
         spawnOnLoginWorlds = new HashSet<>(settings.getProperty(RestrictionSettings.FORCE_SPAWN_ON_WORLDS));
     }
 
-    public void teleportOnLoginEvent(final Player player) {
+    /**
+     * Teleports the player according to the settings when he joins.
+     * <p>
+     * Note: this is triggered by Bukkit's PlayerLoginEvent, during which you cannot use
+     * {@link Player#hasPlayedBefore()}: it always returns {@code false}. We trigger teleportation
+     * from the PlayerLoginEvent and not the PlayerJoinEvent to ensure that the location is overridden
+     * as fast as possible (cf. <a href="https://github.com/Xephi/AuthMeReloaded/issues/682">AuthMe #682</a>).
+     *
+     * @param player the player to process
+     * @see <a href="https://bukkit.atlassian.net/browse/BUKKIT-3521">BUKKIT-3521: Player.hasPlayedBefore() always false</a>
+     */
+    public void teleportOnJoin(final Player player) {
         if (settings.getProperty(RestrictionSettings.NO_TELEPORT)) {
             return;
         }
@@ -65,13 +72,30 @@ public class TeleportationService implements Reloadable {
         }
     }
 
-    public void teleportOnJoin(final Player player) {
+    /**
-        if (settings.getProperty(RestrictionSettings.NO_TELEPORT)) {
+     * Teleports the player to the first spawn if he is new and the first spawn is configured.
+     *
+     * @param player the player to process
+     */
+    public void teleportNewPlayerToFirstSpawn(final Player player) {
+        if (settings.getProperty(RestrictionSettings.NO_TELEPORT) || player.hasPlayedBefore()) {
             return;
         }
-        teleportToFirstSpawn(player);
+        Location firstSpawn = spawnLoader.getFirstSpawn();
+        if (firstSpawn == null) {
+            return;
+        }
+
+        performTeleportation(player, new FirstSpawnTeleportEvent(player, firstSpawn));
     }
 
+    /**
+     * Teleports the player according to the settings after having successfully logged in.
+     *
+     * @param player the player
+     * @param auth corresponding PlayerAuth object
+     * @param limbo corresponding LimboPlayer object
+     */
     public void teleportOnLogin(final Player player, PlayerAuth auth, LimboPlayer limbo) {
         if (settings.getProperty(RestrictionSettings.NO_TELEPORT)) {
             return;
@@ -104,19 +128,6 @@ public class TeleportationService implements Reloadable {
         return new Location(world, auth.getQuitLocX(), auth.getQuitLocY(), auth.getQuitLocZ());
     }
 
-    private boolean teleportToFirstSpawn(final Player player) {
-        if (player.hasPlayedBefore()) {
-            return false;
-        }
-        Location firstSpawn = spawnLoader.getFirstSpawn();
-        if (firstSpawn == null) {
-            return false;
-        }
-
-        performTeleportation(player, new FirstSpawnTeleportEvent(player, firstSpawn));
-        return true;
-    }
-
     private void teleportBackFromSpawn(final Player player, final Location location) {
         performTeleportation(player, new AuthMeTeleportEvent(player, location));
     }
@@ -144,4 +155,8 @@ public class TeleportationService implements Reloadable {
             }
         });
     }
+
+    private static boolean isEventValid(AbstractTeleportEvent event) {
+        return !event.isCancelled() && event.getTo() != null && event.getTo().getWorld() != null;
+    }
 }

src/test/java/fr/xephi/authme/util/TeleportationServiceTest.java

@@ -38,7 +38,6 @@ import static org.mockito.Mockito.verifyZeroInteractions;
 /**
  * Test for {@link TeleportationService}.
  */
-// TODO: Correct me!
 @RunWith(MockitoJUnitRunner.class)
 public class TeleportationServiceTest {
 
@@ -57,20 +56,6 @@ public class TeleportationServiceTest {
     @Mock
     private PlayerCache playerCache;
 
-    // We check that the World in Location is set, this method creates a mock World in Location for us
-    private static Location mockLocation() {
-        Location location = mock(Location.class);
-        given(location.getWorld()).willReturn(mock(World.class));
-        return location;
-    }
-
-    private static PlayerAuth createAuthWithLocation() {
-        return PlayerAuth.builder()
-            .name("bobby")
-            .locX(123.45).locY(23.4).locZ(-4.567)
-            .build();
-    }
-
     @Before
     public void setUpForcedWorlds() {
         given(settings.getProperty(RestrictionSettings.FORCE_SPAWN_ON_WORLDS))
@@ -107,7 +92,7 @@ public class TeleportationServiceTest {
         given(spawnLoader.getFirstSpawn()).willReturn(firstSpawn);
 
         // when
-        teleportationService.teleportOnJoin(player);
+        teleportationService.teleportNewPlayerToFirstSpawn(player);
         runSyncDelayedTask(bukkitService);
 
         // then
@@ -122,13 +107,12 @@ public class TeleportationServiceTest {
         // given
         given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true);
         Player player = mock(Player.class);
-        given(player.hasPlayedBefore()).willReturn(true);
         given(player.isOnline()).willReturn(true);
         Location spawn = mockLocation();
         given(spawnLoader.getSpawnLocation(player)).willReturn(spawn);
 
         // when
-        teleportationService.teleportOnLoginEvent(player);
+        teleportationService.teleportOnJoin(player);
         runSyncDelayedTask(bukkitService);
 
         // then
@@ -150,8 +134,7 @@ public class TeleportationServiceTest {
         given(spawnLoader.getFirstSpawn()).willReturn(null);
 
         // when
-        teleportationService.teleportOnLoginEvent(player);
+        teleportationService.teleportNewPlayerToFirstSpawn(player);
-        teleportationService.teleportOnJoin(player);
 
         // then
         verify(player, never()).teleport(any(Location.class));
@@ -161,10 +144,39 @@ public class TeleportationServiceTest {
     }
 
     @Test
-    public void shouldTeleportPlayerDueToForcedWorld() {
+    public void shouldNotTeleportPlayerToFirstSpawnIfNoTeleportEnabled() {
+        // given
+        Player player = mock(Player.class);
+        given(player.hasPlayedBefore()).willReturn(false);
+        given(settings.getProperty(RestrictionSettings.NO_TELEPORT)).willReturn(true);
+
+        // when
+        teleportationService.teleportNewPlayerToFirstSpawn(player);
+
+        // then
+        verify(player, never()).teleport(any(Location.class));
+        verifyZeroInteractions(bukkitService);
+    }
+
+    @Test
+    public void shouldNotTeleportNotNewPlayerToFirstSpawn() {
         // given
         Player player = mock(Player.class);
         given(player.hasPlayedBefore()).willReturn(true);
+        given(settings.getProperty(RestrictionSettings.NO_TELEPORT)).willReturn(false);
+
+        // when
+        teleportationService.teleportNewPlayerToFirstSpawn(player);
+
+        // then
+        verify(player, never()).teleport(any(Location.class));
+        verifyZeroInteractions(bukkitService);
+    }
+
+    @Test
+    public void shouldTeleportPlayerDueToForcedWorld() {
+        // given
+        Player player = mock(Player.class);
         given(player.isOnline()).willReturn(true);
 
         World playerWorld = mock(World.class);
@@ -177,7 +189,6 @@ public class TeleportationServiceTest {
         given(spawnLoader.getSpawnLocation(player)).willReturn(spawn);
 
         // when
-        teleportationService.teleportOnLoginEvent(player);
         teleportationService.teleportOnJoin(player);
         runSyncDelayedTask(bukkitService);
 
@@ -191,7 +202,6 @@ public class TeleportationServiceTest {
     public void shouldNotTeleportPlayerForRemovedLocationInEvent() {
         // given
         final Player player = mock(Player.class);
-        given(player.hasPlayedBefore()).willReturn(true);
         Location spawn = mockLocation();
         given(spawnLoader.getSpawnLocation(player)).willReturn(spawn);
         given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true);
@@ -206,7 +216,6 @@ public class TeleportationServiceTest {
         }).when(bukkitService).callEvent(any(SpawnTeleportEvent.class));
 
         // when
-        teleportationService.teleportOnLoginEvent(player);
         teleportationService.teleportOnJoin(player);
         runSyncDelayedTask(bukkitService);
 
@@ -219,7 +228,6 @@ public class TeleportationServiceTest {
     public void shouldNotTeleportPlayerForCanceledEvent() {
         // given
         final Player player = mock(Player.class);
-        given(player.hasPlayedBefore()).willReturn(true);
         Location spawn = mockLocation();
         given(spawnLoader.getSpawnLocation(player)).willReturn(spawn);
         given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true);
@@ -234,7 +242,6 @@ public class TeleportationServiceTest {
         }).when(bukkitService).callEvent(any(SpawnTeleportEvent.class));
 
         // when
-        teleportationService.teleportOnLoginEvent(player);
         teleportationService.teleportOnJoin(player);
         runSyncDelayedTask(bukkitService);
 
@@ -415,11 +422,25 @@ public class TeleportationServiceTest {
         verify(player).teleport(location);
     }
 
-    private void assertCorrectLocation(Location location, PlayerAuth auth, World world) {
+    private static void assertCorrectLocation(Location location, PlayerAuth auth, World world) {
         assertThat(location.getX(), equalTo(auth.getQuitLocX()));
         assertThat(location.getY(), equalTo(auth.getQuitLocY()));
         assertThat(location.getZ(), equalTo(auth.getQuitLocZ()));
         assertThat(location.getWorld(), equalTo(world));
     }
 
+    // We check that the World in Location is set, this method creates a mock World in Location for us
+    private static Location mockLocation() {
+        Location location = mock(Location.class);
+        given(location.getWorld()).willReturn(mock(World.class));
+        return location;
+    }
+
+    private static PlayerAuth createAuthWithLocation() {
+        return PlayerAuth.builder()
+            .name("bobby")
+            .locX(123.45).locY(23.4).locZ(-4.567)
+            .build();
+    }
+
 }