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

Allowing a shipment to be ready for resumed orders #2317

Closed
wants to merge 2 commits into from
Closed

Allowing a shipment to be ready for resumed orders #2317

wants to merge 2 commits into from

Conversation

msevestre
Copy link

@radar: To respect the tell don't ask principle, I would like to replace
return 'pending' unless order.complete? || order.resumed?
with something like
return 'pending' unless order.can_ship?

but that might lead to some confusion elsewhere. Let me know what you think and I can adjust it accordingly

@radar
Copy link
Contributor

radar commented Dec 13, 2012

I like the idea of a can_ship? method. Where else are you thinking that it might lead to confusion?

@msevestre
Copy link
Author

It might be a contrived example but let's consider one order in the
complete state where one shipment is backordered. order.can_ship? would be
true although the order cannot be shipped as is.

But if you're ok with that (or if you have a better or less ambiguous name)
I'll make the required changes.

BTW: this state machine implementation is freaking awesome. Congrats on
that.
On Dec 12, 2012 10:23 PM, "Ryan Bigg" notifications@github.com wrote:

I like the idea of a can_ship? method. Where else are you thinking that
it might lead to confusion?


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

@@ -45,7 +45,11 @@
end

context "when order is incomplete" do
before { order.stub :complete? => false }
before do
order.stub :complete? => false
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit-pick: indentation here is one character too far.

@BDQ
Copy link
Member

BDQ commented Dec 13, 2012

I think order.can_ship? is a good idea, and not confusing- you're asking the order it is willing to allow shipments.

Each individual shipment is ultimately responsible for deciding it's state, after the order gives it's permission.

@@ -178,10 +197,10 @@ def compute(computable)
end
end

context "#complete?" do
Copy link
Author

Choose a reason for hiding this comment

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

I allowed myself to change that as well, because it seems to me that the completed? behaviour and not the complete? state were tested

@radar
Copy link
Contributor

radar commented Dec 13, 2012

It might be a contrived example but let's consider one order in the
complete state where one shipment is backordered. order.can_ship? would be
true although the order cannot be shipped as is.

But if you're ok with that (or if you have a better or less ambiguous name)
I'll make the required changes.

I think this is ok. I don't mind that caveat.

BTW: this state machine implementation is freaking awesome. Congrats on that.

Thanks :) I'm really proud that we were able to get something that works that well!

@msevestre
Copy link
Author

I have implemented can_ship? earlier today and updated the PR.
Hope there is no more formatting issue...my bad
On Dec 13, 2012 5:26 PM, "Ryan Bigg" notifications@github.com wrote:

It might be a contrived example but let's consider one order in the
complete state where one shipment is backordered. order.can_ship? would be
true although the order cannot be shipped as is.

But if you're ok with that (or if you have a better or less ambiguous name)
I'll make the required changes.

I think this is ok. I don't mind that caveat.

BTW: this state machine implementation is freaking awesome. Congrats on
that.

Thanks :) I'm really proud that we were able to get something that works
that well!


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

@BDQ
Copy link
Member

BDQ commented Dec 14, 2012

@radar - did you mean to cherry-pick this on your radar/spree? and not spree/spree?

@radar
Copy link
Contributor

radar commented Dec 14, 2012

Yup. That's where I'm running the builds from now rather than pushing to GH.

On 15/12/2012, at 4:27, Brian Quinn notifications@github.com wrote:

@radar - did you mean to cherry-pick this on your radar/spree? and not spree/spree?


Reply to this email directly or view it on GitHub.

@radar radar closed this in 88881ac Dec 14, 2012
radar pushed a commit that referenced this pull request Dec 14, 2012
Fixes #2317

Conflicts:

	core/app/models/spree/shipment.rb
	core/spec/models/order_spec.rb
	core/spec/models/shipment_spec.rb
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

Successfully merging this pull request may close these issues.

None yet

4 participants