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

Fix for invoice tree #1255

Open
wants to merge 6 commits into
base: work-for-release-0.21.x-0-21-8
from

Conversation

@sbrossie
Copy link
Member

sbrossie commented Jan 13, 2020

No description provided.

sbrossie added 2 commits Jan 12, 2020
The change introduced in beca83b#diff-6bbd5e696930ee20c12f24087244c833R234
is actually correct, i.e the recurring billing associated with an event marking cancelation or pause should be set to `NO_BILLING_PERIOD`.

However, our invoicing code contains another bug where we keep adding future notifications for canceled subscriptions. Prior this change,
this was not visible as the notification was set in the future but with the change from commit beca83b, the notification becomes in the past
and what was previously an unused future notification now leads to infinite loops.

The fix is to remove future notifications when there is nothing to invoice in the future.
…nto fix-for-invoice-tree
@sbrossie sbrossie requested a review from pierre Jan 13, 2020
@@ -665,7 +665,7 @@ public void testBCDChangeFromFreePlanToPayingPlanWithTrialAndCHANGE_OF_PLANPolic
// Change to the paying plan (alignment is CHANGE_OF_PLAN: we end up in TRIAL)
// Extra NULL_INVOICE event because invoice computes a future notification effective right away

This comment has been minimized.

Copy link
@pierre

pierre Jan 13, 2020

Member

Comment to update

final InternalCallContext internalCallContext) throws InvoiceApiException {

try {
final List<InvoiceItem> items = new ArrayList<InvoiceItem>();

final LocalDate thisEventEffectiveDate = internalCallContext.toLocalDate(thisEvent.getEffectiveDate());
if (thisEventEffectiveDate.compareTo(targetDate) > 0) {

This comment has been minimized.

Copy link
@pierre

pierre Jan 13, 2020

Member

Is this an optimization (unrelated to the bug)?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Jan 13, 2020

Author Member

No, it's part of the big fix: You could have future events in the billing events (past targetDate). For instance if you have a future cancelation you want to exit quickly otherwise this will clear the perSubscriptionFutureNotificationDates map.

perSubscriptionFutureNotificationDates);
}
} else {
final SubscriptionFutureNotificationDates futureNotificationDates = perSubscriptionFutureNotificationDates.get(thisEvent.getSubscriptionId());

This comment has been minimized.

Copy link
@pierre

pierre Jan 13, 2020

Member

Is my understanding correct that prior to this change, a subscription with a blocked billing period might have had a (NULL) invoice run after the paused date?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Jan 13, 2020

Author Member

I think this is true. I ''ll verify the state of our pause/resume tests for in-advance and add one for in-arrear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.