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

Order Transition on confirm does not make sense. #2044

Closed
liverbool opened this issue Oct 22, 2014 · 6 comments
Closed

Order Transition on confirm does not make sense. #2044

liverbool opened this issue Oct 22, 2014 · 6 comments

Comments

@liverbool
Copy link
Contributor

https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/CoreBundle/Resources/config/state-machine.yml#L85

    sylius_order:
        callbacks:
            after:
                ...
                sylius_update_shipment:
                    on:   'confirm'
                    do:   [@sm.callback.cascade_transition, 'apply']
                    args: ['object.getShipments()', 'event', '"prepare"', '"sylius_shipment"']

This transition will make shipment's state to ready. I think it does not make sense because some case order's state can be backorder,... So shipment's state should not be ready in this case.

ping @stloyd

@stloyd
Copy link
Contributor

stloyd commented Oct 22, 2014

Hmmm, in fact it sounds quite reasonable, but I'm not the author (ping @winzou) of that callback so I can't say why it was that way.

@Arn0d @pjedrzejewski do you remember reason of that callback?

@kayue
Copy link
Contributor

kayue commented Oct 22, 2014

@liverbool Do you think you can do a before callback on prepare transition, and switch "ready" to "backorder"?

If this is not possible then we must do it via a separated callback.

prepare:
from: [checkout, onhold]
to: ready

@liverbool
Copy link
Contributor Author

@kayue i think before not a good choice to use, the sylius_update_shipment task should have to change callback service to do this job.
Do you think?

@kayue
Copy link
Contributor

kayue commented Oct 22, 2014

Make sense to me 👍

@liverbool
Copy link
Contributor Author

PR #2045

@winzou
Copy link
Contributor

winzou commented Oct 23, 2014

👍

@winzou winzou closed this as completed Oct 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants