Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HasStatuses trait and supporting code #26

Merged
merged 5 commits into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 6 additions & 17 deletions database/factories/StatusFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,32 +6,21 @@

use Illuminate\Database\Eloquent\Factories\Factory;
use Illuminate\Support\Str;
use Tipoff\Forms\Models\Status;
use Tipoff\Statuses\Models\Status;

class StatusFactory extends Factory
{
/**
* The name of the factory's corresponding model.
*
* @var string
*/
protected $model = Status::class;

/**
* Define the model's default state.
*
* @return array
*/
public function definition()
{
$word1 = $this->faker->unique->word;
$word2 = $this->faker->word;
$type = $this->faker->randomElement(['order', 'slot', 'game', 'invoice', 'payment']);
$words = $this->faker->unique()->words(2, true);

return [
'slug' => Str::slug(rand(1, 1000).'-'.$word1.'-'.$word2),
'name' => $word1.' '.$word2,
'applies_to' => $this->faker->randomElement(['order', 'slot', 'game', 'invoice', 'payment']),
'note' => $this->faker->sentences(1, true),
'name' => $words,
'type' => $type,
'note' => $this->faker->optional()->sentences(1, true),
];
}
}
28 changes: 28 additions & 0 deletions database/factories/StatusableFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

declare(strict_types=1);

namespace Tipoff\Statuses\Database\Factories;

use Illuminate\Database\Eloquent\Factories\Factory;
use Illuminate\Support\Str;
use Tipoff\Authorization\Models\User;
use Tipoff\Statuses\Models\Status;
use Tipoff\Statuses\Models\Statusable;

class StatusableFactory extends Factory
{
protected $model = Statusable::class;

public function definition()
{
$statusable = User::factory()->create();

return [
'status_id' => randomOrCreate(Status::class),
'statusable_type' => get_class($statusable),
'statusable_id' => $statusable->id,
'creator_id' => randomOrCreate(app('user')),
];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ public function up()
Schema::create('statuses', function (Blueprint $table) {
$table->id();
$table->string('slug')->unique()->index();
$table->string('name')->unique();
$table->string('applies_to')->default('order'); // Values include 'order', 'slot', 'game', 'invoice', 'payment'
$table->string('name');
$table->string('type'); // Typically, full class name for model using status
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note change to 'type' here. The HasStatuses trait defaults to using the fully qualified model name as the type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chx2 Take a look at the changes in this PR for what you're doing in tipoff/bookings with Status:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pdbreen You prefer using type instead of applies_to for this field?

CC: @sl0wik

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do. Its somewhat tied to potentially allowing a single model to have multiple, different types of statuses. But, even if that's not allowed, I think type is still a more appropriate term for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thanks for the explanation

$table->text('note')->nullable();
$table->timestamps();

$table->unique(['name', 'type']);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uniqueness is not name alone - but name for a given type. Its certainly reasonable for two unrelated status types to have overlapping names.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Thanks

});
}
}
9 changes: 9 additions & 0 deletions src/Exceptions/StatusException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

declare(strict_types=1);

namespace Tipoff\Statuses\Exceptions;

interface StatusException
{
}
15 changes: 15 additions & 0 deletions src/Exceptions/UnknownStatusException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

declare(strict_types=1);

namespace Tipoff\Statuses\Exceptions;

use Throwable;

class UnknownStatusException extends \InvalidArgumentException implements StatusException
{
public function __construct(string $type, string $name, $code = 0, Throwable $previous = null)
{
parent::__construct("Unknown status value '{$name}' for status type {$type}", $code, $previous);
}
}
79 changes: 78 additions & 1 deletion src/Models/Status.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,89 @@

namespace Tipoff\Statuses\Models;

use Assert\Assert;
use Carbon\Carbon;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Support\Collection;
use Illuminate\Support\Str;
use Tipoff\Discounts\Models\Discount;
use Tipoff\Support\Models\BaseModel;
use Tipoff\Support\Traits\HasPackageFactory;

/**
* @property int id
* @property string name
* @property string type
* @property string slug
* @property string note
* @property Carbon created_at
* @property Carbon updated_at
*/
class Status extends BaseModel
{
use HasPackageFactory;

protected $casts = [];
protected $casts = [
'id' => 'integer',
];

protected $fillable = [
'type',
'name',
];

public static function publishStatuses(string $type, array $names): Collection
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method would be used in migrations to establish valid statuses for a given type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that. Thanks

{
return collect($names)
->map(function (string $name) use ($type) {
return static::createStatus($type, $name);
});
}

public static function createStatus(string $type, string $name, ?string $note = null): self
{
/** @var Status $status */
$status = static::query()->firstOrNew([
'type' => $type,
'name' => $name
]);

if ($note) {
$status->note = $note;
}

$status->save();

return $status;
}

public static function findStatus(string $type, string $name): ?self
{
/** @var Status $status */
$status = static::query()
->byType($type)
->where('name', '=', $name)
->first();

return $status;
}

protected static function boot()
{
parent::boot();

static::creating(function (Status $status) {
$status->slug = $status->slug ?: Str::slug("{$status->type}-{$status->name}");
});
}

public function scopeByType(Builder $query, string $type): Builder
{
return $query->where('type', '=', $type);
}

public function __toString()
{
return $this->name;
}
}
43 changes: 43 additions & 0 deletions src/Models/Statusable.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php

