Add java code for RegitrarPoc id, replace bulk contacts update with per-contact

[ptkach]

This change is 

[ptkach]

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

---

core/src/test/java/google/registry/ui/server/console/settings/ContactActionTest.java line 114 at r1 (raw file):

  void testSuccess_getContactInfo() throws IOException {
    insertInDb(adminPoc);
    RegistrarPoc adminPocDb = DatabaseHelper.loadByKey(adminPoc.createVKey());

A little annoying but I couldn't figure out a better way to determine id

---

console-webapp/src/app/shared/services/backend.service.ts line 83 at r1 (raw file):

  }

  createContact(

New paradigm is instead of always sending an entire contacts list, we only send an updated contact now that we have a RegistarPoc id added

---

console-webapp/src/app/settings/contact/contact.service.ts line 38 at r1 (raw file):

  MARKETING: 'Marketing contact',
  TECH: 'Technical contact',
  WHOIS: 'RDAP-Inquiry contact',

This will be addressed as a part of global whois clean up

---

core/src/test/java/google/registry/ui/server/console/settings/ContactActionTest.java line 130 at r1 (raw file):

  @Test
  void testSuccess_onlyContactsWithNonEmptyType() throws IOException {

We will now send empty types to front-end as well

---

core/src/test/java/google/registry/ui/server/console/settings/ContactActionTest.java line 305 at r1 (raw file):

  @Test
  void testFailure_postUpdateContactInfo_cannotModifyRegistryLockEmail() throws IOException {

Not needed as we limit the fields that can be used from the entity in request, see above

---

core/src/test/java/google/registry/ui/server/console/settings/ContactActionTest.java line 331 at r1 (raw file):

  @Test
  void testFailure_postUpdateContactInfo_cannotSetIsAllowedToSetRegistryLockPassword()

Not needed as we limit the fields that can be used from the entity in request, see above

---

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

              .addAll(oldContacts)
              .add(
                  new RegistrarPoc()

Make a note for both create and update we specifically say what field can come from the request. This is to prevent surprises in the future and also to clean up tests (see later)

[gbrodman]

probably should update the pull request title / description to reflect the broader changes included in it (it's not just adding the ID)

overall this type of change sounds good to me

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

---

core/src/test/java/google/registry/ui/server/console/settings/ContactActionTest.java line 114 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

A little annoying but I couldn't figure out a better way to determine id

is it possible to use the return value from Databasehelper::persistResource?

---

console-webapp/src/app/settings/contact/contact.service.ts line 27 at r1 (raw file):

  | 'MARKETING'
  | 'TECH'
  | 'WHOIS';

oh dang this is slightly unfortunate, i wonder if we can eventually change this in the backend

e: i see you've even mentioned this elsewhere, cool

---

core/src/main/java/google/registry/model/registrar/RegistrarPoc.java line 97 at r1 (raw file):

  @Expose
  @Column(name = "id", insertable = false, updatable = false)

probably doesn't matter much but i don't think you need the "name" field here

Once everything is deployed and set up, shall we change the SQL table to use the id as the primary key instead of the current registrarId + emailAddress combination?

[ptkach]

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

---

core/src/main/java/google/registry/model/registrar/RegistrarPoc.java line 97 at r1 (raw file):
Removed name.

Once everything is deployed and set up, shall we change the SQL table to use the id as the primary key instead of the current registrarId + emailAddress combination?

Technically yeah, we can do that, however I'm not convinced it's a high prio given how little that would benefit us. What improvement do you expect to get from this change?

---

core/src/test/java/google/registry/ui/server/console/settings/ContactActionTest.java line 114 at r1 (raw file):

Previously, gbrodman wrote…

is it possible to use the return value from Databasehelper::persistResource?

In fact yes, thanks!

[gbrodman]

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

---

core/src/main/java/google/registry/model/registrar/RegistrarPoc.java line 97 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

Removed name.

Once everything is deployed and set up, shall we change the SQL table to use the id as the primary key instead of the current registrarId + emailAddress combination?

Technically yeah, we can do that, however I'm not convinced it's a high prio given how little that would benefit us. What improvement do you expect to get from this change?

it's not a high priority, more of the type of thing that we'd prefer to have if we were setting everything up from scratch to begin with.

---

core/src/test/java/google/registry/ui/server/console/settings/ContactActionTest.java line 114 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

In fact yes, thanks!

we can probably just persist the adminPoc in the beforeEach method so we don't have to do it every time right?

---

core/src/test/java/google/registry/model/registrar/RegistrarTest.java line 344 at r2 (raw file):

                .setTypes(ImmutableSet.of(RegistrarPoc.Type.TECH, RegistrarPoc.Type.ABUSE))
                .build());
    abuseAdminContact = DatabaseHelper.loadByKey(abuseAdminContact.createVKey());

why does this need to be reloaded? i wouldn't expect it to have changed since creation

---

core/src/test/java/google/registry/schema/registrar/RegistrarPocTest.java line 66 at r2 (raw file):

  void testPersistence_succeeds() {
    insertInDb(testRegistrarPoc);
    assertThat(loadByEntity(testRegistrarPoc).toJsonMap()).isEqualTo(testRegistrarPoc.toJsonMap());

up to you with regards to how many fields you expect to have changed, but a possibly-easier way to do this is usually

assertAboutImmutableObjects().that(...).isEqualExceptFields(expected, .....)

(same with RegistrarPocCommandTest::testUpdate) and testCreate_withAdminType

[ptkach]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gbrodman)

---

core/src/test/java/google/registry/model/registrar/RegistrarTest.java line 344 at r2 (raw file):

Previously, gbrodman wrote…

why does this need to be reloaded? i wouldn't expect it to have changed since creation

It's changed upon saving in db line 135, I'm reloading to get the value of id field

---

core/src/test/java/google/registry/schema/registrar/RegistrarPocTest.java line 66 at r2 (raw file):

Previously, gbrodman wrote…

up to you with regards to how many fields you expect to have changed, but a possibly-easier way to do this is usually

assertAboutImmutableObjects().that(...).isEqualExceptFields(expected, .....)

(same with RegistrarPocCommandTest::testUpdate) and testCreate_withAdminType

Done

[gbrodman]

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

---

core/src/test/java/google/registry/model/registrar/RegistrarTest.java line 344 at r2 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

It's changed upon saving in db line 135, I'm reloading to get the value of id field

oh ok, so we can either do this here or do the whole abuseAdminContact = persistResource(...) thing in the beforeEach like we do elsewhere. up to you, i'm fine either way

[ptkach]

Tested on alpha - looks good