Skip to content

Commit

Permalink
subscription, junction: revisit catalog APIs usage
Browse files Browse the repository at this point in the history 
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 <pierre@mouraf.org>
  • Loading branch information
@pierre
pierre committed May 24, 2018
1 parent 2bdf87e commit 3ffaded
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 87 deletions.
52 changes: 32 additions & 20 deletions ...ion/src/main/java/org/killbill/billing/junction/plumbing/billing/DefaultBillingEvent.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 5 additions & 4 deletions ...c/main/java/org/killbill/billing/junction/plumbing/billing/DefaultInternalBillingApi.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -239,18 +240,18 @@ private int calculateBcdForTransition(final Catalog catalog, final Map<UUID, Int
return BillCycleDayCalculator.calculateBcdForAlignment(bcdCache, subscription, baseSubscription, alignment, internalTenantContext, accountBillCycleDayLocal);
}

private PlanPhaseSpecifier getPlanPhaseSpecifierFromTransition(final Catalog catalog, final EffectiveSubscriptionInternalEvent transition) throws CatalogApiException {
private PlanPhaseSpecifier getPlanPhaseSpecifierFromTransition(final Catalog catalog, final SubscriptionInternalEvent transition) throws CatalogApiException {
final Plan prevPlan = (transition.getPreviousPlan() != null) ? catalog.findPlan(transition.getPreviousPlan(), transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null;
final Plan nextPlan = (transition.getNextPlan() != null) ? catalog.findPlan(transition.getNextPlan(), transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null;

final Plan plan = (transition.getTransitionType() != SubscriptionBaseTransitionType.CANCEL) ? nextPlan : prevPlan;

final PlanPhase prevPhase = (transition.getPreviousPhase() != null) ? catalog.findPhase(transition.getPreviousPhase(), transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null;
final PlanPhase nextPhase = (transition.getNextPhase() != null) ? catalog.findPhase(transition.getNextPhase(), transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null;
final PlanPhase prevPhase = prevPlan != null && transition.getPreviousPhase() != null ? prevPlan.findPhase(transition.getPreviousPhase()) : null;
final PlanPhase nextPhase = nextPlan != null && transition.getNextPhase() != null ? nextPlan.findPhase(transition.getNextPhase()) : null;

final PlanPhase phase = (transition.getTransitionType() != SubscriptionBaseTransitionType.CANCEL) ? nextPhase : prevPhase;

return new PlanPhaseSpecifier(plan.getName(), phase.getPhaseType());

}

private boolean is_AUTO_INVOICING_OFF(final List<Tag> tags) {
Expand Down
19 changes: 9 additions & 10 deletions .../java/org/killbill/billing/subscription/api/timeline/DefaultSubscriptionBaseTimeline.java
View file
Expand Up @@ -65,7 +65,7 @@ private List<ExistingEvent> toExistingEvents(final Catalog catalog, final Produc

final List<ExistingEvent> result = new LinkedList<SubscriptionBaseTimeline.ExistingEvent>();

String prevPlanName = null;
Plan prevPlan = null;
String prevProductName = null;
BillingPeriod prevBillingPeriod = null;
String prevPriceListName = null;
Expand All @@ -84,7 +84,7 @@ private List<ExistingEvent> 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;

Expand All @@ -93,9 +93,9 @@ private List<ExistingEvent> 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;
Expand All @@ -109,10 +109,9 @@ private List<ExistingEvent> 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;
Expand All @@ -121,10 +120,10 @@ private List<ExistingEvent> 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() {
Expand Down Expand Up @@ -167,7 +166,7 @@ public Integer getBillCycleDayLocal() {
}
});

prevPlanName = planName;
prevPlan = plan;
prevProductName = productName;
prevBillingPeriod = billingPeriod;
prevPriceListName = priceListName;
Expand Down
18 changes: 7 additions & 11 deletions ...ion/src/main/java/org/killbill/billing/subscription/api/user/DefaultSubscriptionBase.java
View file
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
21 changes: 4 additions & 17 deletions ...on/src/main/java/org/killbill/billing/subscription/engine/dao/DefaultSubscriptionDao.java
View file
Expand Up @@ -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<SubscriptionEventModelDao> 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);
}
Expand Down Expand Up @@ -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<SubscriptionBaseEvent> allEvents = new LinkedList<SubscriptionBaseEvent>();
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());
}
Expand Down
9 changes: 3 additions & 6 deletions subscription/src/test/java/org/killbill/billing/subscription/alignment/TestPlanAligner.java
View file
@@ -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:
*
Expand Down Expand Up @@ -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
//
Expand Down Expand Up @@ -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}
//
Expand Down Expand Up @@ -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}
//
Expand Down Expand Up @@ -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
//
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 3ffaded

Please sign in to comment.