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

Remove fallback first shipping method on shipments #1843

Merged

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Apr 12, 2017

In the case where the system has not picked a shipping rate, or
the user has not picked one, we can not just fall back on the first
shipping rate. In this case, the "selected" shipping rate's amount
and the shipment's amount will be out of sync, which is inconsistent.

This commit removes the fallback.

@mamhoff mamhoff added the WIP label Apr 12, 2017
end

context "when a shipping rate is selected" do

Choose a reason for hiding this comment

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

Extra empty line detected at block body beginning.

In the case where the system has not picked a shipping rate, or
the user has not picked one, we can not just fall back on the first
shipping rate. In this case, the "selected" shipping rate's amount
and the shipment's amount will be out of sync, which is inconsistent.

This commit removes the fallback.
@mamhoff mamhoff force-pushed the do-not-just-take-the-first-method branch from 72a7f63 to 9718fb1 Compare April 12, 2017 20:54
Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

I'm surprised this doesn't make any specs fail, I think that says a lot to some of the factory improvements we have made...

I tracked the change down to 20bed87, hoping to get some more idea on why it exists but didn't get anything reasonable. I half think it might be just to satify code if your specs did have bad factories...

I half expect someone to run into this as a problem during an upgrade, but I think its a good change over all.

@mamhoff mamhoff removed the WIP label May 17, 2017
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Add a changelog, but otherwise 👍

@gmacdougall gmacdougall merged commit 554cbfa into solidusio:master Jun 28, 2017
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.

6 participants