search

commercetools / commercetools-payone-integration

Integration between commercetools eCommerce API and Payone payment service provider API.
https://dev.commercetools.com/
MIT License
7 stars 8 forks source link

HTTP endpoint for payment handling #9

Closed fhaertig closed 8 years ago

fhaertig commented 8 years ago

As a CTP user I want to send the id of a payment object to the integration service to trigger the processing of pending transactions.

Find API specification here or view in Swagger editor.

Server Response Status Code

Response status 403 (Forbidden) is out of scope - implementation extracted to issue #45 because there is no optional ip whitelist and the like, yet.

sesamzoo commented 8 years ago

@nkuehn Is status code 403

The configuration of the service does not allow this request (e.g. not on optional IP whitelist, optional required authentication not passed).

in the scope of the first milestone? If yes (and in general), what is the exact list of conditions to be checked?

http://editor.swagger.io/#/?import=https://raw.githubusercontent.com/nkuehn/payment-integration-specifications/master/Payment-Integration-Api.swagger.yml

sesamzoo commented 8 years ago

@nkuehn According to the specification the response for status code 200 or 202 does not contain any data.

Could you please confirm that in case of redirect (credit card, Paypal,...) the client/checkout implementation has to get the redirect URL from the payment object by itself?

nkuehn commented 8 years ago

about 403: No, we don't have IP whitelisting or other auth on the MVP list and that means 403 will not occur until that changes (that's why the "Optional" in the description).

about 202: thanks for the reminder, @cneijenhuis have forgotten to agree on the response structure. My proposal is in this branch: https://github.com/nkuehn/payment-integration-specifications/blob/full-api-with-responses/Payment-Integration-Api.swagger.yml

nkuehn commented 8 years ago

update: I have now done a minimal response object for the 200x cases in the master branch of the payment-specs. If, then it may need to be extended but these three fields are safe as a status description that does not leak relevant info (which the full redirectUrl would have done).

sesamzoo commented 8 years ago

@nkuehn Regarding the 20X response body:

handledTransactionState: If a Transaction was handled, the value of the state field of the Transaction that was handled.

Please specify "If a Transaction was handled".

Scenarios that come to my mind (and influence implementation to detect handled transaction(s)):

Or would the following approach be sufficient:

cneijenhuis commented 8 years ago

@nkuehn I have said it before: I still have not heard an argument that invalidates what we had defined back in November (3 possible return types, configured on server side: None, Transaction State, Payment). Therefore I don't see a need to re-do/decide for something else.

@sesamzoo The use case is: The client adds a transaction (so, at the end of the array) and calls handle. The transactionState returned should be of that transaction. So in all cases except that someone else (CTP API client or notification processor) adds another transaction, using the last transaction would be fine. Maybe a good approach is to force the client to send the id of the transaction he wants to know the state of. So, if a notification comes in, he will still get the state he is interested in.

sesamzoo commented 8 years ago

@nkuehn I like the idea of @cneijenhuis to require the transaction ID in the request. Would be clear and simple to implement. May I assume that the transaction ID gets added as mandatory query parameter? Or could another path like /handle/payments/{paymentId}/transactions/{transactionId} be an alternative?

cc @fhaertig

sesamzoo commented 8 years ago

@nkuehn Would return type none as mentioned above be sufficient for the MVP?

fhaertig commented 8 years ago

@sesamzoo we could also provide only the path '/handle/transactions/{transactionId}', just as an addition

I'm not against the idea behind that, but in fact it would transfer a whole lot of logic out of the adapter and increases the effort for the shop, because it needs to handle the correct sequence of transactions then. Depends on how "convenient" the adapter should be...

cneijenhuis commented 8 years ago

I don't understand - the correct sequence is always "handle the oldest transaction in state Pending".

The client can not force the adapter to work on a different transaction - but he'll get the status of that transaction back. I.e. if the client first does Authorization and then Charge while the Auth is still pending (e.g. waits for a notification from the PSP), he will get Pending back for the Charge as it can not yet be processed. But he will not be able to trigger the Charge transaction with that.

sesamzoo commented 8 years ago

@cneijenhuis please see 4.2.7 Sample: preauthorization/capture, REC with credit note from the PAYONE server API documentation where "handle the oldest transaction in state Pending" is not correct: The Charge transaction of the invoice is still Pending (buyer hasn't paid anything, yet) while a Refund transaction (credit note issued by merchant) is requested (and completed).

Another scenario (although not that common) that comes to my mind: multi-capture because shipment was split -> the merchant probably wouldn't want the 2nd Charge transaction to be blocked until the 1st Charge transaction is completed, i.e. paid.

sesamzoo commented 8 years ago

@fhaertig I don't think that it would move any more logic to the shop, because the shop has to create the transactions in proper order anyway. As I see it, there is no real difference in logic but in explicitness

create payment P -> create transaction A -> handle payment P (to process transaction A) -> create transaction B -> handle payment P (to process transaction B)

vs.

create payment P -> create transaction A -> handle transaction A of payment P -> create transaction B -> handle transaction B of payment P

fhaertig commented 8 years ago

@sesamzoo the scenarios you described are exactly the ones I had in mind. The thing I got wrong was: if we only get the transaction id, we could validate the basics and directly fire the request from it. If there's an error - not our fault. But a more fail-fast approach makes certainly more sense, e.g. checking the other transactions in the payment.

nkuehn commented 8 years ago

Given that "handle" is defined as just speeding up what would have happened on a Message at the Message Endpoint anyways the question whether moving to "Handle by transaction" would help depends on what happens on a Transaction-Related Message (e.g. new payment transaction).

cneijenhuis commented 8 years ago

@sesamzoo While Payone may support that scenario, other PSPs don't (e.g. this specific scenario is not supported by Stripe).

We want to ease the implementation for the client/shop and offer an abstraction from the specifics of PSPs. "Handle the oldest transaction in state Pending" will be supported by all PSPs (if the transactions make sense).

That Payone supports Refund before Charge may be a nasty surprise when a shop switches from a Stripe adapter to the Payone adapter.

Therefore, the correct sequence - as specified by CT, not by Payone, and as I have implemented it - is "handle the oldest transaction in state Pending" :wink:

sesamzoo commented 8 years ago

@nkuehn The transaction ID is required for the HTTP endpoint only. The scheduled job would not need to care about the transaction because it doesn't have to notify anyone about which transaction was processed.

nkuehn commented 8 years ago

@cneijenhuis in Credit Card (=Stripe's functional scope) you're right that "only the oldest pending" is enough.

But in Invoice or Cash Advance (Vorkass) it is a real-world process that the merchant e.g. triggers a refund before the money has arrived (i.e. the initial Charge is still Pending when the Refund is triggered because the Invoice has not been paid yet).

There is a related issue with #42 , but there it is an edge case (two capture transactions done in very quick order).

nkuehn commented 8 years ago

@sesamzoo OK, i.e. the design of the mini-API of the service does impact the implementation quite a bit here.

I talked to Christoph today and he agrees that it's okay now for the service to just implement the "NONE" case on its one API (no response body). We also considered dropping the "STATUS" variant in the requirements in favor of "all or nothing".

sesamzoo commented 8 years ago

We finally found the reason why concurrent modification isn't reliably handled, yet.

The following scenario (starting with a payment with a Pending Authorization transaction) leads to incorrect behavior and response status code in case of a handle payment request that interferes with a ScheduledJob run:

Time Payment Version TransactionState Action by ScheduledJob Action by HTTP endpoint
0 1 Pending fetch payment -
1 1 Pending - fetch payment
2 1 Pending try to add interaction request, because no recent interaction response/redirect in payment -
3 1 Pending - try to add interaction request, because no recent interaction response/redirect in payment
4 2 Pending receive updated payment -
5 2 Pending - receive concurrent modification exception
6 2 Pending send request to PAYONE -
7 2 Pending - fetch payment again, report status code 200 assuming payment already properly handled concurrently, because version is greater than that of initial fetch result due to adding the interaction request
20 2 Pending process response APPROVED from PAYONE -> try to
  • add interaction response and
  • change state to Success and
  • set amountPlanned
as a bundle of update actions		 -
23 4 Pending - -
25 5 Success - -

If a client (f.i. the Authorization test) fetches the payment (after response status OK for the request to handle it) sometime between

Update: I am not sure whether the state at time 23 could actually be fetched by a client. Probably not, according to statements of @cneijenhuis. But the issue remains if the line with time 23 wouldn't exist: If a client fetches the payment (after response status OK for the request to handle it) sometime between 7 < t < 25, it will report/receive expected status code (false positive) but unexpected amountPlanned and unexpected transaction state.

Good news is, that we (believe to) have found a solution that we are implementing right now.

cc @nkuehn @cneijenhuis @fhaertig

cneijenhuis commented 8 years ago

My original implementation: https://github.com/commercetools/commercetools-payone-integration/commit/2a9f760b21c62b3ecaab3aad3e7aa40f077d9632#diff-a684bbe1ce55a8f4dea792d69dd8397aR62 did not return 200 but run the processing again. It also made a difference between the java.util.ConcurrentModificationException and the sphere.sdk.ConcurrentModificationException.

By switching to the SDK CME, you are now returning 200 where before you would have failed/500. Anyway, in 5 the correct action for the HTTP endpoint when receiving the sphere.sdk.ConcurrentModificationException (was not there in my implementation yet) is fetch payment again, run processor again. Then 7 would have thrown what was the java.util.ConcurrentModificationException, and the HTTP endpoint would have looped until either the timeout was over or the payment was finalized (was there in my implementation).

sesamzoo commented 8 years ago

@cneijenhuis ok, your intend was to process again, but the ResultProcessor was supposed to just extract the response status code from the handled payment, i.e. your solution also had the same problem (because the result processor simply returned 200 by default). The dispatcher has to be invoked again.

What was the reason to mix java.util's and the SDK's ConcurrentModificationException?

Returning 500 in case the SDK reports another concurrent modification while the dispatcher tries to process the payment (again), doesn't feel right.

For the ResultProcessor which would just have checked the latest payment version whether the transaction (which exactly? would be the question, again) was handled that would probably have been reasonable. But we skipped the ResultProcessor due to extra effort to

We (lacking the ResultProcessor, anyway) think that re-invoking the PaymentDispatcher is the solution. The logic to find out which (if any) transaction shall be handled is already implemented there. I know you suggested to let the *Executor classes also implement the ResultProcessor interface, but the extra information/logic that would have been required to find the transaction of interest is already there in the PaymentDispatcher.

cc @fhaertig

sesamzoo commented 8 years ago

@cneijenhuis we switched back to throw/catch java.util.CME because the SDK-CME seems to make more sense when thrown with the respective HTTP request/response.

If a SDK-CME is thrown during payment dispatching/handling, it will be wrapped in a java.util.CME so that another dispatch attempt is possible.

cneijenhuis commented 8 years ago

Ok, https://github.com/commercetools/commercetools-payone-integration/commit/b7d578d4f0a3c48d8fda6660a36d22ac5479b79a LGTM.

What was the reason to mix java.util's and the SDK's ConcurrentModificationException?

Because it's a different type of error, the SDKs exception is specific for failing updates (which is not what happened). Using java.util.CME is probably not fitting here as well, I just used it for my prototype as it was somewhat related... It's probably better to use a domain specific exception.

sesamzoo commented 8 years ago

I guess, meanwhile we got to the same conclusion. step 1 was to not misuse the SDK-CME and finally we should replace the java.util.CME as well. and either introduce a third option to choose when looking up ConcurrentModificationException or find a different name that says the same...

(When I was just replacing the import and could still use the default constructor I thought that it would be ok. After having a look at the SDK's exception implementations, we decided to switch back.)