Allow to try different slugging approaches in conflicts #290

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@tomtastico

This adds the overridable method resolve_slug_conflict, which will be called when there is a slug conflict before adding a sequence number.

It allows to try different names for the slug before relying on the sequence.

For example, you might want to add a country to the slug only if there is a conflict. If this still fails, you might want to add a city. If that fails then a sequence number will be appended on top of the last attempt.

Usage:

class Person < ActiveRecord::Base
  friendly_id :name

  def resolve_slug_conflict(attempts)
    case attempts
      when 1
        "#{name} from #{location}"
      when 2
        "#{name} from #{city} in #{location}"
      else
        super
    end
  end
end

bob1 = Person.create! :name => "Bob Smith", :country => "USA", :city => "New York City"
bob2 = Person.create! :name => "Bob Smith", :country => "USA", :city => "New York City"
bob3 = Person.create! :name => "Bob Smith", :country => "USA", :city => "New York City"
bob4 = Person.create! :name => "Bob Smith", :country => "USA", :city => "New York City"
bob1.friendly_id #=> "bob-smith"
bob2.friendly_id #=> "bob-smith-from-usa"
bob3.friendly_id #=> "bob-smith-from-new-york-city-in-usa"
bob4.friendly_id #=> "bob-smith-from-new-york-city-in-usa--2"
@norman norman and 1 other commented on an outdated diff May 26, 2012
lib/friendly_id/slugged.rb
@@ -266,6 +266,50 @@ def should_generate_new_friendly_id?
slug_base != (current_friendly_id || slug_value).try(:sub, /#{separator}[\d]*\z/, '')
end
+ # Returns a name that is supposed to generate a non-conflicting slug.
+ #
+ # Override this if you want to try different approaches to solve a conflict
+ # before adding a sequence number.
+ #
+ # You can try different approaches if a previous attempt still conflicts.
+ #
+ # For example, you might want to add a country to the slug only if there is
+ # a conflict. If this still fails, you might want to add a city.
+ #
+ # NOTE: Attempts begin at 1 as in "first attempt", not 0!
@tomtastico
tomtastico May 26, 2012

Well, the assumption is that attempt 0 is the normal slug generation. When that yields a conflict, it goes to resolve_slug_conflict for attempt 1.

But following on your second comment of using an enumerable, I think it is better to start at 0 :)

@norman norman and 1 other commented on an outdated diff May 26, 2012
lib/friendly_id/slugged.rb
+ #
+ # You can try different approaches if a previous attempt still conflicts.
+ #
+ # For example, you might want to add a country to the slug only if there is
+ # a conflict. If this still fails, you might want to add a city.
+ #
+ # NOTE: Attempts begin at 1 as in "first attempt", not 0!
+ #
+ # === Example
+ #
+ # class Person < ActiveRecord::Base
+ # friendly_id :name
+ #
+ # def resolve_slug_conflict(attempts)
+ # case attempts
+ # when 1
@norman
norman May 26, 2012 Owner

I really don't like having to number attempts like this, it feels awkward and clunky. Is there a reason you chose to do it this way rather than just iterate through some kind of enumerable?

@tomtastico
tomtastico May 26, 2012

This is more versatile than an enumerable. You might want to run something on an attempt before returning the string.

I can change the example to have an enumerable that returns the item at the attempt index instead, if you think this is the way is going to be used most of the time.

@alternate_names = ["#{name} from #{location}", "#{name} from #{city} in #{location}"]

def resolve_slug_confict(attempts)
    @alternate_names[attempts]
end

Would need to change first attempt to start at 0 though, as per the previous comment.

@norman
Owner
norman commented May 26, 2012

This is great - I definitely want to add this feature, @xymbol and I have spoken about this several times over the years and just never got around to implementing it. Could you take a look at my comments in the commits and see if we can come up with a better way to iterate through the attempts?

@norman
Owner
norman commented May 26, 2012

I had been thinking we could pass an array as the first argument to friendly_id, and if it's an array of symbols, we would try those methods in order, and if it's an array of procs then invoke them successively:

class Foo < ActiveRecord::Base
  extend FriendlyId
  friendly_id [:foo, :bar]
end

What would you think about that?

@xymbol
Collaborator
xymbol commented May 26, 2012

@norman IIRC at some point we discussed having the method itself return an array or enumerable. Then, FriendlyId would try those in order and eventually use a sequence on the first one.

@tomtastico

I have rewritten it so it will behave as you suggested. This is a much bigger and invasive change to the current codebase though!

It's a bit cumbersome as you need to use methods or lambdas to interpolate a string...

extend FriendlyId
friendly_id [:name, :name_and_city, lambda {"#{name} from #{city} in #{country}"}], :use => :slugged    

def name_and_city
  "#{self.name} from #{self.city}"
end

But I couldn't come up with a better way, as if you put the interpolated strings directly in the array the variables get interpolated right when the class is instantiated, which is useless.

@parndt
Collaborator
parndt commented Dec 6, 2012

@norman @xymbol don't want to let this one get stale.. what do you think as you're championing it?

@raymccoy are you able to make this patch work rebased against the current codebase?

@tomtastico

@parndt Yes, will give it a try. Been using my branch in a production app for all these months so it wouldn't hurt to update it.

@xymbol
Collaborator
xymbol commented Dec 6, 2012

@parndt @raymccoy, I would favor having the first candidate used in case subsequent tries are not unique. Adding a sequence number is meant as a way to disambiguate slugs. Typically, you'd want that to happen with the most obvious and shortest option, instead of the most specific one.

@tomtastico

@xymbol Good point. Will try to make it that way when I adapt to the latest codebase.

@norman
Owner
norman commented Apr 16, 2013

I appreciate the effort in this one, but I'm not going to pull this since I'd like to try to resolve the problem in a different way. Closing the issue now since it's been open for a long time with no action taken on my part.

@norman norman closed this Apr 16, 2013
@tomtastico

My bad, really had no time to get back to this. Looking forward to an alternative solution :)

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