invoice: code review integration

See #934 (comment).

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>

invoice/src/main/java/org/killbill/billing/invoice/InvoiceDispatcher.java

@@ -75,7 +75,6 @@
 import org.killbill.billing.invoice.generator.InvoiceWithMetadata.SubscriptionFutureNotificationDates;
 import org.killbill.billing.invoice.generator.InvoiceWithMetadata.SubscriptionFutureNotificationDates.UsageDef;
 import org.killbill.billing.invoice.model.DefaultInvoice;
-import org.killbill.billing.invoice.model.InvoiceItemCatalogBase;
 import org.killbill.billing.invoice.model.InvoiceItemFactory;
 import org.killbill.billing.invoice.model.ItemAdjInvoiceItem;
 import org.killbill.billing.invoice.model.ParentInvoiceItem;
@@ -558,35 +557,20 @@ private Invoice processAccountWithLockAndInputTargetDate(final UUID accountId,
 
         boolean success = false;
         try {
-            // Generate missing credit (> 0 for generation and < 0 for use) prior we call the plugin
+            // Generate missing credit (> 0 for generation and < 0 for use) prior we call the plugin(s)
             final InvoiceItem cbaItemPreInvoicePlugins = computeCBAOnExistingInvoice(invoice, internalCallContext);
-            DefaultInvoice tmpInvoiceForInvoicePlugins = invoice;
             if (cbaItemPreInvoicePlugins != null) {
-                tmpInvoiceForInvoicePlugins = (DefaultInvoice) tmpInvoiceForInvoicePlugins.clone();
-                tmpInvoiceForInvoicePlugins.addInvoiceItem(cbaItemPreInvoicePlugins);
+                invoice.addInvoiceItem(cbaItemPreInvoicePlugins);
             }
+
             //
             // Ask external invoice plugins if additional items (tax, etc) shall be added to the invoice
             //
-            final List<InvoiceItem> additionalInvoiceItemsFromPlugins = invoicePluginDispatcher.getAdditionalInvoiceItems(tmpInvoiceForInvoicePlugins, isDryRun, callContext, internalCallContext);
-            if (additionalInvoiceItemsFromPlugins.isEmpty()) {
-                // PERF: avoid re-computing the CBA if no change was made
+            final boolean invoiceUpdated = invoicePluginDispatcher.updateOriginalInvoiceWithPluginInvoiceItems(invoice, isDryRun, callContext, internalCallContext);
+            if (invoiceUpdated) {
+                // Remove the temporary CBA item as we need to re-compute CBA
                 if (cbaItemPreInvoicePlugins != null) {
-                    invoice.addInvoiceItem(cbaItemPreInvoicePlugins);
-                }
-            } else {
-                // Add or update items from generated invoice
-                for (final InvoiceItem cur : additionalInvoiceItemsFromPlugins) {
-                    final InvoiceItem existingItem = Iterables.tryFind(tmpInvoiceForInvoicePlugins.getInvoiceItems(), new Predicate<InvoiceItem>() {
-                        @Override
-                        public boolean apply(final InvoiceItem input) {
-                            return input.getId().equals(cur.getId());
-                        }
-                    }).orNull();
-                    if (existingItem != null) {
-                        invoice.removeInvoiceItem(existingItem);
-                    }
-                    invoice.addInvoiceItem(cur);
+                    invoice.removeInvoiceItem(cbaItemPreInvoicePlugins);
                 }
 
                 // Use credit after we call the plugin (https://github.com/killbill/killbill/issues/637)

invoice/src/main/java/org/killbill/billing/invoice/InvoicePluginDispatcher.java

@@ -155,32 +155,54 @@ private void onCompletionCall(final boolean isSuccess,
         }
     }
 
-    //
-    // If we have multiple plugins there is a question of plugin ordering and also a 'product' questions to decide whether
-    // subsequent plugins should have access to items added by previous plugins
-    //
-    public List<InvoiceItem> getAdditionalInvoiceItems(final Invoice originalInvoice, final boolean isDryRun, final CallContext callContext, final InternalTenantContext tenantContext) throws InvoiceApiException {
+    public boolean updateOriginalInvoiceWithPluginInvoiceItems(final DefaultInvoice originalInvoice, final boolean isDryRun, final CallContext callContext, final InternalTenantContext tenantContext) throws InvoiceApiException {
         log.debug("Invoking invoice plugins getAdditionalInvoiceItems: isDryRun='{}', originalInvoice='{}'", isDryRun, originalInvoice);
 
-        final List<InvoiceItem> additionalInvoiceItems = new LinkedList<InvoiceItem>();
-
         final Collection<InvoicePluginApi> invoicePlugins = getInvoicePlugins(tenantContext).values();
         if (invoicePlugins.isEmpty()) {
-            return additionalInvoiceItems;
+            return false;
         }
 
-        // We clone the original invoice so plugins don't remove/add items
-        final Invoice clonedInvoice = (Invoice) ((DefaultInvoice) originalInvoice).clone();
+        boolean invoiceUpdated = false;
         for (final InvoicePluginApi invoicePlugin : invoicePlugins) {
+            // We clone the original invoice so plugins don't remove/add items
+            final Invoice clonedInvoice = (Invoice) originalInvoice.clone();
             final List<InvoiceItem> additionalInvoiceItemsForPlugin = invoicePlugin.getAdditionalInvoiceItems(clonedInvoice, isDryRun, ImmutableList.<PluginProperty>of(), callContext);
-            if (additionalInvoiceItemsForPlugin != null) {
+
+            if (additionalInvoiceItemsForPlugin != null && !additionalInvoiceItemsForPlugin.isEmpty()) {
+                final Collection<InvoiceItem> additionalInvoiceItems = new LinkedList<InvoiceItem>();
                 for (final InvoiceItem additionalInvoiceItem : additionalInvoiceItemsForPlugin) {
                     final InvoiceItem sanitizedInvoiceItem = validateAndSanitizeInvoiceItemFromPlugin(originalInvoice, additionalInvoiceItem, invoicePlugin);
                     additionalInvoiceItems.add(sanitizedInvoiceItem);
                 }
+                invoiceUpdated = invoiceUpdated || updateOriginalInvoiceWithPluginInvoiceItems(originalInvoice, additionalInvoiceItems);
             }
         }
-        return additionalInvoiceItems;
+
+        return invoiceUpdated;
+    }
+
+    private boolean updateOriginalInvoiceWithPluginInvoiceItems(final DefaultInvoice originalInvoice, final Collection<InvoiceItem> additionalInvoiceItems) {
+        if (additionalInvoiceItems.isEmpty()) {
+            return false;
+        }
+
+        // Add or update items from generated invoice
+        for (final InvoiceItem additionalInvoiceItem : additionalInvoiceItems) {
+            final InvoiceItem existingItem = Iterables.tryFind(originalInvoice.getInvoiceItems(),
+                                                               new Predicate<InvoiceItem>() {
+                                                                   @Override
+                                                                   public boolean apply(final InvoiceItem originalInvoiceItem) {
+                                                                       return originalInvoiceItem.getId().equals(additionalInvoiceItem.getId());
+                                                                   }
+                                                               }).orNull();
+            if (existingItem != null) {
+                originalInvoice.removeInvoiceItem(existingItem);
+            }
+            originalInvoice.addInvoiceItem(additionalInvoiceItem);
+        }
+
+        return true;
     }
 
     private InvoiceItem validateAndSanitizeInvoiceItemFromPlugin(final Invoice originalInvoice, final InvoiceItem additionalInvoiceItem, final InvoicePluginApi invoicePlugin) throws InvoiceApiException {

invoice/src/main/java/org/killbill/billing/invoice/api/InvoiceApiHelper.java

@@ -92,18 +92,17 @@ public List<InvoiceItem> dispatchToInvoicePluginsAndInsertItems(final UUID accou
 
         boolean success = false;
         GlobalLock lock = null;
-        Iterable<Invoice> invoicesForPlugins = null;
+        Iterable<DefaultInvoice> invoicesForPlugins = null;
         try {
             lock = locker.lockWithNumberOfTries(LockerType.ACCNT_INV_PAY.toString(), accountId.toString(), invoiceConfig.getMaxGlobalLockRetries());
 
             invoicesForPlugins = withAccountLock.prepareInvoices();
 
             final InternalCallContext internalCallContext = internalCallContextFactory.createInternalCallContext(accountId, context);
             final List<InvoiceModelDao> invoiceModelDaos = new LinkedList<InvoiceModelDao>();
-            for (final Invoice invoiceForPlugin : invoicesForPlugins) {
-                // Call plugin
-                final List<InvoiceItem> additionalInvoiceItems = invoicePluginDispatcher.getAdditionalInvoiceItems(invoiceForPlugin, isDryRun, context, internalCallContext);
-                invoiceForPlugin.addInvoiceItems(additionalInvoiceItems);
+            for (final DefaultInvoice invoiceForPlugin : invoicesForPlugins) {
+                // Call plugin(s)
+                invoicePluginDispatcher.updateOriginalInvoiceWithPluginInvoiceItems(invoiceForPlugin, isDryRun, context, internalCallContext);
 
                 // Transformation to InvoiceModelDao
                 final InvoiceModelDao invoiceModelDao = new InvoiceModelDao(invoiceForPlugin);

invoice/src/main/java/org/killbill/billing/invoice/api/WithAccountLock.java

@@ -1,6 +1,6 @@
 /*
- * Copyright 2015 Groupon, Inc
- * Copyright 2015 The Billing Project, LLC
+ * Copyright 2014-2018 Groupon, Inc
+ * Copyright 2014-2018 The Billing Project, LLC
  *
  * The Billing Project licenses this file to you under the Apache License, version 2.0
  * (the "License"); you may not use this file except in compliance with the
@@ -17,7 +17,9 @@
 
 package org.killbill.billing.invoice.api;
 
+import org.killbill.billing.invoice.model.DefaultInvoice;
+
 public interface WithAccountLock {
 
-    public Iterable<Invoice> prepareInvoices() throws InvoiceApiException;
+    public Iterable<DefaultInvoice> prepareInvoices() throws InvoiceApiException;
 }

invoice/src/main/java/org/killbill/billing/invoice/api/svcs/DefaultInvoiceInternalApi.java

@@ -1,7 +1,7 @@
 /*
  * Copyright 2010-2013 Ning, Inc.
- * Copyright 2014-2016 Groupon, Inc
- * Copyright 2014-2016 The Billing Project, LLC
+ * Copyright 2014-2018 Groupon, Inc
+ * Copyright 2014-2018 The Billing Project, LLC
  *
  * The Billing Project licenses this file to you under the Apache License, version 2.0
  * (the "License"); you may not use this file except in compliance with the
@@ -36,7 +36,6 @@
 import org.killbill.billing.invoice.api.InvoiceApiException;
 import org.killbill.billing.invoice.api.InvoiceApiHelper;
 import org.killbill.billing.invoice.api.InvoiceInternalApi;
-import org.killbill.billing.invoice.api.InvoiceItem;
 import org.killbill.billing.invoice.api.InvoicePayment;
 import org.killbill.billing.invoice.api.InvoicePaymentType;
 import org.killbill.billing.invoice.api.InvoiceStatus;
@@ -76,6 +75,10 @@ public DefaultInvoiceInternalApi(final InvoiceDao dao,
 
     @Override
     public Invoice getInvoiceById(final UUID invoiceId, final InternalTenantContext context) throws InvoiceApiException {
+        return getInvoiceByIdInternal(invoiceId, context);
+    }
+
+    private DefaultInvoice getInvoiceByIdInternal(final UUID invoiceId, final InternalTenantContext context) {
         return new DefaultInvoice(dao.getById(invoiceId, context));
     }
 
@@ -132,12 +135,12 @@ public InvoicePayment recordRefund(final UUID paymentId, final BigDecimal amount
 
         // See https://github.com/killbill/killbill/issues/265
         final CallContext callContext = internalCallContextFactory.createCallContext(context);
-        final Invoice invoice = getInvoiceById(refund.getInvoiceId(), context);
+        final DefaultInvoice invoice = getInvoiceByIdInternal(refund.getInvoiceId(), context);
         final UUID accountId = invoice.getAccountId();
         final WithAccountLock withAccountLock = new WithAccountLock() {
             @Override
-            public Iterable<Invoice> prepareInvoices() throws InvoiceApiException {
-                return ImmutableList.<Invoice>of(invoice);
+            public Iterable<DefaultInvoice> prepareInvoices() throws InvoiceApiException {
+                return ImmutableList.<DefaultInvoice>of(invoice);
             }
         };
         invoiceApiHelper.dispatchToInvoicePluginsAndInsertItems(accountId, false, withAccountLock, callContext);

invoice/src/main/java/org/killbill/billing/invoice/api/user/DefaultInvoiceUserApi.java

@@ -204,6 +204,10 @@ public BigDecimal getAccountCBA(final UUID accountId, final TenantContext contex
 
     @Override
     public Invoice getInvoice(final UUID invoiceId, final TenantContext context) throws InvoiceApiException {
+        return getInvoiceInternal(invoiceId, context);
+    }
+
+    private DefaultInvoice getInvoiceInternal(final UUID invoiceId, final TenantContext context) {
         final InternalTenantContext internalTenantContext = internalCallContextFactory.createInternalTenantContext(invoiceId, ObjectType.INVOICE, context);
         return new DefaultInvoice(dao.getById(invoiceId, internalTenantContext), getCatalogSafelyForPrettyNames(internalTenantContext));
     }
@@ -285,13 +289,13 @@ public List<InvoiceItem> insertExternalCharges(final UUID accountId, final Local
         final WithAccountLock withAccountLock = new WithAccountLock() {
 
             @Override
-            public Iterable<Invoice> prepareInvoices() throws InvoiceApiException {
+            public Iterable<DefaultInvoice> prepareInvoices() throws InvoiceApiException {
                 final InternalTenantContext internalTenantContext = internalCallContextFactory.createInternalTenantContext(accountId, context);
                 final LocalDate invoiceDate = internalTenantContext.toLocalDate(context.getCreatedDate());
 
                 // Group all new external charges on the same invoice (per currency)
-                final Map<Currency, Invoice> newInvoicesForExternalCharges = new HashMap<Currency, Invoice>();
-                final Map<UUID, Invoice> existingInvoicesForExternalCharges = new HashMap<UUID, Invoice>();
+                final Map<Currency, DefaultInvoice> newInvoicesForExternalCharges = new HashMap<Currency, DefaultInvoice>();
+                final Map<UUID, DefaultInvoice> existingInvoicesForExternalCharges = new HashMap<UUID, DefaultInvoice>();
 
                 for (final InvoiceItem charge : charges) {
                     final Invoice invoiceForExternalCharge;
@@ -301,13 +305,13 @@ public Iterable<Invoice> prepareInvoices() throws InvoiceApiException {
                         final Currency currency = charge.getCurrency();
                         if (newInvoicesForExternalCharges.get(currency) == null) {
                             final InvoiceStatus status = autoCommit ? InvoiceStatus.COMMITTED : InvoiceStatus.DRAFT;
-                            final Invoice newInvoiceForExternalCharge = new DefaultInvoice(accountId, invoiceDate, effectiveDate, currency, status);
+                            final DefaultInvoice newInvoiceForExternalCharge = new DefaultInvoice(accountId, invoiceDate, effectiveDate, currency, status);
                             newInvoicesForExternalCharges.put(currency, newInvoiceForExternalCharge);
                         }
                         invoiceForExternalCharge = newInvoicesForExternalCharges.get(currency);
                     } else {
                         if (existingInvoicesForExternalCharges.get(invoiceIdForExternalCharge) == null) {
-                            final Invoice existingInvoiceForExternalCharge = getInvoice(invoiceIdForExternalCharge, context);
+                            final DefaultInvoice existingInvoiceForExternalCharge = getInvoiceInternal(invoiceIdForExternalCharge, context);
                             if (InvoiceStatus.COMMITTED.equals(existingInvoiceForExternalCharge.getStatus())) {
                                 throw new InvoiceApiException(ErrorCode.INVOICE_ALREADY_COMMITTED, existingInvoiceForExternalCharge.getId());
                             }
@@ -341,7 +345,7 @@ public Iterable<Invoice> prepareInvoices() throws InvoiceApiException {
                     invoiceForExternalCharge.addInvoiceItem(externalCharge);
                 }
 
-                return Iterables.<Invoice>concat(newInvoicesForExternalCharges.values(), existingInvoicesForExternalCharges.values());
+                return Iterables.<DefaultInvoice>concat(newInvoicesForExternalCharges.values(), existingInvoicesForExternalCharges.values());
             }
         };
 
@@ -382,12 +386,12 @@ private InvoiceItem insertCreditForInvoice(final UUID accountId, final UUID invo
             private InvoiceItem creditItem;
 
             @Override
-            public List<Invoice> prepareInvoices() throws InvoiceApiException {
+            public List<DefaultInvoice> prepareInvoices() throws InvoiceApiException {
                 final InternalTenantContext internalTenantContext = internalCallContextFactory.createInternalTenantContext(accountId, context);
                 final LocalDate invoiceDate = internalTenantContext.toLocalDate(context.getCreatedDate());
 
                 // Create an invoice for that credit if it doesn't exist
-                final Invoice invoiceForCredit;
+                final DefaultInvoice invoiceForCredit;
                 if (invoiceId == null) {
                     final InvoiceStatus status = autoCommit ? InvoiceStatus.COMMITTED : InvoiceStatus.DRAFT;
                     invoiceForCredit = new DefaultInvoice(accountId, invoiceDate, effectiveDate, currency, status);
@@ -411,7 +415,7 @@ public List<Invoice> prepareInvoices() throws InvoiceApiException {
                                                       itemDetails);
                 invoiceForCredit.addInvoiceItem(creditItem);
 
-                return ImmutableList.<Invoice>of(invoiceForCredit);
+                return ImmutableList.<DefaultInvoice>of(invoiceForCredit);
             }
         };
 
@@ -443,8 +447,8 @@ public InvoiceItem insertInvoiceItemAdjustment(final UUID accountId, final UUID
 
         final WithAccountLock withAccountLock = new WithAccountLock() {
             @Override
-            public Iterable<Invoice> prepareInvoices() throws InvoiceApiException {
-                final Invoice invoice = getInvoiceAndCheckCurrency(invoiceId, currency, context);
+            public Iterable<DefaultInvoice> prepareInvoices() throws InvoiceApiException {
+                final DefaultInvoice invoice = getInvoiceAndCheckCurrency(invoiceId, currency, context);
                 final InvoiceItem adjustmentItem = invoiceApiHelper.createAdjustmentItem(invoice,
                                                                                          invoiceItemId,
                                                                                          amount,
@@ -457,7 +461,7 @@ public Iterable<Invoice> prepareInvoices() throws InvoiceApiException {
                     invoice.addInvoiceItem(adjustmentItem);
                 }
 
-                return ImmutableList.<Invoice>of(invoice);
+                return ImmutableList.<DefaultInvoice>of(invoice);
             }
         };
 
@@ -560,8 +564,8 @@ public Invoice apply(final InvoiceModelDao input) {
                                                                     }));
     }
 
-    private Invoice getInvoiceAndCheckCurrency(final UUID invoiceId, @Nullable final Currency currency, final TenantContext context) throws InvoiceApiException {
-        final Invoice invoice = getInvoice(invoiceId, context);
+    private DefaultInvoice getInvoiceAndCheckCurrency(final UUID invoiceId, @Nullable final Currency currency, final TenantContext context) throws InvoiceApiException {
+        final DefaultInvoice invoice = getInvoiceInternal(invoiceId, context);
         // Check the specified currency matches the one of the existing invoice
         if (currency != null && invoice.getCurrency() != currency) {
             throw new InvoiceApiException(ErrorCode.CURRENCY_INVALID, currency, invoice.getCurrency());