Fix slug regeneration behaviour

[urbanautomaton]

Hi,

Firstly, apologies: this is a bit of an epic, but it's taken me a while to get my head round it, so I've tried to explain thoroughly.

I noticed some odd behaviour recently when trying to force slug regeneration, which I traced down to Model#should_generate_new_friendly_id?. This returns inconsistent values depending on the order of certain operations. An example to recreate:

class User
  friendly_id :name, :use => :slugged
end

# Clearing slug on a previously-saved instance
> u = User.create(:name => "geoff")
=> #<User id: 1, name: "geoff", slug: "geoff">
> u.slug
=> "geoff"
> u.slug = nil
=> nil
> u.should_generate_new_friendly_id?
=> false

# Clearing slug on a freshly-loaded instance
> u2 = User.find("geoff")
=> #<User id: 1, name: "geoff", slug: "geoff">
> u2.slug
=> "geoff"
> u2.slug = nil
=> nil
> u2.shoud_generate_new_friendly_id?
=> true

This divergence of behaviour leads to oddities like this test, in which a record needs to be saved twice after its slug is cleared before being restored.

It turns out this was being caused by the mis-ordered setting of @current_friendly_id, which occurs in a before_save lifecycle callback. However #set_slug, which calls #should_generate_new_friendly_id to check whether a new slug is needed, is called in a before_validation callback, which happens prior to before_save.

This means that as far as #should_generate_new_friendly_id is concerned, @current_friendly_id doesn't represent the current friendly_id at all, but rather the friendly_id at the time this particular instance of the record was last saved (if at all).

This could be fixed by setting @current_friendly_id in a prepended before_validation callback. However this would be pointless, as then it would always be identical to the current value of the slug column, which #should_generate_new_friendly_id also fetches.

The attached commits provide updated tests for the regeneration behaviour, and address the issue by only checking the current value of the slug column in #should_generate_new_friendly_id?, removing current_friendly_id completely.

Unfortunately, however, this breaks a test in SimpleI18n:

test "set friendly_id should fall back default locale when none is given" do
  transaction do
    journalist = I18n.with_locale(:es) do
      Journalist.create!(:name => "Juan Fulano")
    end
    journalist.set_friendly_id("John Doe")
    journalist.save!
    assert_equal "john-doe", journalist.slug_en
  end
end

The reason this worked before was another bug in #should_generate_new_friendly_id?, whereby if current_friendly_id was set, only that would be compared against the slug base - the current slug column value was not checked at all. So in the above test, the following happened:

1. A Journalist is created in :es locale, slug_es and @current_friendly_id set to "juan-fulano"
2. friendly_id is set in :en locale with supplied base text "John Doe", slug_en set to "john-doe"
3. #set_slug invoked again during final #save! call
4. #should_generate_new_friendly_id compares normalized name ("juan-fulano") with current_friendly_id - they match, so returns false.
5. Save continues without overwriting slug_en

Now, however, the last two steps go:

• #should_generate_new_friendly_id compares normalized name ("juan-fulano") with current locale slug value, "john-doe". They don't match, so it returns true.
• #set_slug overwrites the slug_en column with a newly-generated slug based on the record name, setting it back to "juan-fulano"

I'm not really sure how to fix this, or if it even can be fixed. Because the SimpleI18n module doesn't do translation, and has no way of keeping track of base texts for different locales over multiple model loads, even if we fix this particular test failure so that slug_en isn't overwritten, the next time the model is loaded and saved in the :en locale, it'll get splatted for sure.

So. A pull request with some fixes, but also a breakage and a conundrum. Sorry about this - I'd be glad to hear your thoughts...

Cheers,
Simon

[parndt]

Wow! Thanks. Sounds like callback hell alright!

So, @urbanautomaton in your estimation does your patch create any new problems?

[xymbol]

@urbanautomaton 👍 That logic needs to be consistent. And thanks for getting the Peugeot brand right. ;-)

[urbanautomaton]

@parndt, well, all the other tests pass, plus the new ones, and we've been using this patch in production since shortly after I submitted the pull request without observing issues. That's not to say there aren't any, obviously - the logic in related areas is still somewhat opaque, but I think the behaviour of #should_generate_new_friendly_id? is now at least internally consistent.

Really not sure what to do about Simplei18n though - I just don't think the behaviour described by the failing test is achievable.

[parndt]

@norman I'm happy for this to be merged if you are.

[norman]

@parndt sure, we'll still need to address the failing test in SimpleI18, but let's move forward and get this into master so it goes out in 4.1.