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

Implement slug field partial #764

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ def decode_id(id)
end

def find(*ids)
super(*ids.map { |id| decode_id(id) })
if respond_to?(:slug_attribute)
send("find_by_#{slug_attribute}".to_sym, ids.first)
gazayas marked this conversation as resolved.
Show resolved Hide resolved
else
super(*ids.map { |id| decode_id(id) })
end
end

def relation
Expand All @@ -46,6 +50,7 @@ def obfuscated_id
end

def to_param
obfuscated_id
# Even if the `slug` column exists, it might be empty (i.e. - Team).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(defined?(slug) && slug.present?) ? slug : obfuscated_id
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
module ObfuscatesId
# We only want to ensure slugging is possible if a developer
# can generate a `slug` field partial with our scaffolder.
if defined?(BulletTrain::SuperScaffolding)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


module Sluggable
extend ActiveSupport::Concern

included do
# Alphanumeric downcased URL identifier
before_validation :downcase_slug
after_validation :set_to_previous_value, on: :update

validates slug_attribute,
uniqueness: true,
length: {minimum: 2, maximum: 30},
format: {with: /\A[a-zA-Z0-9|-]+\Z/}

validates_with ::RestrictedPathsValidator

private

def downcase_slug
send(attribute_name_setter, send(attribute_name).downcase)
end

# Since Rails reloads the same invalid object into the form while displaying errors,
# we ensure the slug value being referenced to on reloading is the original value which
# has already been persisted to the database, not the invalid value from the form object.
def set_to_previous_value
if errors.any?
errors.each do |e|
if e.match?(attribute_name)
previous_slug_value = send("#{attribute_name}_was")
send(attribute_name_setter, previous_slug_value)
end
end
end
end

def attribute_name
self.class.send(:slug_attribute)
end

def attribute_name_setter
"#{attribute_name}=".to_sym
end
end
end

end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class RestrictedPathsValidator < ActiveModel::Validator
def validate(record)
if record.class.restricted_paths.include?(record.slug)
record.errors.add record.class.slug_attribute, :restricted_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end
end
4 changes: 4 additions & 0 deletions bullet_train-obfuscates_id/config/locales/en/errors.en.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
en:
errors:
messages:
restricted_path: "is restricted, please try another path"
1 change: 1 addition & 0 deletions bullet_train-super_scaffolding/lib/scaffolding.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def self.valid_attribute_type?(type)
"options",
"password_field",
"phone_field",
"slug",
"super_select",
"text_area",
"text_field",
Expand Down
3 changes: 3 additions & 0 deletions bullet_train-super_scaffolding/lib/scaffolding/attribute.rb
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ def collection_name
def partial_name
return options[:attribute] if options[:attribute]

# TODO: We should put these in alphabetical order.
case type
when "trix_editor", "ckeditor"
"html"
Expand Down Expand Up @@ -169,6 +170,8 @@ def partial_name
"number"
when "address_field"
"address"
when "slug"
"text"
else
raise "Invalid field type: #{type}."
end
Expand Down
1 change: 1 addition & 0 deletions bullet_train-super_scaffolding/lib/scaffolding/script.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
options: "string",
password_field: "string",
phone_field: "string",
slug: "string",
super_select: "string",
text_area: "text",
text_field: "string",
Expand Down
30 changes: 29 additions & 1 deletion bullet_train-super_scaffolding/lib/scaffolding/transformer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ def add_attributes_to_various_views(attributes, scaffolding_options = {})
# 'primary_key' => '',
# 'references' => '',
"string" => "text_field",
"text" => "text_area"
"text" => "text_area",
# 'time' => '',
# 'timestamp' => '',
}
Expand Down Expand Up @@ -1294,6 +1294,34 @@ def remove_#{attribute.name}
if attribute.is_boolean?
scaffold_add_line_to_file("./app/models/scaffolding/completely_concrete/tangible_thing.rb", "validates :#{attribute.name}, inclusion: [true, false]", VALIDATIONS_HOOK, prepend: true)
end
when "slug"
# TODO: We should really be including Sluggable at the top
# above the concerns hook, not here below the methods.
slug_methods = <<~RUBY
def slug
#{attribute.name}
end

def self.slug_attribute
:#{attribute.name}
end

def self.restricted_paths
[
"admin",
"admins"
]
end

include Sluggable
RUBY

scaffold_add_line_to_file(
"./app/models/scaffolding/completely_concrete/tangible_thing.rb",
slug_methods,
METHODS_HOOK,
prepend: true
)
end

end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<!-- This is the same content as text_field -->
<%
form ||= current_fields_form
options ||= {}
other_options ||= {}
%>

<%= render 'shared/fields/field', form: form, method: method, helper: :text_field, options: options, other_options: other_options %>
Loading