Address some tiny TODOs (#1566)

* Address some tiny TODOs

* Format fix

core/src/main/java/google/registry/model/domain/GracePeriod.java

@@ -194,18 +194,6 @@ public class GracePeriod extends GracePeriodBase implements DatastoreAndSqlEntit
     return clone;
   }
 
-  /**
-   * Returns a clone of this {@link GracePeriod} with {@link #billingEventRecurring} set to the
-   * given value.
-   *
-   * <p>TODO(b/162231099): Remove this function after duplicate id issue is solved.
-   */
-  public GracePeriod cloneWithRecurringBillingEvent(VKey<BillingEvent.Recurring> recurring) {
-    GracePeriod clone = clone(this);
-    clone.billingEventRecurring = BillingRecurrenceVKey.create(recurring);
-    return clone;
-  }
-
   /** Entity class to represent a historic {@link GracePeriod}. */
   @Entity(name = "GracePeriodHistory")
   @Table(indexes = @Index(columnList = "domainRepoId"))

core/src/main/java/google/registry/tools/SetNumInstancesCommand.java

@@ -15,18 +15,14 @@
 package google.registry.tools;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.ImmutableSetMultimap.flatteningToImmutableSetMultimap;
 import static google.registry.util.CollectionUtils.nullToEmpty;
-import static google.registry.util.PreconditionsUtils.checkArgumentPresent;
 
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
 import com.google.api.services.appengine.v1.Appengine;
-import com.google.common.base.Enums;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Maps;
 import com.google.common.flogger.FluentLogger;
@@ -35,8 +31,6 @@ import google.registry.request.Action.Service;
 import google.registry.util.AppEngineServiceUtils;
 import java.io.IOException;
 import java.util.List;
-import java.util.Locale;
-import java.util.Set;
 import javax.inject.Inject;
 
 /** A command to set the number of instances for an App Engine service. */
@@ -49,19 +43,18 @@ final class SetNumInstancesCommand implements CommandWithRemoteApi {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
-  private static final ImmutableSet<Service> ALL_DEPLOYED_SERVICES =
+  private static final ImmutableList<Service> ALL_DEPLOYED_SERVICES =
-      ImmutableSet.copyOf(Service.values());
+      ImmutableList.copyOf(Service.values());
 
   private static final ImmutableMap<String, Service> SERVICE_ID_TO_SERVICE =
       Maps.uniqueIndex(ALL_DEPLOYED_SERVICES, Service::getServiceId);
 
-  // TODO(b/119629679): Use List<Service> after upgrading jcommander to latest version.
   @Parameter(
       names = {"-s", "--services"},
       description =
           "Comma-delimited list of App Engine services to set. "
               + "Allowed values: [DEFAULT, TOOLS, BACKEND, PUBAPI]")
-  private List<String> serviceNames = ImmutableList.of();
+  private List<Service> services = ImmutableList.of();
 
   @Parameter(
       names = {"-v", "--versions"},
@@ -93,19 +86,6 @@ final class SetNumInstancesCommand implements CommandWithRemoteApi {
 
   @Override
   public void run() {
-
-    ImmutableSet<Service> services =
-        serviceNames.stream()
-            .map(s -> s.toUpperCase(Locale.US))
-            .map(
-                name ->
-                    checkArgumentPresent(
-                        Enums.getIfPresent(Service.class, name).toJavaUtil(),
-                        "Invalid service '%s'. Allowed values are %s",
-                        name,
-                        ALL_DEPLOYED_SERVICES))
-            .collect(toImmutableSet());
-
     if (nonLiveVersions) {
       checkArgument(versions.isEmpty(), "--versions cannot be set if --non_live_versions is set");
 
@@ -149,7 +129,7 @@ final class SetNumInstancesCommand implements CommandWithRemoteApi {
         version, service, numInstances);
   }
 
-  private ImmutableSetMultimap<Service, String> getAllLiveVersionsMap(Set<Service> services) {
+  private ImmutableSetMultimap<Service, String> getAllLiveVersionsMap(List<Service> services) {
     try {
       return nullToEmpty(appengine.apps().services().list(projectId).execute().getServices())
           .stream()
@@ -165,7 +145,8 @@ final class SetNumInstancesCommand implements CommandWithRemoteApi {
     }
   }
 
-  private ImmutableSetMultimap<Service, String> getManualScalingVersionsMap(Set<Service> services) {
+  private ImmutableSetMultimap<Service, String> getManualScalingVersionsMap(
+      List<Service> services) {
     return services.stream()
         .collect(
             flatteningToImmutableSetMultimap(

core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java

@@ -56,7 +56,6 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi {
       validateWith = PathParameter.InputFile.class)
   private Path clientCertificatePath;
 
-  // TODO(sarahbot@): Remove this after hash fallback is removed
   @Nullable
   @Parameter(
       names = {"-h", "--cert_hash"},

core/src/test/java/google/registry/tools/SetNumInstancesCommandTest.java

@@ -58,22 +58,28 @@ public class SetNumInstancesCommandTest extends CommandTestCase<SetNumInstancesC
 
   @Test
   void test_emptyService_throwsException() {
-    IllegalArgumentException thrown =
+    ParameterException thrown =
         assertThrows(
-            IllegalArgumentException.class,
+            ParameterException.class,
             () -> runCommand("--services=", "--versions=version", "--num_instances=5"));
-    assertThat(thrown).hasMessageThat().contains("Invalid service ''");
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains(
+            "Invalid value for -s parameter. Allowed values:[DEFAULT, TOOLS, BACKEND, PUBAPI]");
   }
 
   @Test
   void test_invalidService_throwsException() {
-    IllegalArgumentException thrown =
+    ParameterException thrown =
         assertThrows(
-            IllegalArgumentException.class,
+            ParameterException.class,
             () ->
                 runCommand(
                     "--services=INVALID,DEFAULT", "--versions=version", "--num_instances=5"));
-    assertThat(thrown).hasMessageThat().contains("Invalid service 'INVALID'");
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains(
+            "Invalid value for -s parameter. Allowed values:[DEFAULT, TOOLS, BACKEND, PUBAPI]");
   }
 
   @Test

core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java

@@ -456,39 +456,6 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
         (builder, s) -> builder.setFailoverClientCertificate(s, clock.nowUtc()));
   }
 
-  @TestOfyAndSql
-  void testUpdate_failoverClientCertificateWithViolationsAlreadyExistedSucceeds() {
-    // TODO(sarahbot): remove this test after November 1, 2020.
-
-    // The frontend will always send the entire registrar entity back for an update, so the checks
-    // on the certificate should only run if it is a new certificate
-
-    // Set a bad certificate before checks on uploads are enforced
-    clock.setTo(DateTime.parse("2018-07-02T00:00:00Z"));
-    Registrar existingRegistrar = loadRegistrar(CLIENT_ID);
-    existingRegistrar =
-        existingRegistrar
-            .asBuilder()
-            .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT, clock.nowUtc())
-            .build();
-    persistResource(existingRegistrar);
-
-    // Update with the same certificate after enforcement starts
-    clock.setTo(DateTime.parse("2020-11-02T00:00:00Z"));
-    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
-    args.put("failoverClientCertificate", CertificateSamples.SAMPLE_CERT);
-    Map<String, Object> response =
-        action.handleJsonRequest(
-            ImmutableMap.of(
-                "op", "update",
-                "id", CLIENT_ID,
-                "args", args));
-
-    assertThat(response).containsEntry("status", "SUCCESS");
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
-    cloudTasksHelper.assertNoTasksEnqueued("sheet");
-  }
-
   @TestOfyAndSql
   void testUpdate_failoverClientCertificateWithViolationsFails() {
     clock.setTo(DateTime.parse("2020-11-02T00:00:00Z"));