Add console backend for editing registrar

[ptkach]

This change is 

[gbrodman]

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ptkach)

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 34 at r1 (raw file):

    method = {POST},
    auth = Auth.AUTH_PUBLIC_LOGGED_IN)
public class ConsoleUpdateRegistrar extends ConsoleApiAction {

Should be ConsoleUpdateRegistrarAction

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 36 at r1 (raw file):

public class ConsoleUpdateRegistrar extends ConsoleApiAction {

  protected static final String EMAIL_SUBJ = "Registrar %s has been updated";

these can be private, and then moved below PATH

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 57 at r1 (raw file):

  protected void postHandler(User user) {
    var errorMsg = "Missing param(s): %s";
    Registrar maybeRegistrar =

it's not "maybe" at this point

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 63 at r1 (raw file):

        user, maybeRegistrar.getRegistrarId(), ConsolePermission.EDIT_REGISTRAR_DETAILS);

    tm().transact(

This is a personal choice (up to you), we might have better indentation if you put everything in the transact block in a separate method

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 73 at r1 (raw file):

              // Only allow modifying allowed TLDs if we're in a non-PRODUCTION environment, if the
              // registrar

comment formatting

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 93 at r1 (raw file):

                          .asBuilder()
                          .setAllowedTlds(
                              ImmutableSet.copyOf(

we shouldn't need ImmutableSet.copyOf here. The builder does that for us (as all things of that type should)

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 95 at r1 (raw file):

                              ImmutableSet.copyOf(
                                  maybeRegistrar.getAllowedTlds().stream()
                                      .map((String label) -> canonicalizeHostname(label))

can just refer to the canonicalizeHostname method directly in the map call rather than using a lambda

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 96 at r1 (raw file):

                                  maybeRegistrar.getAllowedTlds().stream()
                                      .map((String label) -> canonicalizeHostname(label))
                                      .collect(Collectors.toList())))

just map it to a set instead?

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 117 at r1 (raw file):

      diff.append(
          String.format(
              "Registry Lock: %s -> %s",

probably should include "Allowed" in the message to indicate that it was a permission that changed

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 129 at r1 (raw file):

    }

    if (hasChanges) {

Can just check if the StringBuilder is empty instead of using an additional variable

---

core/src/test/java/google/registry/ui/server/console/ConsoleUpdateRegistrarTest.java line 49 at r1 (raw file):

import org.junit.jupiter.api.extension.RegisterExtension;

class ConsoleUpdateRegistrarTest {

nit, but needs the minor javadoc

---

core/src/test/java/google/registry/ui/server/console/ConsoleUpdateRegistrarTest.java line 70 at r1 (raw file):

  @BeforeEach
  void beforeEach() throws Exception {
    createTld("app");

createTlds(app, dev)

---

core/src/test/java/google/registry/ui/server/console/ConsoleUpdateRegistrarTest.java line 72 at r1 (raw file):

    createTld("app");
    createTld("dev");
    registrar = persistNewRegistrar("registrarId");

Why not just use the existing TheRegistrar / NewRegistrar test objects? Since we're changing things anyway it shouldn't matter right?

[ptkach]

Reviewable status: 0 of 2 files reviewed, 12 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 34 at r1 (raw file):

Previously, gbrodman wrote…

Should be ConsoleUpdateRegistrarAction

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 36 at r1 (raw file):

Previously, gbrodman wrote…

these can be private, and then moved below PATH

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 57 at r1 (raw file):

Previously, gbrodman wrote…

it's not "maybe" at this point

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 63 at r1 (raw file):

Previously, gbrodman wrote…

This is a personal choice (up to you), we might have better indentation if you put everything in the transact block in a separate method

I think it's relatively short and can be read without a problem. The new method is not needed as it is now, as it will increase the complexity of error handing

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 73 at r1 (raw file):

Previously, gbrodman wrote…

comment formatting

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 93 at r1 (raw file):

Previously, gbrodman wrote…

we shouldn't need ImmutableSet.copyOf here. The builder does that for us (as all things of that type should)

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 95 at r1 (raw file):

Previously, gbrodman wrote…

can just refer to the canonicalizeHostname method directly in the map call rather than using a lambda

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 96 at r1 (raw file):

Previously, gbrodman wrote…

just map it to a set instead?

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 117 at r1 (raw file):

Previously, gbrodman wrote…

probably should include "Allowed" in the message to indicate that it was a permission that changed

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleUpdateRegistrar.java line 129 at r1 (raw file):

Previously, gbrodman wrote…

Can just check if the StringBuilder is empty instead of using an additional variable

Done.

---

core/src/test/java/google/registry/ui/server/console/ConsoleUpdateRegistrarTest.java line 70 at r1 (raw file):

Previously, gbrodman wrote…

createTlds(app, dev)

Done. Interesting that automplete only offers createTld when I type createTl.. only when createdTlds is types is when I noticed it exists. Thanks

---

core/src/test/java/google/registry/ui/server/console/ConsoleUpdateRegistrarTest.java line 72 at r1 (raw file):

Previously, gbrodman wrote…

Why not just use the existing TheRegistrar / NewRegistrar test objects? Since we're changing things anyway it shouldn't matter right?

We've had this discussion before, I'm not changing registryLockAllowed or allowedTlds here and I prefer to rely on objects created within test to make sure any future changes to TheRegistrar or NewRegistrar will affect the test.

[gbrodman]

Reviewed 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ptkach)

---

core/src/test/java/google/registry/ui/server/console/ConsoleUpdateRegistrarTest.java line 70 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

Done. Interesting that automplete only offers createTld when I type createTl.. only when createdTlds is types is when I noticed it exists. Thanks

Very weird!

---

core/src/test/java/google/registry/ui/server/console/ConsoleUpdateRegistrarTest.java line 86 at r1 (raw file):

                new UserRoles.Builder()
                    .setGlobalRole(GlobalRole.FTE)
                    // .setRegistrarRoles(

???