Implement slug field partial

[gazayas]

Closes bullet-train-co/bullet_train#1232

Joint PR

• Implement slug field partial bullet_train#1321

rails generate super_scaffold Site Team name:text_field foo:slug

TODO

• Fix a bug related to nil object when an error is raised on an invalid slug, and then trying to save a valid slug.
• Write Super Scaffolding tests in the starter repository

[gazayas]

We have a slug column on Team from a really long time ago:

https://github.com/bullet-train-co/bullet_train/blob/6c11323d5fbeb93eb6974e4d67b6531651d3b9c6/db/migrate/20161117154605_create_teams.rb#L5

[gazayas]

Considering changing this, it may not be too big of a deal if BulletTrain::SuperScaffolding is defined or not (maybe I can just write a comment saying the the module is only intended for use with BulletTrain::SuperScaffolding).

[jagthedrummer]

@gazayas, thanks for tackling this, it looks like a great start! I have kind of a mix of questions that I'm not sure how to answer and suggestions for this PR.

Questions, which are kind of related:

• Do we want slug to be a field type, or a magic field, or something in between? (To oversimplify, I'm generally thinking that if it's a field type you could have multiple slugs named whatever you want, but if it's a magic field you can only have one and it's always named slug. I think currently we're sort of in between and we expect only a single slug, but it can be named whatever you want.)
• Like, if someone (for whatever reason) did rails generate super_scaffold Site Team name:text_field internal_slug:slug external_slug:slug what would we want to happen? (I don't think that what this PR currently does with that is what we want, but I'm not sure what we do want.)
• Are there scenarios where someone might want a model to have a slug field, but for that field to NOT be used inside the BT admin?

Suggestions:

If you do rails generate super_scaffold Site Team name:text_field foo:slug you end up with this in your model:

  include Sluggable
  
  def slug
    foo
  end

  def self.slug_attribute
    :foo
  end

  # Define which paths you don't want to show up when defining slugs.
  def self.restricted_paths
    [
      "admin",
      "admins"
    ]
  end

I'm wondering if we can condense all of this, so that it's something more like:

  include Sluggable
  slug_field :foo

I'm thinking that we probably want some kind of global "slugs to avoid" list that would be shared among all models by default, and that we probably don't want to have to list them on each and every model with a slug.

So maybe in config/initializers/bullet_train.rb we'd have something like:

config.forbidden_slugs = ["admin", "admins"]

And then if you wanted to add additional restrictions for a particular model you could do something like:

  include Sluggable
  slug_field :foo, forbidden: ["root"]

And if then we'd probably need some way to opt out of the global block list in a model:

  include Sluggable
  slug_field :foo, forbidden: ["root"], use_global_forbidden_list: false

And if you wanted to have a slug, but not use it inside of BT maybe you'd do something like:

  include Sluggable
  slug_field :foo, use_in_urls: false

Or maybe instead of opting-out of using it in urls you'd have to opt-in, which would make it clear which of multiple slugs you wanted to be used in urls:

  include Sluggable
  slug_field :foo
  slug_field :bar, use_in_urls: true

I'm sure the naming of things could be greatly improved over what I've suggested here, I just wanted to get some thoughts down. What do you think?

[gazayas]

(I don't think that what this PR currently does with that is what we want, but I'm not sure what we do want.)

I think this is definitely the main question at hand, and if I'm understanding your idea of magic field properly, here's my idea for the two possibilities.

Field Partial or Magic Field

1. Field Partial

This would keep the functionality of this PR (and I think implementing all the changes you proposed would improve the PR a lot), so developers could do the following:

1. Generate a slug field partial via rails g super_scaffold.
2. Allow them to write in their own custom URL, independent of any other attributes on the model.

The reason I phrased the second point that way is for how we would handle it if we went the magic field route, which I'll explain below

2. Magic Field

This would tie a slug to a specific attribute on the model. We would then automatically generate the URL based on the attribute name. This would mean that developers wouldn't need to run something like rails g super_scaffold Project title:text_field foo:slug. However, we could potentially do something like this:

rails g super_scaffold Project title:text_field{slug}

By adding this option, we would designate which attribute to sluggify. So, say you super scaffold a Project with this command, and then create a new Project with a title named My Project. This would automatically set the url to my-project. I think this could work well for articles, etc.

Setting restricted paths in an initializer

I think this is a good idea regardless if we make it a field partial or a magic field. I think the next step is to just decide which one we want to go with.

[jagthedrummer]

Just kinda thinking out loud here, so apologies if this is a bit scatter shot.

Allow them to write in their own custom URL, independent of any other attributes on the model.

I think that enabling this is very important. It's problematic to try to automatically and invisibly generate the slug based on some other attribute.

Just a couple of scenarios to illustrate:

• You have a post called "My Test Post" that gets automatically sluggified to my-test-post. When you try to add a new post called "My-Test-Post!" it would also be sluggified to my-test-post and you'd need a way to disambiguate the slug, even though the titles are already unique.
• You notice a typo in the title of a post that's been published for some time. You probably want to be able to correct that typo without automatically (and unknowingly) breaking links to URLs that contain the slug.

So, in that sense I think we definitely want it to be closer to the "field partial" model. I also think that it makes sense to let people name it how they like. "Slug" might make sense to some people, but other people might want to call it "URL fragment" or whatever.

I thought some more about whether or not it makes sense to have multiple slugs on a model, and I'm not sure that it does (though I'm open to arguments). I kind of think that "slug" in this context basically means "a URL compatible identifier that's also human settable/readable which is used in URLs". I think that if you wanted to have two slugs on a model, that only one of them would meet that general definition, and the other would just be a text field with some validations.

So I'm not sure that it makes sense to treat a non-URL slug as a Slug™.

For instance, I had proposed that maybe there's a need to be able to do this:

  include Sluggable
  slug_field :foo
  slug_field :bar, use_in_urls: true

But I think that it would probably be more straightforward, and more understandable when coming across the code in the future if it were this instead:

  include Sluggable
  slug_field :bar

  validates :foo, presence: true, uniqueness: true, format: {...}

So, my leanings currently are that the slug feature should have the following qualities:

• One slug per model - If you try to generate a model with multiple slugs, or add a slug to a model that already has one we should throw an error.
• Slug field can be named anything
• Slug field is user settable

A nice bonus quality if it's relatively easy to do:

• Connect the slug to some other field so that for new records the slug will be auto-populated directly in the form as the other field is being initially populated.

Like if a Site has a name and a slug it would be nice for the slug field (only on new records) to automatically be populated as you're typing into the name field. So as you enter "My New Site" into the name field the slug field gets progressively set to my-new-site. Once you interact with (or change?) the slug field we'd probably want to sever the auto-population connection.

Maybe you could generate it by doing something like:

rails g super_scaffold Site Team name:text_field foo:slug{generate_from:name}

I don't really know what that would generate code-wise to let everything get hooked up properly. Maybe something like:

  include Sluggable
  slug_field :bar, generate_from: :name

Dunno. Maybe that's a little more than we want to bite off at this stage.

[jagthedrummer]

Here we may also need to validate that the slug doesn't match any of the controller actions for this model. For instance if someone tried to use the slug new for a team, then that would cause a collision for the path /teams/new. We'd always want that to point to new_team_path and not to team_path(slug: 'new').

This article suggests that something like TeamsController.action_methods would give us a good list to validate against.

For our Account::TeamsController the action_methods method returns this big ol' list:

Account::TeamsController.action_methods
=>
#<Set:
 {"team_params",
  "switch_to",
  "show",
  "index",
  "create",
  "edit",
  "destroy",
  "update",
  "new",
  "ensure_onboarding_is_complete",
  "assign_select_options",
  "create_model_if_new",
  "create_models_if_new",
  "ensure_backing_models_on",
  "assign_checkboxes",
  "assign_date",
  "assign_date_and_time",
  "assign_boolean",
  "load_team",
  "managing_account?",
  "ensure_onboarding_is_complete_and_set_next_step",
  "set_last_seen_at",
  "adding_team?",
  "accepting_invitation?",
  "adding_user_email?",
  "adding_user_details?",
  "switching_teams?",
  "set_current_user",
  "invited?",
  "show_sign_up_options?",
  "after_sign_up_path_for",
  "enforce_invitation_only",
  "only_allow_path",
  "delegate_json_to_api",
  "current_team",
  "current_membership",
  "current_locale",
  "set_locale",
  "layout_by_resource",
  "set_sentry_context"}
>

That seems like too much. This StackOverflow answer suggests a method like this:

def actions_for_controller(controller_path)
  route_defaults = Rails.application.routes.routes.map(&:defaults)
  route_defaults = route_defaults.select { |x| x[:controller] == controller_path }
  route_defaults.map { |x| x[:action] }.uniq
end

Which produces a much more reasonable list:

actions_for_controller('account/teams')
=> [
  "index",
  "create",
  "new",
  "edit",
  "show",
  "update",
  "destroy",
  "switch_to"
]

I'm not sure that we'd need to prevent member actions like edit and show from being used as a slug, though allowing them might result in weird URLs like /teams/show/edit (the edit action for the team with a slug of show).

Determining which controller to inspect for a given model may be difficult.

[gazayas]

All of this sounds good to me. Let me recap to see if I'm processing everything correctly:

1. We want to make this a field partial as opposed to a magic partial to ensure the slug is user-settable.
2. In the future we can consider adding multiple slugs to a model, but for now we can put a restriction in place to only have one slug at a time (if we wanted a non-url "slug" we could potentially use a text field instead to make things simpler).
3. Add an extra step to the RestrictedPathsValidator to ensure users don't accidentally create a collision between controller actions and their custom slug.

A nice bonus quality if it's relatively easy to do:

• Connect the slug to some other field so that for new records the slug will be auto-populated directly in the form as the other field is being initially populated.

Seems like a cool feature to me, I think we could accomplish this with a simple stimulus controller. The dynamic forms/dependent fields feature came to mind, but that seems too heavy duty for the job. From a abstract/design standpoint though, if it SHOULD belong there I think it's worth looking into.

[jagthedrummer]

That all sounds right to me @gazayas.

The auto-fill thing does sound very similar to the dynamic-forms/dependent-fields thing. But I agree (after only very brief consideration) that it sounds a little heavy since there wouldn't need to be a round trip to the server.

Though... as I'm typing that out I'm realizing that we would need to validate uniqueness (among other things) for the slug that we auto-generate, so... maybe it's closer to that pattern than I thought at first?

[jagthedrummer]

Converting this to draft for now.