Add console history api

[ptkach]

Frontend is coming in a separate PR. Tested this on Alpha.

This change is 

[gbrodman]

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

---

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

// Copyright 2023 The Nomulus Authors. All Rights Reserved.

the nittiest nit: 2025

---

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

      SELECT *
      FROM "ConsoleUpdateHistory"
      WHERE SPLIT_PART(description, '|', 1) = :registrarId;

i think we can possibly create an index on the first field of the split_part? That seems like a good idea I think

---

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

                        .createNativeQuery(SQL_USER_HISTORY, ConsoleUpdateHistory.class)
                        .setParameter("actingUser", consoleUserEmail)
                        .setHint("org.hibernate.fetchSize", 1000)

out of curiosity, what's the point of setting the fetch size?

---

core/src/main/java/google/registry/ui/server/console/settings/ContactAction.java line 186 at r1 (raw file):

              finishAndPersistConsoleUpdateHistory(
                  new ConsoleUpdateHistory.Builder()
                      .setType(ConsoleUpdateHistory.Type.REGISTRAR_SECURITY_UPDATE)

i don't think this is the right ConsoleUpdateHistory.Type

---

core/src/main/java/google/registry/model/console/ConsoleRoleDefinitions.java line 70 at r1 (raw file):

          .addAll(SUPPORT_LEAD_PERMISSIONS)
          .add(ConsolePermission.CHANGE_NOMULUS_PASSWORD)
          .add(ConsolePermission.AUDIT_ACTIVITY_BY_USER)

I don't have a strong feeling here, but do you have any thoughts about giving this permission to support agents/leads as well? I guess I'm not exactly sure how this will usually be used, if it would be necessary or helpful for support folks to see it.

---

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

  @BeforeEach
  void beforeEach() {
    adminUser = createAdminUser("admin@example.com");

for what it's worth, the base test case creates an FTE/Admin user already

and i think that already populates the ConsoleApiParams with that user by default

---

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

            .build();

    DatabaseHelper.persistResources(

i think it might be better to use persistResource at the point where we create the object -- i think it populates any autopopulating ID that might exist, and it makes the error case of "creating an object in the beforeEach method but forgetting to save it" harder to hit.

---

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

            .setRegistryLockPassword("registryLockPassword")
            .build();
    DatabaseHelper.putInDb(user);

to be quite honest we should probably get rid of this method and just use persistResource instead, but eh we don't have to do that here

[ptkach]

Reviewable status: 9 of 15 files reviewed, 7 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/model/console/ConsoleRoleDefinitions.java line 70 at r1 (raw file):

Previously, gbrodman wrote…

I don't have a strong feeling here, but do you have any thoughts about giving this permission to support agents/leads as well? I guess I'm not exactly sure how this will usually be used, if it would be necessary or helpful for support folks to see it.

Yeah I think it makes sense

---

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

Previously, gbrodman wrote…

the nittiest nit: 2025

Done.

---

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

Previously, gbrodman wrote…

i think we can possibly create an index on the first field of the split_part? That seems like a good idea I think

I will submit a separate PR for that

---

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

Previously, gbrodman wrote…

out of curiosity, what's the point of setting the fetch size?

It tells JDBC how many rows to fetch in a single DB request round trip. For registrars with many history entries in the future that will allow for faster response. I've noticed our DB network connection is a bottleneck, so as tested with domains download this improves the performance.

---

core/src/main/java/google/registry/ui/server/console/settings/ContactAction.java line 186 at r1 (raw file):

Previously, gbrodman wrote…

i don't think this is the right ConsoleUpdateHistory.Type

Good catch!

---

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

Previously, gbrodman wrote…

for what it's worth, the base test case creates an FTE/Admin user already

and i think that already populates the ConsoleApiParams with that user by default

Done.

---

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

Previously, gbrodman wrote…

i think it might be better to use persistResource at the point where we create the object -- i think it populates any autopopulating ID that might exist, and it makes the error case of "creating an object in the beforeEach method but forgetting to save it" harder to hit.

Done.

---

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

Previously, gbrodman wrote…

to be quite honest we should probably get rid of this method and just use persistResource instead, but eh we don't have to do that here

Done.

[gbrodman]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ptkach)

---

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

Previously, ptkach (Pavlo Tkach) wrote…

I will submit a separate PR for that

sounds good to me

[gbrodman]

Reviewed 2 of 4 files at r3, all commit messages.
Reviewable status: 15 of 17 files reviewed, 1 unresolved discussion

---

core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java line 67 at r3 (raw file):

                .setActingUser(fteUser)
                .setDescription("TheRegistrar|Some change")
                .setModificationTime(DateTime.now(DateTimeZone.UTC))

actually we should use the clock from the base class instead of the current time (as a matter of principle -- we've had tests start randomly failing before because tests relied on the actual time which keeps moving forward)

[ptkach]

Reviewable status: 15 of 17 files reviewed, 1 unresolved discussion (waiting on @gbrodman)

---

core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java line 67 at r3 (raw file):

Previously, gbrodman wrote…

actually we should use the clock from the base class instead of the current time (as a matter of principle -- we've had tests start randomly failing before because tests relied on the actual time which keeps moving forward)

Done.

[gbrodman]

Reviewed 1 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ptkach)

[gbrodman]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ptkach)