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

Move spree user methods to module #13

Closed
wants to merge 7 commits into from
Closed

Move spree user methods to module #13

wants to merge 7 commits into from

Conversation

qr8r
Copy link
Contributor

@qr8r qr8r commented May 5, 2015

Move Spree::User logic into a mixin that can be included in the Spree.user_class that is defined.

error = "Spree.user_class MUST be a String or Symbol object, not a Class object."
raise error if @@user_class.is_a?(Class)

if !@@user_class.constantize.included_modules.include? Spree::UserMethods
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work instead?

if !@@user_class.constantize.is_a? Spree::UserMethods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, @@user_class.constantize is a class. After talking with @jhawthorn, figured out it would need to be @@user_class.constantize.new.is_a? ... but that we didn't like that syntax

@jhawthorn
Copy link
Contributor

test this please

1 similar comment
@qr8r
Copy link
Contributor Author

qr8r commented May 7, 2015

test this please

@@ -11,58 +11,58 @@ class BaseController < Spree::BaseController

Copy link

Choose a reason for hiding this comment

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

the changes in this file seem unrelated, and the previous style didn't necessarily break a convention, do you mind backing them out?

qr8r added 7 commits May 8, 2015 10:49
Also update LegacyUser to include User methods and move appropriate
methods into the UserMethods module
Add deprication warning to initializer. If User class doesnt include
UserMethods, add it and warn.
For api compatibility

if !methods_included
ActiveSupport::Deprecation.warn "#{ Spree.user_class.name } must include Spree::UserMethods"
Spree.user_class.class_eval { include UserMethods }
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to class_eval this.

Spree.user_class.include UserMethods

I like the deprecation warning getting people to move this to their own definition rather than hacking it in for them.

Copy link

Choose a reason for hiding this comment

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

agreed with Gregor's comments

@jhawthorn
Copy link
Contributor

This was merged in #153

@jhawthorn jhawthorn closed this Jul 8, 2015
Senjai pushed a commit to Senjai/solidus that referenced this pull request Feb 17, 2016
JDutil added a commit to JDutil/solidus that referenced this pull request Mar 26, 2020
mamhoff added a commit to mamhoff/solidus that referenced this pull request Jun 26, 2024
…for-friendly-promotions

Use different path for friendly promotions
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.

4 participants