asantibanez / laravel-eloquent-state-machines

State Machines for your Laravel Eloquent models
MIT License
525 stars 56 forks source link

Ability to have multiple pending transitions for same column #14

Open binaryweavers opened 3 years ago

binaryweavers commented 3 years ago

Summary

@asantibanez, Thank you for the awesome and simple package. I have a use case when there are multiple pending transitions that need to run one after another. Currently when transitionTo(...) method deletes all the pending transitions of the column, so the first pending transition i.e. call to transitionTo(...) would wipe the rest.

Proposal

I think there might be some sort of opt-in boolean type flag default to true on the pending transition itself which can be configured when pushing a pending transition. e.g. if it should be canceled or not after the latest transition.

asantibanez commented 3 years ago

Hi @binaryweavers !

Thanks for your interest in the package.

I've also thought about the use case you mention. I think adding a cancellable flag of some sorts to the pending transitions might work. Will give it a try.

Thanks for the suggestion.

binaryweavers commented 3 years ago

Hi @binaryweavers !

Thanks for your interest in the package.

I've also thought about the use case you mention. I think adding a cancellable flag of some sorts to the pending transitions might work. Will give it a try.

Thanks for the suggestion.

Pleasure @asantibanez , in fact thank you for building for community. I am currently using the solution as I mentioned above by overriding migration and couple of methods in StateMachine.

binaryweavers commented 3 years ago

One more thing I felt in v4, not sure if i should open a new issue or so, but in transition hooks, specicially ,afterTransitionHooks, they are triggered for the previous state. I think afterTransitionHooks should be triggered for new state a field has been transitioned to 🤔 https://github.com/asantibanez/laravel-eloquent-state-[machines/blob/f2a70b554473825833b227bbb65617f6cedc495d/src/StateMachines/StateMachine.php#L132]

asantibanez commented 3 years ago

Thanks @binaryweavers for the feedback.

It felt confusing for me that before transitions key were the "previous state" and after transitions key were the "new transitioned state". That's why I left both on the "previous state" for key.

Definitely a breaking change but made more sense to me. What do you think? Did the previous approach make more sense?

binaryweavers commented 3 years ago

Yes, I think "previous state" for before is fine. But for after, they should be triggered based on 'new transitioned state'. I have a use case where some actions are after performed based on "new transitioned state", in v4 it requires if checks for the passed '$to' state to figure out what the new state is.

asantibanez commented 3 years ago

Yeah. You have to add checks now. I think it makes sense to have that change reversed. 🤔

binaryweavers commented 3 years ago

Yes, That covers most of the use cases đź‘Ť

asantibanez commented 3 years ago

Sounds good.

Did you try the new changed attributes feature?

On Sun, 21 Feb 2021 at 13:57 Wali Razzaq notifications@github.com wrote:

Yes, That covers most of the use cases đź‘Ť

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/asantibanez/laravel-eloquent-state-machines/issues/14#issuecomment-782907051, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHDT6CWCO4RDUAV6ZXFWVDTAFJPZANCNFSM4XA6KQ2Q .

-- Saludos, Andrés Santibáñez B

binaryweavers commented 3 years ago

Sounds good. Did you try the new changed attributes feature? On Sun, 21 Feb 2021 at 13:57 Wali Razzaq @.***> wrote: Yes, That covers most of the use cases 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHDT6CWCO4RDUAV6ZXFWVDTAFJPZANCNFSM4XA6KQ2Q . -- Saludos, Andrés Santibáñez B

Unfortunately, I couldn't get to that yet. But that feature is a good addition though đź‘Ť. I'll definitely give a feedback on that as soon I use that