Create a scrap command to cancel OneTime billing events by ID

[gbrodman]

This allows us to correct situations where we have erroneously charged registrars for an action, without explicitly issuing a refund.

---

This change is 

[jianglai]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gbrodman and @sarahcaseybot)

---

core/src/main/java/google/registry/tools/javascrap/CreateCancellationsForOneTimesCommand.java line 60 at r1 (raw file):

                VKey<OneTime> key = VKey.createSql(OneTime.class, billingEventId);
                if (tm().exists(key)) {
                  OneTime oneTime = tm().loadByKey(key);

Might be more efficient to use loadByKeysIfPresent and do the processing outside the transaction? Since you are creating a cancellation in a different transaction in the execute, it's not like transactionality is preserved anyway. I don't think cancellations can be created in other ways so we should be safe to do it in two transactions.

Code quote:

OneTime oneTime = tm().loadByKey(key);

---

core/src/main/java/google/registry/tools/javascrap/CreateCancellationsForOneTimesCommand.java line 96 at r1 (raw file):

          tm().transact(
                  () -> {
                    if (alreadyCancelled(oneTime)) {

It seems a bit redundant to check for this twice, here and above when you create the oneTimesToCancel, given that the odds of some OneTime becomes cancelled between now and then is practically nonexistent.

A better approach might be to just load all existing OneTimes from the list and filter out the ones that are referenced in Cancellation in one query.

But I do recognize that we may be over engineering for a scrap tool that only have to deal with a couple thousand entities at most...

Code quote:

alreadyCancelled

---

core/src/main/java/google/registry/tools/javascrap/CreateCancellationsForOneTimesCommand.java line 102 at r1 (raw file):

                    }
                    tm().put(
                            new Cancellation.Builder()

Technically these values should be for the "thing" that created the cancellation, not just values copied over from the onetime. I understand for a lot of them we simply cannot find a better proxy than the value in onetime because this is a manual operation. But maybe there should be a better Reason at least? How about the deprecated ERROR enum value, which according to b/31676071 is created for similar reasons. In fact, the bug mentions a scrap tool to manually create cancellations to correct errors...

Also, what happened to the history entries of the change that were reverted during the resave?

Code quote:

                            new Cancellation.Builder()
                                .setOneTimeEventKey(oneTime.createVKey())
                                .setBillingTime(oneTime.getBillingTime())
                                .setDomainHistoryId(oneTime.getDomainHistoryId())
                                .setRegistrarId(oneTime.getRegistrarId())
                                .setEventTime(oneTime.getEventTime())
                                .setFlags(oneTime.getFlags())
                                .setReason(oneTime.getReason())
                                .setTargetId(oneTime.getTargetId())
                                .build());

[gbrodman]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jianglai and @sarahcaseybot)

---

core/src/main/java/google/registry/tools/javascrap/CreateCancellationsForOneTimesCommand.java line 60 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Might be more efficient to use loadByKeysIfPresent and do the processing outside the transaction? Since you are creating a cancellation in a different transaction in the execute, it's not like transactionality is preserved anyway. I don't think cancellations can be created in other ways so we should be safe to do it in two transactions.

Yeah Cancellations are only created by cancelling grace periods, but to be honest I wanted to be overly cautious here just in case we accidentally ran the command twice in close proximity to each other.

---

core/src/main/java/google/registry/tools/javascrap/CreateCancellationsForOneTimesCommand.java line 96 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

It seems a bit redundant to check for this twice, here and above when you create the oneTimesToCancel, given that the odds of some OneTime becomes cancelled between now and then is practically nonexistent.

A better approach might be to just load all existing OneTimes from the list and filter out the ones that are referenced in Cancellation in one query.

But I do recognize that we may be over engineering for a scrap tool that only have to deal with a couple thousand entities at most...

yeah, my thinking was that this whole thing was caused by a separate-transaction race condition, so better to be safe than to risk anything

---

core/src/main/java/google/registry/tools/javascrap/CreateCancellationsForOneTimesCommand.java line 102 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Technically these values should be for the "thing" that created the cancellation, not just values copied over from the onetime. I understand for a lot of them we simply cannot find a better proxy than the value in onetime because this is a manual operation. But maybe there should be a better Reason at least? How about the deprecated ERROR enum value, which according to b/31676071 is created for similar reasons. In fact, the bug mentions a scrap tool to manually create cancellations to correct errors...

Also, what happened to the history entries of the change that were reverted during the resave?

The history entries are still there but those are harmless, no issues with those.

I noticed for existing cancellation BillingEvents they usually do tend to map to the BillingEvent that they're cancelling. My thinking is that effectively we should treat these cancellations as "occurring" at the same time as the OneTime event since they should never have happened.

Good to know about ERROR though, changed.

[jianglai]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman and @sarahcaseybot)

---

core/src/main/java/google/registry/tools/javascrap/CreateCancellationsForOneTimesCommand.java line 102 at r1 (raw file):

Previously, gbrodman wrote…

The history entries are still there but those are harmless, no issues with those.

I noticed for existing cancellation BillingEvents they usually do tend to map to the BillingEvent that they're cancelling. My thinking is that effectively we should treat these cancellations as "occurring" at the same time as the OneTime event since they should never have happened.

Good to know about ERROR though, changed.

Yeah, if you read b/31676071 and it's associated CL, Nick mentioned that the entries in cancellations map to onetime because, like you said earlier, it's caused by cancellation during grace period, so it was an intentional decision to map them.

Any reason to not copy the flag? I guess it shouldn't really matter.

[sarahcaseybot]

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)

[gbrodman]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai)

---

core/src/main/java/google/registry/tools/javascrap/CreateCancellationsForOneTimesCommand.java line 102 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Yeah, if you read b/31676071 and it's associated CL, Nick mentioned that the entries in cancellations map to onetime because, like you said earlier, it's caused by cancellation during grace period, so it was an intentional decision to map them.

Any reason to not copy the flag? I guess it shouldn't really matter.

yeah my thinking was that the flags are unnecessary for cancellations and none of them really apply here