Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Can't explicitly set slug when using scoped #983

Open
joe-reich opened this issue Nov 5, 2021 · 6 comments
Open

Can't explicitly set slug when using scoped #983

joe-reich opened this issue Nov 5, 2021 · 6 comments

Comments

@joe-reich
Copy link

Hiya. Thanks for creating and maintaining this gem.

I ran into an issue: explicitly setting the slug doesn't seem to work when the model uses scoped.

Here's some pseudo-rails to give the gist.

Without scoped, it works fine:

class State < ApplicationRecord
  include FriendlyId

  friendly_id: :state_name
end

oh = State.create(state_name: 'Ohio')
oh.slug # returns "ohio" (normal)

ny = State.create(state_name: 'New York', slug: 'ny')
ny.slug #returns 'ny' (success)

With scoped, it runs into trouble:

class County < ApplicationRecord
  include FriendlyId
  belongs_to :state

  friendly_id: :county_name, use: :scoped, scope: :state
end

franklin = County.create(county_name: "Franklin", state: oh)
franklin.slug # returns "franklin" (normal)

kings = County.create(county_name: "Kings", state: ny, slug: "kgs")
kings.slug # returns "kings" (the bug)

Looks like the issue is here:

def should_generate_new_friendly_id?
(changed & friendly_id_config.scope_columns).any? || super
end

Within the gem codebase, it looks like this issue only arises in Scoped. But it would also arise for users who followed the suggestion to redefine should_generate_new_friendly_id? in their models.

If the maintainers agree with what I'm suggesting is the expected behavior, I might be able to find time to put a PR together.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@luctus
Copy link

luctus commented Apr 22, 2022

Hey there @joe-reich! I just run into the same issue... were you able to find a workaround?
Thanks!

@stale stale bot removed the stale label Apr 22, 2022
@joe-reich
Copy link
Author

@luctus I monkey-patched should_generate_new_friendly_id?. We ended up not shipping this for various reasons, and IIRC even this redefinition doesn't produce natural behavior in every set of circumstances. Proceed with caution!

    def should_generate_new_friendly_id?
      # hopeless if the base for setting slug is nil
      return false if public_send(friendly_id_config.base).nil?

      # Slug is nil.  Gotta try.
      return true if public_send(friendly_id_config.slug_column).nil?

      # Slug was set explicitly. Leave it.
      return false if friendly_id_config.slug_column.in?(changed)

      # Base for setting slug changed. Change the slug.
      return true if slug_base_changed?

      # Basis for scoping has changed.  Reset slug.
      return true if scoping_base_changed?

      false
    end

    # Can be overwritten in the model
    def slug_base_changed?
      friendly_id_config.base.to_s.in?(changed)
    end

    private

    def scoping_base_changed?
      return false unless friendly_id_config.uses?(:scoped)

      (changed & friendly_id_config.scope_columns).any?
    end

@Adeynack
Copy link

One of our model had a non-scoped slug for many years and creating directly with a manual slug, when user specify it, is established behavior. We have a change of model and now, that slug is scoped. This bug sadly breaks the slug overriding at creation time that our users are used to. We can of course patch this in the controller, updating right behind with the slug if it's in the parameters, but it would simply be cleaner to solve it.

I will try to find time to propose a PR, but to be realistic, I do not see that happening in the near future. I am however writing those words to emphasis our interest in it being solved at some point, if a maintainer can find the time.

Many thanks in advance.

@sandrasi
Copy link

sandrasi commented Nov 8, 2022

The documentation says that the should_generate_new_friendly_id? method is added to the model and therefore it can be overridden if needed (i.e. no need to monkey-patch the gem or send a new PR that changes the current behavior).

class MyModel
  extend FriendlyId
  friendly_id :base_attr, use: %i[scoped slugged], scope: :scope_attr
  ...
  # Override method added by FriendlyId::Scoped to prevent overwriting explicitly set slugs.
  def should_generate_new_friendly_id?
    send(friendly_id_config.slug_column).nil? && super
  end
end

@dedekm
Copy link

dedekm commented Jan 10, 2023

I've also ran into this, very confusing...

I'm not sure if this behaviour (force new slug when scoped column is changed) is bug or it is required for some reason?

we used this approach to fix this

def slug_candidates
  %i[slug to_label]
end

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

No branches or pull requests

5 participants