declare(strict_types=1);

namespace Tipoff\Statuses\Models;

use Carbon\Carbon;
use Illuminate\Database\Eloquent\Model;
use Tipoff\Authorization\Models\User;
use Tipoff\Support\Models\BaseModel;
use Tipoff\Support\Traits\HasCreator;
use Tipoff\Support\Traits\HasPackageFactory;

/**
* @property int id
* @property Status status
* @property Model statusable
* @property User creator
* @property Carbon created_at
* @property Carbon updated_at
* // Raw Relations
* @property int|null creator_id
*/
class Statusable extends BaseModel
{
use HasPackageFactory;
use HasCreator;

protected $casts = [
'id' => 'integer',
'creator_id' => 'integer',
];

public function status()
{
return $this->belongsTo(Status::class);
}

public function statusable()
{
return $this->morphTo();
}
}
65 changes: 65 additions & 0 deletions src/Traits/HasStatuses.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php

declare(strict_types=1);

namespace Tipoff\Statuses\Traits;


use Tipoff\Statuses\Exceptions\UnknownStatusException;
use Tipoff\Statuses\Models\Status;
use Tipoff\Statuses\Models\Statusable;

/**
* @property Statusable statusable
*/
trait HasStatuses
{
// When true, new statuses will be added automatically on first use
// When false, exception occurs if unknown status value is used
protected bool $dynamicStatusCreation = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be pulled if you only want to allow pre-defined statuses to be used. Included only because it was easy to add.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting. Not sure if it is necessary since Nova allows a create resource option from within another model.


// Assumes models can only have a single type of status
public function statusType(): string
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to decide whether or not its necessary to support a single model having multiple statuses. If yes, then a type field will be needed in the statusables record. The status(...) method would also need to allow for fetching status by arbitrary type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have a single model supporting multiple statuses so that I have timestamps recorded of when the model was switched from one Status to another. That history will be important for keeping records.

Therefore, the current Status of a single model will be the most recent one attached to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking if a single model can have multiple, distinct types of status. A hypothetical example, order model has both "payment status", "shipment status" (this is same as allowing for one model to have multiple addresses).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't even consider that. Yes, that would be a great use case! Let's allow for it. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - type field added to Statusable model and trait methods updated to allow for status by type. If no type is supplied, full class name of model is used as type designation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

{
// Default to using full class name as type
return get_class($this);
}

public function status(?string $name = null): ?Status
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the trait needs to contain a statusable() method to define the morphOne relation, this is the key interface method which returns the Status model essentially hiding the Statusable model from external use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could rename the Statusable model to AssignedStatus or StatusChange or StatusRecord or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of those, I think AssignedStatus is most appropriate because it represents the current, point in time status. The other names imply history tracking which would require an additional table to track transitions over time (which it sounds like you want, should create an issue to add it).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I originally created the Statusables table and why it doesn't have a unique combination requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather see change history tracked in another table. Using most recent timestamp as determination for current status makes me nervous (direct DB change for example). Your call. If history is retained in this table, the HasStatuses trait needs some changes to support it properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. For name, I'd lean towards StatusRecord - can be read as both current record & historical record. The others imply only current or only history.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Let's go with that. Thank you

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#27

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Name change to StatusRecord and retention of past records is now in place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

{
/** @var Statusable|null $statusable */
$statusable = $this->statusable;

if ($name) {
$status = $this->getStatusByName($name);

if (!$statusable) {
$statusable = new Statusable();
$statusable->statusable()->associate($this);
}

$statusable->status()->associate($status)->save();
$this->load('statusable');
}

return $statusable ? $statusable->status : null;
}

public function statusable()
{
return $this->morphOne(Statusable::class, 'statusable');
}

private function getStatusByName(string $name): Status
{
if ($this->dynamicStatusCreation) {
return Status::createStatus($this->statusType(), $name);
}

if ($status = Status::findStatus($this->statusType(), $name)) {
return $status;
}

throw new UnknownStatusException($this->statusType(), $name);
}
}
15 changes: 0 additions & 15 deletions tests/Support/Providers/NovaPackageServiceProvider.php

This file was deleted.

2 changes: 1 addition & 1 deletion tests/TestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
use Spatie\Permission\PermissionServiceProvider;
use Tipoff\Authorization\AuthorizationServiceProvider;
use Tipoff\Statuses\StatusesServiceProvider;
use Tipoff\Statuses\Tests\Support\Providers\NovaPackageServiceProvider;
use Tipoff\Support\SupportServiceProvider;
use Tipoff\TestSupport\BaseTestCase;
use Tipoff\TestSupport\Providers\NovaPackageServiceProvider;

class TestCase extends BaseTestCase
{
Expand Down
Loading