From 3ffaded87b90f35edd9f3dfab5ec6de58db726a0 Mon Sep 17 00:00:00 2001 From: Pierre-Alexandre Meyer Date: Thu, 24 May 2018 02:50:43 -0700 Subject: [PATCH] subscription, junction: revisit catalog APIs usage Avoid fetching the catalog over and over again: when we already have the plan, we can directly fetch the phase. This also fixes the implementation of rebuildSubscriptionAndNotifyBusOfEffectiveImmediateChange: we cannot simply take the old subscription events and add the new event because in some cases, we have had to de-active some events on disk (e.g. future phase event during a change plan). It looks like we were incorrectly building subscription events in some cases, even though it didn't affect most codepaths. As a side effect, a "spiced-up test" had to be modified (creating garbage data on disk now throws an exception as the code refuses to build such incorrect timelines). Signed-off-by: Pierre-Alexandre Meyer --- .../plumbing/billing/DefaultBillingEvent.java | 52 ++++++++++++------- .../billing/DefaultInternalBillingApi.java | 9 ++-- .../DefaultSubscriptionBaseTimeline.java | 19 ++++--- .../api/user/DefaultSubscriptionBase.java | 18 +++---- .../engine/dao/DefaultSubscriptionDao.java | 21 ++------ .../alignment/TestPlanAligner.java | 9 ++-- .../api/user/TestUserApiChangePlan.java | 19 ------- 7 files changed, 60 insertions(+), 87 deletions(-) diff --git a/junction/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultBillingEvent.java b/junction/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultBillingEvent.java index 5c66e3c34a..c08a7bec9a 100644 --- a/junction/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultBillingEvent.java +++ b/junction/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultBillingEvent.java @@ -31,7 +31,7 @@ import org.killbill.billing.catalog.api.Plan; import org.killbill.billing.catalog.api.PlanPhase; import org.killbill.billing.catalog.api.Usage; -import org.killbill.billing.events.EffectiveSubscriptionInternalEvent; +import org.killbill.billing.events.SubscriptionInternalEvent; import org.killbill.billing.junction.BillingEvent; import org.killbill.billing.subscription.api.SubscriptionBase; import org.killbill.billing.subscription.api.SubscriptionBaseTransitionType; @@ -59,35 +59,47 @@ public class DefaultBillingEvent implements BillingEvent { private final boolean isDisableEvent; private final PlanPhase nextPlanPhase; - public DefaultBillingEvent(final EffectiveSubscriptionInternalEvent transition, final SubscriptionBase subscription, final int billCycleDayLocal, final Currency currency, final Catalog catalog) throws CatalogApiException { - - this.catalog = catalog; - + public DefaultBillingEvent(final SubscriptionInternalEvent transition, + final SubscriptionBase subscription, + final int billCycleDayLocal, + final Currency currency, + final Catalog catalog) throws CatalogApiException { final boolean isActive = transition.getTransitionType() != SubscriptionBaseTransitionType.CANCEL; - this.billCycleDayLocal = billCycleDayLocal; - this.subscription = subscription; - this.effectiveDate = transition.getEffectiveTransitionTime(); - final String planPhaseName = isActive ? transition.getNextPhase() : transition.getPreviousPhase(); - this.planPhase = (planPhaseName != null) ? catalog.findPhase(planPhaseName, transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null; + if (isActive) { + final String planName = transition.getNextPlan(); + this.plan = (planName != null) ? catalog.findPlan(planName, transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null; - final String planName = isActive ? transition.getNextPlan() : transition.getPreviousPlan(); - this.plan = (planName != null) ? catalog.findPlan(planName, transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null; + final String planPhaseName = transition.getNextPhase(); + this.planPhase = (planPhaseName != null && this.plan != null) ? this.plan.findPhase(planPhaseName) : null; + this.nextPlanPhase = this.planPhase; - final String nextPhaseName = transition.getNextPhase(); - this.nextPlanPhase = (nextPhaseName != null) ? catalog.findPhase(nextPhaseName, transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null; + this.billingPeriod = getRecurringBillingPeriod(nextPlanPhase); + } else { + final String planName = transition.getPreviousPlan(); + this.plan = (planName != null) ? catalog.findPlan(planName, transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null; + final Plan prevPlan = this.plan; - final String prevPhaseName = transition.getPreviousPhase(); - final PlanPhase prevPlanPhase = (prevPhaseName != null) ? catalog.findPhase(prevPhaseName, transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null; + final String planPhaseName = transition.getPreviousPhase(); + this.planPhase = (planPhaseName != null && this.plan != null) ? this.plan.findPhase(planPhaseName) : null; + this.nextPlanPhase = null; - this.fixedPrice = transition.getTransitionType() != SubscriptionBaseTransitionType.BCD_CHANGE ? getFixedPrice(nextPlanPhase, currency) : null; + final String prevPhaseName = transition.getPreviousPhase(); + final PlanPhase prevPlanPhase = (prevPhaseName != null && prevPlan != null) ? prevPlan.findPhase(prevPhaseName) : null; + this.billingPeriod = getRecurringBillingPeriod(prevPlanPhase); + } + + this.billCycleDayLocal = billCycleDayLocal; + this.catalog = catalog; this.currency = currency; this.description = transition.getTransitionType().toString(); - this.billingPeriod = getRecurringBillingPeriod(isActive ? nextPlanPhase : prevPlanPhase); - this.type = transition.getTransitionType(); + this.effectiveDate = transition.getEffectiveTransitionTime(); + this.fixedPrice = transition.getTransitionType() != SubscriptionBaseTransitionType.BCD_CHANGE ? getFixedPrice(nextPlanPhase, currency) : null; + this.isDisableEvent = false; + this.subscription = subscription; this.totalOrdering = transition.getTotalOrdering(); + this.type = transition.getTransitionType(); this.usages = initializeUsage(isActive); - this.isDisableEvent = false; } public DefaultBillingEvent(final SubscriptionBase subscription, final DateTime effectiveDate, final boolean isActive, diff --git a/junction/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultInternalBillingApi.java b/junction/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultInternalBillingApi.java index 73dc2cb9bb..e033ad6163 100644 --- a/junction/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultInternalBillingApi.java +++ b/junction/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultInternalBillingApi.java @@ -43,6 +43,7 @@ import org.killbill.billing.catalog.api.PlanPhaseSpecifier; import org.killbill.billing.entitlement.api.SubscriptionEventType; import org.killbill.billing.events.EffectiveSubscriptionInternalEvent; +import org.killbill.billing.events.SubscriptionInternalEvent; import org.killbill.billing.invoice.api.DryRunArguments; import org.killbill.billing.junction.BillingEvent; import org.killbill.billing.junction.BillingEventSet; @@ -239,18 +240,18 @@ private int calculateBcdForTransition(final Catalog catalog, final Map tags) { diff --git a/subscription/src/main/java/org/killbill/billing/subscription/api/timeline/DefaultSubscriptionBaseTimeline.java b/subscription/src/main/java/org/killbill/billing/subscription/api/timeline/DefaultSubscriptionBaseTimeline.java index 9c677fd2c4..8bb206ecc1 100644 --- a/subscription/src/main/java/org/killbill/billing/subscription/api/timeline/DefaultSubscriptionBaseTimeline.java +++ b/subscription/src/main/java/org/killbill/billing/subscription/api/timeline/DefaultSubscriptionBaseTimeline.java @@ -65,7 +65,7 @@ private List toExistingEvents(final Catalog catalog, final Produc final List result = new LinkedList(); - String prevPlanName = null; + Plan prevPlan = null; String prevProductName = null; BillingPeriod prevBillingPeriod = null; String prevPriceListName = null; @@ -84,7 +84,7 @@ private List toExistingEvents(final Catalog catalog, final Produc BillingPeriod billingPeriod = null; String priceListName = null; PhaseType phaseType = null; - String planName = null; + Plan plan = null; String planPhaseName = null; Integer billCycleDayLocal = null; @@ -93,9 +93,9 @@ private List toExistingEvents(final Catalog catalog, final Produc case PHASE: final PhaseEvent phaseEV = (PhaseEvent) cur; planPhaseName = phaseEV.getPhase(); - phaseType = catalog.findPhase(phaseEV.getPhase(), cur.getEffectiveDate(), startDate).getPhaseType(); // A PHASE event always occurs within the same plan (and is never the first event) - planName = prevPlanName; + phaseType = prevPlan != null ? prevPlan.findPhase(phaseEV.getPhase()).getPhaseType() : null; + plan = prevPlan; productName = prevProductName; billingPeriod = getBillingPeriod(catalog, phaseEV.getPhase(), cur.getEffectiveDate(), startDate); priceListName = prevPriceListName; @@ -109,10 +109,9 @@ private List toExistingEvents(final Catalog catalog, final Produc case API_USER: final ApiEvent userEV = (ApiEvent) cur; apiType = userEV.getApiEventType(); - planName = userEV.getEventPlan(); planPhaseName = userEV.getEventPlanPhase(); - final Plan plan = (userEV.getEventPlan() != null) ? catalog.findPlan(userEV.getEventPlan(), cur.getEffectiveDate(), startDate) : null; - phaseType = (userEV.getEventPlanPhase() != null) ? catalog.findPhase(userEV.getEventPlanPhase(), cur.getEffectiveDate(), startDate).getPhaseType() : prevPhaseType; + plan = (userEV.getEventPlan() != null) ? catalog.findPlan(userEV.getEventPlan(), cur.getEffectiveDate(), startDate) : null; + phaseType = (plan != null && userEV.getEventPlanPhase() != null) ? plan.findPhase(userEV.getEventPlanPhase()).getPhaseType() : prevPhaseType; productName = (plan != null) ? plan.getProduct().getName() : prevProductName; billingPeriod = (userEV.getEventPlanPhase() != null) ? getBillingPeriod(catalog, userEV.getEventPlanPhase(), cur.getEffectiveDate(), startDate) : prevBillingPeriod; priceListName = (userEV.getPriceList() != null) ? userEV.getPriceList() : prevPriceListName; @@ -121,10 +120,10 @@ private List toExistingEvents(final Catalog catalog, final Produc final SubscriptionBaseTransitionType transitionType = SubscriptionBaseTransitionData.toSubscriptionTransitionType(cur.getType(), apiType); - final String planNameWithClosure = planName; + final String planNameWithClosure = plan != null ? plan.getName() : null; final String planPhaseNameWithClosure = planPhaseName; final Integer billCycleDayLocalWithClosure = billCycleDayLocal; - final PlanPhaseSpecifier spec = new PlanPhaseSpecifier(planName, phaseType); + final PlanPhaseSpecifier spec = new PlanPhaseSpecifier(planNameWithClosure, phaseType); result.add(new ExistingEvent() { @Override public SubscriptionBaseTransitionType getSubscriptionTransitionType() { @@ -167,7 +166,7 @@ public Integer getBillCycleDayLocal() { } }); - prevPlanName = planName; + prevPlan = plan; prevProductName = productName; prevBillingPeriod = billingPeriod; prevPriceListName = priceListName; diff --git a/subscription/src/main/java/org/killbill/billing/subscription/api/user/DefaultSubscriptionBase.java b/subscription/src/main/java/org/killbill/billing/subscription/api/user/DefaultSubscriptionBase.java index 0fe6c5ad91..62bb35804a 100644 --- a/subscription/src/main/java/org/killbill/billing/subscription/api/user/DefaultSubscriptionBase.java +++ b/subscription/src/main/java/org/killbill/billing/subscription/api/user/DefaultSubscriptionBase.java @@ -263,7 +263,7 @@ public boolean cancelWithDate(final DateTime requestedDate, final CallContext co } @Override - public boolean cancelWithPolicy(final BillingActionPolicy policy, int accountBillCycleDayLocal, final CallContext context) throws SubscriptionBaseApiException { + public boolean cancelWithPolicy(final BillingActionPolicy policy, final int accountBillCycleDayLocal, final CallContext context) throws SubscriptionBaseApiException { return apiService.cancelWithPolicy(this, policy, accountBillCycleDayLocal, context); } @@ -695,10 +695,10 @@ public int compare(final SubscriptionBaseEvent o1, final SubscriptionBaseEvent o removeEverythingPastCancelEvent(events); - UUID nextUserToken = null; + final UUID nextUserToken = null; - UUID nextEventId = null; - DateTime nextCreatedDate = null; + UUID nextEventId; + DateTime nextCreatedDate; EntitlementState nextState = null; String nextPlanName = null; String nextPhaseName = null; @@ -777,13 +777,9 @@ public int compare(final SubscriptionBaseEvent o1, final SubscriptionBaseEvent o "Unexpected Event type = %s", cur.getType())); } - Plan nextPlan = null; - PlanPhase nextPhase = null; - PriceList nextPriceList = null; - - nextPlan = (nextPlanName != null) ? catalog.findPlan(nextPlanName, cur.getEffectiveDate(), getAlignStartDate()) : null; - nextPhase = (nextPhaseName != null) ? catalog.findPhase(nextPhaseName, cur.getEffectiveDate(), getAlignStartDate()) : null; - nextPriceList = (nextPlan != null) ? catalog.findPriceListForPlan(nextPlanName, cur.getEffectiveDate(), getAlignStartDate()) : null; + final Plan nextPlan = (nextPlanName != null) ? catalog.findPlan(nextPlanName, cur.getEffectiveDate(), getAlignStartDate()) : null; + final PlanPhase nextPhase = (nextPlan != null && nextPhaseName != null) ? nextPlan.findPhase(nextPhaseName) : null; + final PriceList nextPriceList = (nextPlan != null) ? catalog.findPriceListForPlan(nextPlanName, cur.getEffectiveDate(), getAlignStartDate()) : null; final SubscriptionBaseTransitionData transition = new SubscriptionBaseTransitionData( cur.getId(), id, bundleId, bundleExternalKey, cur.getType(), apiEventType, diff --git a/subscription/src/main/java/org/killbill/billing/subscription/engine/dao/DefaultSubscriptionDao.java b/subscription/src/main/java/org/killbill/billing/subscription/engine/dao/DefaultSubscriptionDao.java index 2277355976..c9d1444c7e 100644 --- a/subscription/src/main/java/org/killbill/billing/subscription/engine/dao/DefaultSubscriptionDao.java +++ b/subscription/src/main/java/org/killbill/billing/subscription/engine/dao/DefaultSubscriptionDao.java @@ -1091,8 +1091,10 @@ private void recordBusOrFutureNotificationFromTransaction(final DefaultSubscript private void rebuildSubscriptionAndNotifyBusOfEffectiveImmediateChange(final EntitySqlDaoWrapperFactory entitySqlDaoWrapperFactory, final DefaultSubscriptionBase subscription, final SubscriptionBaseEvent immediateEvent, final int seqId, final Catalog catalog, final InternalCallContext context) { try { - final DefaultSubscriptionBase upToDateSubscription = createSubscriptionWithNewEvent(subscription, immediateEvent, catalog, context); - notifyBusOfEffectiveImmediateChange(entitySqlDaoWrapperFactory, upToDateSubscription, immediateEvent, seqId, context); + // We need to rehydrate the subscription, as some events might have been canceled on disk (e.g. future PHASE after while doing a change plan) + final List activeSubscriptionEvents = entitySqlDaoWrapperFactory.become(SubscriptionEventSqlDao.class).getActiveEventsForSubscription(subscription.getId().toString(), context); + subscription.rebuildTransitions(toSubscriptionBaseEvents(activeSubscriptionEvents), catalog); + notifyBusOfEffectiveImmediateChange(entitySqlDaoWrapperFactory, subscription, immediateEvent, seqId, context); } catch (final CatalogApiException e) { log.warn("Failed to post effective event for subscriptionId='{}'", subscription.getId(), e); } @@ -1171,21 +1173,6 @@ private void transferBundleDataFromTransaction(final BundleTransferData bundleTr createAndRefresh(transBundleDao, new SubscriptionBundleModelDao(bundleData), context); } - // - // Creates a copy of the existing subscriptions whose 'transitions' will reflect the new event - // - private DefaultSubscriptionBase createSubscriptionWithNewEvent(final DefaultSubscriptionBase subscription, final SubscriptionBaseEvent newEvent, final Catalog catalog, final InternalTenantContext context) throws CatalogApiException { - - final DefaultSubscriptionBase subscriptionWithNewEvent = new DefaultSubscriptionBase(subscription, null, clock); - final List allEvents = new LinkedList(); - if (subscriptionWithNewEvent.getEvents() != null) { - allEvents.addAll(subscriptionWithNewEvent.getEvents()); - } - allEvents.add(newEvent); - subscriptionWithNewEvent.rebuildTransitions(allEvents, catalog); - return subscriptionWithNewEvent; - } - private InternalCallContext contextWithUpdatedDate(final InternalCallContext input) { return new InternalCallContext(input, input.getCreatedDate()); } diff --git a/subscription/src/test/java/org/killbill/billing/subscription/alignment/TestPlanAligner.java b/subscription/src/test/java/org/killbill/billing/subscription/alignment/TestPlanAligner.java index 6fe78b1abb..ab1a3eaca8 100644 --- a/subscription/src/test/java/org/killbill/billing/subscription/alignment/TestPlanAligner.java +++ b/subscription/src/test/java/org/killbill/billing/subscription/alignment/TestPlanAligner.java @@ -1,7 +1,9 @@ /* * Copyright 2010-2013 Ning, Inc. + * Copyright 2014-2018 Groupon, Inc + * Copyright 2014-2018 The Billing Project, LLC * - * Ning licenses this file to you under the Apache License, version 2.0 + * 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 * License. You may obtain a copy of the License at: * @@ -143,7 +145,6 @@ public void testCreationSubscriptionAlignment() throws Exception { Assert.assertNull(newPhase); } - // // Scenario : change Plan with START_OF_SUBSCRIPTION after skipping TRIAL on Create to a new Plan that only has EVERGREEN // @@ -173,7 +174,6 @@ public void testCreateWithTargetPhaseType1() throws Exception { Assert.assertEquals(currentPhase.getPhase().getPhaseType(), PhaseType.EVERGREEN); } - // // Scenario : change Plan with START_OF_SUBSCRIPTION after skipping TRIAL on Create to a new Plan that has {TRIAL, EVERGREEN} // @@ -203,7 +203,6 @@ public void testCreateWithTargetPhaseType2() throws Exception { Assert.assertEquals(currentPhase.getPhase().getPhaseType(), PhaseType.EVERGREEN); } - // // Scenario : change Plan with START_OF_SUBSCRIPTION after skipping TRIAL on Create to a new Plan that has {TRIAL, DISCOUNT, EVERGREEN} // @@ -300,7 +299,6 @@ public void testCreateWithTargetPhaseType5() throws Exception { Assert.assertEquals(nextPhase.getPhase().getPhaseType(), PhaseType.FIXEDTERM); } - // // Scenario : change Plan with START_OF_SUBSCRIPTION to a new Plan that has {TRIAL, DISCOUNT, EVERGREEN} and specifying a target PhaseType = DISCOUNT // @@ -372,7 +370,6 @@ public void testChangeWithTargetPhaseType2() throws Exception { Assert.assertNull(nextPhase); } - private DefaultSubscriptionBase createSubscription(final DateTime bundleStartDate, final DateTime alignStartDate, final String productName, final PhaseType phaseType) throws CatalogApiException { final SubscriptionBuilder builder = new SubscriptionBuilder(); builder.setBundleStartDate(bundleStartDate); diff --git a/subscription/src/test/java/org/killbill/billing/subscription/api/user/TestUserApiChangePlan.java b/subscription/src/test/java/org/killbill/billing/subscription/api/user/TestUserApiChangePlan.java index 04e31de1c0..31a211ea65 100644 --- a/subscription/src/test/java/org/killbill/billing/subscription/api/user/TestUserApiChangePlan.java +++ b/subscription/src/test/java/org/killbill/billing/subscription/api/user/TestUserApiChangePlan.java @@ -521,25 +521,6 @@ public void testChangePlanOnPendingSubscription() throws SubscriptionBaseApiExce // Check we are seeing the right PENDING transition (pistol-monthly), not the START but the CHANGE on the same date assertEquals(((DefaultSubscriptionBase) result2.get(0)).getCurrentOrPendingPlan().getName(), "pistol-monthly"); - // To spice up the test, we insert manually an additional CHANGE event on the exact same dateas the CREATE to verify that code will also invalidate such event when doing the changePlanXX - - final SubscriptionEventModelDao event = new SubscriptionEventModelDao(subscription.getEvents().get(0)); - event.setId(UUID.randomUUID()); - event.setUserType(ApiEventType.CHANGE); - event.setPlanName("blowdart-monthly"); - event.setPhaseName("blowdart-monthly-trial"); - dbi.withHandle(new HandleCallback() { - @Override - public Void withHandle(Handle handle) throws Exception { - final SubscriptionEventSqlDao sqlDao = handle.attach(SubscriptionEventSqlDao.class); - sqlDao.create(event, internalCallContext); - return null; - } - }); - - final DefaultSubscriptionBase refreshed1 = (DefaultSubscriptionBase) subscriptionInternalApi.getSubscriptionFromId(subscription.getId(), internalCallContext); - assertEquals(refreshed1.getEvents().size(), subscription.getEvents().size() + 1); - subscription.changePlanWithDate(spec, null, subscription.getStartDate(), callContext); assertListenerStatus();