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 release 0.21 catalog exp #1199

Merged

Conversation

@sbrossie
Copy link
Member

commented Aug 31, 2019

No description provided.

sbrossie added 13 commits Aug 29, 2019
catalog: Remove change introduced in 81d8748 where we don't return a …
…empty versionned catalog from the cached.

It looks like our code treats null entries and empty entries differently (yack). /cc @pierre
subscription: Pass existing VersionedCatalog from caller to internal …
…subscription api whenever this is already available.
subscription: Change SubscriptionBillingEvent#Plan semantics for canc…
…elled transition. See #1191

Previous code would compute such Plan to be the previous non null Plan which created an asymetric
meaning of what this Plan really mean and how it is compute. Instead we simply return null and need to
update the rest of the code such as junction (BillingEvent) and invoice to correctly understand this.

In addition we expose SubscriptionBillingEvent#getCatalogEffectiveDate which is required (when Plan is null)
and which should provide what we need to implement #1191 (as each BillingEvent now comes with its catalog version).
@pierre
Copy link
Member

left a comment

Very nice 😅

It's probably worth re-running the ruby integration tests as well, to make sure existing behaviors (e.g. grandfathering) haven't subtlety changed....

@@ -238,7 +239,7 @@ public VersionedPluginCatalog getVersionedPluginCatalog(final Iterable<PluginPro
}
}

return new TestModelVersionedPluginCatalog(versionedCatalog.getCatalogName(), toStandalonePluginCatalogs(versionedCatalog.getVersions()));
return new TestModelVersionedPluginCatalog(versionedCatalog.getVersions().get(0).getCatalogName(), toStandalonePluginCatalogs(versionedCatalog.getVersions()));

This comment has been minimized.

Copy link
@pierre

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

Ack will add.

@@ -98,12 +98,10 @@

// For deserialization

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

⚠️ I think we need the no-arg constructor for off-heap and Redis caching (hence the comment). See 4dc99af

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

Confused, the no-arg CTOR exists

@@ -248,7 +249,7 @@ public void addCatalogVersion(final String catalogResource) throws Exception {
final StandaloneCatalogWithPriceOverride inputCatalogVersionWithOverride = new StandaloneCatalogWithPriceOverride(inputCatalogVersion, priceOverride, internalTenantContext.getTenantRecordId(), internalCallContextFactory);

if (versionedCatalog == null) {
versionedCatalog = new DefaultVersionedCatalog(clock);
versionedCatalog = new DefaultVersionedCatalog(new ArrayList<>());

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

You could use the no-arg constructor instead.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

Will remove the CTOR with the array -- it is now useless.

@@ -107,17 +105,9 @@ private int indexOfVersionForDate(final Date date) throws CatalogApiException {
if (!versions.isEmpty()) {
return 0;
}
throw new CatalogApiException(ErrorCode.CAT_NO_CATALOG_FOR_GIVEN_DATE, date.toString());
throw new IllegalStateException("No existing versions in the VersionedCatalog catalog ??");

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

Maybe add some debugging information (Date and/or known versions).

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

ack

@@ -82,7 +83,7 @@ public DefaultVersionedCatalog loadDefaultCatalog(final String uriString) throws
xmlURIs = findXmlReferences(directoryContents, url);
}

final DefaultVersionedCatalog result = new DefaultVersionedCatalog(clock);
final DefaultVersionedCatalog result = new DefaultVersionedCatalog(new ArrayList<>());

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

You could use the no-arg constructor.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

yes will fix all those.

@@ -25,7 +25,8 @@
import org.joda.time.DateTime;
import org.killbill.billing.callcontext.InternalCallContext;
import org.killbill.billing.callcontext.InternalTenantContext;
import org.killbill.billing.catalog.api.Catalog;
import org.killbill.billing.catalog.api.StaticCatalog;

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

Here (and in other entitlement files), you don't need the StaticCatalog import anymore.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

ack.

@@ -79,6 +80,7 @@
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Ordering;
import jdk.internal.dynalink.beans.StaticClass;

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

⚠️ Wrong import.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

ack


// Handle recurring items
final BillingPeriod billingPeriod = thisEvent.getBillingPeriod();
if (billingPeriod != BillingPeriod.NO_BILLING_PERIOD) {

final Plan currentPlan = thisEvent.getPlan();
Preconditions.checkNotNull(currentPlan, String.format("Unexpected null Plan name event = %s", thisEvent));

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

No need for String.format with Preconditions.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

ack :-)

@@ -421,7 +422,7 @@ public Boolean apply(@Nullable final String key) {
tenantOverdueConfigCacheController.remove(tenantRecordId);

// clear tenant-catalog cache by tenantRecordId
final CacheController<Long, Catalog> tenantCatalogCacheController = cacheControllerDispatcher.getCacheController(CacheType.TENANT_CATALOG);
final CacheController<Long, List<StaticCatalog>> tenantCatalogCacheController = cacheControllerDispatcher.getCacheController(CacheType.TENANT_CATALOG);

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

I'm fine with this but isn't it easier to cache an object VersionedCatalog instead of an erased list?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

yes, will fix

@@ -49,6 +50,7 @@
import org.killbill.billing.util.glue.CustomFieldModule;

import com.google.inject.name.Names;
import jdk.internal.dynalink.beans.StaticClass;

This comment has been minimized.

Copy link
@pierre

pierre Sep 3, 2019

Member

⚠️ Wrong import

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

ack

sbrossie added 3 commits Sep 3, 2019

@sbrossie sbrossie merged commit 14f2337 into work-for-release-0.21-catalog Sep 3, 2019

1 of 4 checks passed

ci/circleci: test-h2 CircleCI is running your tests
Details
ci/circleci: test-mysql CircleCI is running your tests
Details
ci/circleci: test-postgresql CircleCI is running your tests
Details
ci/circleci: build Your tests passed on CircleCI!
Details

@sbrossie sbrossie deleted the work-for-release-0.21-catalog-exp branch Sep 3, 2019

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.