Skip to content
New issue

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

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

STI test failure when looking up record from SubClass #516

Closed
wants to merge 1 commit into from

Conversation

ScotterC
Copy link
Contributor

class Journalist < ActiveRecord::Base
  extend FriendlyId
  friendly_id :name, :use => :slugged
end

class Editorialist < Journalist
  extend FriendlyId
  friendly_id :name, :use => :slugged
end

editorialist = Editorialist.create name: "foo"

Editorialist.find editorialist.slug
# => Couldn't find Editorialist 

Also:

class Journalist < ActiveRecord::Base
end

class Editorialist < Journalist
  extend FriendlyId
  friendly_id :name, :use => :slugged
end

Editorialist.create name: "foo"
# => Couldn't create Editorialist 
`undefined method `exists_by_friendly_id?'`

@parndt
Copy link
Collaborator

parndt commented Jan 13, 2014

Thanks for this.. my first impression is that somewhere we're not referencing the base_class where we should be.

@ScotterC
Copy link
Contributor Author

Got a little closer just now.

Editorialist.friendly.find editorialist.slug works. So it must be something with the finder method module not being added to subclasses?

@parndt
Copy link
Collaborator

parndt commented Jan 13, 2014

Oh, right, no that's totally expected.. FriendlyId scopes don't apply unless you use .friendly.find. Try it on the base class and you'll get the same result. I think, given that, this should probably be closed.

@ScotterC
Copy link
Contributor Author

Well the reason I came across this is because of what I have in my code:

class Product < ActiveRecord::Base
  extend FriendlyId
  friendly_id :name, :use => [:slugged, :finders]
end

class GiftCard < Product
end

gift_card = GiftCard.create name: "foo"

GiftCard.find gift_card.slug #=> AR::NotFound

So the finders module is not being inherited to the subclass. Is that intentional?

@parndt
Copy link
Collaborator

parndt commented Jan 13, 2014

Your example code and test case didn't specifically include the :finders module, unless I'm mistaken?

I don't think that the module should automatically inherit to the subclass... @norman what do you think?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 09606c0 on ScotterC:sti-test-failure into * on norman:master*.

@ScotterC
Copy link
Contributor Author

I just now added the finders module to the test case. Your right. before it should have failed. But it still fails with the module added.

@parndt
Copy link
Collaborator

parndt commented Jan 13, 2014

Does your test case pass if you add the same friendly_id line to the subclass?

@ScotterC
Copy link
Contributor Author

Added. And yes it does still fail. Any ideas on why they wouldn't be inherited? It's the same extension of class methods in the finders module no?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2d7fba5 on ScotterC:sti-test-failure into * on norman:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2d7fba5 on ScotterC:sti-test-failure into * on norman:master*.

relation_class_name = :"#{klass.to_s.gsub('::', '_')}_#{self.to_s.gsub('::', '_')}"
klass.const_get(relation_class_name)
end
def relation_delegate_class(klass)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was just a spacing issue. nothing changed.

@ScotterC
Copy link
Contributor Author

Cleaned up the PR so it doesn't overwrite the other tests.

The finder methods are just not being included in the sub classes. From what I can tell, FriendlyId::Configuration is not being initialized in subclasses when friendly_id is included.

This is because the extension of the class returns if it responds to friendly_id and therefore the configuration is not initialized and finder_methods are not included.

I see two approaches depending on what the core team wants. Have the finder methods get inherited to sub classes if they are included in the parent or change the extended method to allow a new configuration per class.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 910cdc9 on ScotterC:sti-test-failure into * on norman:master*.

adding finders to both
cleaned up test

sub class of STI can't create records if parent
does not extend friendly id
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d957e8e on ScotterC:sti-test-failure into * on norman:master*.

@emoreth
Copy link

emoreth commented Jan 29, 2014

Any news on this one ? I got the same exact problem. Any workarounds to not have to use GiftCard.friendly.find 'slug' ?

@ScotterC
Copy link
Contributor Author

That's my current workaround.

With 5.0 it seems that the core wants finder methods to be optional. However, the design of finder methods getting included into each subclass seems a bit tricky. I can't figure out why they're in the model's ancestors but the methods aren't included.

@jaredbeck
Copy link

