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

Add support to link usage tracking ids with generated invoices. #1068

Merged
merged 8 commits into from Dec 4, 2018

Conversation

Projects
None yet
2 participants
@sbrossie
Member

sbrossie commented Dec 3, 2018

There is a new table, invoice_tracking_ids, that is populated by the invoice code when generating USAGE items, and whose goal is to track usage records with invoices.

Due to the nature of our usage record apis, where we can record multiple data points for multiple usage sections and dates in one api call, there is no guarantee for a 1-1 mapping between a given tracking_id and a given invoice_id.

Also, at this point, there is no api to retrieve data from this new invoice_tracking_ids table as this would require an api change -- from for 0.20.x releases.

@sbrossie sbrossie changed the title from Usage tracking to Add support to link usage tracking ids with generated invoices. Dec 3, 2018

@pierre

pierre approved these changes Dec 4, 2018

A few small suggestions and comments. Looks good overall, my main concern would be duplicating information from usage in invoice (see comment on the columns for the new table).

final List<UsageRecord> bp1StoneRecords2 = new ArrayList();
bp1StoneRecords2.add(new UsageRecord(new LocalDate(2012, 4, 23), 10L));
// Outside of range for this period -> It's tracking ID spreads across 2 invoices

This comment has been minimized.

@pierre

pierre Dec 4, 2018

Member

Nit: Its 🤓

@@ -204,7 +206,7 @@ public void processSubscriptionStartRequestedDate(final RequestedSubscriptionInt
final ImmutableAccountData account = accountApi.getImmutableAccountDataById(accountId, context);
commitInvoiceAndSetFutureNotifications(account, null, notificationsBuilder.build(), context);
commitInvoiceAndSetFutureNotifications(account, null, ImmutableSet.of(), notificationsBuilder.build(), context);

This comment has been minimized.

@pierre

pierre Dec 4, 2018

Member

Nit: it seems we could have two signatures:

void commitInvoiceAndSetFutureNotifications(final ImmutableAccountData account,
                                            final FutureAccountNotifications futureAccountNotifications,
                                            final InternalCallContext context);

void commitInvoiceAndSetFutureNotifications(final ImmutableAccountData account,
                                            final InvoiceModelDao invoiceModelDao,
                                            final Set<InvoiceTrackingModelDao> trackingIds,
                                            final FutureAccountNotifications futureAccountNotifications,
                                            final InternalCallContext context);
private String trackingId;
private UUID invoiceId;
private UUID subscriptionId;

This comment has been minimized.

@pierre

pierre Dec 4, 2018

Member

The mapping trackingId -> {subscriptionId,unitType,recordDate} already exists in the usage module. Should we just keep the mapping trackingId->invoiceId here?

final TrackingRecordId that = (TrackingRecordId) o;
// !!! Exclude invoiceId on purpose.
//
// The Set methods (Sets.difference) is used to exclude usage record already invoiced (on a specified invoiceId),

This comment has been minimized.

@pierre

pierre Dec 4, 2018

Member

In the past, for such cases, we did not rely on the native methods but introduced our own (see InvoiceItem#matches for instance). In other words, should we keep equals as-is and implement our custom Sets.difference utility function calling a different method?

I'm just worried about future automatic refactoring. Also, it could introduce other weird behaviors when we implicitly truly expect a well-behaved implementation (for instance, doesn't HashSet need a real hash function for the add operation?).

@@ -170,6 +180,26 @@ public void checkChargedThroughDate(final UUID entitlementId, final LocalDate ex
}
}
public void checkTrackingIds(final Invoice invoice, final Set<String> expectedTrackingIds, final InternalCallContext internalCallContext) {
final InvoiceTrackingSqlDao dao = dbi.onDemand(InvoiceTrackingSqlDao.class);

This comment has been minimized.

@pierre

pierre Dec 4, 2018

Member

Might be worth creating a bug already for 0.22 to introduce a real API and fix tests.

@@ -80,6 +83,73 @@ public boolean apply(final InvoiceItem input) {
});
}
public Set<TrackingRecordId> getTrackingIds() {

This comment has been minimized.

@pierre

pierre Dec 4, 2018

Member

I'm guessing that in 0.22, we will want to expose these in the Invoice object. In the meantime, should we at least pass the information to invoice plugins as a custom/hidden plugin property? That way, it could help folks access that information without having to query the table directly.

Since you have it in InvoiceWithMetadata, it seems pretty straightforward to send it to onSuccessCall and onFailureCall. It's a bit more difficult for the priorCall but we could maybe punt that for now.

@pierre

pierre approved these changes Dec 4, 2018

@sbrossie sbrossie merged commit d6af651 into master Dec 4, 2018

1 of 4 checks passed

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

@pierre pierre deleted the usage-tracking branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment