Skip to content

Commit

Permalink
payment: set processed amount to 0 in case of failure
Browse files Browse the repository at this point in the history 
This fixes #483.

Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
  • Loading branch information
@pierre
pierre committed Feb 4, 2016
1 parent ef21369 commit 6b25dcd
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 16 deletions.
15 changes: 15 additions & 0 deletions beatrix/src/test/java/org/killbill/billing/beatrix/integration/TestInvoicePayment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,13 +223,20 @@ public void testWithPaymentFailure() throws Exception {
assertTrue(invoice1.getPaidAmount().compareTo(BigDecimal.ZERO) == 0);
assertTrue(invoice1.getChargedAmount().compareTo(new BigDecimal("249.95")) == 0);
assertEquals(invoice1.getPayments().size(), 1);
assertEquals(invoice1.getPayments().get(0).getAmount().compareTo(BigDecimal.ZERO), 0);
assertEquals(invoice1.getPayments().get(0).getCurrency(), Currency.USD);
assertFalse(invoice1.getPayments().get(0).isSuccess());

final BigDecimal accountBalance1 = invoiceUserApi.getAccountBalance(account.getId(), callContext);
assertTrue(accountBalance1.compareTo(new BigDecimal("249.95")) == 0);

final List<Payment> payments = paymentApi.getAccountPayments(account.getId(), false, ImmutableList.<PluginProperty>of(), callContext);
assertEquals(payments.size(), 1);
assertEquals(payments.get(0).getTransactions().size(), 1);
assertEquals(payments.get(0).getTransactions().get(0).getAmount().compareTo(new BigDecimal("249.95")), 0);
assertEquals(payments.get(0).getTransactions().get(0).getCurrency(), Currency.USD);
assertEquals(payments.get(0).getTransactions().get(0).getProcessedAmount().compareTo(BigDecimal.ZERO), 0);
assertEquals(payments.get(0).getTransactions().get(0).getProcessedCurrency(), Currency.USD);

// Trigger the payment retry
busHandler.pushExpectedEvents(NextEvent.PAYMENT, NextEvent.INVOICE_PAYMENT);
Expand All @@ -245,5 +252,13 @@ public void testWithPaymentFailure() throws Exception {

final BigDecimal accountBalance2 = invoiceUserApi.getAccountBalance(account.getId(), callContext);
assertTrue(accountBalance2.compareTo(BigDecimal.ZERO) == 0);

final List<Payment> payments2 = paymentApi.getAccountPayments(account.getId(), false, ImmutableList.<PluginProperty>of(), callContext);
assertEquals(payments2.size(), 1);
assertEquals(payments2.get(0).getTransactions().size(), 2);
assertEquals(payments2.get(0).getTransactions().get(1).getAmount().compareTo(new BigDecimal("249.95")), 0);
assertEquals(payments2.get(0).getTransactions().get(1).getCurrency(), Currency.USD);
assertEquals(payments2.get(0).getTransactions().get(1).getProcessedAmount().compareTo(new BigDecimal("249.95")), 0);
assertEquals(payments2.get(0).getTransactions().get(1).getProcessedCurrency(), Currency.USD);
}
}
28 changes: 17 additions & 11 deletions ...main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,23 @@ private boolean updatePaymentAndTransactionInternal(final PaymentModelDao paymen
// Recompute new lastSuccessPaymentState. This is important to be able to allow new operations on the state machine (for e.g an AUTH_SUCCESS would now allow a CAPTURE operation)
final String lastSuccessPaymentState = paymentStateMachineHelper.isSuccessState(newPaymentState) ? newPaymentState : null;

// Update the processedAmount, processedCurrency if we got a paymentTransactionInfoPlugin from the plugin and if this is a non error state
final BigDecimal processedAmount = (paymentTransactionInfoPlugin != null && isPendingOrFinalTransactionStatus(transactionStatus)) ?
paymentTransactionInfoPlugin.getAmount() : paymentTransaction.getProcessedAmount();
final Currency processedCurrency = (paymentTransactionInfoPlugin != null && isPendingOrFinalTransactionStatus(transactionStatus)) ?
paymentTransactionInfoPlugin.getCurrency() : paymentTransaction.getProcessedCurrency();
// Update processedAmount and processedCurrency
final BigDecimal processedAmount;
if (TransactionStatus.SUCCESS.equals(transactionStatus) || TransactionStatus.PENDING.equals(transactionStatus)) {
if (paymentTransactionInfoPlugin == null || paymentTransactionInfoPlugin.getAmount() == null) {
processedAmount = paymentTransaction.getProcessedAmount();

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Feb 4, 2016

Member

In that, do we have the correct info set in paymentTransaction.getProcessedAmount(). Is it not what we are trying to compute?

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Feb 4, 2016

Author Member

Not sure I follow: for a final state, we just use the current processed amount from paymentTransaction if the plugin didn't give us a value.

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Feb 4, 2016

Member

My question was not very clear: I was wondering if the value in paymentTransaction.getProcessedAmount() contains the right info and can indeed be used to set the processedAmount. I think you can probably ignore the question (after reviewing code).
} else {
processedAmount = paymentTransactionInfoPlugin.getAmount();
}
} else {
processedAmount = BigDecimal.ZERO;
}
final Currency processedCurrency;
if (paymentTransactionInfoPlugin == null || paymentTransactionInfoPlugin.getCurrency() == null) {
processedCurrency = paymentTransaction.getProcessedCurrency();
} else {
processedCurrency = paymentTransactionInfoPlugin.getCurrency();
}

// Update the gatewayErrorCode, gatewayError if we got a paymentTransactionInfoPlugin
final String gatewayErrorCode = paymentTransactionInfoPlugin != null ? paymentTransactionInfoPlugin.getGatewayErrorCode() : paymentTransaction.getGatewayErrorCode();
Expand All @@ -239,12 +251,6 @@ private TransactionStatus computeNewTransactionStatusFromPaymentTransactionInfoP
return (newTransactionStatus != TransactionStatus.UNKNOWN) ? newTransactionStatus : currentTransactionStatus;
}

