mirror of
https://github.com/google/nomulus.git
synced 2025-04-30 12:07:51 +02:00
Fix missing autorenew onetime billing events (#1854)
This PR fixes the issue where the onetime billing event for an autorenew is not correctly created if the recurrence of the autorenew is closed during the autorenew grace period, such as the case if a manual renew happens during the same grace period. The detailed analysis of the issue is captured in b/258822640. Note that this is a quick and dirty fix to make ongoing billing event expanse work correctly in the future. It does not fix the missing events in the past, nor can it be used to reconstruct the missing ones (by providing a different cursor time), due to timeout when triggering the action from nomulus curl. Per Weimin, the recurrences that fits the new condition along, based on the current cursor, would increase from 382k to 430k, a 12% increase. I checked last nights cron job run, which starts on 22:00 EST and seemed to finish at 22:15 EST (when the last log for this request was recorded), so it should definitely still finish in time for the nightly runs with the new condition.
This commit is contained in:
parent
ff6599a95b
commit
22ef38fc3f
2 changed files with 56 additions and 2 deletions
|
@ -131,12 +131,14 @@ public class ExpandRecurringBillingEventsAction implements Runnable {
|
||||||
+ "WHERE eventTime <= :executeTime "
|
+ "WHERE eventTime <= :executeTime "
|
||||||
+ "AND eventTime < recurrenceEndTime "
|
+ "AND eventTime < recurrenceEndTime "
|
||||||
+ "AND id > :maxProcessedRecurrenceId "
|
+ "AND id > :maxProcessedRecurrenceId "
|
||||||
+ "AND recurrenceEndTime > :cursorTime "
|
+ "AND recurrenceEndTime > :adjustedCursorTime "
|
||||||
+ "ORDER BY id ASC",
|
+ "ORDER BY id ASC",
|
||||||
Recurring.class)
|
Recurring.class)
|
||||||
.setParameter("executeTime", executeTime)
|
.setParameter("executeTime", executeTime)
|
||||||
.setParameter("maxProcessedRecurrenceId", prevMaxProcessedRecurrenceId)
|
.setParameter("maxProcessedRecurrenceId", prevMaxProcessedRecurrenceId)
|
||||||
.setParameter("cursorTime", cursorTime)
|
.setParameter(
|
||||||
|
"adjustedCursorTime",
|
||||||
|
cursorTime.minus(Registry.DEFAULT_AUTO_RENEW_GRACE_PERIOD))
|
||||||
.setMaxResults(batchSize)
|
.setMaxResults(batchSize)
|
||||||
.getResultList();
|
.getResultList();
|
||||||
for (Recurring recurring : recurrings) {
|
for (Recurring recurring : recurrings) {
|
||||||
|
@ -279,6 +281,19 @@ public class ExpandRecurringBillingEventsAction implements Runnable {
|
||||||
.setReason("Domain autorenewal by ExpandRecurringBillingEventsAction")
|
.setReason("Domain autorenewal by ExpandRecurringBillingEventsAction")
|
||||||
.setRequestedByRegistrar(false)
|
.setRequestedByRegistrar(false)
|
||||||
.setType(DOMAIN_AUTORENEW)
|
.setType(DOMAIN_AUTORENEW)
|
||||||
|
// Note: the following statement seems to not be entirely correct as manual renewal
|
||||||
|
// during the autorenew grace period also closes out the existing recurrence, but in
|
||||||
|
// that instance the autorenew history entry should still have the transaction records
|
||||||
|
// for obvious reasons. It can be argued the history entry should always have the
|
||||||
|
// transaction record, regardless of what happens afterward. If the domain is deleted
|
||||||
|
// later during the autorenew grace period, another history entry for the delete would
|
||||||
|
// record that mutation separately, but the previous autorenew should not have its
|
||||||
|
// history entry retroactively altered, or in this case have the transaction records
|
||||||
|
// omitted when its created belatedly (when billing time is in scope). However, since
|
||||||
|
// we will be rewriting this action and only want to do the absolute minimum change to
|
||||||
|
// fix it for now, we will leave the current logic in place to avoid any unnecessary
|
||||||
|
// complications.
|
||||||
|
//
|
||||||
// Don't write a domain transaction record if the recurrence was
|
// Don't write a domain transaction record if the recurrence was
|
||||||
// ended prior to the billing time (i.e. a domain was deleted
|
// ended prior to the billing time (i.e. a domain was deleted
|
||||||
// during the autorenew grace period).
|
// during the autorenew grace period).
|
||||||
|
|
|
@ -455,6 +455,45 @@ public class ExpandRecurringBillingEventsActionTest {
|
||||||
assertCursorAt(currentTestTime);
|
assertCursorAt(currentTestTime);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void testSuccess_noEventExpanded_recurrenceEndAfterEvent_cursorTimeTooLate() throws Exception {
|
||||||
|
// This can occur when a domain is transferred/renewed/deleted during autorenew grace peroid.
|
||||||
|
recurring =
|
||||||
|
persistResource(
|
||||||
|
recurring
|
||||||
|
.asBuilder()
|
||||||
|
.setRecurrenceEndTime(recurring.getEventTime().plusDays(5))
|
||||||
|
.build());
|
||||||
|
action.cursorTimeParam = Optional.of(recurring.getRecurrenceEndTime().plusDays(46));
|
||||||
|
runAction();
|
||||||
|
// No new history entries should be generated because cursor time is more than 45 days after
|
||||||
|
// recurrence end time.
|
||||||
|
assertThat(getHistoryEntriesOfType(domain, DOMAIN_AUTORENEW)).isEmpty();
|
||||||
|
assertBillingEventsForResource(domain, recurring);
|
||||||
|
assertCursorAt(currentTestTime);
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void testSuccess_expandSingleEvent_recurrenceEndAfterEvent() throws Exception {
|
||||||
|
// This can occur when a domain is transferred/renewed/deleted during autorenew grace peroid.
|
||||||
|
recurring =
|
||||||
|
persistResource(
|
||||||
|
recurring
|
||||||
|
.asBuilder()
|
||||||
|
.setRecurrenceEndTime(recurring.getEventTime().plusDays(5))
|
||||||
|
.build());
|
||||||
|
action.cursorTimeParam = Optional.of(recurring.getRecurrenceEndTime().plusDays(35));
|
||||||
|
runAction();
|
||||||
|
DomainHistory persistedEntry =
|
||||||
|
getOnlyHistoryEntryOfType(domain, DOMAIN_AUTORENEW, DomainHistory.class);
|
||||||
|
assertHistoryEntryMatches(
|
||||||
|
domain, persistedEntry, "TheRegistrar", DateTime.parse("2000-02-19T00:00:00Z"), false);
|
||||||
|
BillingEvent.OneTime expected =
|
||||||
|
defaultOneTimeBuilder().setDomainHistory(persistedEntry).build();
|
||||||
|
assertBillingEventsForResource(domain, expected, recurring);
|
||||||
|
assertCursorAt(currentTestTime);
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void testSuccess_expandSingleEvent_dryRun() throws Exception {
|
void testSuccess_expandSingleEvent_dryRun() throws Exception {
|
||||||
persistResource(recurring);
|
persistResource(recurring);
|
||||||
|
|
Loading…
Add table
Reference in a new issue