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 "Canceled" shipment state. #1208

Merged
merged 1 commit into from
Mar 17, 2014
Merged

Conversation

kayue
Copy link
Contributor

@kayue kayue commented Mar 16, 2014

screen shot 2014-03-14 at 9 49 35 pm

Should we...

  • Add a new "Canceled" shipment state.
  • Remove the old "Dispatched" state, it is not being use in anywhere.

Reference: http://guides.spreecommerce.com/developer/shipments.html

@pjedrzejewski
Copy link
Member

Yes, I think it makes perfect sense. @winzou opinions?

@kayue
Copy link
Contributor Author

kayue commented Mar 16, 2014

Done.

@pjedrzejewski
Copy link
Member

@Sylius Cancelled vs. Canceled? Afaik, Canceled is OK in American English, but Cancelled is preferred in other countries... opinions?

@umpirsky
Copy link
Contributor

I prefer cancelled.
On 16 Mar 2014 20:10, "Paweł Jędrzejewski" notifications@github.com wrote:

@Sylius https://github.com/Sylius Cancelled vs. Canceled? Afaik,
Canceled is OK in American English, but Cancelled is preferred in other
countries... opinions?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1208#issuecomment-37766761
.

@ruudkobes
Copy link
Contributor

+1 for "cancelled"

@kayue
Copy link
Contributor Author

kayue commented Mar 17, 2014

@pjedrzejewski Ready to merge :)

ShipmentInterface::STATE_SHIPPED,
ShipmentInterface::STATE_READY,
ShipmentInterface::STATE_PENDING,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't remove this

@winzou
Copy link
Contributor

winzou commented Mar 17, 2014

@kayue it's fine but can you keep the pending state.
I have a plan to do some sort of UI for shippers. This state is important as it says that shipment should not be sent yet (= in preparation).

@kayue
Copy link
Contributor Author

kayue commented Mar 17, 2014

@winzou Are you planning to do it in core?
Will it be hard to distinguish onhold and pending?

@winzou
Copy link
Contributor

winzou commented Mar 17, 2014

In Core or another bundle.
These 2 states are different:

  • onhold = a transaction is pending, we do anything with this shipment
  • pending = shipment will be sent, but not now, the shipment itself is pending (because it's being packed, waiting for inventory, etc.). Shipments with this state will be displayed to packers UI
  • ready = shipment is to be sent, now. Shipments with this state will be displayed to shippers UI

This is just to separate 2 different business: shippers and packers.

We could rename pending to packing to be more explicit? But pending is more general, for the shippers point of view. It could be being packed, but also just waiting for anything else, shippers don't care.

@kayue
Copy link
Contributor Author

kayue commented Mar 17, 2014

@winzou I see. We have packing state in our project too. 👍

I agree we should be more general here. How about calling it processing? It indicate "some action is required / happening".

@kayue
Copy link
Contributor Author

kayue commented Mar 17, 2014

@winzou Are you going to contribute this "packers" logic / UI to Sylius?
If not, I don't think we should have it this state in core. We cannot pre-define all the states for developer anyway.

(In our project we have "picking", "packed" then "shipped"..)

@winzou
Copy link
Contributor

winzou commented Mar 17, 2014

Uhm processing for shipment state would mean that the shipment is processing. Whereas here we have items being packed (or whatever else), which is not (so) shipment related.

Yes I'll contribute this to Sylius, that's the aim. I know we cannot have every states for every project, so let's just have this pending state in the middle. With the next states management (finite machine), all this will be anyway highly configurable and changed easily.

@kayue
Copy link
Contributor Author

kayue commented Mar 17, 2014

Will revert the changes 👍

@winzou
Copy link
Contributor

winzou commented Mar 17, 2014

Can you just squash commits and we're good ;)

@kayue
Copy link
Contributor Author

kayue commented Mar 17, 2014

Done 👍

winzou added a commit that referenced this pull request Mar 17, 2014
@winzou winzou merged commit 0938aad into Sylius:master Mar 17, 2014
@stloyd stloyd added this to the 1.0.0-BETA1 milestone Mar 17, 2014
@jjanvier
Copy link
Contributor

@winzou @kayue just for my curiosity, do you update your awesome wiki pages each time you make a change on this ?

@kayue
Copy link
Contributor Author

kayue commented Mar 19, 2014

I don't. I try to follow it instead of change it.

On Wednesday, March 19, 2014, Julien Janvier notifications@github.com
wrote:

@winzou https://github.com/winzou @kayue https://github.com/kayuejust for my curiosity, do you update your awesome wiki pages each time you
make a change on this ?

Reply to this email directly or view it on GitHubhttps://github.com//pull/1208#issuecomment-38046834
.

@jjanvier
Copy link
Contributor

@kayue :( this could be so helpful

@kayue
Copy link
Contributor Author

kayue commented Mar 21, 2014

I need to make sure I understand it completely first. Will have a second
look when I have time :)

On Friday, March 21, 2014, Julien Janvier notifications@github.com wrote:

@kayue https://github.com/kayue :( this could be so helpful

Reply to this email directly or view it on GitHubhttps://github.com//pull/1208#issuecomment-38265280
.

@jjanvier
Copy link
Contributor

thank you very much :)

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Discussions about potential changes or new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants