Page MenuHomePhorge

New Payment dialog structure
ClosedPublic

Authored by mollekopf on Feb 10 2021, 2:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 8:54 AM
Unknown Object (File)
Fri, Mar 22, 4:47 AM
Unknown Object (File)
Sun, Mar 17, 1:24 PM
Unknown Object (File)
Sun, Mar 10, 6:02 AM
Unknown Object (File)
Thu, Feb 29, 11:28 PM
Unknown Object (File)
Feb 14 2024, 11:19 PM
Unknown Object (File)
Feb 9 2024, 10:44 PM
Unknown Object (File)
Feb 9 2024, 7:40 PM
Subscribers

Details

Summary

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.

Test Plan

phpunit tests

Diff Detail

Repository
rK kolab
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 33721
Build 13333: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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)

This revision now requires changes to proceed.Mar 2 2021, 11:56 AM

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.

mollekopf added inline comments.
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?

mollekopf added inline comments.
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?
Just fill with currency=CHF and currency_amount=amount?

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.

mollekopf added inline comments.
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.

mollekopf marked 4 inline comments as done.

Addressed comments

machniak added inline comments.
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.

This revision now requires changes to proceed.Mar 5 2021, 10:19 AM
mollekopf marked 6 inline comments as done.

Addressed comments & properly handle the non-nullable columns

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.

machniak added inline comments.
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.

This revision now requires changes to proceed.Mar 17 2021, 11:30 AM
mollekopf added inline comments.
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

@mollekopf, it looks good. Please, fix phpstan/phpcs issues and we're good to go.

Fixed storing the currency_amount, displaying exchanged amount and centralized currency conversion

I added a nother function to do the conversion because:

  • While the calculation is not exactly complex this seemed a little cleaner and less error prone
  • This should give us a place to add a safety margin to the conversion should we decide to
machniak added inline comments.
src/app/Providers/PaymentProvider.php
211

I would use one space before $, and no double-colon after the argument name. Descriptions should be aligned with each other.

213

I would prefer the amount to be the 1st argument, but of course it's not a strong requirement.

This revision now requires changes to proceed.Mar 22 2021, 9:58 AM
mollekopf marked 2 inline comments as done.

Addressed comments

When executing some tests for Stripe:

  1. Undefined index: currency_amount {"userId":950757598,"exception":"[object] (ErrorException(code: 0): Undefined index: currency_amount at /home/alec/repos/kolab/src/app/Providers/PaymentProvider.php:185)

[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()

  1. Undefined index: currency_amount {"userId":4205758110,"exception":"[object] (ErrorException(code: 0): Undefined index: currency_amount at /home/alec/repos/kolab/src/app/Providers/PaymentProvider.php:185)

[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()

  1. Invalid currency: foo. Stripe currently supports these currencies: usd, aed, afn, all, amd, ang, aoa, ars, aud, awg, azn, bam, bbd, bdt, bgn, bhd, bif, bmd, bnd, bob, brl, bsd, bwp, bzd, cad, cdf, chf, clp, cny, cop, crc, cve, czk, djf, dkk, dop, dzd, egp, etb, eur, fjd, fkp, gbp, gel, gip, gmd, gnf, gtq, gyd, hkd, hnl, hrk, htg, huf, idr, ils, inr, isk, jmd, jod, jpy, kes, kgs, khr, kmf, krw, kwd, kyd, kzt, lak, lbp, lkr, lrd, lsl, mad, mdl, mga, mkd, mmk, mnt, mop, mro, mur, mvr, mwk, mxn, myr, mzn, nad, ngn, nio, nok, npr, nzd, omr, pab, pen, pgk, php, pkr, pln, pyg, qar, ron, rsd, rub, rwf, sar, sbd, scr, sek, sgd, shp, sll, sos, srd, std, szl, thb, tjs, tnd, top, try, ttd, twd, tzs, uah, ugx, uyu, uzs, vnd, vuv, wst, xaf, xcd, xof, xpf, yer, zar, zmw, eek, lvl, svc, vef, ltl {"userId":3073936362,"exception":"[object] (Stripe\\Exception\\InvalidRequestException(code: 0)... at /home/alec/repos/kolab/src/vendor/stripe/stripe-php/lib/Exception/ApiErrorException.php:38)

[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()

This revision now requires changes to proceed.Mar 23 2021, 12:25 PM
mollekopf edited the test plan for this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Mar 30 2021, 3:54 PM
This revision was automatically updated to reflect the committed changes.