This implements the new payment dialog structure as requested in https://bifrost.kolabsystems.com/T380342.
No new payment methods are enabled yet, so impact should be minimal.
machniak |
Restricted Project |
This implements the new payment dialog structure as requested in https://bifrost.kolabsystems.com/T380342.
No new payment methods are enabled yet, so impact should be minimal.
phpunit tests
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
src/app/Payment.php | ||
---|---|---|
35 | It would be good to have some logic to make sure the currency code is always upper-case'd. I think different payment providers may use different casing. It could be done in an observer, but probably a mutator would be better here. See App\Domain::setNamespaceAttribute() for an example. | |
src/resources/vue/Widgets/PaymentLog.vue | ||
14 | The list displays unfinished credit card/paypal payments. I'm not sure we should do this. I'm thinking about users having problem with payments. Soon after payment failes the return to the wallet page and see pending payments they could do not much about, and after a while (about 15 minutes) they will disappear which might be even more confusing. | |
18 | This list will contain only positive values, maybe we should not use .text-success. | |
23 | payments -> pending payments. 5 -> 4. | |
45 | We do the request twice. Once when the wallet page is mounted, and second when user clicks on the Pending payments tab. I guess the other option would be to display the tab always, which might not be what we want. | |
src/tests/Browser/PaymentMollieTest.php | ||
133 | It would be good to have some assertions on the payment methods list. | |
254 | Also assert on expected methods for auto-payment. | |
src/tests/Feature/Controller/PaymentsMollieTest.php | ||
885 | We test the webhook in other places, so I guess you could just change the payment status in DB to make it simpler/faster. All we test here is that the paid payment is not included in the list. | |
904 | We should probably have this test also in PaymentsStripeTest.php | |
910 | You can do get() and put the param in the url. get('api/v4/payments/methods?type=' . PaymentProvider::TYPE_ONEOFF) |
Also, 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.
src/app/Providers/PaymentProvider.php | ||
---|---|---|
328 | There's still some duplication, but attempting to deduplicate this any further will make it overall less maintainable. | |
src/resources/vue/Wallet.vue | ||
29 | A way to group things in vue that will not appear in the finally generated html. | |
src/resources/vue/Widgets/PaymentLog.vue | ||
45 | Yeah, this could be improved by adding an extra controller method that only checks if there are any payments at all, but I figured this is good enough. | |
src/tests/Browser/PaymentMollieTest.php | ||
254 | I think we're already testing this on the controller, so I wouldn't test all methods here again. Otherwise we'll have to adjust in all places whenever anything changes. Do you still want this tested? |
src/database/migrations/2021_02_23_084157_payment_table_add_currency_columns.php | ||
---|---|---|
23 | Not sure if necessary, but I suppose would make future queries simpler? | |
src/resources/vue/Wallet.vue | ||
403 | I like the new version better (surprise! ;-) ), but I don't suppose it matters much. You want the titles to be "Top up your wallet" ? | |
407 | Maybe, but I don't know how. | |
src/resources/vue/Widgets/PaymentLog.vue | ||
14 | Before we just displayed nothing while the pending payment was not credited, so it seems not any worse to me at least, right? | |
18 | It looked better to me this way, and provides some consistency with other lists of currency amounts (where we might have negative values). |
src/database/migrations/2021_02_23_084157_payment_table_add_currency_columns.php | ||
---|---|---|
23 | Yes. | |
src/resources/vue/Wallet.vue | ||
403 | I liked the old titles. It was "Top up your wallet" and "Add auto-payment". So, they were related to the button pressed, not the dialog content which is self-exlanatory. | |
407 | See how addLoader() in app.js works. and maybe add an argument to make addition of .small class optional. It might be enough. | |
src/resources/vue/Widgets/PaymentLog.vue | ||
14 | If a new feature adds to the confusion then it is worse. Isn't it? Maybe someone else should take a look at this part. Maybe not Jeroen, he's attitude is "let's push it and see" ;) | |
45 | That would be nice. Especially this method could not have to connect to the payment provider. So, it would be faster just because of that. | |
src/tests/Browser/PaymentMollieTest.php | ||
254 | Yes. We want the vue code tested. |
src/resources/vue/Widgets/PaymentLog.vue | ||
---|---|---|
14 | I think not having a placeholder while a payment is in flight is also confusing (since the old balance would still be reflected), so I think we should be listing all pending payments, not depending on the type. A failed payment should give some sort of feedback, but removing it from the pending payments list makes sense to me. A possible alternative is to mark it as failed in the list of pending payments, but then pending stops making sense. The Transactions log is also not ideal because I don't think we have a corresponding record anywhere. So maybe the way to address your concern is to add some sort of notifier that the last payment failed (similar to what we do for failed auto-payments)? I suppose this could be done using another api call that queries the payments. |
src/app/Http/Controllers/API/V4/PaymentsController.php | ||
---|---|---|
255 | We must check if the payment belongs to the wallet. Otherwise users will be able to cancel other users' payments, assuming they know the payment id. | |
331 | It will work for now, but this should be a method from the mandate. | |
440 | Sorting by id does not make sense, it is not an autoincrement field. Use created_at column. | |
src/app/Providers/Payment/Mollie.php | ||
237 | These two lines could be replaced with $db_payment = Payment::find($paymentId); | |
src/app/Providers/Payment/Stripe.php | ||
553 | I know we do not use Stripe right now, but if we ever wanted to switch to it how do we know that this needs to be implemented first? | |
src/app/Providers/PaymentProvider.php | ||
202 | This probably should be a case-insensitive check. | |
374 | You 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. |
src/app/Providers/PaymentProvider.php | ||
---|---|---|
374 | I 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. |
src/app/Http/Controllers/API/V4/PaymentsController.php | ||
---|---|---|
259 | I think this should be just $this->errorResponse(404); | |
267 | This $result will overwrite the success $result above. Also, 'failure' => 'error'. Also, in case of an error it should defined 'message' property and it should not use default code 200. As this route is commented-out and not tested you probably could also just comment the method body and leave it for later. | |
src/app/Providers/Payment/Mollie.php | ||
43–44 | Mention methodId argument here. | |
127 | Mention methodId here. | |
611 | Looks to me like it always returns an array and never null. | |
src/app/Providers/PaymentProvider.php | ||
315 | Should 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? | |
374 | I'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. | |
384 | I guess we could cache for longer, but maybe not include the exchange-rate in the cache. |
src/app/Providers/PaymentProvider.php | ||
---|---|---|
374 | Let's defer it to a later iteration then please. I've created a ticket for it: https://bifrost.kolabsystems.com/T423548 | |
384 | I guess we could, but then I also don't know what is reasonable. A couple reuqests per hour seem unproblematic to me. |
Addressed comments, removed payment methods from whitelist that we don't currently offer
Fixed storing the currency_amount, displaying exchanged amount and centralized currency conversion
I added a nother function to do the conversion because:
When executing some tests for Stripe:
[stacktrace]
#0 /home/alec/repos/kolab/src/app/Providers/PaymentProvider.php(185): Illuminate\\Foundation\\Bootstrap\\HandleExceptions->handleError()
#1 /home/alec/repos/kolab/src/app/Providers/Payment/Stripe.php(89): App\\Providers\\PaymentProvider->storePayment()
#2 /home/alec/repos/kolab/src/app/Http/Controllers/API/V4/PaymentsController.php(70): App\\Providers\\Payment\\Stripe->createMandate()
[stacktrace]
#0 /home/alec/repos/kolab/src/app/Providers/PaymentProvider.php(185): Illuminate\\Foundation\\Bootstrap\\HandleExceptions->handleError()
#1 /home/alec/repos/kolab/src/app/Providers/Payment/Stripe.php(205): App\\Providers\\PaymentProvider->storePayment()
#2 /home/alec/repos/kolab/src/app/Http/Controllers/API/V4/PaymentsController.php(230): App\\Providers\\Payment\\Stripe->payment()
[stacktrace]
#0 /home/alec/repos/kolab/src/vendor/stripe/stripe-php/lib/Exception/InvalidRequestException.php(35): Stripe\\Exception\\ApiErrorException::factory()
#1 /home/alec/repos/kolab/src/vendor/stripe/stripe-php/lib/ApiRequestor.php(189): Stripe\\Exception\\InvalidRequestException::factory()
#2 /home/alec/repos/kolab/src/vendor/stripe/stripe-php/lib/ApiRequestor.php(151): Stripe\\ApiRequestor::_specificAPIError()
#3 /home/alec/repos/kolab/src/vendor/stripe/stripe-php/lib/ApiRequestor.php(489): Stripe\\ApiRequestor->handleErrorResponse()
#4 /home/alec/repos/kolab/src/vendor/stripe/stripe-php/lib/ApiRequestor.php(120): Stripe\\ApiRequestor->_interpretResponse()
#5 /home/alec/repos/kolab/src/vendor/stripe/stripe-php/lib/ApiOperations/Request.php(63): Stripe\\ApiRequestor->request()
#6 /home/alec/repos/kolab/src/vendor/stripe/stripe-php/lib/ApiOperations/Create.php(25): Stripe\\ApiResource::_staticRequest()
#7 /home/alec/repos/kolab/src/app/Providers/Payment/Stripe.php(200): Stripe\\Checkout\\Session::create()
#8 /home/alec/repos/kolab/src/app/Http/Controllers/API/V4/PaymentsController.php(230): App\\Providers\\Payment\\Stripe->payment()
#9 /home/alec/repos/kolab/src/vendor/laravel/framework/src/Illuminate/Routing/Controller.php(54): App\\Http\\Controllers\\API\\V4\\PaymentsController->store()