private boolean isPendingOrFinalTransactionStatus(final TransactionStatus transactionStatus) {
return (transactionStatus == TransactionStatus.PENDING ||
transactionStatus == TransactionStatus.SUCCESS ||
transactionStatus == TransactionStatus.PAYMENT_FAILURE);
}

private PaymentPluginApi getPaymentPluginApi(final PaymentModelDao item, final String pluginName) {
final PaymentPluginApi pluginApi = pluginRegistry.getServiceForName(pluginName);
Preconditions.checkState(pluginApi != null, "Janitor IncompletePaymentTransactionTask cannot retrieve PaymentPluginApi for plugin %s (payment id %s), skipping", pluginName, item.getId());
Expand Down
18 changes: 16 additions & 2 deletions payment/src/main/java/org/killbill/billing/payment/core/sm/PaymentAutomatonDAOHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,22 @@ public void createNewPaymentTransaction() throws PaymentApiException {

public void processPaymentInfoPlugin(final TransactionStatus transactionStatus, @Nullable final PaymentTransactionInfoPlugin paymentInfoPlugin,
final String currentPaymentStateName) {
final BigDecimal processedAmount = paymentInfoPlugin == null ? null : paymentInfoPlugin.getAmount();
final Currency processedCurrency = paymentInfoPlugin == null ? null : paymentInfoPlugin.getCurrency();
final BigDecimal processedAmount;
if (TransactionStatus.SUCCESS.equals(transactionStatus) || TransactionStatus.PENDING.equals(transactionStatus)) {
if (paymentInfoPlugin == null || paymentInfoPlugin.getAmount() == null) {
processedAmount = paymentStateContext.getAmount();
} else {
processedAmount = paymentInfoPlugin.getAmount();
}
} else {
processedAmount = BigDecimal.ZERO;
}
final Currency processedCurrency;
if (paymentInfoPlugin == null || paymentInfoPlugin.getCurrency() == null) {

This comment has been minimized.

Sign in to view

Copy link
@sbrossie

sbrossie Feb 4, 2016

Member

Could we set both processedAmount and processedCurrency in the same if branches (looks like we are handling the same use case (e.g if (paymentInfoPlugin == null || paymentInfoPlugin.getAmount() == null)) ?

This comment has been minimized.

Sign in to view

Copy link
@pierre

pierre Feb 4, 2016

Author Member

The difference is that we always populate the currency whereas we populate the amount only in a successful or pending case: we would still need nested if, no?
processedCurrency = paymentStateContext.getCurrency();
} else {
processedCurrency = paymentInfoPlugin.getCurrency();
}
final String gatewayErrorCode = paymentInfoPlugin == null ? null : paymentInfoPlugin.getGatewayErrorCode();
final String gatewayErrorMsg = paymentInfoPlugin == null ? null : paymentInfoPlugin.getGatewayError();

Expand Down
2 changes: 1 addition & 1 deletion ...nt/src/main/java/org/killbill/billing/payment/invoice/InvoicePaymentControlPluginApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ public OnFailurePaymentControlResult onFailureCall(final PaymentControlContext p
try {
log.debug("Notifying invoice of failed payment: id={}, amount={}, currency={}, invoiceId={}", paymentControlContext.getPaymentId(), paymentControlContext.getAmount(), paymentControlContext.getCurrency(), invoiceId);
invoiceApi.notifyOfPayment(invoiceId,
paymentControlContext.getAmount(),
BigDecimal.ZERO,
paymentControlContext.getCurrency(),
// processed currency may be null so we use currency; processed currency will be updated if/when payment succeeds
paymentControlContext.getCurrency(),
Expand Down
4 changes: 2 additions & 2 deletions payment/src/test/java/org/killbill/billing/payment/api/TestPaymentApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ public void testCreateFailedPurchase() throws PaymentApiException {
assertEquals(payment.getTransactions().get(0).getPaymentId(), payment.getId());
assertEquals(payment.getTransactions().get(0).getAmount().compareTo(requestedAmount), 0);
assertEquals(payment.getTransactions().get(0).getCurrency(), Currency.AED);
assertEquals(payment.getTransactions().get(0).getProcessedAmount().compareTo(requestedAmount), 0);
assertEquals(payment.getTransactions().get(0).getProcessedAmount().compareTo(BigDecimal.ZERO), 0);
assertEquals(payment.getTransactions().get(0).getProcessedCurrency(), Currency.AED);

assertEquals(payment.getTransactions().get(0).getTransactionStatus(), TransactionStatus.PAYMENT_FAILURE);
Expand Down Expand Up @@ -405,7 +405,7 @@ public void testCreateFailedPurchaseWithPaymentControl() throws PaymentApiExcept
assertEquals(payment.getTransactions().get(0).getPaymentId(), payment.getId());
assertEquals(payment.getTransactions().get(0).getAmount().compareTo(requestedAmount), 0);
assertEquals(payment.getTransactions().get(0).getCurrency(), Currency.USD);
assertEquals(payment.getTransactions().get(0).getProcessedAmount().compareTo(requestedAmount), 0); // This is weird...
assertEquals(payment.getTransactions().get(0).getProcessedAmount().compareTo(BigDecimal.ZERO), 0);
assertEquals(payment.getTransactions().get(0).getProcessedCurrency(), Currency.USD);

assertEquals(payment.getTransactions().get(0).getTransactionStatus(), TransactionStatus.PAYMENT_FAILURE);
Expand Down

0 comments on commit 6b25dcd

Please sign in to comment.