Simplify last map update check parsing and set threshold for last che…

…ck to 7 days 1. Simplify: Rather than storing a formatted string for the last time we checked for map updates, just store the equivalent epoch milli timestamp. 2. Adjust threshold for checking for maps to be every 7 days rather than once a month. The once a month made more sense when the map list was hosted on source forge and the download time was very slow, with a fast download of the map list we can check more often and thereby get better responsiveness to map version updates. Fixes last map-date-check parsing error reported in: #7014
triplea-game · Jul 25, 2020 · 1e172d5 · 1e172d5
1 parent 83da4b0
commit 1e172d5
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 131 deletions.
diff --git a/game-core/src/main/java/games/strategy/engine/auto/update/UpdatedMapsCheck.java b/game-core/src/main/java/games/strategy/engine/auto/update/UpdatedMapsCheck.java
@@ -1,53 +1,30 @@
 package games.strategy.engine.auto.update;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Splitter;
 import games.strategy.triplea.settings.ClientSetting;
-import games.strategy.triplea.settings.GameSetting;
-import java.time.LocalDate;
-import java.time.ZoneId;
-import java.util.List;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import lombok.experimental.UtilityClass;
 
-final class UpdatedMapsCheck {
-  private UpdatedMapsCheck() {}
+@UtilityClass
+class UpdatedMapsCheck {
+
+  static final int THRESHOLD_DAYS = 7;
 
   static boolean isMapUpdateCheckRequired() {
     return isMapUpdateCheckRequired(
-        LocalDate.now(ZoneId.systemDefault()),
-        ClientSetting.lastCheckForMapUpdates,
-        ClientSetting::flush);
+        ClientSetting.lastCheckForMapUpdates.getValue().orElse(0L),
+        () -> ClientSetting.lastCheckForMapUpdates.setValueAndFlush(Instant.now().toEpochMilli()));
   }
 
   @VisibleForTesting
   static boolean isMapUpdateCheckRequired(
-      final LocalDate now,
-      final GameSetting<String> updateCheckDateSetting,
-      final Runnable flushSetting) {
-    // check at most once per month
-    final boolean updateCheckRequired =
-        updateCheckDateSetting
-            .getValue()
-            .map(
-                encodedUpdateCheckDate ->
-                    !parseUpdateCheckDate(encodedUpdateCheckDate).isAfter(now.minusMonths(1)))
-            .orElse(true);
-    if (!updateCheckRequired) {
-      return false;
-    }
-
-    updateCheckDateSetting.setValue(formatUpdateCheckDate(now));
-    flushSetting.run();
-    return true;
-  }
+      final long lastCheckEpochMilli, final Runnable lastCheckSetter) {
+    final Instant cutOff = Instant.now().minus(THRESHOLD_DAYS, ChronoUnit.DAYS);
+    final Instant lastCheck = Instant.ofEpochMilli(lastCheckEpochMilli);
 
-  @VisibleForTesting
-  static LocalDate parseUpdateCheckDate(final String encodedUpdateCheckDate) {
-    final List<String> tokens = Splitter.on(':').splitToList(encodedUpdateCheckDate);
-    return LocalDate.of(Integer.parseInt(tokens.get(0)), Integer.parseInt(tokens.get(1)), 1);
-  }
+    lastCheckSetter.run();
 
-  @VisibleForTesting
-  static String formatUpdateCheckDate(final LocalDate updateCheckDate) {
-    return updateCheckDate.getYear() + ":" + updateCheckDate.getMonthValue();
+    return lastCheck.isBefore(cutOff);
   }
 }
