Use cursor to track updating of registrar sheet

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=138221931

docs/code-structure.md

@@ -74,6 +74,8 @@ The following cursor types are defined:
     escrow provider's SFTP server for a given TLD.
 *   **`RECURRING_BILLING`** - Expansion of `Recurring` (renew) billing events
     into `OneTime` events.
+*   **`SYNC_REGISTRAR_SHEET`** - Tracks the last time the registrar spreadsheet
+    was successfully synced.
 
 All `Cursor` entities in Datastore contain a `DateTime` that represents the next
 timestamp at which an operation should resume processing and a `CursorType` that

java/google/registry/config/ConfigModule.java

@@ -618,17 +618,6 @@ public final class ConfigModule {
     }
   }
 
-  /**
-   * Amount of time between synchronizations of the Registrar spreadsheet.
-   *
-   * @see google.registry.export.sheet.SyncRegistrarsSheetAction
-   */
-  @Provides
-  @Config("sheetRegistrarInterval")
-  public static Duration provideSheetRegistrarInterval() {
-    return Duration.standardHours(1);
-  }
-
   /**
    * Returns SSH client connection and read timeout.
    *

java/google/registry/export/sheet/BUILD

@@ -21,6 +21,7 @@ java_library(
         "//third_party/java/joda_time",
         "//third_party/java/jsr305_annotations",
         "//third_party/java/jsr330_inject",
+        "//third_party/java/objectify:objectify-v4_1",
         "//third_party/java/servlet/servlet_api",
         "//java/google/registry/config",
         "//java/google/registry/model",

java/google/registry/export/sheet/SyncRegistrarsSheet.java

@@ -15,6 +15,8 @@
 package google.registry.export.sheet;
 
 import static com.google.common.base.MoreObjects.firstNonNull;
+import static google.registry.model.common.Cursor.CursorType.SYNC_REGISTRAR_SHEET;
+import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.model.registrar.RegistrarContact.Type.ABUSE;
 import static google.registry.model.registrar.RegistrarContact.Type.ADMIN;
 import static google.registry.model.registrar.RegistrarContact.Type.BILLING;
@@ -22,6 +24,7 @@ import static google.registry.model.registrar.RegistrarContact.Type.LEGAL;
 import static google.registry.model.registrar.RegistrarContact.Type.MARKETING;
 import static google.registry.model.registrar.RegistrarContact.Type.TECH;
 import static google.registry.model.registrar.RegistrarContact.Type.WHOIS;
+import static google.registry.util.DateTimeUtils.START_OF_TIME;
 
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
@@ -32,6 +35,8 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Ordering;
 import com.google.gdata.util.ServiceException;
+import com.googlecode.objectify.VoidWork;
+import google.registry.model.common.Cursor;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.registrar.RegistrarAddress;
 import google.registry.model.registrar.RegistrarContact;
@@ -41,10 +46,9 @@ import java.io.IOException;
 import javax.annotation.Nullable;
 import javax.inject.Inject;
 import org.joda.time.DateTime;
-import org.joda.time.Duration;
 
 /**
- * Class for synchronizing all {@link Registrar} datastore objects to a Google Spreadsheet.
+ * Class for synchronizing all {@link Registrar} Datastore objects to a Google Spreadsheet.
  *
  * @see SyncRegistrarsSheetAction
  */
