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

Work for #1191 #1202

Merged
merged 6 commits into from Sep 5, 2019

Conversation

@sbrossie
Copy link
Member

commented Sep 5, 2019

No description provided.

sbrossie added 3 commits Sep 4, 2019

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

@pierre
Copy link
Member

left a comment

For Kaui, we could display that information similar to the subscription id popover on the bundles screen (when the cursor is placed above the produce name)?

Something like that:

image

@@ -26,6 +26,7 @@
import org.killbill.billing.catalog.api.BillingPeriod;
import org.killbill.billing.catalog.api.PlanPhaseSpecifier;
import org.killbill.billing.catalog.api.ProductCategory;
import org.killbill.billing.catalog.api.VersionedCatalog;

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

Nit: extra import.

new ExpectedInvoiceItemCheck(new LocalDate(2015, 12, 5), new LocalDate(2016, 1, 5), InvoiceItemType.RECURRING, new BigDecimal("295.95")),
new ExpectedInvoiceItemCheck(new LocalDate(2015, 12, 5), new LocalDate(2016, 1, 5), InvoiceItemType.REPAIR_ADJ, new BigDecimal("-500.00")),
new ExpectedInvoiceItemCheck(new LocalDate(2015, 12, 5), new LocalDate(2015, 12, 5), InvoiceItemType.CBA_ADJ, new BigDecimal("204.05")));
final VersionedCatalog catalog = catalogUserApi.getCatalog("foo", callContext);
// RECURRING should be set against V2
Assert.assertEquals(curInvoice.getInvoiceItems().get(0).getCatalogEffectiveDate(), catalog.getVersions().get(1).getEffectiveDate());

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

Would it be easier to embed this in the invoiceChecker instead?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 5, 2019

Author Member

I don't know about easier because now every test would need to pull the catalog, and understand which catalog version to pass, but i agree this would make the testing more systematic, probably a good thing from a regression point of view - we can talk to decide whether this is worth the effort.

@@ -92,6 +94,10 @@ public void testWithChangeAcrossCatalogs() throws Exception {
invoiceChecker.checkTrackingIds(curInvoice, ImmutableSet.of("t3", "t4"), internalCallContext);


// Check items catalogEffectiveDate are correctly marked against each version
final VersionedCatalog catalog = catalogUserApi.getCatalog("foo", callContext);
Assert.assertEquals(curInvoice.getInvoiceItems().get(0).getCatalogEffectiveDate(), catalog.getVersions().get(0).getEffectiveDate());

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

Are we guaranteed assertEquals will always work (instead of compareTo)? 🤔

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 5, 2019

Author Member

Will fix...

@@ -20,11 +20,13 @@
import java.util.UUID;

import org.joda.time.DateTime;
import org.joda.time.DateTimeUtils;

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

Extra import

@@ -19,6 +19,7 @@
package org.killbill.billing.invoice.model;

import java.math.BigDecimal;
import java.util.Date;

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

Extra import.

@@ -248,13 +250,15 @@ boolean isSameDayAndSameSubscription(final InvoiceItem prevComputedFixedItem, fi
final BigDecimal rate = thisEvent.getRecurringPrice();
if (rate != null) {
final BigDecimal amount = KillBillMoney.of(itemDatum.getNumberOfCycles().multiply(rate), currency);
final Date catalogEffectiveDate = thisEvent.getCatalogEffectiveDate() != null ? thisEvent.getCatalogEffectiveDate().toDate() : null;

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

Why a java.util.Date?

Assert.assertNull(result1.getCatalogEffectiveDate());


// No catalogEffectiveDate

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

With

@@ -17,6 +17,7 @@
package org.killbill.billing.jaxrs.json;

import java.math.BigDecimal;
import java.util.Date;

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

Same question here: a bit weird to mix java.util.Date with Joda stuff.

@Override
public boolean matches(final Object o) {

if (!super.matches(o)) {
return false;
}
final InvoiceItemCatalogBase that = (InvoiceItemCatalogBase) o;
if (catalogEffectiveDate != null && that.catalogEffectiveDate != null ? catalogEffectiveDate.compareTo(that.catalogEffectiveDate) != 0 : that.catalogEffectiveDate != null) {

This comment has been minimized.

Copy link
@pierre

pierre Sep 5, 2019

Member

⚠️ Not sure this is correct:

  • catalogEffectiveDate = now()
  • that.catalogEffectiveDate = null
  • => catalogEffectiveDate != null && that.catalogEffectiveDate is false and that.catalogEffectiveDate != null if false
  • => if is false but they are not equal?

@sbrossie sbrossie merged commit 474f035 into work-for-release-0.21.x Sep 5, 2019

1 check was pending

ci/circleci: build CircleCI is running your tests
Details
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.