killbill-aplcache

diff --git a/api/src/main/java/org/killbill/billing/payment/plugin/api/NoOpPaymentPluginApi.java b/api/src/main/java/org/killbill/billing/payment/plugin/api/NoOpPaymentPluginApi.java
index bab9f06..40eee4f 100644
--- a/api/src/main/java/org/killbill/billing/payment/plugin/api/NoOpPaymentPluginApi.java
+++ b/api/src/main/java/org/killbill/billing/payment/plugin/api/NoOpPaymentPluginApi.java
@@ -16,13 +16,18 @@
 
 package org.killbill.billing.payment.plugin.api;
 
+import java.util.List;
+import java.util.UUID;
+
 public interface NoOpPaymentPluginApi extends PaymentPluginApi {
 
-    public void clear();
+    void clear();
+
+    void makeNextPaymentFailWithError();
 
-    public void makeNextPaymentFailWithError();
+    void makeNextPaymentFailWithException();
 
-    public void makeNextPaymentFailWithException();
+    void makeAllInvoicesFailWithError(boolean failure);
 
-    public void makeAllInvoicesFailWithError(boolean failure);
+    void updatePaymentTransactions(UUID paymentId, List<PaymentTransactionInfoPlugin> newTransactions);
 }

diff --git a/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java b/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java
index 8c013d4..94d318b 100644
--- a/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java
+++ b/payment/src/main/java/org/killbill/billing/payment/core/janitor/IncompletePaymentTransactionTask.java
@@ -142,7 +142,7 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
         if (!TRANSACTION_STATUSES_TO_CONSIDER.contains(event.getStatus())) {
             return;
         }
-        insertNewNotificationForUnresolvedTransactionIfNeeded(event.getPaymentTransactionId(), 1, event.getUserToken(), event.getSearchKey1(), event.getSearchKey2());
+        insertNewNotificationForUnresolvedTransactionIfNeeded(event.getPaymentTransactionId(), 0, event.getUserToken(), event.getSearchKey1(), event.getSearchKey2());
     }
 
     public boolean updatePaymentAndTransactionIfNeededWithAccountLock(final PaymentModelDao payment, final PaymentTransactionModelDao paymentTransaction, final PaymentTransactionInfoPlugin paymentTransactionInfoPlugin, final InternalTenantContext internalTenantContext) {
@@ -200,6 +200,14 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
                 return false;
         }
 
+        // Our status did not change, so we just insert a new notification (attemptNumber will be incremented)
+        if (transactionStatus == paymentTransaction.getTransactionStatus()) {
+            log.debug("Janitor IncompletePaymentTransactionTask repairing payment {}, transaction {}, transitioning transactionStatus from {} -> {}",
+                      payment.getId(), paymentTransaction.getId(), paymentTransaction.getTransactionStatus(), transactionStatus);
+            insertNewNotificationForUnresolvedTransactionIfNeeded(paymentTransaction.getId(), attemptNumber, userToken, internalTenantContext.getAccountRecordId(), internalTenantContext.getTenantRecordId());
+            return false;
+        }
+
         // 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;
 
@@ -213,13 +221,9 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
         final String gatewayErrorCode = paymentTransactionInfoPlugin != null ? paymentTransactionInfoPlugin.getGatewayErrorCode() : paymentTransaction.getGatewayErrorCode();
         final String gatewayError = paymentTransactionInfoPlugin != null ? paymentTransactionInfoPlugin.getGatewayError() : paymentTransaction.getGatewayErrorMsg();
 
-        if (transactionStatus == paymentTransaction.getTransactionStatus()) {
-            log.debug("Janitor IncompletePaymentTransactionTask repairing payment {}, transaction {}, transitioning transactionStatus from {} -> {}",
-                      payment.getId(), paymentTransaction.getId(), paymentTransaction.getTransactionStatus(), transactionStatus);
-        } else {
-            log.info("Janitor IncompletePaymentTransactionTask repairing payment {}, transaction {}, transitioning transactionStatus from {} -> {}",
-                     payment.getId(), paymentTransaction.getId(), paymentTransaction.getTransactionStatus(), transactionStatus);
-        }
+
+        log.info("Janitor IncompletePaymentTransactionTask repairing payment {}, transaction {}, transitioning transactionStatus from {} -> {}",
+                 payment.getId(), paymentTransaction.getId(), paymentTransaction.getTransactionStatus(), transactionStatus);
 
         final InternalCallContext internalCallContext = internalCallContextFactory.createInternalCallContext(payment.getAccountId(), callContext);
         paymentDao.updatePaymentAndTransactionOnCompletion(payment.getAccountId(), payment.getId(), paymentTransaction.getTransactionType(), newPaymentState, lastSuccessPaymentState,
@@ -248,10 +252,10 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
     }
 
     @VisibleForTesting
-    DateTime getNextNotificationTime(@Nullable final Integer attemptNumber) {
+    DateTime getNextNotificationTime(final Integer attemptNumber) {
 
         final List<TimeSpan> retries = paymentConfig.getIncompleteTransactionsRetries();
-        if (attemptNumber == null || attemptNumber > retries.size()) {
+        if (attemptNumber > retries.size()) {
             return null;
         }
         final TimeSpan nextDelay = retries.get(attemptNumber - 1);
@@ -259,8 +263,15 @@ public class IncompletePaymentTransactionTask extends CompletionTaskBase<Payment
     }
 
     private void insertNewNotificationForUnresolvedTransactionIfNeeded(final UUID paymentTransactionId, @Nullable final Integer attemptNumber, @Nullable final UUID userToken, final Long accountRecordId, final Long tenantRecordId) {
-        final NotificationEvent key = new JanitorNotificationKey(paymentTransactionId, IncompletePaymentTransactionTask.class.toString(), attemptNumber);
-        final DateTime notificationTime = getNextNotificationTime(attemptNumber);
+        // When we come from a GET path, we don't want to insert a new notification
+        if (attemptNumber == null) {
+            return;
+        }
+
+        // Increment value before we insert
+        final Integer newAttemptNumber = attemptNumber.intValue() + 1;
+        final NotificationEvent key = new JanitorNotificationKey(paymentTransactionId, IncompletePaymentTransactionTask.class.toString(), newAttemptNumber);
+        final DateTime notificationTime = getNextNotificationTime(newAttemptNumber);
         // Will be null in the GET path or when we run out opf attempts..
         if (notificationTime != null) {
             try {

diff --git a/payment/src/main/java/org/killbill/billing/payment/provider/DefaultNoOpPaymentProviderPlugin.java b/payment/src/main/java/org/killbill/billing/payment/provider/DefaultNoOpPaymentProviderPlugin.java
index 2908799..a194300 100644
--- a/payment/src/main/java/org/killbill/billing/payment/provider/DefaultNoOpPaymentProviderPlugin.java
+++ b/payment/src/main/java/org/killbill/billing/payment/provider/DefaultNoOpPaymentProviderPlugin.java
@@ -93,6 +93,11 @@ public class DefaultNoOpPaymentProviderPlugin implements NoOpPaymentPluginApi {
     }
 
     @Override
+    public void updatePaymentTransactions(final UUID paymentId, final List<PaymentTransactionInfoPlugin> newTransactions) {
+        throw new IllegalStateException("Not implemented");
+    }
+
+    @Override
     public PaymentTransactionInfoPlugin authorizePayment(final UUID kbAccountId, final UUID kbPaymentId, final UUID kbTransactionId, final UUID kbPaymentMethodId, final BigDecimal amount, final Currency currency, final Iterable<PluginProperty> properties, final CallContext context)
             throws PaymentPluginApiException {
         return getInternalNoopPaymentInfoResult(kbPaymentId, kbTransactionId, TransactionType.AUTHORIZE, amount, currency);

diff --git a/payment/src/test/java/org/killbill/billing/payment/core/janitor/TestIncompletePaymentTransactionTask.java b/payment/src/test/java/org/killbill/billing/payment/core/janitor/TestIncompletePaymentTransactionTask.java
index 569070f..9e81218 100644
--- a/payment/src/test/java/org/killbill/billing/payment/core/janitor/TestIncompletePaymentTransactionTask.java
+++ b/payment/src/test/java/org/killbill/billing/payment/core/janitor/TestIncompletePaymentTransactionTask.java
@@ -35,7 +35,6 @@ public class TestIncompletePaymentTransactionTask extends PaymentTestSuiteNoDB {
 
     @Test(groups = "fast")
     public void testGetNextNotificationTime() {
-        assertNull(incompletePaymentTransactionTask.getNextNotificationTime(null));
         final DateTime initTime = clock.getUTCNow();
 
         // Based on config "15s,1m,3m,1h,1d,1d,1d,1d,1d"

diff --git a/payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java b/payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java
index 9e8243d..f3b7088 100644
--- a/payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java
+++ b/payment/src/test/java/org/killbill/billing/payment/provider/MockPaymentProviderPlugin.java
@@ -209,6 +209,13 @@ public class MockPaymentProviderPlugin implements NoOpPaymentPluginApi {
     }
 
     @Override
+    public void updatePaymentTransactions(final UUID paymentId, final List<PaymentTransactionInfoPlugin> newTransactions) {
+        if (paymentTransactions.containsKey(paymentId.toString())) {
+            paymentTransactions.put (paymentId.toString(), newTransactions);
+        }
+    }
+
+    @Override
     public PaymentTransactionInfoPlugin authorizePayment(final UUID kbAccountId, final UUID kbPaymentId, final UUID kbTransactionId, final UUID kbPaymentMethodId, final BigDecimal amount, final Currency currency, final Iterable<PluginProperty> properties, final CallContext context)
             throws PaymentPluginApiException {
         return getPaymentTransactionInfoPluginResult(kbPaymentId, kbTransactionId, TransactionType.AUTHORIZE, amount, currency, properties);

diff --git a/payment/src/test/java/org/killbill/billing/payment/TestJanitor.java b/payment/src/test/java/org/killbill/billing/payment/TestJanitor.java
index 1753af8..e99ccaa 100644
--- a/payment/src/test/java/org/killbill/billing/payment/TestJanitor.java
+++ b/payment/src/test/java/org/killbill/billing/payment/TestJanitor.java
@@ -47,6 +47,10 @@ import org.killbill.billing.payment.dao.PaymentAttemptModelDao;
 import org.killbill.billing.payment.dao.PaymentTransactionModelDao;
 import org.killbill.billing.payment.glue.DefaultPaymentService;
 import org.killbill.billing.payment.invoice.InvoicePaymentControlPluginApi;
+import org.killbill.billing.payment.plugin.api.PaymentPluginApiException;
+import org.killbill.billing.payment.plugin.api.PaymentPluginStatus;
+import org.killbill.billing.payment.plugin.api.PaymentTransactionInfoPlugin;
+import org.killbill.billing.payment.provider.DefaultNoOpPaymentInfoPlugin;
 import org.killbill.billing.payment.provider.MockPaymentProviderPlugin;
 import org.killbill.billing.platform.api.KillbillConfigSource;
 import org.killbill.billing.util.callcontext.InternalCallContextFactory;
@@ -55,6 +59,7 @@ import org.killbill.notificationq.api.NotificationEvent;
 import org.killbill.notificationq.api.NotificationEventWithMetadata;
 import org.killbill.notificationq.api.NotificationQueueService;
 import org.killbill.notificationq.api.NotificationQueueService.NoSuchNotificationQueue;
+import org.skife.config.TimeSpan;
 import org.skife.jdbi.v2.Handle;
 import org.skife.jdbi.v2.tweak.HandleCallback;
 import org.testng.Assert;
@@ -393,6 +398,58 @@ public class TestJanitor extends PaymentTestSuiteWithEmbeddedDB {
         Assert.assertEquals(updatedPayment.getTransactions().get(0).getTransactionStatus(), TransactionStatus.SUCCESS);
     }
 
+    // The test will check that when a PENDING entry stays PENDING, we go through all our retries and evebtually give up (no infinite loop of retries)
+    @Test(groups = "slow")
+    public void testPendingEntriesThatDontMove() throws PaymentApiException, EventBusException, NoSuchNotificationQueue, PaymentPluginApiException, InterruptedException {
+
+        final BigDecimal requestedAmount = BigDecimal.TEN;
+        final String paymentExternalKey = "haha";
+        final String transactionExternalKey = "hoho!";
+
+        testListener.pushExpectedEvent(NextEvent.PAYMENT);
+        final Payment payment = paymentApi.createAuthorization(account, account.getPaymentMethodId(), null, requestedAmount, account.getCurrency(), paymentExternalKey,
+                                                               transactionExternalKey, ImmutableList.<PluginProperty>of(), callContext);
+        testListener.assertListenerStatus();
+
+        final InternalCallContext internalCallContext = internalCallContextFactory.createInternalCallContext(account.getId(), callContext);
+
+        // Artificially move the transaction status to PENDING AND update state on the plugin as well
+        final List<PaymentTransactionInfoPlugin> paymentTransactions = mockPaymentProviderPlugin.getPaymentInfo(account.getId(), payment.getId(), ImmutableList.<PluginProperty>of(), callContext);
+        final PaymentTransactionInfoPlugin oTx = paymentTransactions.remove(0);
+        final PaymentTransactionInfoPlugin updatePaymentTransaction = new DefaultNoOpPaymentInfoPlugin(oTx.getKbPaymentId(), oTx.getKbTransactionPaymentId(), oTx.getTransactionType(), oTx.getAmount(), oTx.getCurrency(), oTx.getCreatedDate(), oTx.getCreatedDate(), PaymentPluginStatus.PENDING, null);
+        paymentTransactions.add(updatePaymentTransaction);
+        mockPaymentProviderPlugin.updatePaymentTransactions(payment.getId(), paymentTransactions);
+
+        final String paymentStateName = paymentSMHelper.getPendingStateForTransaction(TransactionType.AUTHORIZE).toString();
+
+
+        testListener.pushExpectedEvent(NextEvent.PAYMENT);
+        paymentDao.updatePaymentAndTransactionOnCompletion(account.getId(), payment.getId(), TransactionType.AUTHORIZE, paymentStateName, paymentStateName,
+                                                           payment.getTransactions().get(0).getId(), TransactionStatus.PENDING, requestedAmount, account.getCurrency(),
+                                                           "loup", "chat", internalCallContext);
+        testListener.assertListenerStatus();
+
+        // 15s,1m,3m,1h,1d,1d,1d,1d,1d
+        for (TimeSpan cur : paymentConfig.getIncompleteTransactionsRetries()) {
+            // Verify there is a notification to retry updating the value
+            assertEquals(getPendingNotificationCnt(internalCallContext), 1);
+
+            clock.addDeltaFromReality(cur.getMillis() + 1);
+
+            // We add a sleep here to make sure the notification gets processed. Note that calling assertNotificationsCompleted would not work
+            // because there is a point in time where the notification  queue is empty (showing notification was processed), but the processing of the notification
+            // will itself enter a new notification, and so the synchronization is difficult without writing *too much code*.
+            Thread.sleep(1000);
+
+            //assertNotificationsCompleted(internalCallContext, 5);
+            final Payment updatedPayment = paymentApi.getPayment(payment.getId(), false, ImmutableList.<PluginProperty>of(), callContext);
+            Assert.assertEquals(updatedPayment.getTransactions().get(0).getTransactionStatus(), TransactionStatus.PENDING);
+        }
+
+        assertEquals(getPendingNotificationCnt(internalCallContext), 0);
+    }
+
+
     private List<PluginProperty> createPropertiesForInvoice(final Invoice invoice) {
         final List<PluginProperty> result = new ArrayList<PluginProperty>();
         result.add(new PluginProperty(InvoicePaymentControlPluginApi.PROP_IPCD_INVOICE_ID, invoice.getId().toString(), false));
@@ -441,5 +498,15 @@ public class TestJanitor extends PaymentTestSuiteWithEmbeddedDB {
             fail("Test failed ", e);
         }
     }
+
+    private int getPendingNotificationCnt(final InternalCallContext internalCallContext) {
+        try {
+            return notificationQueueService.getNotificationQueue(DefaultPaymentService.SERVICE_NAME, Janitor.QUEUE_NAME).getFutureOrInProcessingNotificationForSearchKeys(internalCallContext.getAccountRecordId(), internalCallContext.getTenantRecordId()).size();
+        } catch (final Exception e) {
+            fail("Test failed ", e);
+        }
+        // Not reached..
+        return -1;
+    }
 }