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

Deprecate simple_current_order #1915

Merged
merged 2 commits into from
May 22, 2017

Conversation

ericsaupe
Copy link
Contributor

@ericsaupe ericsaupe commented May 22, 2017

Fixes the bug found in #1700 and deprecates simple_current_order because it's function is duplicated with current_order

@ericsaupe ericsaupe changed the title Added current_order_params to simple_current_order Deprecate simple_current_order May 22, 2017
@ericsaupe ericsaupe force-pushed the simple-current-store branch 2 times, most recently from 2032ce5 to f361b4f Compare May 22, 2017 10:17
Copy link
Contributor

@jordan-brough jordan-brough left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

I have a minor nitpick about writing the deprecation in a different way.

Would you also elaborate a little bit in the commit message why we think it's a good idea to deprecate this method?

We would also want this to appear in the CHANGELOG.md.

@@ -14,8 +14,9 @@ module Order
helper_method :simple_current_order
end

# Used in the link_to_cart helper.
def simple_current_order
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use

deprecate simple_current_order: :current_order, deprecator: Spree::Deprecation

Which does the same thing more concisely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this a lot better, I'll make the change. Thanks!

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.

👍 thanks!

ericsaupe added 2 commits May 22, 2017 12:02
Using the current_order_params sets the store and user similar
to the current_order method.
The `#simple_current_order` method is duplicating many of the
functions already found in `#current_order`. `simple_current_order`
was finds an order by token or user, updates ip_address, and
creates an order if one is not found. All functions that are
already accomplised in `#current_order` which also does more
than `#simple_current_order` making it the preferred method.
@ericsaupe ericsaupe force-pushed the simple-current-store branch from f361b4f to 26d81b2 Compare May 22, 2017 11:09
Copy link
Contributor

@jhawthorn jhawthorn left a comment

Choose a reason for hiding this comment

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

👍

@mamhoff mamhoff merged commit 548b54f into solidusio:master May 22, 2017
harmonymjb pushed a commit to alarmgrid/solidus_bootstrap_frontend that referenced this pull request Jan 21, 2020
harmonymjb pushed a commit to alarmgrid/solidus_bootstrap_frontend that referenced this pull request Jan 24, 2020
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.

5 participants