Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Refinery::Blog.user_class #290

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

jseitel commented Sep 21, 2012

This issue was tracked in #286. I've added a config setting so that existing Rails applications can specify their own user class (generally User, but could be anything).

Only added in 2-0-stable as that's what I use in prod, but happy to take a crack at merging the patch into master as well (still learning the ins and outs of Git, so might take me a little bit to get the hang of it).

Adding support for Refinery::Blog.user_class. This will enable the Re…
…finery::Blog engine to work out-of-box with existing Rails applications.

This issue was tracked in #286.

Thanks for this. It solved my problem using the blog engine with an existing app with its own User class. The one thing I had to change is lib/refinery/blog/configuration.rb line 27. I needed User instead of Refinery::User. Also my User table did not have a username field so I had to create a method for it concatenating first and last names.

Thanks! this was a big help. Hope it gets merged.

Owner

jseitel replied Sep 24, 2012

The one thing I had to change is lib/refinery/blog/configuration.rb line 27

You shouldn't have to change configuration.rb. Instead, go to config/initializers/refinery.rb and uncomment line #15. That config setting is the whole point of the change. :)

Also my User table did not have a username field so I had to create a method for it concatenating first and last names.

Yep, I had to do the same thing. The framework seems to assume the existence of the property.

Thanks! this was a big help. Hope it gets merged.

Thank you! Glad it was helpful. My first contribution on Github. :)

Contributor

jseitel commented Oct 27, 2012

@parndt Hey, just checking in - any way I can help to get this merged?

@parndt parndt commented on an outdated diff Oct 28, 2012

...og/templates/config/initializers/refinery/blog.rb.erb
@@ -10,4 +10,7 @@ Refinery::Blog.configure do |config|
# config.share_this_key = <%= Refinery::Blog.share_this_key.inspect %>
# config.page_url = <%= Refinery::Blog.page_url.inspect %>
+
+ # If you're grafting onto an existing app, change this to your User class
+ # Refinery::Blog.user_class = User
@parndt

parndt Oct 28, 2012

Owner

This should read:

  # Refinery::Blog.user_class = <%= Refinery::Blog.user_class.inspect %>

@parndt parndt commented on an outdated diff Oct 28, 2012

lib/refinery/blog/configuration.rb
self.validate_source_url = false
self.comments_per_page = 10
self.posts_per_page = 10
self.post_teaser_length = 250
self.share_this_key = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
self.page_url = "/blog"
+
+ # Refinery::User isn't available when this line gets hit, so we use static methods instead
+ @@user_class_name = nil
+ def self.user_class= (class_name)
@parndt

parndt Oct 28, 2012

Owner

Please remove the space before the parentheses :-)

@parndt parndt commented on an outdated diff Oct 28, 2012

lib/refinery/blog/configuration.rb
self.validate_source_url = false
self.comments_per_page = 10
self.posts_per_page = 10
self.post_teaser_length = 250
self.share_this_key = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
self.page_url = "/blog"
+
+ # Refinery::User isn't available when this line gets hit, so we use static methods instead
+ @@user_class_name = nil
+ def self.user_class= (class_name)
+ if class_name.class == Class
+ @@user_class_name = class_name
+ else
+ raise TypeError, "Expecting configuration input of type Class."
+ end
+ end
+
+ def self.user_class
+ @@user_class_name || Refinery::User
+ end
@parndt

parndt Oct 28, 2012

Owner

Given that you have two self. definitions I personally think it would look better this way:

# Refinery::User isn't available when this line gets hit, so we use static methods instead
@@user_class_name = nil
class << self
  def user_class=(class_name)
    if class_name.class == Class
      @@user_class_name = class_name
    else 
      raise TypeError, "Expecting configuration input of type Class."
    end
  end

  def user_class
    @@user_class_name || Refinery::User
  end
end

@parndt parndt commented on an outdated diff Oct 28, 2012

lib/refinery/blog/configuration.rb
self.validate_source_url = false
self.comments_per_page = 10
self.posts_per_page = 10
self.post_teaser_length = 250
self.share_this_key = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
self.page_url = "/blog"
+
+ # Refinery::User isn't available when this line gets hit, so we use static methods instead
+ @@user_class_name = nil
+ def self.user_class= (class_name)
+ if class_name.class == Class
+ @@user_class_name = class_name
+ else
+ raise TypeError, "Expecting configuration input of type Class."
@parndt

parndt Oct 28, 2012

Owner

I don't think that this error message is particularly clear

Owner

parndt commented Oct 28, 2012

I think that the user_class should be implemented like forem due to radar/forem#88

Owner

parndt commented Oct 28, 2012

Thanks for the reminder!

Owner

parndt commented Nov 22, 2012

@radar hey are you able to give valuable input on this PR based on your experience with Spree and Forem?

Contributor

jseitel commented Nov 26, 2012

@parndt Sorry for the delay - just got back from vacation. I'll implement your feedback and check in by this weekend.

Contributor

jseitel commented Jan 31, 2013

Hey, sorry for the extreme lateness on this. I just pushed a commit to make everything work like forem i.e. you pass in a string instead of a class literal.

Please take a look and let me know if you have any more feedback. Thanks!

Contributor

jseitel commented Jan 31, 2013

Sorry forgot to copy you @parndt

Owner

parndt commented Jan 31, 2013

Don't worry I'm already subscribed to the issue.. I was just sleeping 💤

I really think this should go against the master branch as it's a new feature..

Contributor

jseitel commented Feb 1, 2013

OK, I've merged the feature into master branch (new pull request).

@ugisozols ugisozols closed this Feb 1, 2013

Owner

ugisozols commented Feb 1, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment