search

Portalum / Portalum.Zvt

A .NET Zvt Library for Payment Terminals (C#)
MIT License
88 stars 36 forks source link

asynchronous and partial completion of a payment #23

Closed rettier closed 2 years ago

rettier commented 2 years ago

Summary

An unattended ECR has the problem where it has to decide if it finishes the transaction with the payment terminal first, and then issues the goods to the customer, or vice versa. For vending machines with mechanical parts, or other cash registers where the failure is an option the workflow according to the ZVT standard is, that the goods are issued first and only then the transaction is finalized in the PT. If the issuing of goods fails, an auto reversal is triggered. This is essential in unattended use cases (compared to attended ones) because the card does not need to be presented again to the PT.

For us such failures are an option and the current implementation does not support a callback for issuing goods. This PR is a proposal to implement such a callback.

Changes

This branch builds on #21 (required) and #22 (optional) so the diff is larger then necessary. For the actual changes you can only view the last commit (5bbd3b8098b4e5f2f747017137e1bd7cc3d87ff4).

In order for the ProcessData function of the ZvtCommunication class to decide if a successful payment was made, the processing functions were refactored to return a ProcessData object which includes the State aswell as the parsed message. As mentioned in the https://github.com/Portalum/Portalum.Zvt/issues/11#issuecomment-1161685860 otherwise it is hard to determine if and when a successful payment has been made.

The first prototype did not differentiate between response types, so AskForCompletion was always called, regardless if it was a payment or other command. With the changes in the processing pipeline the ProcessData can now check if it is a successful status information.

I have Introduced an additional callback which is currently only used for payments StartAsyncCompletion. This is called once per payment, as otherwise users need to be aware that StatusInformation's with ErrorCode == 0 will be called multiple times. This avoids that each user needs to implement the "start issuing goods only once" gate in their code. A not yet added documentation should clearly outline that this function should be used when using async completion.

Open questions

Tests

Real World Tests: Worldline VALINA from Card Complete

Testcases: i have included 3 tests for asynchronous completion, and added two for non-asynchronous completion which show that the async callbacks will not be called in this case.

Backwards Compatibility

From the protocol side, nothing should change as long as the GetAsyncCompletionInfo is not registered. The PamynetAsync method also avoids setting the additional parameter if asynchronous completion is not used.

From the code side the main internal change is that now the processed state is not passed as a State enum or a boolean, but always as ProcessData object which includes the State and the parsed message.

tinohager commented 2 years ago

Do we still have discussion needs in the Open Questions section? Or should the implementation fit so far for the merge?

rettier commented 2 years ago

About the open question: i have read the diff wrong, your prototype is using the same timeout behaviour. This voids the question, there is no difference as previously mentioned.

The PR is fit to merge from our side, we have not seen any issues or made further changes within the last week using this code.

rettier commented 2 years ago

i don't know what the issue here is, but i am having trouble getting the ci to run through lately. Do you have a different ci setup, i could not replicate the issues in my repository.

tinohager commented 2 years ago

I need to look at this in detail on the quick I can not determine the reason. But it will take a few days until I find time for it.

tinohager commented 2 years ago

I think it should fit now.

Due to the changeover from private to protected, we now have many places where the summary is missing. Is protected really needed everywhere?

image

rettier commented 2 years ago

Thanks for your work on the testcases!

I have reduced the protected / virtual members to what i think is reasonable, and what we currently use to catch some special cases. Most other members can be intercepted or overwritten in the constructors. For the remaining ones i have added documentation.

tinohager commented 2 years ago

@rettier I just tried to change the amount to a lower amount with our test terminal but had no success. Have you tested this variant with you?

rettier commented 2 years ago

@tinohager to be honest i have never, and i can observe the same thing on our Worldline VALINA (Card Complete), i am receiving 6C (=Abort) as a respones to the change amount message.

https://github.com/Portalum/Portalum.Zvt/issues/11#issuecomment-1079999689 mentioned it working, looking at the zvt standard i cannot see what we are doing wrong.

image our message seems fine: Portalum.Zvt.TcpNetworkDeviceCommunication[0] SendAsync - 84-9D-07-04-00-00-00-00-00-10

CardComplete is also not mentioning any differences to the standard: image

tinohager commented 2 years ago

@rettier I have just consulted with CardComplete. I was told that the function of changing the amount is not supported.

tinohager commented 2 years ago

@rettier So from my point of view we are actually so far through would like to incorporate the pull request into the main code next week. 👍🏻

Before that I would just like to discuss the naming of the events again. Whether the names make sense. I don't know if Async makes sense in the event name. And that we have no verbs here. naming convention and style

Existing events

New events

rettier commented 2 years ago

@tinohager great, thanks for working with us on that PR. I'm glad your project exists, and even more so that we were able to contribute something to it.

I am fine with a overhaul of the event names and i do like your proposal.

I was considering "issue goods" as the base for event names. But looking at the current event names the events are named what has happened, not what the effect is.

i.e. ReceiptReceived and not PrintReceipt

thus naming them IssueGoods / GetIssueGoodsStatus would be equally bad names.

tinohager commented 2 years ago

@rettier a new nuget package is available