killbill-memoizeit

Code review from Martin

2/23/2012 3:11:51 PM

Details

diff --git a/entitlement/src/main/java/com/ning/billing/entitlement/alignment/PlanAligner.java b/entitlement/src/main/java/com/ning/billing/entitlement/alignment/PlanAligner.java
index 2514263..b7574e1 100644
--- a/entitlement/src/main/java/com/ning/billing/entitlement/alignment/PlanAligner.java
+++ b/entitlement/src/main/java/com/ning/billing/entitlement/alignment/PlanAligner.java
@@ -139,7 +139,7 @@ public class PlanAligner  {
 
         DateTime planStartDate = null;
         PlanAlignmentCreate alignement = null;
-        alignement = catalog.planCreateAlignment(planSpecifier, requestedDate); 
+        alignement = catalog.planCreateAlignment(planSpecifier, requestedDate);
 
         switch(alignement) {
         case START_OF_SUBSCRIPTION:
@@ -187,7 +187,8 @@ public class PlanAligner  {
             planStartDate = subscription.getBundleStartDate();
             break;
         case CHANGE_OF_PLAN:
-            throw new EntitlementError(String.format("Not implemented yet %s", alignment));
+            planStartDate = requestedDate;
+            break;
         case CHANGE_OF_PRICELIST:
             throw new EntitlementError(String.format("Not implemented yet %s", alignment));
         default:
@@ -217,6 +218,7 @@ public class PlanAligner  {
 
             result.add(new TimedPhase(cur, curPhaseStart));
 
+            // STEPH check for duration null instead TimeUnit UNLIMITED
             if (cur.getPhaseType() != PhaseType.EVERGREEN) {
                 Duration curPhaseDuration = cur.getDuration();
                 nextPhaseStart = DefaultClock.addDuration(curPhaseStart, curPhaseDuration);
@@ -233,6 +235,7 @@ public class PlanAligner  {
         return result;
     }
 
+    // STEPH check for non evergreen Plans and what happens
     private TimedPhase getTimedPhase(List<TimedPhase> timedPhases, DateTime effectiveDate, WhichPhase which) {
         TimedPhase cur = null;
         TimedPhase next = null;
diff --git a/entitlement/src/main/java/com/ning/billing/entitlement/api/migration/DefaultEntitlementMigrationApi.java b/entitlement/src/main/java/com/ning/billing/entitlement/api/migration/DefaultEntitlementMigrationApi.java
index 05a063d..7a1f9a9 100644
--- a/entitlement/src/main/java/com/ning/billing/entitlement/api/migration/DefaultEntitlementMigrationApi.java
+++ b/entitlement/src/main/java/com/ning/billing/entitlement/api/migration/DefaultEntitlementMigrationApi.java
@@ -166,7 +166,7 @@ public class DefaultEntitlementMigrationApi implements EntitlementMigrationApi {
         for (TimedMigration cur : migrationEvents) {
 
             if (cur.getEventType() == EventType.PHASE) {
-                PhaseEvent nextPhaseEvent = PhaseEventData.getNextPhaseEvent(cur.getPhase().getName(), subscriptionData, now, cur.getEventTime());
+                PhaseEvent nextPhaseEvent = PhaseEventData.createNextPhaseEvent(cur.getPhase().getName(), subscriptionData, now, cur.getEventTime());
                 events.add(nextPhaseEvent);
 
             } else if (cur.getEventType() == EventType.API_USER) {
diff --git a/entitlement/src/main/java/com/ning/billing/entitlement/api/user/SubscriptionApiService.java b/entitlement/src/main/java/com/ning/billing/entitlement/api/user/SubscriptionApiService.java
index 3143b7c..d825e12 100644
--- a/entitlement/src/main/java/com/ning/billing/entitlement/api/user/SubscriptionApiService.java
+++ b/entitlement/src/main/java/com/ning/billing/entitlement/api/user/SubscriptionApiService.java
@@ -73,7 +73,7 @@ public class SubscriptionApiService {
 
             TimedPhase nextTimedPhase = curAndNextPhases[1];
             PhaseEvent nextPhaseEvent = (nextTimedPhase != null) ?
-                    PhaseEventData.getNextPhaseEvent(nextTimedPhase.getPhase().getName(), subscription, processedDate, nextTimedPhase.getStartPhase()) :
+                    PhaseEventData.createNextPhaseEvent(nextTimedPhase.getPhase().getName(), subscription, processedDate, nextTimedPhase.getStartPhase()) :
                         null;
             List<EntitlementEvent> events = new ArrayList<EntitlementEvent>();
             events.add(creationEvent);
@@ -148,7 +148,7 @@ public class SubscriptionApiService {
         DateTime planStartDate = subscription.getCurrentPlanStart();
         TimedPhase nextTimedPhase = planAligner.getNextTimedPhase(subscription.getCurrentPlan(), subscription.getInitialPhaseOnCurrentPlan().getPhaseType(), now, planStartDate);
         PhaseEvent nextPhaseEvent = (nextTimedPhase != null) ?
-                PhaseEventData.getNextPhaseEvent(nextTimedPhase.getPhase().getName(), subscription, now, nextTimedPhase.getStartPhase()) :
+                PhaseEventData.createNextPhaseEvent(nextTimedPhase.getPhase().getName(), subscription, now, nextTimedPhase.getStartPhase()) :
                     null;
         if (nextPhaseEvent != null) {
             uncancelEvents.add(nextPhaseEvent);
@@ -201,7 +201,7 @@ public class SubscriptionApiService {
             ActionPolicy policy = planChangeResult.getPolicy();
             PriceList newPriceList = planChangeResult.getNewPriceList();
 
-            Plan newPlan = catalogService.getFullCatalog().findPlan(productName, term, newPriceList.getName(), requestedDate);
+            Plan newPlan = catalogService.getFullCatalog().findPlan(productName, term, newPriceList.getName(), requestedDate, subscription.getStartDate());
             DateTime effectiveDate = subscription.getPlanChangeEffectiveDate(policy, requestedDate);
 
             TimedPhase currentTimedPhase = planAligner.getCurrentTimedPhaseOnChange(subscription, newPlan, newPriceList.getName(), requestedDate, effectiveDate);
@@ -218,7 +218,7 @@ public class SubscriptionApiService {
 
             TimedPhase nextTimedPhase = planAligner.getNextTimedPhaseOnChange(subscription, newPlan, newPriceList.getName(), requestedDate, effectiveDate);
             PhaseEvent nextPhaseEvent = (nextTimedPhase != null) ?
-                    PhaseEventData.getNextPhaseEvent(nextTimedPhase.getPhase().getName(), subscription, now, nextTimedPhase.getStartPhase()) :
+                    PhaseEventData.createNextPhaseEvent(nextTimedPhase.getPhase().getName(), subscription, now, nextTimedPhase.getStartPhase()) :
                         null;
                     List<EntitlementEvent> changeEvents = new ArrayList<EntitlementEvent>();
                     // Only add the PHASE if it does not coincide with the CHANGE, if not this is 'just' a CHANGE.
diff --git a/entitlement/src/main/java/com/ning/billing/entitlement/api/user/SubscriptionData.java b/entitlement/src/main/java/com/ning/billing/entitlement/api/user/SubscriptionData.java
index f4bb22b..7030ad4 100644
--- a/entitlement/src/main/java/com/ning/billing/entitlement/api/user/SubscriptionData.java
+++ b/entitlement/src/main/java/com/ning/billing/entitlement/api/user/SubscriptionData.java
@@ -201,6 +201,7 @@ public class SubscriptionData implements Subscription {
         return null;
     }
 
+    @Override
     public SubscriptionTransition getPreviousTransition() {
 
         if (transitions == null) {
@@ -306,17 +307,13 @@ public class SubscriptionData implements Subscription {
             throw new EntitlementError(String.format("Unexpected policy type %s", policy.toString()));
         }
 
-        //
-        // If CTD is null or CTD in the past, we default to the start date of the current phase
-        //
-        DateTime effectiveDate = chargedThroughDate;
-        if (chargedThroughDate == null || chargedThroughDate.isBefore(clock.getUTCNow())) {
-            effectiveDate = getCurrentPhaseStart();
+        if (chargedThroughDate == null) {
+            return requestedDate;
+        } else {
+            return chargedThroughDate.isBefore(requestedDate) ? requestedDate : chargedThroughDate;
         }
-        return effectiveDate;
     }
 
-
     public DateTime getCurrentPhaseStart() {
 
         if (transitions == null) {
diff --git a/entitlement/src/main/java/com/ning/billing/entitlement/engine/core/Engine.java b/entitlement/src/main/java/com/ning/billing/entitlement/engine/core/Engine.java
index a2e6839..6b800a8 100644
--- a/entitlement/src/main/java/com/ning/billing/entitlement/engine/core/Engine.java
+++ b/entitlement/src/main/java/com/ning/billing/entitlement/engine/core/Engine.java
@@ -187,7 +187,7 @@ public class Engine implements EventListener, EntitlementService {
             DateTime now = clock.getUTCNow();
             TimedPhase nextTimedPhase = planAligner.getNextTimedPhase(subscription.getCurrentPlan(), subscription.getInitialPhaseOnCurrentPlan().getPhaseType(), now, subscription.getCurrentPlanStart());
             PhaseEvent nextPhaseEvent = (nextTimedPhase != null) ?
-                    PhaseEventData.getNextPhaseEvent(nextTimedPhase.getPhase().getName(), subscription, now, nextTimedPhase.getStartPhase()) :
+                    PhaseEventData.createNextPhaseEvent(nextTimedPhase.getPhase().getName(), subscription, now, nextTimedPhase.getStartPhase()) :
                         null;
             if (nextPhaseEvent != null) {
                 dao.createNextPhaseEvent(subscription.getId(), nextPhaseEvent);
diff --git a/entitlement/src/main/java/com/ning/billing/entitlement/events/phase/PhaseEventData.java b/entitlement/src/main/java/com/ning/billing/entitlement/events/phase/PhaseEventData.java
index 9744363..47546ea 100644
--- a/entitlement/src/main/java/com/ning/billing/entitlement/events/phase/PhaseEventData.java
+++ b/entitlement/src/main/java/com/ning/billing/entitlement/events/phase/PhaseEventData.java
@@ -56,7 +56,7 @@ public class PhaseEventData extends EventBase implements PhaseEvent {
                 + ", isActive()=" + isActive() + "]\n";
     }
 
-    public static final PhaseEvent getNextPhaseEvent(String phaseName, SubscriptionData subscription, DateTime now, DateTime effectiveDate) {
+    public static final PhaseEvent createNextPhaseEvent(String phaseName, SubscriptionData subscription, DateTime now, DateTime effectiveDate) {
         return (phaseName == null) ?
                 null :
                     new PhaseEventData(new PhaseEventBuilder()