Refactor: Unify transaction flow#1258
Conversation
|
Hey @beesaferoot From what I understand, currently, when using We need to add a migration for this right? Note that the seeded data doesn't have this problem. |
Didn't realize this side effect until you mentioned it, but yes, it is intended. I don't think we need a migration, the token is rightly tied to the right appliance (device) having the same result as previous cash processing logic. |
I understand it's not strictly required. But I think I'd still like to have it. Else we will be in the situation where some (older) tokns don't have |
With our use of Token with relation to transactions, I can't foresee issues entirely based on missing However, I could look into this to be on the safe side. To populate the missing device ids don't we need to still resort to the action above? |
|
Added a migration to backfill empty device ids on the Token table. |
dmohns
left a comment
dmohns
left a comment
There was a problem hiding this comment.
Some things seem broken with Non-PayGO devices. When I try to register a cash payment for non paygo this happens:
On the Demo it works fine, and registers a log here
|
Apart from my message above, everything seems to work! Nice, nice. I will admit that I don't fully understand everything, the different relation between all the jobs and events involved in processing. But hopefully this will become cleaner with the reworker transaction logic 🤞 |
Thanks for pointing this out, will look into it. |
We previously duplicated transaction processing for Cash payments and "normal" transaction flow. This commit unifies them thereby simplying the overall flow.
beesaferoot
force-pushed
the
unify-transaction-flow
branch
from
f8aa821 to
7787b34
Compare
|
The Transaction Payments for Non Paygo appliances have now been addressed. |
| public function getPaymentForAppliance(Request $request, AppliancePerson $appliancePerson): array { | ||
| $creatorId = auth('api')->user()->id; | ||
| $this->paymentAmount = $amount = (float) $request->input('amount'); | ||
| $applianceDetail = $this->appliancePersonService->getApplianceDetails($appliancePerson->id); | ||
| $this->validateAmount($applianceDetail, $amount); | ||
| $deviceSerial = $applianceDetail->device_serial; | ||
| $applianceOwner = $appliancePerson->person; | ||
| $companyId = $request->attributes->get('companyId'); |
There was a problem hiding this comment.
I think a lot of confusion comes from this function call. It is called getPaymentForAppliance which is highly, highly misleading.
What this does, it creates a Cash Transaction for an Appliance, and thus creates a tight coupling.
Could you move this function out here into ... for example ... ApplianceCashTransactionService.php (or something similar).
This way, we would remove the coupling between CashTransaction and AppliancePaymentService. AppliancePaymentService would then only deal with the logic of handling Appliance payments, independant of the provider.
There was a problem hiding this comment.
As discussed, a valid alternative would be to move this to the controller directly (assuming this the only place it's getting called.)
|
Updated as discussed earlier. Please have a look. @dmohns |
dmohns
left a comment
dmohns
left a comment
There was a problem hiding this comment.
Great work!!! Have been doing some testing locally and everything seems to work as expected.
None-the-less, it's a big change, I think we would need a stand alone release just for this.
2 participants
We previously duplicated transaction processing for Cash payments and "normal" transaction flow. This PR unifies them, thereby simplifying the overall flow.
Closes #1257
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.