Add HasStatuses trait and supporting code #26
Conversation
| $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 |
There was a problem hiding this comment.
Note change to 'type' here. The HasStatuses trait defaults to using the fully qualified model name as the type.
There was a problem hiding this comment.
@chx2 Take a look at the changes in this PR for what you're doing in tipoff/bookings with Status:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Great. Thanks for the explanation
| $table->text('note')->nullable(); | ||
| $table->timestamps(); | ||
|
|
||
| $table->unique(['name', 'type']); |
There was a problem hiding this comment.
Uniqueness is not name alone - but name for a given type. Its certainly reasonable for two unrelated status types to have overlapping names.
src/Traits/HasStatuses.php
Outdated
| protected bool $dynamicStatusCreation = false; | ||
|
|
||
| // Assumes models can only have a single type of status | ||
| public function statusType(): string |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Oh, I didn't even consider that. Yes, that would be a great use case! Let's allow for it. Thank you!
There was a problem hiding this comment.
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.
src/Traits/HasStatuses.php
Outdated
| return get_class($this); | ||
| } | ||
|
|
||
| public function status(?string $name = null): ?Status |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We could rename the Statusable model to AssignedStatus or StatusChange or StatusRecord or something like that.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
That's why I originally created the Statusables table and why it doesn't have a unique combination requirement.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Perfect. Let's go with that. Thank you
There was a problem hiding this comment.
Ok. Name change to StatusRecord and retention of past records is now in place.
| { | ||
| // 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; |
There was a problem hiding this comment.
This can be pulled if you only want to allow pre-defined statuses to be used. Included only because it was easy to add.
There was a problem hiding this comment.
That's interesting. Not sure if it is necessary since Nova allows a create resource option from within another model.
| 'name', | ||
| ]; | ||
|
|
||
| public static function publishStatuses(string $type, array $names): Collection |
There was a problem hiding this comment.
This method would be used in migrations to establish valid statuses for a given type.
| // Check if desired status already set | ||
| if ($currentStatus = $this->getStatus($type)) { | ||
| if ($currentStatus->id === $status->id) { | ||
| return $currentStatus; | ||
| } | ||
| } |
There was a problem hiding this comment.
@drewroberts - want to call this out. If the requested status is the same as the current status, a new record is NOT created. If you always want a record, even if the requested status matches current status, it can be removed and tests updated.
There was a problem hiding this comment.
That is a correct implementation for what I want. Thank you!
There was a problem hiding this comment.
@joshtorres Could you make sure this logic is also added to GmbDetails and GmbHours in the the tipoff/location (TIPOFF/locations#44) package and PlaceDetails and PlaceHours in the tipoff/seo package (TIPOFF/seo#22)?
No description provided.