Page MenuHomePhorge
Differential D1102 Diff 3412 src/app/Transaction.php

Changeset View

Standalone View

View Options

src/app/Transaction.php

  • This file was added.
<?php
namespace App;
use Illuminate\Database\Eloquent\Model;
class Transaction extends Model
{
protected $fillable = [
// actor, if any
'user_email',
// entitlement, wallet
'object_id',
'object_type',
// entitlement: created, deleted, billed
// wallet: debit, credit, award, penalty
'type',
'amount',
'description',
// parent, for example wallet debit is parent for entitlements charged.
'transaction_id'
];
/** @var array Casts properties as type */
protected $casts = [
'amount' => 'integer',
];
/** @var boolean This model uses an automatically incrementing integer primary key? */
public $incrementing = false;
/** @var string The type of the primary key */
protected $keyType = 'string';
public const ENTITLEMENT_BILLED = 'billed';
public const ENTITLEMENT_CREATED = 'created';
public const ENTITLEMENT_DELETED = 'deleted';
public const WALLET_AWARD = 'award';
public const WALLET_CREDIT = 'credit';
public const WALLET_DEBIT = 'debit';
public const WALLET_PENALTY = 'penalty';
public function entitlement()
{
if ($this->object_type !== \App\Entitlement::class) {
return null;
machniakUnsubmitted
Done
Inline Actions

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

machniak: We call entitlement() so many times that we should cache it (in-memory) here or do some magic.
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

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.

vanmeeuwen: It should be in the SQL query cache, but it can of course also be cached "here". Caching…
}
return \App\Entitlement::withTrashed()->where('id', $this->object_id)->first();
}
public function setTypeAttribute($value)
{
switch ($value) {
case self::ENTITLEMENT_BILLED:
case self::ENTITLEMENT_CREATED:
case self::ENTITLEMENT_DELETED:
// TODO: Must be an entitlement.
$this->attributes['type'] = $value;
break;
case self::WALLET_AWARD:
machniakUnsubmitted
Done
Inline Actions

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

machniak: I would propose to create public class constants for these labels.
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

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

vanmeeuwen: And I completely understand why -- I've already missed "credited" vs. "credit" ;-)
case self::WALLET_CREDIT:
case self::WALLET_DEBIT:
case self::WALLET_PENALTY:
// TODO: This must be a wallet.
$this->attributes['type'] = $value;
break;
default:
throw new \Exception("Invalid type value");
}
}
public function toArray()
{
$result = [
'user_email' => $this->user_email,
'entitlement_cost' => $this->getEntitlementCost(),
'object_email' => $this->getEntitlementObjectEmail(),
machniakUnsubmitted
Done
Inline Actions

Why syntax with quotes+brackets?

machniak: Why syntax with quotes+brackets?
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

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.

vanmeeuwen: `description` is considered a keyword by my highlighter. It happens elsewhere for 'value' and…
'sku_title' => $this->getEntitlementSkuTitle(),
'wallet_description' => $this->getWalletDescription(),
'description' => $this->{'description'},
'amount' => $this->amount
];
return $result;
}
public function toString()
{
$label = $this->objectTypeToLabelString() . '-' . $this->{'type'};
return \trans("transactions.{$label}", $this->toArray());
}
public function wallet()
{
if ($this->object_type !== \App\Wallet::class) {
return null;
}
return \App\Wallet::where('id', $this->object_id)->first();
}
/**
* Return the costs for this entitlement.
*
* @return int|null
*/
private function getEntitlementCost(): ?int
{
if (!$this->entitlement()) {
return null;
}
// FIXME: without wallet discount
// FIXME: in cents
// FIXME: without wallet currency
$cost = $this->entitlement()->cost;
$discount = $this->entitlement()->wallet->getDiscountRate();
return $cost * $discount;
}
/**
* Return the object email if any. This is the email for the target user entitlement.
*
* @return string|null
*/
private function getEntitlementObjectEmail(): ?string
{
$entitlement = $this->entitlement();
if (!$entitlement) {
return null;
}
$entitleable = $entitlement->entitleable;
if (!$entitleable) {
\Log::debug("No entitleable for {$entitlement->id} ?");
return null;
}
return $entitleable->email;
}
/**
machniakUnsubmitted
Done
Inline Actions

Use $entitlement var.

machniak: Use $entitlement var.
* Return the title for the SKU this entitlement is for.
*
* @return string|null
*/
private function getEntitlementSkuTitle(): ?string
{
if (!$this->entitlement()) {
return null;
}
return $this->entitlement()->sku->{'title'};
}
/**
* Return the description for the wallet, if any, or 'default wallet'.
*
* @return string
*/
public function getWalletDescription()
machniakUnsubmitted
Done
Inline Actions

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

machniak: Would be better to call ->entitlement() once, The object_type check is redundant.
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

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.

vanmeeuwen: While I'm aware it doesn't address your remark as to the duplicate execution of the `$this…
machniakUnsubmitted
Done
Inline Actions

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';
machniak: My version would probably looked like this: ``` if ($entitlement = $this->entitlement()) {…
{
machniakUnsubmitted
Done
Inline Actions

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

machniak: This line could be made a last line in this function. This way you would de-deduplicate code.
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

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.

vanmeeuwen: I'm not confident the only types of objects to recognize are entitlement|wallet. I'm also not…
$description = null;
if ($entitlement = $this->entitlement()) {
$description = $entitlement->wallet->{'description'};
}
if ($wallet = $this->wallet()) {
$description = $wallet->{'description'};
}
return $description ?: 'Default wallet';
}
/**
machniakUnsubmitted
Done
Inline Actions

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

machniak: If $description is always initialized, we don't have to use empty(), i.e. it could be: `return…
* Get a string for use in translation tables derived from the object type.
*
* @return string|null
*/
private function objectTypeToLabelString(): ?string
{
if ($this->object_type == \App\Entitlement::class) {
return 'entitlement';
}
if ($this->object_type == \App\Wallet::class) {
return 'wallet';
}
return null;
}
}