Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major junction billing event refactoring allowing to fix issue with catalog versioning. See #1110. #1117

Merged
merged 11 commits into from Mar 11, 2019

Conversation

Projects
None yet
2 participants
@sbrossie
Copy link
Member

sbrossie commented Mar 9, 2019

No description provided.

sbrossie added some commits Mar 8, 2019

junction: Introducing a new SubscriptionBillingEvent type to better c…
…ompute the BillingEvent

This code change also removes a bunch of hacks that we added over the years in the DefaultBillingEvent class.
There are still remaining TODO that will hopefully be tackeled later

Also, we are slowly introducing a new concept of a `lastChangePlanDate` which will be plugged later when fixing the issue
with change of Plan across catalog versions.
junction: Additional cleanup into DefaultBillingEvent (remove test co…
…mplication leaking into code, and aggregate method logic)
beatrix: Add test assertion to demonstrate current issue when changin…
…g plan (price is based on old catalog version price)
@sbrossie

This comment has been minimized.

Copy link
Member Author

sbrossie commented Mar 9, 2019

Some highlights in this PR:

@pierre
Copy link
Member

pierre left a comment

  • Overall, good clean-up job! I especially like that the BillingEvent object doesn't have the Subscription object anymore.
  • Regarding my review, I did my best going commit by commit but it's still a lot of changes. I trust CircleCI on that one...
  • 🆗 to merge in master per our discussion last week.

My main feedback is that I'm still having a hard time convincing myself that both subscription and junction are doing the same thing.

On one hand, subscription is calling various catalog APIs at the time of the subscription change, when doing a dry-run change, etc. On the other hand, junction is doing the same, both at the time of creating the BilingEvent (constructor) and on-the-fly for invoice to get the right price (grandfathering).

Regardless of semantics / behavior choices, subsystems need to have the same view at a given point in time. Maybe it's the case today, and it's just a matter of following the code?

To put it differently, we communicate catalog information as String from subscription to junction (SubscriptionBillingEvent), only for junction to resolve them again as objects by calling the catalog in DefaultBillingEvent. It would be nice if the catalog would only be called inside subscription. For the price grandfathering use-case, ideally we would have the information of prices as intervals (t1-t2: P1, t2-t3: P2, etc.) instead of having to call back the catalog APIs with tricky dates.

Not sure if I make sense and even if I do, not sure if it's realistic to make such changes, but let's at least the conversation regardless 😄

final BillingEvent result = Mockito.mock(BillingEvent.class);
Mockito.when(result.getCurrency()).thenReturn(Currency.BTC);
Mockito.when(result.getBillCycleDayLocal()).thenReturn(bcd);
Mockito.when(result.getEffectiveDate()).thenReturn(effectiveDate);
Mockito.when(result.getBillingPeriod()).thenReturn(billingPeriod);
Mockito.when(result.getSubscriptionId()).thenReturn(subscriptionId);
Mockito.when(result.getBundleId()).thenReturn(bundleId);

final Account account = Mockito.mock(Account.class);
Mockito.when(account.getId()).thenReturn(accountId);

final SubscriptionBase subscription = Mockito.mock(SubscriptionBase.class);

This comment has been minimized.

@pierre

pierre Mar 11, 2019

Member

You can get rid of this.


private static BigDecimal computeFixedPrice(final boolean isCancelledOrBlocked, final PlanPhase effectivePlanPhase, final Currency currency, final SubscriptionBaseTransitionType type) throws CatalogApiException {
if (isCancelledOrBlocked ||
type == SubscriptionBaseTransitionType.BCD_CHANGE /* We don't want to bill twice for the same fixed price */) {

This comment has been minimized.

@pierre

pierre Mar 11, 2019

Member

This is a bit sneaky, no? Shouldn't that logic be in invoice instead?

This comment has been minimized.

@sbrossie

sbrossie Mar 11, 2019

Author Member

I wanted to move in CTOR as discussed but realized, this computeFixedPrice is basically the CTOR fir the fixedPrice piece. Also i thought of other use cases where maybe our fixedPrice is broken, but don't want to start into another tangent... left it as-is for now.

if (isActive) {
final String planName = transition.getNextPlan();
this.plan = (planName != null) ? catalog.findPlan(planName, transition.getEffectiveTransitionTime(), transition.getSubscriptionStartDate()) : null;
this.isCancelledOrBlocked = inputEvent.getType() == SubscriptionBaseTransitionType.CANCEL;

This comment has been minimized.

@pierre

pierre Mar 11, 2019

Member

I'm confused by the name: why OrBlocked?
Also, I'm guessing we don't care about START_BILLING_DISABLED at this stage because these are computed later on in junction (if so, a bit sneaky - you might want to add a comment)?

final String description,
final long totalOrdering,
final SubscriptionBaseTransitionType type,
final boolean isDisableEvent,

This comment has been minimized.

@pierre

pierre Mar 11, 2019

Member

Nit: isCancelledOrBlocked.

This comment has been minimized.

@sbrossie

sbrossie Mar 11, 2019

Author Member

Actually re-reading the code, the variable is correct -- this CTOR is only used for blocking state billing events.

final List<SubscriptionBaseBundle> bundles = subscriptionApi.getBundlesForAccount(accountId, context);
final ImmutableAccountData account = accountApi.getImmutableAccountDataById(accountId, context);
result = new DefaultBillingEventSet(false, found_INVOICING_DRAFT, found_INVOICING_REUSE_DRAFT);
addBillingEventsForBundles(bundles, account, dryRunArguments, context, result, skippedSubscriptions, subscriptionsForAccount, fullCatalog, tagsForAccount);
if (result.isEmpty()) {
log.info("No billing event for accountId='{}'", accountId);

This comment has been minimized.

@pierre

pierre Mar 11, 2019

Member

When does this happen now in the non AUTO_INVOICING_OFF case? Block billing right at the start?

This comment has been minimized.

@sbrossie

sbrossie Mar 11, 2019

Author Member

Moved it, and then moved it back, it required some code refactoring...

busHandler.pushExpectedEvents(NextEvent.CHANGE, NextEvent.INVOICE);
bpEntitlement.changePlanWithDate(new DefaultEntitlementSpecifier(spec1), clock.getUTCToday(), ImmutableList.<PluginProperty>of(), callContext);
assertListenerStatus();

//
// The code normally goes through the grandfathering logic to find the version but specifies the transitionTime of the latest CHANGE (and not the subscriptionStartDate)

This comment has been minimized.

@pierre

pierre Mar 11, 2019

Member

Worth doing a subsequent change to verify the logic with multiple changes?

This comment has been minimized.

@sbrossie

sbrossie Mar 11, 2019

Author Member

Yes, will do, but i think it's more interesting to it in the new test. See 261277a

beatrix: Add more test assertion to test the new logic around grandfa…
…thering + effectiveDateForExistingSubscriptions better. See #1110

@sbrossie sbrossie merged commit a504f72 into master Mar 11, 2019

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: integration-tests Your tests passed on CircleCI!
Details
ci/circleci: test-h2 Your tests passed on CircleCI!
Details
ci/circleci: test-mysql Your tests passed on CircleCI!
Details
ci/circleci: test-postgresql Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.