Page MenuHomePhorge

An initial implementation of transactions.
ClosedPublic

Authored by vanmeeuwen on Mar 31 2020, 11:59 AM.
Tags
None
Referenced Files
F11586720: D1102.id3409.diff
Thu, Mar 28, 3:37 PM
F11586021: D1102.id3283.diff
Thu, Mar 28, 1:43 PM
F11585442: D1102.id3412.diff
Thu, Mar 28, 11:54 AM
Unknown Object (File)
Sun, Mar 24, 4:52 PM
Unknown Object (File)
Sun, Mar 17, 7:01 AM
Unknown Object (File)
Tue, Mar 5, 12:39 PM
Unknown Object (File)
Tue, Mar 5, 12:39 PM
Unknown Object (File)
Tue, Mar 5, 12:39 PM
Subscribers
Restricted Project

Details

Reviewers
machniak
Group Reviewers
Restricted Project
Commits
rKad257f28b608: An initial implementation of transactions.
Summary

This is functionally the audit log of the financial bits of the new hkccp, and should transcribe all transactions for a wallet -- credit & debit line items.

Test Plan

None

Diff Detail

Repository
rK kolab
Branch
dev/transactions
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 29488
Build 10789: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
src/app/User.php
581 ↗(On Diff #2866)

These could be __toString(), but if you don't like magic methods then it's ok.

This revision now requires changes to proceed.Mar 31 2020, 12:32 PM
vanmeeuwen added inline comments.
src/app/Http/Controllers/API/PaymentsController.php
114 ↗(On Diff #2866)

I'm actually thinking of extending the transaction table with a 'type' string, such that we can also transcribe 'User account jane@kolab.org was created (by john@kolab.org)', etc.

118 ↗(On Diff #2866)

Yes and yes.

220 ↗(On Diff #2866)

The ->mollie_status field is what is updated, just like it was for Payment.

The duplicate record is indeed the accreditation against the wallet -- but I think that is still needed in that the underlying financial transactions with references to Mollie/Stripe/Paypal customer IDs and the likes are receipt details.

One thing more. I think this table would also contain one-off bonus/penalty entries. So, that would be another reason why it should use negative amount values.

Also, maybe we should make the payment provider metadata unified in some way. That it's not depended on the provider. We could have provider, provider_status , provider_id, or sth like that instead of mollie_*. But still I'd expect some unified status value of the operation.

  • The $id parameter is supposed to be a string
  • A second proposal to transactions
machniak added inline comments.
src/app/Console/Commands/WalletTransactions.php
66

I would remove the date for "sub-transactions". It will always be the same as the main transaction, right?

src/app/Observers/EntitlementObserver.php
128

Will this work properly when updatedAt->day is e.g. March 30. Could we just calculate $pricePerDay as 1/30 of the monthly cost?

140

I see similar code in a few places. Probably adding Entitlement::createTransaction($type, $amount) method would be useful.

src/app/Observers/TransactionObserver.php
20

This is potentially biggest table in the system. Maybe it would make sense to do normal bigint autoincrement identifier.

src/app/Transaction.php
52

We call entitlement() so many times that we should cache it (in-memory) here or do some magic.

68

I would propose to create public class constants for these labels.

86

Why syntax with quotes+brackets?

src/app/Wallet.php
83

Put it as a first check, it's the less expensive one.

178

When displaying transaction log it would be nice to display wallet balance after the operation. My bank for example does this. So, I think it would make sense to store that information with the on-wallet transaction entries (credit/debit/award/penalty).

188

This probably would be better to do with a single sql query.

src/database/migrations/2020_05_21_080131_create_transactions_table.php
30

Some indexes will be needed here. ONe on object_id+object_type, second on transaction_id.

This revision now requires changes to proceed.May 22 2020, 5:06 PM
vanmeeuwen marked 6 inline comments as done.

OK, I read no objections against the principles underpinning this implementation, but there are a few things that need fixing.

src/app/Console/Commands/WalletTransactions.php
66

Sure, this is more to show us, I suppose this is the structure in the UX:

Top-level is just actual debit/credit, zoom-in is individual entitlements.

src/app/Observers/EntitlementObserver.php
128

If $daysLeftInLastMonth ends up 0, the $dayOfThisMonth is also 1. From 03/30 to 04/01, is one day (or two days, edges included, but that's not what we're doing).

I would argue that it does work properly.

140

Yes, a better version of these de-duplicating code would indeed improve the overall consistency.

src/app/Observers/TransactionObserver.php
20

I intend to expire entries older than 6 months, to an archive table.

I'm not sure to which extent this table would grow compared to say, entitlements.

In either case, we'll know when it happens.

The consideration with any primary key that could potentially be retrieved through an API call is that the target URL must be unpredictable.

I.e. if I examined my transaction log, and I got /transactions: [1, 4, 7, 10, 13], I disclose the following details;

  • The processing occurs sequentially, account after account.
  • The size of the group of SQL servers is 3 at the moment.
  • The most likely valid transaction IDs I could probe for are 1+(n*3) for 0<n<infinity.

Furthermore, it would not be unlikely that the URL to get to the individual entitlements billed in a debit transaction would have some /transactions/<id> URL.

Overall, maintaining the unpredictability of the primary key is an additional layer of security (through obscurity), making the application rely on solely access control just that little bit less.

src/app/Transaction.php
52

It should be in the SQL query cache, but it can of course also be cached "here".

Caching overall shall be a different revision, because it would principally modify a number of approaches in a large number of places wrt. to proper cache invalidation.

68

And I completely understand why -- I've already missed "credited" vs. "credit" ;-)

86

description is considered a keyword by my highlighter. It happens elsewhere for 'value' and 'key' as well, although these I believe are also different PHP or SQL implementation things.

src/app/Wallet.php
83

Sure.

178

If we wanted to in the future, we can derive either the column value or the displayed data by going backwards from "current balance", and I would like to not have to duplicate data that can potentially mismatch (because of rounding or whatever).

188

Is there an $ets->update('transaction_id', $transaction->id); that you know of?

I'll investigate, but I would not at this moment consider it a blocker -- and I don't know if $ets->update() would skip observers in a way that is similar to an Something::where('blah', $blah)->delete();.

src/database/migrations/2020_05_21_080131_create_transactions_table.php
30

Sounds fair enough.

src/app/Observers/EntitlementObserver.php
128

My point is that if $updatedAt->day is 30 (or 31) and $daysInLastMonth is 28 (or 29 or 30) we end up with $daysLeftInLastMonth < 0. It's maybe not a problem, but you've got the case for a test.

src/app/Wallet.php
178

You could, but it would be more complicated/less efficient if you want to archive older entries.

188

Yes, update() will skip observers, and for this case we don't have observers, so it's all right. You can't call it on $ets here as it's an array, not query builder, so you'd have to build a query using:

  1. Collect the transactions' identifiers: $ids = collect($ets)->pluck('id')->all()
  2. Run the query: Transaction::whereIn($ids)->update(...).
vanmeeuwen marked 6 inline comments as done.
  • Update for D1102 review
machniak added inline comments.
src/app/Entitlement.php
58

$type is a string, and the labels in comment are wrong.

src/app/Transaction.php
175

Would be better to call ->entitlement() once, The object_type check is redundant.

176

This line could be made a last line in this function. This way you would de-deduplicate code.

src/app/Wallet.php
152

Use a constant here.

168

$eTIDs do not have to be a reference, does it?

183

A contant here. And Wallet could have it's own createTransaction() method too.

This revision now requires changes to proceed.May 25 2020, 4:59 PM
vanmeeuwen marked 2 inline comments as not done.

Let us review the one single line item for the moment.

src/app/Observers/EntitlementObserver.php
128

All full months not already billed moving forward up to the point where there is not another full month left.

$daysInLastMonth is therefore 28, 29, 30 or 31.

$updatedAt, as a copy of having moved forward by monthWithoutOverflow should not yield a March date for anything having moved forward a month from January 31st. It should yield in the 28th or 29th day of the month of February.

If you think of a reason I'm wrong, please point out the flaw in my thinking.

Otherwise, yes, it is indeed a case of testing.

src/app/Entitlement.php
58

Yes.

src/app/Transaction.php
175

While I'm aware it doesn't address your remark as to the duplicate execution of the $this->entitlement() method;

I personally very much dislike the construct of the following where the clause somewhat depends on function successful, as well as not false, as well as not null, and consider it a dysfunction of the language implementation.

I.e., very much dislike this;

if ($var = function()) {
}

I would rather;

$var = function();
if ($var) {
}

In the case above, I would therefore rewrite as;

if ($this->object_type == \App\Entitlement::class) {
  $smt = $this->entitlement();

  return $smt ? $smt->wallet()->{'description'} : 'Default wallet';
}

I don't know if that's what you were aiming at though.

176

I'm not confident the only types of objects to recognize are entitlement|wallet.

I'm also not confident we will not have to insert log statements in between the retrieval and the return -- hence the assignment to the function-local variable.

src/app/Transaction.php
175

My version would probably looked like this:

if ($entitlement = $this->entitlement()) {
    $description = $entitlement->wallet->{'description'};
} elseif ($wallet = $this->wallet()) {
    $description = $wallet->{'description'};
}

return !empty($description) ? $description : 'Default wallet';
vanmeeuwen marked 2 inline comments as done.

Rebase on current master;

  • The $id parameter is supposed to be a string
  • A second proposal to transactions
  • Update for D1102 review
  • Resolve some comments for D1102
vanmeeuwen marked 13 inline comments as done.
vanmeeuwen added inline comments.
src/app/Wallet.php
168

No, but why copy if we don't have to?

My thinking is that it is potentially a very large list.

src/app/Wallet.php
168

PHP is smart. It will not create a copy if you do not modify the local variable. It's called "copy on write".

vanmeeuwen marked an inline comment as done.
  • The $id parameter is supposed to be a string
  • A second proposal to transactions
  • Update for D1102 review
  • Resolve some comments for D1102
  • Use constants
  • Remove unintentional commenting out
  • Add some tests
src/app/Transaction.php
156

Use $entitlement var.

190

If $description is always initialized, we don't have to use empty(), i.e. it could be: return $description ?: 'Default wallet';.

src/tests/Unit/TransactionTest.php
43

This test does not make much sense to me.

vanmeeuwen marked 2 inline comments as done.
  • Use the $entitleable variable
  • Use initialized variable
src/tests/Unit/TransactionTest.php
43

It only added the single line coverage that was missing.

One last change request. Is it finished?

bin/quickstart.sh
38

This need to be uncommented, I guess.

This revision now requires changes to proceed.May 28 2020, 9:12 AM
  • Uncomment
  • Rebase on current master
This revision is now accepted and ready to land.May 28 2020, 12:07 PM
This revision was automatically updated to reflect the committed changes.