#457 autoCommit, tests updated, payment actions and more PR suggestions.

beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestIntegrationInvoice.java

@@ -244,7 +244,9 @@ public void testApplyCreditOnExistingBalance() throws Exception {
         final BigDecimal accountBalance1 = invoiceUserApi.getAccountBalance(account.getId(), callContext);
         assertTrue(accountBalance1.compareTo(new BigDecimal("249.95")) == 0);
 
-        invoiceUserApi.insertCredit(account.getId(), new BigDecimal("300"), new LocalDate(clock.getUTCNow(), account.getTimeZone()), account.getCurrency(), callContext);
+        busHandler.pushExpectedEvents(NextEvent.INVOICE);
+        invoiceUserApi.insertCredit(account.getId(), new BigDecimal("300"), new LocalDate(clock.getUTCNow(), account.getTimeZone()), account.getCurrency(), true, callContext);
+        assertListenerStatus();
 
         final BigDecimal accountBalance2 = invoiceUserApi.getAccountBalance(account.getId(), callContext);
         assertTrue(accountBalance2.compareTo(new BigDecimal("-50.05")) == 0);
@@ -308,7 +310,7 @@ public void testDraftInvoice() throws Exception {
         final List<InvoiceItem> invoiceItemList = new ArrayList<InvoiceItem>();
         ExternalChargeInvoiceItem item = new ExternalChargeInvoiceItem(null, account.getId(), subscription.getBundleId(), "", date, BigDecimal.TEN, account.getCurrency());
         invoiceItemList.add(item);
-        final List<InvoiceItem> draftInvoiceItems = invoiceUserApi.insertExternalCharges(account.getId(), date, invoiceItemList, callContext);
+        final List<InvoiceItem> draftInvoiceItems = invoiceUserApi.insertExternalCharges(account.getId(), date, invoiceItemList, false, callContext);
 
         // add expected invoice
         final List<ExpectedInvoiceItemCheck> expectedDraftInvoices = new ArrayList<ExpectedInvoiceItemCheck>();

beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestInvoicePayment.java

@@ -56,8 +56,10 @@ public void testPartialPayments() throws Exception {
 
         clock.setDay(new LocalDate(2012, 4, 1));
 
+        busHandler.pushExpectedEvents(NextEvent.INVOICE);
         final InvoiceItem externalCharge = new ExternalChargeInvoiceItem(null, account.getId(), null, "Initial external charge", clock.getUTCToday(), BigDecimal.TEN, Currency.USD);
-        final InvoiceItem item1 = invoiceUserApi.insertExternalCharges(account.getId(), clock.getUTCToday(), ImmutableList.<InvoiceItem>of(externalCharge), callContext).get(0);
+        final InvoiceItem item1 = invoiceUserApi.insertExternalCharges(account.getId(), clock.getUTCToday(), ImmutableList.<InvoiceItem>of(externalCharge), true, callContext).get(0);
+        assertListenerStatus();
 
         final Invoice invoice = invoiceUserApi.getInvoice(item1.getInvoiceId(), callContext);
         final Payment payment1 = createPaymentAndCheckForCompletion(account, invoice, new BigDecimal("4.00"), account.getCurrency(),  NextEvent.PAYMENT);

beatrix/src/test/java/org/killbill/billing/beatrix/integration/overdue/TestOverdueIntegration.java

@@ -687,7 +687,7 @@ public void testShouldBeInOverdueAfterExternalCharge() throws Exception {
         addDaysAndCheckForCompletion(5);
         busHandler.pushExpectedEvents(NextEvent.INVOICE_ADJUSTMENT);
         final InvoiceItem externalCharge = new ExternalChargeInvoiceItem(null, account.getId(), bundle.getId(), "For overdue", new LocalDate(2012, 5, 6), BigDecimal.TEN, Currency.USD);
-        invoiceUserApi.insertExternalCharges(account.getId(), clock.getUTCToday(), ImmutableList.<InvoiceItem>of(externalCharge), callContext).get(0);
+        invoiceUserApi.insertExternalCharges(account.getId(), clock.getUTCToday(), ImmutableList.<InvoiceItem>of(externalCharge), true, callContext).get(0);
         assertListenerStatus();
         invoiceChecker.checkInvoice(account.getId(), 2, callContext, new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 6), null, InvoiceItemType.EXTERNAL_CHARGE, BigDecimal.TEN));
 

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

@@ -370,13 +370,11 @@ public Invoice apply(final InvoiceModelDao input) {
                 setChargedThroughDates(billingEvents.getAccountDateAndTimeZoneContext(), invoice.getInvoiceItems(FixedPriceInvoiceItem.class), invoice.getInvoiceItems(RecurringInvoiceItem.class), context);
 
                 if (InvoiceStatus.COMMITTED.equals(invoice.getStatus())) {
-                    // TODO we should send bus events when we commit the ionvoice on disk in commitInvoice
+                    // TODO we should send bus events when we commit the invoice on disk in commitInvoice
                     postEvents(account, invoice, adjustedUniqueOtherInvoiceId, isRealInvoiceWithNonEmptyItems, context);
-
+                    notifyAccountIfEnabled(account, invoice, isRealInvoiceWithNonEmptyItems, context);
                 }
 
-                // TODO should we include this notification inside of previous if clause?
-                notifyAccountIfEnabled(account, invoice, isRealInvoiceWithNonEmptyItems, context);
             }
             return invoice;
         } catch (final AccountApiException e) {

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

@@ -262,7 +262,7 @@ public InvoiceItem getExternalChargeById(final UUID externalChargeId, final Tena
     }
 
     @Override
-    public List<InvoiceItem> insertExternalCharges(final UUID accountId, final LocalDate effectiveDate, final Iterable<InvoiceItem> charges, final CallContext context) throws InvoiceApiException {
+    public List<InvoiceItem> insertExternalCharges(final UUID accountId, final LocalDate effectiveDate, final Iterable<InvoiceItem> charges, final boolean autoCommit, final CallContext context) throws InvoiceApiException {
         for (final InvoiceItem charge : charges) {
             if (charge.getAmount() == null || charge.getAmount().compareTo(BigDecimal.ZERO) <= 0) {
                 throw new InvoiceApiException(ErrorCode.EXTERNAL_CHARGE_AMOUNT_INVALID, charge.getAmount());
@@ -284,7 +284,8 @@ public Iterable<Invoice> prepareInvoices() throws InvoiceApiException {
                     if (invoiceIdForExternalCharge == null) {
                         final Currency currency = charge.getCurrency();
                         if (newInvoicesForExternalCharges.get(currency) == null) {
-                            final Invoice newInvoiceForExternalCharge = new DefaultInvoice(accountId, effectiveDate, effectiveDate, currency, InvoiceStatus.DRAFT);
+                            final InvoiceStatus status = autoCommit ? InvoiceStatus.COMMITTED : InvoiceStatus.DRAFT;
+                            final Invoice newInvoiceForExternalCharge = new DefaultInvoice(accountId, effectiveDate, effectiveDate, currency, status);
                             newInvoicesForExternalCharges.put(currency, newInvoiceForExternalCharge);
                         }
                         invoiceForExternalCharge = newInvoicesForExternalCharges.get(currency);
@@ -328,13 +329,18 @@ public InvoiceItem getCreditById(final UUID creditId, final TenantContext contex
 
     @Override
     public InvoiceItem insertCredit(final UUID accountId, final BigDecimal amount, final LocalDate effectiveDate,
-                                    final Currency currency, final CallContext context) throws InvoiceApiException {
-        return insertCreditForInvoice(accountId, null, amount, effectiveDate, currency, context);
+                                    final Currency currency, final boolean autoCommit, final CallContext context) throws InvoiceApiException {
+        return insertCreditForInvoice(accountId, null, amount, effectiveDate, currency, autoCommit, context);
     }
 
     @Override
     public InvoiceItem insertCreditForInvoice(final UUID accountId, final UUID invoiceId, final BigDecimal amount,
                                               final LocalDate effectiveDate, final Currency currency, final CallContext context) throws InvoiceApiException {
+        return insertCreditForInvoice(accountId, invoiceId, amount, effectiveDate, currency, false, context);
+    }
+
+    private InvoiceItem insertCreditForInvoice(final UUID accountId, final UUID invoiceId, final BigDecimal amount, final LocalDate effectiveDate,
+                                               final Currency currency, final boolean autoCommit, final CallContext context) throws InvoiceApiException {
         if (amount == null || amount.compareTo(BigDecimal.ZERO) <= 0) {
             throw new InvoiceApiException(ErrorCode.CREDIT_AMOUNT_INVALID, amount);
         }
@@ -348,15 +354,13 @@ public List<Invoice> prepareInvoices() throws InvoiceApiException {
                 // Create an invoice for that credit if it doesn't exist
                 final Invoice invoiceForCredit;
                 if (invoiceId == null) {
-                    invoiceForCredit = new DefaultInvoice(accountId, effectiveDate, effectiveDate, currency, InvoiceStatus.DRAFT);
+                    final InvoiceStatus status = autoCommit ? InvoiceStatus.COMMITTED : InvoiceStatus.DRAFT;
+                    invoiceForCredit = new DefaultInvoice(accountId, effectiveDate, effectiveDate, currency, status);
                 } else {
                     invoiceForCredit = getInvoiceAndCheckCurrency(invoiceId, currency, context);
-                    // TODO check with @sbrossie if really want to add this validation
-                    /*
                     if (InvoiceStatus.COMMITTED.equals(invoiceForCredit.getStatus())) {
-                        throw new InvoiceApiException(ErrorCode.INVOICE_INVALID_STATUS_CREDIT, invoiceId);
+                        throw new InvoiceApiException(ErrorCode.INVOICE_ALREADY_COMMITTED, invoiceId);
                     }
-                    */
                 }
 
                 // Create the new credit

invoice/src/main/java/org/killbill/billing/invoice/dao/DefaultInvoiceDao.java

@@ -35,7 +35,6 @@
 import org.killbill.billing.callcontext.InternalTenantContext;
 import org.killbill.billing.catalog.api.Currency;
 import org.killbill.billing.entity.EntityPersistenceException;
-import org.killbill.billing.events.BusInternalEvent;
 import org.killbill.billing.invoice.InvoiceDispatcher.FutureAccountNotifications;
 import org.killbill.billing.invoice.InvoiceDispatcher.FutureAccountNotifications.SubscriptionNotification;
 import org.killbill.billing.invoice.api.Invoice;
@@ -278,11 +277,13 @@ public List<InvoiceItemModelDao> inTransaction(final EntitySqlDaoWrapperFactory
                 final List<InvoiceItemModelDao> createdInvoiceItems = new LinkedList<InvoiceItemModelDao>();
                 for (final InvoiceModelDao invoiceModelDao : invoices) {
                     boolean madeChanges = false;
+                    boolean newInvoice = false;
 
                     // Create the invoice if needed
                     if (invoiceSqlDao.getById(invoiceModelDao.getId().toString(), context) == null) {
                         invoiceSqlDao.create(invoiceModelDao, context);
                         madeChanges = true;
+                        newInvoice = true;
                     }
 
                     // Create the invoice items if needed
@@ -296,8 +297,12 @@ public List<InvoiceItemModelDao> inTransaction(final EntitySqlDaoWrapperFactory
 
                     if (madeChanges) {
                         cbaDao.addCBAComplexityFromTransaction(invoiceModelDao.getId(), entitySqlDaoWrapperFactory, context);
+                    }
 
-                        if (InvoiceStatus.COMMITTED.equals(invoiceModelDao.getStatus())) {
+                    if (InvoiceStatus.COMMITTED.equals(invoiceModelDao.getStatus())) {
+                        if (newInvoice) {
+                            notifyBusOfInvoiceCreation(entitySqlDaoWrapperFactory, invoiceModelDao, context);
+                        } else if (madeChanges) {
                             // Notify the bus since the balance of the invoice changed (only if the invoice is COMMITTED)
                             // TODO should we post an InvoiceCreationInternalEvent event instead? Note! This will trigger a payment (see InvoiceHandler)
                             notifyBusOfInvoiceAdjustment(entitySqlDaoWrapperFactory, invoiceModelDao.getId(), invoiceModelDao.getAccountId(), context.getUserToken(), context);
@@ -881,24 +886,23 @@ public Void inTransaction(final EntitySqlDaoWrapperFactory entitySqlDaoWrapperFa
                 transactional.updateStatus(invoiceId.toString(), newStatus.toString(), context);
 
                 // notify invoice creation event
-                final BigDecimal balance = InvoiceModelDaoHelper.getBalance(invoice);
-                final DefaultInvoiceCreationEvent defaultInvoiceCreationEvent = new DefaultInvoiceCreationEvent(invoice.getId(), invoice.getAccountId(),
-                                                                                                                balance, invoice.getCurrency(),
-                                                                                                                context.getAccountRecordId(), context.getTenantRecordId(),
-                                                                                                                context.getUserToken());
-                postEvent(defaultInvoiceCreationEvent, invoice.getAccountId());
+                notifyBusOfInvoiceCreation(entitySqlDaoWrapperFactory, invoice, context);
 
                 return null;
             }
         });
     }
 
-    /* Use this method to post any event that implements BusInternalEvent */
-    private void postEvent(final BusInternalEvent event, final UUID accountId) {
+    private void notifyBusOfInvoiceCreation(final EntitySqlDaoWrapperFactory entitySqlDaoWrapperFactory, final InvoiceModelDao invoice, final InternalCallContext context) {
         try {
-            eventBus.post(event);
+            final BigDecimal balance = InvoiceModelDaoHelper.getBalance(invoice);
+            final DefaultInvoiceCreationEvent event = new DefaultInvoiceCreationEvent(invoice.getId(), invoice.getAccountId(),
+                                                                                                            balance, invoice.getCurrency(),
+                                                                                                            context.getAccountRecordId(), context.getTenantRecordId(),
+                                                                                                            context.getUserToken());
+            eventBus.postFromTransaction(event, entitySqlDaoWrapperFactory.getHandle().getConnection());
         } catch (final EventBusException e) {
-            log.error(String.format("Failed to post event %s for account %s", event.getBusEventType(), accountId), e);
+            log.error(String.format("Failed to post invoice creation event %s for account %s", invoice.getAccountId()), e);
         }
     }
 

invoice/src/main/java/org/killbill/billing/invoice/dao/InvoiceDaoHelper.java

@@ -38,6 +38,7 @@
 import org.killbill.billing.catalog.api.Currency;
 import org.killbill.billing.invoice.api.InvoiceApiException;
 import org.killbill.billing.invoice.api.InvoiceItemType;
+import org.killbill.billing.invoice.api.InvoiceStatus;
 import org.killbill.billing.tag.TagInternalApi;
 import org.killbill.billing.util.entity.dao.EntitySqlDaoWrapperFactory;
 import org.killbill.billing.util.tag.ControlTagType;
@@ -171,7 +172,7 @@ public List<InvoiceModelDao> getUnpaidInvoicesByAccountFromTransaction(final Lis
         final Collection<InvoiceModelDao> unpaidInvoices = Collections2.filter(invoices, new Predicate<InvoiceModelDao>() {
             @Override
             public boolean apply(final InvoiceModelDao in) {
-                return (InvoiceModelDaoHelper.getBalance(in).compareTo(BigDecimal.ZERO) >= 1) && (upToDate == null || !in.getTargetDate().isAfter(upToDate));
+                return (InvoiceStatus.COMMITTED.equals(in.getStatus())) && (InvoiceModelDaoHelper.getBalance(in).compareTo(BigDecimal.ZERO) >= 1) && (upToDate == null || !in.getTargetDate().isAfter(upToDate));
             }
         });
         return new ArrayList<InvoiceModelDao>(unpaidInvoices);