Changeset View
Standalone View
src/app/Providers/PaymentProvider.php
| Show First 20 Lines • Show All 162 Lines • ▼ Show 20 Lines | abstract class PaymentProvider | ||||
| * | * | ||||
| * @param \App\Wallet $wallet A wallet object | * @param \App\Wallet $wallet A wallet object | ||||
| * @param array $refund A refund or chargeback data (id, type, amount, description) | * @param array $refund A refund or chargeback data (id, type, amount, description) | ||||
| * | * | ||||
| * @return void | * @return void | ||||
| */ | */ | ||||
| protected function storeRefund(Wallet $wallet, array $refund): void | protected function storeRefund(Wallet $wallet, array $refund): void | ||||
| { | { | ||||
| if (empty($refund) || empty($refund['amount'])) { | if (empty($refund) || empty($refund['amount'])) { | ||||
machniak: This probably should be a case-insensitive check. | |||||
| return; | return; | ||||
| } | } | ||||
Done Inline ActionsI would throw an exceptions here. machniak: I would throw an exceptions here. | |||||
| $wallet->balance -= $refund['amount']; | $wallet->balance -= $refund['amount']; | ||||
| $wallet->save(); | $wallet->save(); | ||||
| if ($refund['type'] == self::TYPE_CHARGEBACK) { | if ($refund['type'] == self::TYPE_CHARGEBACK) { | ||||
| $transaction_type = Transaction::WALLET_CHARGEBACK; | $transaction_type = Transaction::WALLET_CHARGEBACK; | ||||
| } else { | } else { | ||||
Done Inline ActionsI would use one space before $, and no double-colon after the argument name. Descriptions should be aligned with each other. machniak: I would use one space before $, and no double-colon after the argument name. Descriptions… | |||||
| $transaction_type = Transaction::WALLET_REFUND; | $transaction_type = Transaction::WALLET_REFUND; | ||||
| } | } | ||||
Done Inline Actions$amount would have to be rounded. machniak: $amount would have to be rounded. | |||||
Done Inline ActionsI would prefer the amount to be the 1st argument, but of course it's not a strong requirement. machniak: I would prefer the amount to be the 1st argument, but of course it's not a strong requirement. | |||||
| Transaction::create([ | Transaction::create([ | ||||
| 'object_id' => $wallet->id, | 'object_id' => $wallet->id, | ||||
| 'object_type' => Wallet::class, | 'object_type' => Wallet::class, | ||||
| 'type' => $transaction_type, | 'type' => $transaction_type, | ||||
| 'amount' => $refund['amount'], | 'amount' => $refund['amount'], | ||||
| 'description' => $refund['description'] ?? '', | 'description' => $refund['description'] ?? '', | ||||
| ]); | ]); | ||||
| $refund['status'] = self::STATUS_PAID; | $refund['status'] = self::STATUS_PAID; | ||||
| $refund['amount'] *= -1; | $refund['amount'] *= -1; | ||||
| $this->storePayment($refund, $wallet->id); | $this->storePayment($refund, $wallet->id); | ||||
| } | } | ||||
| /** | |||||
| * List supported payment methods. | |||||
| * | |||||
| * @param string $type type: oneoff/recurring | |||||
| * | |||||
| * @return array Array of array with available payment methods: | |||||
| * - id: id of the method | |||||
| * - name: User readable name of the payment method | |||||
| * - minimumAmount: Minimum amount to be charged | |||||
| * - currency: Currency used for the method | |||||
| * - image|icon: An image (url) or icon (icon name) representing the method | |||||
| */ | |||||
| abstract public function paymentMethods($type): ?array; | |||||
Done Inline ActionsI think there should be static paymentMethods() method in the PaymentProvider. I.e. I'm thinking about a case when we'd like to enable methods from more that one payment provider. I.e. if we add bitcoin provider we would use Mollie and this other provider. We need a common method that returns all enabled (whitelisted) methods from all providers. This is also another reason to cache the list, and even without it, I think it would make sense to omit a request to the provider's server whenever possible. machniak: I think there should be static paymentMethods() method in the PaymentProvider. I.e. I'm… | |||||
| /** | |||||
| * Get a payment. | |||||
| * | |||||
| * @param string $paymentId Payment identifier | |||||
| * | |||||
| * @return array|null Payment information: | |||||
| * - id: Payment identifier | |||||
| * - status: Payment status | |||||
| * - isCancelable: The payment can be canceled | |||||
| * - chceckoutUrl: The checkout url to complete the payment or null if non | |||||
| */ | |||||
| abstract public function getPayment($paymentId): ?array; | |||||
| } | } | ||||
Done Inline ActionsCode duplication. I would probably create a class property with type-to-icon map. machniak: Code duplication. I would probably create a class property with type-to-icon map. | |||||
Done Inline ActionsThere's still some duplication, but attempting to deduplicate this any further will make it overall less maintainable. mollekopf: There's still some duplication, but attempting to deduplicate this any further will make it… | |||||
Done Inline ActionsSingle empty line between methods, please. machniak: Single empty line between methods, please. | |||||
Done Inline ActionsProper spacing, please. machniak: Proper spacing, please. | |||||
Not Done Inline ActionsYou ignored my comment so again, this time inline: Because it's likely that user will open both Add credit and Setup auto-payment dialogs. It would make sense to fetch the list of payment methods only once. I.e. it could return all methods with some type flag so we could filter it client-side. This also would make less requests to Mollie. I think it would be worth implementing. machniak: You ignored my comment so again, this time inline:
Because it's likely that user will open… | |||||
Done Inline ActionsI think it's more complex and all that we're saving is potentially a single request from the client to the server. For mollie I don't think we can request all types in one go (omitting the type defaults to oneoff), and we're caching the result anyways. So overall I'm not convinced the alternative of requesting all methods and filtering clientside is better at all, and the current implementation already exists. So unless you feel strongly about this I would rather not change it. mollekopf: I think it's more complex and all that we're saving is potentially a single request from the… | |||||
Not Done Inline ActionsI'll not die for it, but it looks like Mollie has an API to ask for all methods, https://docs.mollie.com/reference/v2/methods-api/list-all-methods. machniak: I'll not die for it, but it looks like Mollie has an API to ask for all methods, https://docs. | |||||
Done Inline ActionsLet's defer it to a later iteration then please. I've created a ticket for it: https://bifrost.kolabsystems.com/T423548 mollekopf: Let's defer it to a later iteration then please. I've created a ticket for it: https://bifrost. | |||||
Done Inline ActionsShould we comment out the bank transfer for now? E.g. we could enable the method in Mollie while working on bank transfers without revealing it to users on production. Or do I miss something? machniak: Should we comment out the bank transfer for now? E.g. we could enable the method in Mollie… | |||||
Not Done Inline ActionsI guess we could cache for longer, but maybe not include the exchange-rate in the cache. machniak: I guess we could cache for longer, but maybe not include the exchange-rate in the cache. | |||||
Not Done Inline ActionsI guess we could, but then I also don't know what is reasonable. A couple reuqests per hour seem unproblematic to me. mollekopf: I guess we could, but then I also don't know what is reasonable. A couple reuqests per hour… | |||||
This probably should be a case-insensitive check.