diff --git a/game-core/src/main/java/games/strategy/triplea/settings/ClientSetting.java b/game-core/src/main/java/games/strategy/triplea/settings/ClientSetting.java
@@ -137,8 +137,8 @@ public abstract class ClientSetting<T> implements GameSetting<T> {
       new BooleanClientSetting("TRIPLEA_FIRST_TIME_THIS_VERSION_PROPERTY", true);
   public static final ClientSetting<String> lastCheckForEngineUpdate =
       new StringClientSetting("TRIPLEA_LAST_CHECK_FOR_ENGINE_UPDATE");
-  public static final ClientSetting<String> lastCheckForMapUpdates =
-      new StringClientSetting("TRIPLEA_LAST_CHECK_FOR_MAP_UPDATES");
+  public static final ClientSetting<Long> lastCheckForMapUpdates =
+      new LongClientSetting("TRIPLEA_LAST_CHECK_FOR_MAP_UPDATES_EPOCH_MILLI", 0);
   public static final ClientSetting<Boolean> promptToDownloadTutorialMap =
       new BooleanClientSetting("TRIPLEA_PROMPT_TO_DOWNLOAD_TUTORIAL_MAP", true);
   public static final ClientSetting<UnitsDrawer.UnitFlagDrawMode> unitFlagDrawMode =

diff --git a/game-core/src/main/java/games/strategy/triplea/settings/LongClientSetting.java b/game-core/src/main/java/games/strategy/triplea/settings/LongClientSetting.java
@@ -0,0 +1,31 @@
+package games.strategy.triplea.settings;
+
+import javax.annotation.Nullable;
+
+final class LongClientSetting extends ClientSetting<Long> {
+  LongClientSetting(final String name) {
+    super(Long.class, name);
+  }
+
+  LongClientSetting(final String name, final long defaultValue) {
+    super(Long.class, name, defaultValue);
+  }
+
+  @Override
+  protected String encodeValue(final Long value) {
+    return value.toString();
+  }
+
+  @Override
+  @Nullable
+  protected Long decodeValue(final String encodedValue) throws ValueEncodingException {
+    try {
+      if (encodedValue.isEmpty()) {
+        return null;
+      }
+      return Long.valueOf(encodedValue);
+    } catch (final NumberFormatException e) {
+      throw new ValueEncodingException(e);
+    }
+  }
+}
diff --git a/game-core/src/test/java/games/strategy/engine/auto/update/UpdatedMapsCheckTest.java b/game-core/src/test/java/games/strategy/engine/auto/update/UpdatedMapsCheckTest.java
@@ -1,116 +1,69 @@
 package games.strategy.engine.auto.update;
 
-import static games.strategy.engine.auto.update.UpdatedMapsCheck.formatUpdateCheckDate;
-import static games.strategy.engine.auto.update.UpdatedMapsCheck.parseUpdateCheckDate;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
-import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
 
-import games.strategy.triplea.settings.GameSetting;
-import java.time.LocalDate;
+import java.time.Instant;
 import java.time.temporal.ChronoUnit;
-import java.time.temporal.TemporalUnit;
-import java.util.Optional;
-import org.junit.jupiter.api.Nested;
-import org.junit.jupiter.api.Test;
+import java.util.List;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 
+@SuppressWarnings("unused")
+@ExtendWith(MockitoExtension.class)
 final class UpdatedMapsCheckTest {
-  @ExtendWith(MockitoExtension.class)
-  @Nested
-  final class IsMapUpdateCheckRequiredTest {
-    private final LocalDate now = LocalDate.of(2008, 6, 1);
-    @Mock private GameSetting<String> updateCheckDateSetting;
-    @Mock private Runnable flushSetting;
+  private static final Instant NOW = Instant.now();
 
-    private void givenMapUpdateCheckNeverRun() {
-      when(updateCheckDateSetting.getValue()).thenReturn(Optional.empty());
-    }
+  @Mock private Runnable lastCheckSetter;
 
-    private void givenMapUpdateCheckLastRunRelativeToNow(
-        final long amountToAdd, final TemporalUnit unit) {
-      when(updateCheckDateSetting.getValue())
-          .thenReturn(Optional.of(formatUpdateCheckDate(now.plus(amountToAdd, unit))));
-    }
+  @ParameterizedTest
+  @MethodSource
+  @DisplayName(
+      "If last update check is epoch start or beyond last check threshold, then "
+          + "we expect a map update check to be needed.")
+  void mapUpdateCheckNeeded(final long lastCheckEpochMilli) {
+    final boolean result =
+        UpdatedMapsCheck.isMapUpdateCheckRequired(lastCheckEpochMilli, lastCheckSetter);
 
-    private boolean whenIsMapUpdateCheckRequired() {
-      return UpdatedMapsCheck.isMapUpdateCheckRequired(now, updateCheckDateSetting, flushSetting);
-    }
+    assertThat(result, is(true));
 
-    @Test
-    void shouldReturnTrueWhenMapUpdateCheckLastRunOneYearAgo() {
-      givenMapUpdateCheckLastRunRelativeToNow(-1, ChronoUnit.YEARS);
-
-      assertThat(whenIsMapUpdateCheckRequired(), is(true));
-    }
-
-    @Test
-    void shouldReturnTrueWhenMapUpdateCheckLastRunOneMonthAgo() {
-      givenMapUpdateCheckLastRunRelativeToNow(-1, ChronoUnit.MONTHS);
-
-      assertThat(whenIsMapUpdateCheckRequired(), is(true));
-    }
-
-    @Test
-    void shouldReturnTrueWhenMapUpdateCheckNeverRun() {
-      givenMapUpdateCheckNeverRun();
-
-      assertThat(whenIsMapUpdateCheckRequired(), is(true));
-    }
-
-    @Test
-    void shouldSaveAndFlushLastUpdateCheckDateSettingWhenReturnsTrue() {
-      givenMapUpdateCheckLastRunRelativeToNow(-1, ChronoUnit.YEARS);
-
-      assertThat(whenIsMapUpdateCheckRequired(), is(true));
-      verify(updateCheckDateSetting).setValue(formatUpdateCheckDate(now));
-      verify(flushSetting).run();
-    }
-
-    @Test
-    void shouldReturnFalseWhenMapUpdateCheckLastRunThisMonth() {
-      givenMapUpdateCheckLastRunRelativeToNow(0, ChronoUnit.MONTHS);
-
-      assertThat(whenIsMapUpdateCheckRequired(), is(false));
-    }
-
-    @Test
-    void shouldReturnFalseWhenMapUpdateCheckLastRunOneMonthHence() {
-      givenMapUpdateCheckLastRunRelativeToNow(1, ChronoUnit.MONTHS);
+    verify(lastCheckSetter).run();
+  }
 
-      assertThat(whenIsMapUpdateCheckRequired(), is(false));
-    }
+  static List<Long> mapUpdateCheckNeeded() {
+    return List.of(
+        0L, NOW.minus(UpdatedMapsCheck.THRESHOLD_DAYS + 1, ChronoUnit.DAYS).toEpochMilli());
+  }
 
-    @Test
-    void shouldNotSaveAndFlushLastUpdateCheckDateSettingWhenReturnsFalse() {
-      givenMapUpdateCheckLastRunRelativeToNow(0, ChronoUnit.MONTHS);
+  @ParameterizedTest
+  @MethodSource
+  @DisplayName(
+      "If last update check is in future or is before the last update check threshold,"
+          + "then we do not need a map update check")
+  void updateCheckNotNeeded(final long lastCheckTime) {
+    final boolean result =
+        UpdatedMapsCheck.isMapUpdateCheckRequired(lastCheckTime, lastCheckSetter);
 
-      assertThat(whenIsMapUpdateCheckRequired(), is(false));
-      verify(updateCheckDateSetting, never()).setValue(anyString());
-      verify(flushSetting, never()).run();
-    }
-  }
+    assertThat(result, is(false));
 
-  @Nested
-  final class ParseUpdateCheckDateTest {
-    @Test
-    void shouldParseStringAsDate() {
-      assertThat(parseUpdateCheckDate("2018:1"), is(LocalDate.of(2018, 1, 1)));
-      assertThat(parseUpdateCheckDate("1941:12"), is(LocalDate.of(1941, 12, 1)));
-    }
+    verify(lastCheckSetter).run();
   }
 
-  @Nested
-  final class FormatUpdateCheckDateTest {
-    @Test
-    void shouldFormatDateAsString() {
-      assertThat(formatUpdateCheckDate(LocalDate.of(2018, 1, 1)), is("2018:1"));
-      assertThat(formatUpdateCheckDate(LocalDate.of(1941, 12, 1)), is("1941:12"));
-    }
+  static List<Long> updateCheckNotNeeded() {
+    // no need to check when:
+    return List.of(
+        // last check time is now
+        NOW.toEpochMilli(),
+        // last check is in future
+        NOW.plus(1, ChronoUnit.DAYS).toEpochMilli(),
+        // last check is one day short of the threshold
+        NOW.minus(UpdatedMapsCheck.THRESHOLD_DAYS - 1, ChronoUnit.DAYS).toEpochMilli(),
+        // last check is within a minute of the threshold but not beyond
+        NOW.minus(UpdatedMapsCheck.THRESHOLD_DAYS, ChronoUnit.DAYS).plusSeconds(60).toEpochMilli());
   }
 }