Refactor: Generialize payment initiation#1385
Conversation
This changes attempts to consolidate Payment initiation in on central location PaymentInitialization. It also introduces another payment method with sold appliance.
dmohns
left a comment
dmohns
left a comment
There was a problem hiding this comment.
I'm still struggling a little bit to follow the payment processing flow. Both, with and without your changes 😅
After coming back to this multiple times, I think one of the confusions for me lies in the redundancy between Transaction Providers (Providers here mean Laravel Providers, not Payment Providers) and Transaction Services
For Providers we already have a Contract ITransactionProvider. For Services we have AbstractPaymentAggregatorTransactionService
Now, looking at ITransactionProvider it seems like it was intended to already cover a lot of aspects that you are trying to add here.
For example it has
public function init(BasePaymentProviderTransaction $transaction): void;Do you think it within the scope of this PR to clean up this redundancy?
If I understand your comment correctly
Yes, this is very much part of the chain of refactor we are ultimately aiming to achieve.
To be fair, this only PR tries to make it possible to bring all payment initiation steps in one place. My hope is that this will open up better abstractions that could simplify the entire flow, which is still rather complex. |
|
Hey @beesaferoot coming back to this one. Can you resolve merge conflicts? |
dmohns
left a comment
dmohns
left a comment
There was a problem hiding this comment.
Couple of comments, hope they make sense
| if ($providerId === $this::CASH_TRANSACTION_PROVIVER) { | ||
| dispatch(new ProcessPayment($companyId, $result['transaction']->id)); | ||
| } |
There was a problem hiding this comment.
Having this here feel odd. Can we move this into CashTransactionService::initializePayment(...). I.e. for a CashTransaction if it's get "initialised" it get's immediate processed. By design of cash transaction
|
LGTM, can you fix the |
2 participants
This changes attempts to consolidate Payment initiation in on central location PaymentInitialization. It also introduces another payment method with sold appliance.
Makes progress on #1243
Brief summary of the change made
Are there any other side effects of this change that we should be aware of?
Describe how you tested your changes?
Pull Request checklist
Please confirm you have completed any of the necessary steps below.