norman/friendly_id — pull request #334 by urbanautomaton (2012-11-06 11:33:41+00:00)
Title: Fix slug regeneration behaviour

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:

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

```ruby
# Clearing slug on a previously-saved instance
> u = User.create(:name => "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")
=> #
> u2.slug
=> "geoff"
> u2.slug = nil
=> nil
> u2.shoud_generate_new_friendly_id?
=> true
```

This divergence of behaviour leads to oddities like [this test](https://github.com/norman/friendly_id/blob/d5888e6a667be29a980edd46b8680abce72693f2/test/shared.rb#L44), 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](https://github.com/norman/friendly_id/blob/b07a13c943bd8a9432305adf4ce8c1ab5258bafa/lib/friendly_id/base.rb#L192). 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](https://github.com/norman/friendly_id/blob/b07a13c943bd8a9432305adf4ce8c1ab5258bafa/lib/friendly_id/slugged.rb#L254).

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`](https://github.com/norman/friendly_id/blob/b07a13c943bd8a9432305adf4ce8c1ab5258bafa/test/simple_i18n_test.rb#L65):

```ruby
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](https://github.com/norman/friendly_id/blob/b07a13c943bd8a9432305adf4ce8c1ab5258bafa/lib/friendly_id/slugged.rb#L266) - 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