killbill-memoizeit

invoice: fix isRepareeItemForRepairedItem logic We can't

4/26/2013 4:34:17 PM

Details

diff --git a/beatrix/src/test/java/com/ning/billing/beatrix/integration/TestIntegrationInvoiceWithRepairLogic.java b/beatrix/src/test/java/com/ning/billing/beatrix/integration/TestIntegrationInvoiceWithRepairLogic.java
index a1c7f10..4132011 100644
--- a/beatrix/src/test/java/com/ning/billing/beatrix/integration/TestIntegrationInvoiceWithRepairLogic.java
+++ b/beatrix/src/test/java/com/ning/billing/beatrix/integration/TestIntegrationInvoiceWithRepairLogic.java
@@ -114,7 +114,7 @@ public class TestIntegrationInvoiceWithRepairLogic extends TestIntegrationBase {
         //
         // FORCE AN IMMEDIATE CHANGE OF THE BILLING PERIOD
         //
-        busHandler.pushExpectedEvents(NextEvent.CHANGE, NextEvent.INVOICE_ADJUSTMENT);
+        busHandler.pushExpectedEvents(NextEvent.CHANGE, NextEvent.INVOICE, NextEvent.INVOICE_ADJUSTMENT);
         assertTrue(bpSubscription.changePlanWithPolicy(productName, BillingPeriod.MONTHLY, planSetName, clock.getUTCNow(), ActionPolicy.IMMEDIATE, callContext));
         assertTrue(busHandler.isCompleted(DELAY));
         assertListenerStatus();
@@ -123,20 +123,42 @@ public class TestIntegrationInvoiceWithRepairLogic extends TestIntegrationBase {
 
 
         invoices = invoiceUserApi.getInvoicesByAccount(account.getId(), callContext);
-        assertEquals(invoices.size(), 2);
+        assertEquals(invoices.size(), 3);
 
         toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
                 new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2013, 5, 1), InvoiceItemType.RECURRING, new BigDecimal("2399.95")),
-                new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 1), new LocalDate(2013, 5, 1), InvoiceItemType.REPAIR_ADJ, new BigDecimal("-2150.00")),
-                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 5, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("2150.00")));
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2013, 5, 1), InvoiceItemType.REPAIR_ADJ, new BigDecimal("-2399.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 5, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("2399.95")));
         invoiceChecker.checkInvoice(invoices.get(1).getId(), callContext, toBeChecked);
 
+        toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 6, 1), InvoiceItemType.RECURRING, new BigDecimal("249.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 5, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
+        invoiceChecker.checkInvoice(invoices.get(2).getId(), callContext, toBeChecked);
 
-        // Nothing to do
+        busHandler.pushExpectedEvents(NextEvent.INVOICE);
         clock.addMonths(1);
         assertTrue(busHandler.isCompleted(DELAY));
         assertListenerStatus();
 
+        invoices = invoiceUserApi.getInvoicesByAccount(account.getId(), callContext);
+        assertEquals(invoices.size(), 4);
+
+        toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2013, 5, 1), InvoiceItemType.RECURRING, new BigDecimal("2399.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2013, 5, 1), InvoiceItemType.REPAIR_ADJ, new BigDecimal("-2399.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 5, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("2399.95")));
+        invoiceChecker.checkInvoice(invoices.get(1).getId(), callContext, toBeChecked);
+
+        toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 6, 1), InvoiceItemType.RECURRING, new BigDecimal("249.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 5, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
+        invoiceChecker.checkInvoice(invoices.get(2).getId(), callContext, toBeChecked);
+
+        toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 1), new LocalDate(2012, 7, 1), InvoiceItemType.RECURRING, new BigDecimal("249.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 1), new LocalDate(2012, 6, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
+        invoiceChecker.checkInvoice(invoices.get(3).getId(), callContext, toBeChecked);
 
         busHandler.pushExpectedEvents(NextEvent.INVOICE);
         clock.addMonths(1);
@@ -144,22 +166,30 @@ public class TestIntegrationInvoiceWithRepairLogic extends TestIntegrationBase {
         assertListenerStatus();
 
         invoices = invoiceUserApi.getInvoicesByAccount(account.getId(), callContext);
-        assertEquals(invoices.size(), 3);
+        assertEquals(invoices.size(), 5);
 
         toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
                 new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2013, 5, 1), InvoiceItemType.RECURRING, new BigDecimal("2399.95")),