.. the finders module is not being inherited to the subclass. ..
I can't figure out why they're in the model's ancestors but the methods aren't included.

I'm seeing this too, in rails 4.0.2 and friendly_id 5.0.2. Changing my app to use Model.friendly.find would be a major effort, so this issue is kind of blocking my upgrade to rails 4. Thanks to everyone working on this, looking forward to an update.

@niklas
Copy link

niklas commented Feb 5, 2014

I have the same problem as @jaredbeck. Luckily, we have a module encapsulating our Slug behaviour (friendly id is the third library we use). The :finders module is switched on in an initializer.

Somehow, this fixes the problem of having to use .friendly to find our STI models.

# Include Shop::Slugs in a Model to get application-wide solid slug behaviour
#
# Please define #to_slug on your model, so that this works.
module Shop
  module Slugs
    def self.included(model)
      super
      model.class_eval do
        extend FriendlyId
        friendly_id :to_slug, :use => [:history]
        extend Inheriting
      end
    end

    # friendly_id keeps a config per class, but does not inherit the :finders
    # module, so we have to re-apply the settings
    module Inheriting
      def inherited(child)
        super
        child.class_eval do
          include ::Shop::Slugs
        end
      end
    end
  end
end

@niklas
Copy link

niklas commented Feb 6, 2014

Sadly, my approach above causes an early DB access while booting the Rails app, because friendly_id attaches finder_methods to relation, which has to look for the type column. (all caused by active admin loading the routes and with it all models). Migrating the DB from scratch is not possible anymore. Trying to find another way..

@niklas
Copy link

niklas commented Feb 6, 2014

Checking for child.table_exists? fixes the migrate problem for now. Would still be great if :finders module would be inherited 🍰

@jaredbeck
Copy link

Any word from @parndt or @norman on this? We can't really say FriendlyId supports rails 4 if it doesn't support STI ..

@jaredbeck
Copy link

As an ugly workaround, it looks like we can put something like this in each of our child classes:

class MySTIChildClass
  class << MySTIChildClass
    alias_method :super_find, :find
  end

  def self.find(*args)
    friendly.super_find(*args)
  end
end

norman added a commit that referenced this pull request Feb 14, 2014
Without this change, STI classes skipped adding the finders because
FriendyIdConfig#uses? would return true for FriendlyId::Finders when the
parent class had this module loaded, so the module loading would always
be skipped in the child module.

However, since FriendId::Finders.setup is always invoked when adding a
FriendlyId addon, the module will now get added to child classes.

See #516
norman added a commit that referenced this pull request Feb 14, 2014
Previously, if the child class had the finder modules loaded but the
parent class did not, then slugs could not be created because the slug
generator assumed friendly finders were available, but were not because
the scope is uses comes from the super class rather than the child
class.

The child class scope is not used in order to maintain slug uniqueness
across child classes.

See #516
@norman
Copy link
Owner

norman commented Feb 14, 2014

@ScotterC Thanks for the work on this. I've fixed the test failures and merged your changes. This will go out in the next few minutes to Rubygems.

@norman norman closed this Feb 14, 2014
@ScotterC
Copy link
Contributor Author

Cool. From the commits it looks like you found the root of the issue. Thanks @norman

@jaredbeck
Copy link

I just tried 5.0.3. Looks like find is friendly once again in STI subclasses. Thanks @norman. However, it seems that it's necessary to duplicate the call to friendly_id(:foo, use: [:finders]) in the child classes. Can you confirm that it's necessary to duplicate this configuration? Thanks.

@parndt
Copy link
Collaborator

parndt commented Feb 15, 2014

Shouldn't subclasses have to opt in?

@norman
Copy link
Owner

norman commented Feb 15, 2014

Yes, @jaredbeck, you must call friendly_id in each class. That's by design.

@jaredbeck
Copy link

That makes sense. One last thing: The documentation (http://norman.github.io/friendly_id/file.Guide.html) says:

you must extend FriendlyId in all classes that participate in STI, both your parent classes and their children.

but it doesn't mention invoking the friendly_id method. Thanks!

@norman
Copy link
Owner

norman commented Feb 17, 2014

You don't have to invoke friendly_id in the parent class if you're only
going to use it in the child classes. But you do need to extend FriendlyId.

Sent from my phone

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants