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

Enhance credit api to take additional fields such as rate and quantit… #1144

Merged
merged 2 commits into from Jun 7, 2019

Conversation

Projects
None yet
2 participants
@sbrossie
Copy link
Member

commented Jun 5, 2019

…y. See #1141

@pierre

pierre approved these changes Jun 6, 2019

Copy link
Member

left a comment

👍

My main feedback would be about the shape of the API (see killbill-api PR / the insertExternalCharges in the description of #1141).

@@ -85,7 +86,8 @@ public void testWrittenOffInvoiceBeforeAccountCredit() throws Exception {
assertEquals(accountCBA1.compareTo(BigDecimal.ZERO), 0);

// Add credit on the account
invoiceUserApi.insertCredit(accountId, BigDecimal.TEN, null, accountCurrency, true, null, null, null, callContext);
final InvoiceItem inputCredit = new CreditAdjInvoiceItem(null, accountId, clock.getUTCToday(), "some description", BigDecimal.TEN, accountCurrency, null);

This comment has been minimized.

Copy link
@pierre

pierre Jun 6, 2019

Member

Maybe add an assertion in one of these tests that we can correctly retrieve the description.

if (externalPayment && remainingRequestPayment.compareTo(BigDecimal.ZERO) > 0) {
invoiceApi.insertCredit(account.getId(), remainingRequestPayment, clock.getUTCToday(), account.getCurrency(), true, "pay all invoices", null, pluginProperties, callContext);

final InvoiceItem creditItem = new InvoiceItem() {

This comment has been minimized.

Copy link
@pierre

pierre Jun 6, 2019

Member

Nit: extract the class (at least make it static in the file).


final Iterable<PluginProperty> pluginProperties = extractPluginProperties(pluginPropertiesString);
final CallContext callContext = context.createCallContextWithAccountId(json.getAccountId(), createdBy, reason, comment, request);

final Account account = accountUserApi.getAccountById(json.getAccountId(), callContext);
final LocalDate effectiveDate = new LocalDate(callContext.getCreatedDate(), account.getTimeZone());

// TODO Having a common method is great, but the validation is quite minimal...

This comment has been minimized.

Copy link
@pierre

pierre Jun 6, 2019

Member

Related to our discussion from yesterday: is that really a TODO (i.e. do we want to do anything)?

@sbrossie sbrossie merged commit 0fe17b2 into work-for-release-0.21.x Jun 7, 2019

4 of 5 checks passed

ci/circleci: integration-tests Your tests failed on CircleCI
Details
ci/circleci: build 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.