-                new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 1), new LocalDate(2013, 5, 1), InvoiceItemType.REPAIR_ADJ, new BigDecimal("-2150.00")),
-                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 5, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("2150.00")));
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2013, 5, 1), InvoiceItemType.REPAIR_ADJ, new BigDecimal("-2399.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 5, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("2399.95")));
         invoiceChecker.checkInvoice(invoices.get(1).getId(), callContext, toBeChecked);
 
         toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 6, 1), InvoiceItemType.RECURRING, new BigDecimal("249.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2012, 5, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
+        invoiceChecker.checkInvoice(invoices.get(2).getId(), callContext, toBeChecked);
+
+        toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
                 new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 1), new LocalDate(2012, 7, 1), InvoiceItemType.RECURRING, new BigDecimal("249.95")),
-                new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 11), new LocalDate(2012, 6, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 1), new LocalDate(2012, 6, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
         invoiceChecker.checkInvoice(invoices.get(3).getId(), callContext, toBeChecked);
 
+        toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 7, 1), new LocalDate(2012, 8, 1), InvoiceItemType.RECURRING, new BigDecimal("249.95")),
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 7, 1), new LocalDate(2012, 7, 1), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
+        invoiceChecker.checkInvoice(invoices.get(4).getId(), callContext, toBeChecked);
     }
 
-
     @Test(groups = "slow")
     public void testInvoiceLogicWithFullRepairFollowedByPartialRepair() throws Exception {
 
@@ -208,7 +238,7 @@ public class TestIntegrationInvoiceWithRepairLogic extends TestIntegrationBase {
         //
         // FORCE AN IMMEDIATE CHANGE OF THE BILLING PERIOD
         //
-        busHandler.pushExpectedEvents(NextEvent.CHANGE, NextEvent.INVOICE);
+        busHandler.pushExpectedEvents(NextEvent.CHANGE, NextEvent.INVOICE, NextEvent.INVOICE_ADJUSTMENT);
         assertTrue(bpSubscription.changePlanWithPolicy(productName, BillingPeriod.MONTHLY, planSetName, clock.getUTCNow(), ActionPolicy.IMMEDIATE, callContext));
         assertEquals(entitlementUserApi.getSubscriptionFromId(bpSubscription.getId(), callContext).getCurrentPlan().getBillingPeriod(), BillingPeriod.MONTHLY);
 
@@ -255,12 +285,10 @@ public class TestIntegrationInvoiceWithRepairLogic extends TestIntegrationBase {
                 new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 11), new LocalDate(2012, 5, 11), InvoiceItemType.CBA_ADJ, new BigDecimal("-235.08")));
         invoiceChecker.checkInvoice(invoices.get(2).getId(), callContext, toBeChecked);
 
-        invoiceChecker.checkInvoice(invoices.get(2).getId(), callContext, toBeChecked);
-
         // AND THEN CHECK NEW INVOICE
         toBeChecked = ImmutableList.<ExpectedInvoiceItemCheck>of(
                 new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 1), new LocalDate(2012, 7, 1), InvoiceItemType.RECURRING, new BigDecimal("249.95")),
-                new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 10), new LocalDate(2012, 6, 10), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
+                new ExpectedInvoiceItemCheck(new LocalDate(2012, 6, 11), new LocalDate(2012, 6, 11), InvoiceItemType.CBA_ADJ, new BigDecimal("-249.95")));
         invoiceChecker.checkInvoice(invoices.get(3).getId(), callContext, toBeChecked);
 
         busHandler.pushExpectedEvents(NextEvent.INVOICE);
diff --git a/invoice/src/main/java/com/ning/billing/invoice/generator/DefaultInvoiceGenerator.java b/invoice/src/main/java/com/ning/billing/invoice/generator/DefaultInvoiceGenerator.java
index 36c8a53..68c57f8 100644
--- a/invoice/src/main/java/com/ning/billing/invoice/generator/DefaultInvoiceGenerator.java
+++ b/invoice/src/main/java/com/ning/billing/invoice/generator/DefaultInvoiceGenerator.java
@@ -52,6 +52,8 @@ import com.ning.billing.util.svcapi.junction.BillingEvent;
 import com.ning.billing.util.svcapi.junction.BillingEventSet;
 import com.ning.billing.util.svcapi.junction.BillingModeType;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Objects;
 import com.google.common.base.Predicate;
 import com.google.common.collect.Collections2;
 import com.google.inject.Inject;
