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

Replace ShippingMethod#display_on with ShippingMethod#available_to_users #1611

Merged
merged 5 commits into from
Dec 1, 2016

Conversation

jhawthorn
Copy link
Contributor

Echoing the work done in #1540 to PaymentMethods

This replaces ShippingMethod#display_on with ShippingMethod#available_to_users.

Unlike with payment_methods, this doesn't add an #available_to_admin column, because the existing functionality did not provide a way to hide shipping methods from admins. Having display_on="front_end" was identical to having display_on="both" (or actually any string other than "back_end").

This replaces ShippingMethod#display_on with
ShippingMethod#available_to_users to match a similar change made to
payment methods.

Unlike with payment_methods, this doesn't add an #available_to_admin
column, because the existing functionality did not provide a way to
hide shipping methods from admins. Having display_on="front_end" was
identical to having display_on="both" (or actually any string other than
"back_end").
@jhawthorn jhawthorn force-pushed the shipping_method_display_on branch from e835cef to 58ab9eb Compare November 24, 2016 00:44
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.

Looks good to me and makes more sense, thanks.

Can we deprecate constants? Spree::ShippingMethod::DISPLAY isn't used anymore.

@jhawthorn jhawthorn mentioned this pull request Nov 30, 2016
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.

Great 👍

There is this DeprecatedConstantProxy, although I doubt that anybody ever used this constant and it's safe to be used removed.

@jhawthorn
Copy link
Contributor Author

There is this DeprecatedConstantProxy, although I doubt that anybody ever used this constant and it's safe to be used removed.

DeprecatedConstantProxy is really cool, but only works if there is a replacement. We can manually define const_missing, which we have done elsewhere.

What's best here is probably to use DeprecatedObjectProxy

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.

ActiveSupport is just awesome

@jhawthorn jhawthorn merged commit 422f701 into solidusio:master Dec 1, 2016
@jhawthorn jhawthorn deleted the shipping_method_display_on branch December 1, 2016 20:49
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.

3 participants