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

Initial for #1025 (step 3 only) #1206

Merged
merged 2 commits into from Sep 10, 2019

Conversation

@sbrossie
Copy link
Member

commented Sep 10, 2019

No description provided.

sbrossie added 2 commits Sep 10, 2019
invoice. Fix issue when there is no billing events bu invoicing still…
… needs to be running to REPAIR existing invoices. See #1205

@sbrossie sbrossie requested a review from pierre Sep 10, 2019

@pierre
pierre approved these changes Sep 10, 2019
Copy link
Member

left a comment

The fix itself is subtle! 😅

Did you check where else we call isEmpty?

@@ -535,10 +535,14 @@ public void testWithFullBlockBilling() throws Exception {

// 2012-04-21 -- block billing effective date is 2012-04-01

This comment has been minimized.

Copy link
@pierre

pierre Sep 10, 2019

Member

2012-05-21, right?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 10, 2019

Author Member

Yes, will fix comment.

final BlockingState blockingState1 = new DefaultBlockingState(bpSubscription.getBundleId(), BlockingStateType.SUBSCRIPTION_BUNDLE, "state1", "Service", true, true, true, null);
subscriptionApi.addBlockingState(blockingState1, new LocalDate(2012, 4, 1), ImmutableList.<PluginProperty>of(), callContext);
assertListenerStatus();

invoiceChecker.checkInvoice(account.getId(), 3, callContext,
new ExpectedInvoiceItemCheck(new LocalDate(2012, 5, 1), new LocalDate(2013, 5, 1), InvoiceItemType.REPAIR_ADJ, new BigDecimal("-2399.95")),

This comment has been minimized.

Copy link
@pierre

pierre Sep 10, 2019

Member

There's no repair on the 2012-04-1 - 2012-04-30 period because it's $0?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 10, 2019

Author Member

Yep, just a FIXED price and regardless of amount, we don't repair FIXED items.

@sbrossie

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

The fix itself is subtle! 😅

Not super subtle but definitely a bit hidden...

Did you check where else we call isEmpty?

Unfortunately this is really hard to do, because we are calling Set#isEmpty so the answer from IntelliJ is not super useful. But my test shows that after removing these 2 places, the test works as expect -- and all circle-ci tests shows green.

@sbrossie sbrossie merged commit cf2de41 into master Sep 10, 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
@pierre

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Unfortunately this is really hard to do, because we are calling Set#isEmpty so the answer from IntelliJ is not super useful.

I'm sure there's a better way, but here's what I did:

  • Add boolean isEmpty(); to the BillingEventSet interface
  • Remove extends SortedSet<BillingEvent> from BillingEventSet
  • Search for usage

There are 4 results, 2 in InvoiceDispatcher and 2 in FixedAndRecurringInvoiceItemGenerator :-)

@sbrossie

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

@pierre Looking at the use case for BillingEventSet#isEmpty, there are 3 left in my branch:

  • FixedAndRecurringInvoiceItemGenerator#processRecurringBillingEvents and FixedAndRecurringInvoiceItemGenerator#processFixedBillingEvents still make sense => when there is no billing events, we should not generate any proposed items
  • InvoiceDispatcher#processSubscriptionStartRequestedDateWithLock is used to handle RequestedSubscriptionInternalEvent to compute the notification date, i.e dryRunNotificationTime, and therefore it also makes sense to ignore those as there would be nothing to notify
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.