@@ -54,11 +58,15 @@ class SyncRegistrarsSheet {
   @Inject SheetSynchronizer sheetSynchronizer;
   @Inject SyncRegistrarsSheet() {}
 
-  /** Returns true if a {@link Registrar} entity was modified in past {@code duration}. */
-  boolean wasRegistrarsModifiedInLast(Duration duration) {
-    DateTime watermark = clock.nowUtc().minus(duration);
+  /**
+   * Returns true if any {@link Registrar} entity was modified since the last time this task
+   * successfully completed, as measured by a cursor.
+   */
+  boolean wereRegistrarsModified() {
+    Cursor cursor = ofy().load().key(Cursor.createGlobalKey(SYNC_REGISTRAR_SHEET)).now();
+    DateTime lastUpdateTime = (cursor == null) ? START_OF_TIME : cursor.getCursorTime();
     for (Registrar registrar : Registrar.loadAll()) {
-      if (DateTimeUtils.isAtOrAfter(registrar.getLastUpdateTime(), watermark)) {
+      if (DateTimeUtils.isAtOrAfter(registrar.getLastUpdateTime(), lastUpdateTime)) {
         return true;
       }
     }
@@ -67,6 +75,7 @@ class SyncRegistrarsSheet {
 
   /** Performs the synchronization operation. */
   void run(String spreadsheetId) throws IOException, ServiceException {
+    final DateTime executionTime = clock.nowUtc();
     sheetSynchronizer.synchronize(
         spreadsheetId,
         FluentIterable
@@ -167,6 +176,11 @@ class SyncRegistrarsSheet {
                   }
                 })
             .toList());
+    ofy().transact(new VoidWork() {
+      @Override
+      public void vrun() {
+        ofy().save().entity(Cursor.createGlobal(SYNC_REGISTRAR_SHEET, executionTime));
+      }});
   }
 
   private static String convertContacts(

java/google/registry/export/sheet/SyncRegistrarsSheetAction.java

@@ -111,7 +111,6 @@ public class SyncRegistrarsSheetAction implements Runnable {
   @Inject SyncRegistrarsSheet syncRegistrarsSheet;
   @Inject @Config("sheetLockTimeout") Duration timeout;
   @Inject @Config("sheetRegistrarId") Optional<String> idConfig;
-  @Inject @Config("sheetRegistrarInterval") Duration interval;
   @Inject @Parameter("id") Optional<String> idParam;
   @Inject SyncRegistrarsSheetAction() {}
 
@@ -123,8 +122,7 @@ public class SyncRegistrarsSheetAction implements Runnable {
       return;
     }
     if (!idParam.isPresent()) {
-      // TODO(b/19082368): Use a cursor.
-      if (!syncRegistrarsSheet.wasRegistrarsModifiedInLast(interval)) {
+      if (!syncRegistrarsSheet.wereRegistrarsModified()) {
         Result.NOTMODIFIED.send(response, null);
         return;
       }

java/google/registry/model/common/Cursor.java

@@ -71,7 +71,14 @@ public class Cursor extends ImmutableObject {
      * for which Recurring billing events have been expanded (i.e. the inclusive first billing time
      * for the next expansion job).
      */
-    RECURRING_BILLING(EntityGroupRoot.class);
+    RECURRING_BILLING(EntityGroupRoot.class),
+
+    /**
+     * Cursor for {@link google.registry.export.sheet.SyncRegistrarsSheetAction}. The DateTime
+     * stored is the last time that registrar changes were successfully synced to the sheet. If
+     * there were no changes since the last time the action run, the cursor is not updated.
+     */
+    SYNC_REGISTRAR_SHEET(EntityGroupRoot.class);
 
     /** See the definition of scope on {@link #getScopeClass}. */
     private final Class<? extends ImmutableObject> scope;

javatests/google/registry/export/sheet/BUILD

@@ -18,6 +18,7 @@ java_library(
         "//third_party/java/jsr305_annotations",
         "//third_party/java/junit",
         "//third_party/java/mockito",
+        "//third_party/java/objectify:objectify-v4_1",
         "//third_party/java/servlet/servlet_api",
         "//third_party/java/truth",
         "//java/google/registry/config",

javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java

@@ -16,7 +16,6 @@ package google.registry.export.sheet;
 
 import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8;
 import static com.google.common.truth.Truth.assertThat;
-import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -55,7 +54,6 @@ public class SyncRegistrarsSheetActionTest {
     action.syncRegistrarsSheet = syncRegistrarsSheet;
     action.idConfig = Optional.fromNullable(idConfig);
     action.idParam = Optional.fromNullable(idParam);
-    action.interval = Duration.standardHours(1);
     action.timeout = Duration.standardHours(1);
     action.run();
   }
@@ -69,22 +67,22 @@ public class SyncRegistrarsSheetActionTest {
 
   @Test
   public void testPost_withoutParams_runsSyncWithDefaultIdAndChecksIfModified() throws Exception {
-    when(syncRegistrarsSheet.wasRegistrarsModifiedInLast(any(Duration.class))).thenReturn(true);
+    when(syncRegistrarsSheet.wereRegistrarsModified()).thenReturn(true);
     runAction("jazz", null);
     assertThat(response.getStatus()).isEqualTo(200);
     assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8);
     assertThat(response.getPayload()).startsWith("OK");
-    verify(syncRegistrarsSheet).wasRegistrarsModifiedInLast(any(Duration.class));
+    verify(syncRegistrarsSheet).wereRegistrarsModified();
     verify(syncRegistrarsSheet).run(eq("jazz"));
     verifyNoMoreInteractions(syncRegistrarsSheet);
   }
 
   @Test
   public void testPost_noModificationsToRegistrarEntities_doesNothing() throws Exception {
-    when(syncRegistrarsSheet.wasRegistrarsModifiedInLast(any(Duration.class))).thenReturn(false);
+    when(syncRegistrarsSheet.wereRegistrarsModified()).thenReturn(false);
     runAction("NewRegistrar", null);
     assertThat(response.getPayload()).startsWith("NOTMODIFIED");
-    verify(syncRegistrarsSheet).wasRegistrarsModifiedInLast(any(Duration.class));
+    verify(syncRegistrarsSheet).wereRegistrarsModified();
     verifyNoMoreInteractions(syncRegistrarsSheet);
   }
 

javatests/google/registry/export/sheet/SyncRegistrarsSheetTest.java

@@ -16,12 +16,14 @@ package google.registry.export.sheet;
 
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.model.common.Cursor.CursorType.SYNC_REGISTRAR_SHEET;
+import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.testing.DatastoreHelper.createTld;
 import static google.registry.testing.DatastoreHelper.deleteResource;
 import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.DatastoreHelper.persistSimpleResources;
 import static org.joda.time.DateTimeZone.UTC;
-import static org.joda.time.Duration.standardHours;
+import static org.joda.time.Duration.standardMinutes;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.verify;
 
@@ -29,6 +31,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import google.registry.config.RegistryEnvironment;
+import google.registry.model.common.Cursor;
 import google.registry.model.ofy.Ofy;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.registrar.RegistrarAddress;
@@ -37,7 +40,6 @@ import google.registry.testing.AppEngineRule;
 import google.registry.testing.FakeClock;
 import google.registry.testing.InjectRule;
 import org.joda.time.DateTime;
-import org.joda.time.Duration;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -87,14 +89,12 @@ public class SyncRegistrarsSheetTest {
   }
 
   @Test
-  public void testWasRegistrarsModifiedInLast_noRegistrars_returnsFalse() throws Exception {
-    SyncRegistrarsSheet sync = newSyncRegistrarsSheet();
-    assertThat(sync.wasRegistrarsModifiedInLast(Duration.standardHours(1))).isFalse();
+  public void test_wereRegistrarsModified_noRegistrars_returnsFalse() throws Exception {
+    assertThat(newSyncRegistrarsSheet().wereRegistrarsModified()).isFalse();
   }
 
   @Test
-  public void testWasRegistrarsModifiedInLastInterval() throws Exception {
-    Duration interval = standardHours(1);
+  public void test_wereRegistrarsModified_atDifferentCursorTimes() throws Exception {
     persistResource(new Registrar.Builder()
         .setClientId("SomeRegistrar")
         .setRegistrarName("Some Registrar Inc.")
@@ -102,14 +102,15 @@ public class SyncRegistrarsSheetTest {
         .setIanaIdentifier(8L)
         .setState(Registrar.State.ACTIVE)
         .build());
-    clock.advanceBy(interval);
-    assertThat(newSyncRegistrarsSheet().wasRegistrarsModifiedInLast(interval)).isTrue();
-    clock.advanceOneMilli();
-    assertThat(newSyncRegistrarsSheet().wasRegistrarsModifiedInLast(interval)).isFalse();
+    persistResource(Cursor.createGlobal(SYNC_REGISTRAR_SHEET, clock.nowUtc().minusHours(1)));
+    assertThat(newSyncRegistrarsSheet().wereRegistrarsModified()).isTrue();
+    persistResource(Cursor.createGlobal(SYNC_REGISTRAR_SHEET, clock.nowUtc().plusHours(1)));
+    assertThat(newSyncRegistrarsSheet().wereRegistrarsModified()).isFalse();
   }
 
   @Test
   public void testRun() throws Exception {
+    DateTime beforeExecution = clock.nowUtc();
     persistResource(new Registrar.Builder()
         .setClientId("anotherregistrar")
         .setRegistrarName("Another Registrar LLC")
@@ -190,6 +191,7 @@ public class SyncRegistrarsSheetTest {
     persistSimpleResources(contacts);
     persistResource(registrar);
 
+    clock.advanceBy(standardMinutes(1));
     newSyncRegistrarsSheet().run("foobar");
 
     verify(sheetSynchronizer).synchronize(eq("foobar"), rowsCaptor.capture());
@@ -254,8 +256,8 @@ public class SyncRegistrarsSheetTest {
     assertThat(row).containsEntry("address.countryCode", "US");
     assertThat(row).containsEntry("phoneNumber", "+1.2223334444");
     assertThat(row).containsEntry("faxNumber", "");
-    assertThat(row.get("creationTime")).isEqualTo(clock.nowUtc().toString());
-    assertThat(row.get("lastUpdateTime")).isEqualTo(clock.nowUtc().toString());
+    assertThat(row.get("creationTime")).isEqualTo(beforeExecution.toString());
+    assertThat(row.get("lastUpdateTime")).isEqualTo(beforeExecution.toString());
     assertThat(row).containsEntry("allowedTlds", "example");
     assertThat(row).containsEntry("blockPremiumNames", "false");
     assertThat(row).containsEntry("ipAddressWhitelist", "");
@@ -289,8 +291,8 @@ public class SyncRegistrarsSheetTest {
     assertThat(row).containsEntry("address.countryCode", "US");
     assertThat(row).containsEntry("phoneNumber", "+1.2125551212");
     assertThat(row).containsEntry("faxNumber", "+1.2125551213");
-    assertThat(row.get("creationTime")).isEqualTo(clock.nowUtc().toString());
-    assertThat(row.get("lastUpdateTime")).isEqualTo(clock.nowUtc().toString());
+    assertThat(row.get("creationTime")).isEqualTo(beforeExecution.toString());
+    assertThat(row.get("lastUpdateTime")).isEqualTo(beforeExecution.toString());
     assertThat(row).containsEntry("allowedTlds", "");
     assertThat(row).containsEntry("whoisServer", "whois.example.com");
     assertThat(row).containsEntry("blockPremiumNames", "false");
@@ -299,6 +301,10 @@ public class SyncRegistrarsSheetTest {
     assertThat(row).containsEntry("referralUrl",
         ENVIRONMENT.config().getRegistrarDefaultReferralUrl().toString());
     assertThat(row).containsEntry("icannReferralEmail", "jim@example.net");
+
+    Cursor cursor = ofy().load().key(Cursor.createGlobalKey(SYNC_REGISTRAR_SHEET)).now();
+    assertThat(cursor).isNotNull();
+    assertThat(cursor.getCursorTime()).isGreaterThan(beforeExecution);
   }
 
   @Test