@@ -220,16 +222,27 @@ public class DefaultInvoiceGenerator implements InvoiceGenerator {
      * @param invoiceItem         any invoice item to compare to
      * @return true if invoiceItem is the reparee for that repaired invoice item
      */
-    private boolean isRepareeItemForRepairedItem(final InvoiceItem repairedInvoiceItem, final InvoiceItem invoiceItem) {
-        return repairedInvoiceItem.getInvoiceItemType().equals(invoiceItem.getInvoiceItemType()) &&
+    @VisibleForTesting
+    boolean isRepareeItemForRepairedItem(final InvoiceItem repairedInvoiceItem, final InvoiceItem invoiceItem) {
+        return !repairedInvoiceItem.getId().equals(invoiceItem.getId()) &&
+               repairedInvoiceItem.getInvoiceItemType().equals(invoiceItem.getInvoiceItemType()) &&
+               // We assume the items are correctly created, so that the subscription id check implicitly
+               // verifies that account id and bundle id matches
                repairedInvoiceItem.getSubscriptionId().equals(invoiceItem.getSubscriptionId()) &&
+               // The reparee item is the "portion used" of the repaired item, hence it will have the same start date
                repairedInvoiceItem.getStartDate().compareTo(invoiceItem.getStartDate()) == 0 &&
-               // FIXED items have a null end date
+               // Similarly, check the "portion used" is less than the original service end date. The check
+               // is strict, otherwise there wouldn't be anything to repair
                ((repairedInvoiceItem.getEndDate() == null && invoiceItem.getEndDate() == null) ||
                 (repairedInvoiceItem.getEndDate() != null && invoiceItem.getEndDate() != null &&
-                 // We need to look for stricly after otherwsie we could return thew new item for that period in case of a complete repair
                  repairedInvoiceItem.getEndDate().isAfter(invoiceItem.getEndDate()))) &&
-               !repairedInvoiceItem.getId().equals(invoiceItem.getId());
+               // Finally, for the tricky part... In case of complete repairs, the new item will always meet all of the
+               // following conditions: same type, subscription, start date. Depending on the catalog configuration, the end
+               // date check could also match (e.g. repair from annual to monthly). For that scenario, we need to default
+               // to catalog checks (the rate check is a lame check for versioned catalogs).
+               Objects.firstNonNull(repairedInvoiceItem.getPlanName(), "").equals(Objects.firstNonNull(invoiceItem.getPlanName(), "")) &&
+               Objects.firstNonNull(repairedInvoiceItem.getPhaseName(), "").equals(Objects.firstNonNull(invoiceItem.getPhaseName(), "")) &&
+               Objects.firstNonNull(repairedInvoiceItem.getRate(), "").equals(Objects.firstNonNull(invoiceItem.getRate(), ""));
     }
 
 
diff --git a/invoice/src/test/java/com/ning/billing/invoice/generator/DefaultInvoiceGeneratorWithSwitchRepairLogic.java b/invoice/src/test/java/com/ning/billing/invoice/generator/DefaultInvoiceGeneratorWithSwitchRepairLogic.java
index 84155a3..f6f95ec 100644
--- a/invoice/src/test/java/com/ning/billing/invoice/generator/DefaultInvoiceGeneratorWithSwitchRepairLogic.java
+++ b/invoice/src/test/java/com/ning/billing/invoice/generator/DefaultInvoiceGeneratorWithSwitchRepairLogic.java
@@ -47,12 +47,14 @@ public class DefaultInvoiceGeneratorWithSwitchRepairLogic extends DefaultInvoice
         setDefaultRepairLogic(REPAIR_INVOICE_LOGIC.PARTIAL_REPAIR);
     }
 
+    @Override
     protected void removeProposedRepareeForPartialrepair(final InvoiceItem repairedItem, final List<InvoiceItem> proposedItems) {
         if (repairtLogic == REPAIR_INVOICE_LOGIC.PARTIAL_REPAIR) {
             super.removeProposedRepareeForPartialrepair(repairedItem, proposedItems);
         }
     }
 
+    @Override
     void addRepairItem(final InvoiceItem repairedItem, final RepairAdjInvoiceItem candidateRepairItem, final List<InvoiceItem> proposedItems) {
         if (repairtLogic == REPAIR_INVOICE_LOGIC.PARTIAL_REPAIR) {
             super.addRepairItem(repairedItem, candidateRepairItem, proposedItems);
diff --git a/invoice/src/test/java/com/ning/billing/invoice/generator/TestDefaultInvoiceGeneratorUnit.java b/invoice/src/test/java/com/ning/billing/invoice/generator/TestDefaultInvoiceGeneratorUnit.java
index c726f0e..8d12312 100644
--- a/invoice/src/test/java/com/ning/billing/invoice/generator/TestDefaultInvoiceGeneratorUnit.java
+++ b/invoice/src/test/java/com/ning/billing/invoice/generator/TestDefaultInvoiceGeneratorUnit.java
@@ -26,17 +26,15 @@ import org.testng.annotations.Test;
 
 import com.ning.billing.catalog.api.Currency;
 import com.ning.billing.invoice.InvoiceTestSuiteNoDB;
-import com.ning.billing.invoice.api.Invoice;
 import com.ning.billing.invoice.api.InvoiceItem;
 import com.ning.billing.invoice.api.InvoiceItemType;
-import com.ning.billing.invoice.model.CreditBalanceAdjInvoiceItem;
 import com.ning.billing.invoice.model.FixedPriceInvoiceItem;
 import com.ning.billing.invoice.model.RecurringInvoiceItem;
 import com.ning.billing.invoice.model.RepairAdjInvoiceItem;
 
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
 
 public class TestDefaultInvoiceGeneratorUnit extends InvoiceTestSuiteNoDB {
 
@@ -257,7 +255,41 @@ public class TestDefaultInvoiceGeneratorUnit extends InvoiceTestSuiteNoDB {
         assertEquals(newItem2.getInvoiceItemType(), InvoiceItemType.REPAIR_ADJ);
         assertEquals(newItem2.getAmount(), item1.getAmount().negate());
         assertEquals(newItem2.getLinkedItemId(), item1.getId());
+    }
 
+    @Test(groups = "fast")
+    public void testShouldFindRepareeForPartialRepairs() throws Exception {
+        final LocalDate startDate = new LocalDate(2012, 5, 1);
+        final LocalDate endDate = new LocalDate(2012, 6, 1);
+        // Repaired item
+        final InvoiceItem silver = new RecurringInvoiceItem(invoiceId, accountId, bundleId, subscriptionId, planName, phaseName, startDate, endDate, BigDecimal.TEN, BigDecimal.TEN, currency);
+
+        // Reparee item
+        final LocalDate actualEndDateSilver = new LocalDate(2012, 5, 10);
+        final InvoiceItem actualSilver = new RecurringInvoiceItem(invoiceId, accountId, bundleId, subscriptionId, planName, phaseName, startDate, actualEndDateSilver, new BigDecimal("3"), BigDecimal.TEN, currency);
+
+        // New item
+        final InvoiceItem gold = new RecurringInvoiceItem(invoiceId, accountId, bundleId, subscriptionId, "new-" + planName, phaseName, actualEndDateSilver, endDate, BigDecimal.TEN, new BigDecimal("15"), currency);
+
+        assertFalse(((DefaultInvoiceGenerator) generator).isRepareeItemForRepairedItem(silver, silver));
+        assertFalse(((DefaultInvoiceGenerator) generator).isRepareeItemForRepairedItem(silver, gold));
+        assertTrue(((DefaultInvoiceGenerator) generator).isRepareeItemForRepairedItem(silver, actualSilver));
     }
 
+    @Test(groups = "fast")
+    public void testShouldntFindRepareeForFullRepairs() throws Exception {
+        final LocalDate startDate = new LocalDate(2012, 5, 1);
+        final LocalDate endDate = new LocalDate(2013, 5, 1);
+        // Repaired item
+        final InvoiceItem annual = new RecurringInvoiceItem(invoiceId, accountId, bundleId, subscriptionId, planName, phaseName, startDate, endDate, BigDecimal.TEN, BigDecimal.TEN, currency);
+
+        // There is no reparee - full repair
+
+        // New item
+        final LocalDate endDate2 = new LocalDate(2012, 6, 1);
+        final InvoiceItem monthly = new RecurringInvoiceItem(invoiceId, accountId, bundleId, subscriptionId, "new-" + planName, phaseName, startDate, endDate2, BigDecimal.TEN, BigDecimal.TEN, currency);
+
+        assertFalse(((DefaultInvoiceGenerator) generator).isRepareeItemForRepairedItem(annual, annual));
+        assertFalse(((DefaultInvoiceGenerator) generator).isRepareeItemForRepairedItem(annual, monthly));
+    }
 }