killbill-aplcache

Merge pull request #804 from killbill/fix-for-767 invoice:

10/2/2017 1:02:48 PM

Details

diff --git a/beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestWithInvoicePlugin.java b/beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestWithInvoicePlugin.java
index 6ae2936..03dcc30 100644
--- a/beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestWithInvoicePlugin.java
+++ b/beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestWithInvoicePlugin.java
@@ -51,6 +51,7 @@ import org.killbill.notificationq.api.NotificationEventWithMetadata;
 import org.killbill.notificationq.api.NotificationQueue;
 import org.killbill.notificationq.api.NotificationQueueService;
 import org.killbill.notificationq.api.NotificationQueueService.NoSuchNotificationQueue;
+import org.killbill.queue.retry.RetryNotificationEvent;
 import org.killbill.queue.retry.RetryableService;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
@@ -112,11 +113,12 @@ public class TestWithInvoicePlugin extends TestIntegrationBase {
         assertEquals(invoiceUserApi.getInvoicesByAccount(account.getId(), false, callContext).size(), 0);
 
         // Verify bus event has moved to the retry service (can't easily check the timestamp unfortunately)
-        checkRetryBusEvents();
+        // No future notification at this point (FIXED item, the PHASE event is the trigger for the next one)
+        checkRetryBusEvents(1, 0);
 
         // Add 5'
         clock.addDeltaFromReality(5 * 60 * 1000);
-        checkRetryBusEvents();
+        checkRetryBusEvents(2, 0);
 
         // Fix invoice plugin
         testInvoicePluginApi.shouldThrowException = false;
@@ -156,11 +158,12 @@ public class TestWithInvoicePlugin extends TestIntegrationBase {
         assertEquals(invoiceUserApi.getInvoicesByAccount(account.getId(), false, callContext).size(), 2);
 
         // Verify notification has moved to the retry service
-        checkRetryNotifications("2012-06-01T00:05:00");
+        checkRetryNotifications("2012-06-01T00:05:00", 1);
 
         // Add 5'
         clock.addDeltaFromReality(5 * 60 * 1000);
-        checkRetryNotifications("2012-06-01T00:15:00");
+        // Verify there are no notification duplicates
+        checkRetryNotifications("2012-06-01T00:15:00", 1);
 
         // Fix invoice plugin
         testInvoicePluginApi.shouldThrowException = false;
@@ -188,7 +191,7 @@ public class TestWithInvoicePlugin extends TestIntegrationBase {
         assertEquals(invoiceUserApi.getInvoicesByAccount(account.getId(), false, callContext).size(), 3);
 
         // Verify notification has moved to the retry service
-        checkRetryNotifications("2012-07-01T00:05:00");
+        checkRetryNotifications("2012-07-01T00:05:00", 1);
 
         testInvoicePluginApi.shouldThrowException = false;
 
@@ -200,20 +203,19 @@ public class TestWithInvoicePlugin extends TestIntegrationBase {
         assertEquals(invoiceUserApi.getInvoicesByAccount(account.getId(), false, callContext).size(), 4);
     }
 
-    private void checkRetryBusEvents() throws NoSuchNotificationQueue {
+    private void checkRetryBusEvents(final int retryNb, final int expectedFutureInvoiceNotifications) throws NoSuchNotificationQueue {
         // Verify notification(s) moved to the retry queue
         Awaitility.await().atMost(5, TimeUnit.SECONDS).until(new Callable<Boolean>() {
             @Override
             public Boolean call() throws Exception {
                 final List<NotificationEventWithMetadata> futureInvoiceRetryableBusEvents = getFutureInvoiceRetryableBusEvents();
-                return futureInvoiceRetryableBusEvents.size() == 1;
+                return futureInvoiceRetryableBusEvents.size() == 1 && ((RetryNotificationEvent) futureInvoiceRetryableBusEvents.get(0).getEvent()).getRetryNb() == retryNb;
             }
         });
-        assertEquals(getFutureInvoiceRetryableBusEvents().size(), 1);
-        assertEquals(getFutureInvoiceNotifications().size(), 0);
+        assertEquals(getFutureInvoiceNotifications().size(), expectedFutureInvoiceNotifications);
     }
 
-    private void checkRetryNotifications(final String retryDateTime) throws NoSuchNotificationQueue {
+    private void checkRetryNotifications(final String retryDateTime, final int expectedFutureInvoiceNotifications) throws NoSuchNotificationQueue {
         // Verify notification(s) moved to the retry queue
         Awaitility.await().atMost(5, TimeUnit.SECONDS).until(new Callable<Boolean>() {
             @Override
@@ -222,8 +224,7 @@ public class TestWithInvoicePlugin extends TestIntegrationBase {
                 return futureInvoiceRetryableNotifications.size() == 1 && futureInvoiceRetryableNotifications.get(0).getEffectiveDate().compareTo(new DateTime(retryDateTime, DateTimeZone.UTC)) == 0;
             }
         });
-        assertEquals(getFutureInvoiceRetryableNotifications().size(), 1);
-        assertEquals(getFutureInvoiceNotifications().size(), 0);
+        assertEquals(getFutureInvoiceNotifications().size(), expectedFutureInvoiceNotifications);
     }
 
     private void checkNotificationsNoRetry(final int main) throws NoSuchNotificationQueue {
diff --git a/invoice/src/main/java/org/killbill/billing/invoice/InvoiceDispatcher.java b/invoice/src/main/java/org/killbill/billing/invoice/InvoiceDispatcher.java
index 2d547ac..d55ebac 100644
--- a/invoice/src/main/java/org/killbill/billing/invoice/InvoiceDispatcher.java
+++ b/invoice/src/main/java/org/killbill/billing/invoice/InvoiceDispatcher.java
@@ -364,54 +364,38 @@ public class InvoiceDispatcher {
 
     private Invoice processAccountWithLockAndInputTargetDate(final UUID accountId, final LocalDate targetDate,
                                                              final BillingEventSet billingEvents, final boolean isDryRun, final InternalCallContext context) throws InvoiceApiException {
+        final ImmutableAccountData account;
         try {
-            final ImmutableAccountData account = accountApi.getImmutableAccountDataById(accountId, context);
-
-            final List<Invoice> invoices = billingEvents.isAccountAutoInvoiceOff() ?
-                                           ImmutableList.<Invoice>of() :
-                                           ImmutableList.<Invoice>copyOf(Collections2.transform(invoiceDao.getInvoicesByAccount(context),
-                                                                                                new Function<InvoiceModelDao, Invoice>() {
-                                                                                                    @Override
-                                                                                                    public Invoice apply(final InvoiceModelDao input) {
-                                                                                                        return new DefaultInvoice(input);
-                                                                                                    }
-                                                                                                }));
-
-            final Currency targetCurrency = account.getCurrency();
-
-            final UUID targetInvoiceId;
-            if (billingEvents.isAccountAutoInvoiceReuseDraft()) {
-                final InvoiceModelDao earliestDraftInvoice = invoiceDao.getEarliestDraftInvoiceByAccount(context);
-                targetInvoiceId = earliestDraftInvoice != null ? earliestDraftInvoice.getId() : null;
-            } else {
-                targetInvoiceId = null;
-            }
-
-            final InvoiceWithMetadata invoiceWithMetadata = generator.generateInvoice(account, billingEvents, invoices, targetInvoiceId, targetDate, targetCurrency, context);
-            final DefaultInvoice invoice = invoiceWithMetadata.getInvoice();
+            account = accountApi.getImmutableAccountDataById(accountId, context);
+        } catch (final AccountApiException e) {
+            log.error("Unable to generate invoice for accountId='{}', a future notification has NOT been recorded", accountId, e);
+            return null;
+        }
 
-            // Compute future notifications
-            final FutureAccountNotifications futureAccountNotifications = createNextFutureNotificationDate(invoiceWithMetadata, context);
+        final InvoiceWithMetadata invoiceWithMetadata = generateKillBillInvoice(account, targetDate, billingEvents, context);
+        final DefaultInvoice invoice = invoiceWithMetadata.getInvoice();
 
-            //
+        // Compute future notifications
+        final FutureAccountNotifications futureAccountNotifications = createNextFutureNotificationDate(invoiceWithMetadata, context);
 
-            // If invoice comes back null, there is nothing new to generate, we can bail early
-            //
-            if (invoice == null) {
-                if (isDryRun) {
-                    log.info("Generated null dryRun invoice for accountId='{}', targetDate='{}'", accountId, targetDate);
-                } else {
-                    log.info("Generated null invoice for accountId='{}', targetDate='{}'", accountId, targetDate);
+        // If invoice comes back null, there is nothing new to generate, we can bail early
+        if (invoice == null) {
+            if (isDryRun) {
+                log.info("Generated null dryRun invoice for accountId='{}', targetDate='{}'", accountId, targetDate);
+            } else {
+                log.info("Generated null invoice for accountId='{}', targetDate='{}'", accountId, targetDate);
 
-                    final BusInternalEvent event = new DefaultNullInvoiceEvent(accountId, clock.getUTCToday(),
-                                                                               context.getAccountRecordId(), context.getTenantRecordId(), context.getUserToken());
+                final BusInternalEvent event = new DefaultNullInvoiceEvent(accountId, clock.getUTCToday(),
+                                                                           context.getAccountRecordId(), context.getTenantRecordId(), context.getUserToken());
 
-                    commitInvoiceAndSetFutureNotifications(account, null, futureAccountNotifications, context);
-                    postEvent(event);
-                }
-                return null;
+                commitInvoiceAndSetFutureNotifications(account, null, futureAccountNotifications, context);
+                postEvent(event);
             }
+            return null;
+        }
 
+        boolean success = false;
+        try {
             // Generate missing credit (> 0 for generation and < 0 for use) prior we call the plugin
             final InvoiceItem cbaItemPreInvoicePlugins = computeCBAOnExistingInvoice(invoice, context);
             DefaultInvoice tmpInvoiceForInvoicePlugins = invoice;
@@ -453,21 +437,47 @@ public class InvoiceDispatcher {
                 invoiceModelDao.addInvoiceItems(invoiceItemModelDaos);
 
                 // Commit invoice on disk
-                final boolean isThereAnyItemsLeft = commitInvoiceAndSetFutureNotifications(account, invoiceModelDao, futureAccountNotifications, context);
+                commitInvoiceAndSetFutureNotifications(account, invoiceModelDao, futureAccountNotifications, context);
+                success = true;
+
+                try {
+                    setChargedThroughDates(invoice.getInvoiceItems(FixedPriceInvoiceItem.class), invoice.getInvoiceItems(RecurringInvoiceItem.class), context);
+                } catch (final SubscriptionBaseApiException e) {
+                    log.error("Failed handling SubscriptionBase change.", e);
+                    return null;
+                }
+            }
+        } finally {
+            // Make sure we always set future notifications in case of errors
+            if (!isDryRun && !success) {
+                commitInvoiceAndSetFutureNotifications(account, null, futureAccountNotifications, context);
+            }
+        }
 
-                final boolean isRealInvoiceWithNonEmptyItems = isThereAnyItemsLeft ? isRealInvoiceWithItems : false;
+        return invoice;
+    }
 
-                setChargedThroughDates(invoice.getInvoiceItems(FixedPriceInvoiceItem.class), invoice.getInvoiceItems(RecurringInvoiceItem.class), context);
+    private InvoiceWithMetadata generateKillBillInvoice(final ImmutableAccountData account, final LocalDate targetDate, final BillingEventSet billingEvents, final InternalCallContext context) throws InvoiceApiException {
+        final List<Invoice> invoices = billingEvents.isAccountAutoInvoiceOff() ?
+                                       ImmutableList.<Invoice>of() :
+                                       ImmutableList.<Invoice>copyOf(Collections2.transform(invoiceDao.getInvoicesByAccount(context),
+                                                                                            new Function<InvoiceModelDao, Invoice>() {
+                                                                                                @Override
+                                                                                                public Invoice apply(final InvoiceModelDao input) {
+                                                                                                    return new DefaultInvoice(input);
+                                                                                                }
+                                                                                            }));
+        final Currency targetCurrency = account.getCurrency();
 
-            }
-            return invoice;
-        } catch (final AccountApiException e) {
-            log.error("Failed handling SubscriptionBase change.", e);
-            return null;
-        } catch (final SubscriptionBaseApiException e) {
-            log.error("Failed handling SubscriptionBase change.", e);
-            return null;
+        final UUID targetInvoiceId;
+        if (billingEvents.isAccountAutoInvoiceReuseDraft()) {
+            final InvoiceModelDao earliestDraftInvoice = invoiceDao.getEarliestDraftInvoiceByAccount(context);
+            targetInvoiceId = earliestDraftInvoice != null ? earliestDraftInvoice.getId() : null;
+        } else {
+            targetInvoiceId = null;
         }
+
+        return generator.generateInvoice(account, billingEvents, invoices, targetInvoiceId, targetDate, targetCurrency, context);
     }
 
     private FutureAccountNotifications createNextFutureNotificationDate(final InvoiceWithMetadata invoiceWithMetadata, final InternalCallContext context) {
@@ -549,17 +559,16 @@ public class InvoiceDispatcher {
         log.info(tmp.toString());
     }
 
-    private boolean commitInvoiceAndSetFutureNotifications(final ImmutableAccountData account,
-                                                           @Nullable final InvoiceModelDao invoiceModelDao,
-                                                           final FutureAccountNotifications futureAccountNotifications,
-                                                           final InternalCallContext context) throws SubscriptionBaseApiException, InvoiceApiException {
+    private void commitInvoiceAndSetFutureNotifications(final ImmutableAccountData account,
+                                                        @Nullable final InvoiceModelDao invoiceModelDao,
+                                                        final FutureAccountNotifications futureAccountNotifications,
+                                                        final InternalCallContext context) {
         final boolean isThereAnyItemsLeft = invoiceModelDao != null && !invoiceModelDao.getInvoiceItems().isEmpty();
         if (isThereAnyItemsLeft) {
             invoiceDao.createInvoice(invoiceModelDao, futureAccountNotifications, context);
         } else {
             invoiceDao.setFutureAccountNotificationsForEmptyInvoice(account.getId(), futureAccountNotifications, context);
         }
-        return isThereAnyItemsLeft;
     }
 
     private InvoiceItem computeCBAOnExistingInvoice(final Invoice invoice, final InternalCallContext context) throws InvoiceApiException {