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

Adding new Slugify.generate method to generate a slug value #821

Merged
merged 4 commits into from
Mar 12, 2022

Conversation

jwoertink
Copy link
Member

Fixes #813

This method will generate a String value that can be used for a slug, but without setting any attribute value for you. This would be more used for an escape hatch than anything. Since it's not passing in the slug column, there's no way to query to know if it's been taken already. This means we can't do the query lookup to ensure the value is unique.

before_save do
  if title.changed?
    slug.value = Avram::Slugify.generate(using: title)
  end

  validate_uniqueness slug, query: ArticleQuery.new.slug
end

… the slug value. This can be used as an escape hatch. Fixes #813
Comment on lines -38 to -44
it "it sets slug from a single string" do
op = build_op

slugify(op.slug, "Software Developer")

op.slug.value.should eq("software-developer")
end
Copy link
Member Author

Choose a reason for hiding this comment

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

turns out this was a duplicate spec with the one right above it.

Copy link
Member

@matthewmcgarvey matthewmcgarvey left a comment

Choose a reason for hiding this comment

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

Not sure how useful it would be without the ability to check the slug in the database. another option would be to pass in everything like the other method but not to check the value. Or maybe just the column name instead of the whole attribute but that might be confusing

@jwoertink
Copy link
Member Author

Yeah, I was kind of stumped on this one too. I went back and forth with it a few times and finally just said I'll push something up so we can hash it out.

I agree passing in just the column name might be confusing, and we'd lose the type-safety there anyway.

Maybe what we could do is something like..

def set(...)
  if slug.value.blank?
    slug.value = generate(...)
  end
end

and just have generate handle the logic ... 🤔

private def format_candidates(slug_candidates : Array(String | Avram::Attribute(String) | Array(Avram::Attribute(String)))) : Array(String)
slug_candidates.map do |candidate|
parameterize(candidate)
end.reject(&.blank?)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this include a call to #strip as well?

def generate(using slug_candidates : Array(String | Avram::Attribute(String) | Array(Avram::Attribute(String)))) : String
slug_candidates = format_candidates(slug_candidates)

slug_candidates.first? || UUID.random.to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

A uuid fallback would not be my expectation here, but I'm not sure what would have been my expectation. A slug is usually designed to be more "human friendly" than a UUID... so I think I'd just expect it to raise if it couldn't build a slug.

it "sets when using multiple attributes" do
op = build_op(title: "How Do Magnets Work?", sub_heading: "And Why?")

slugify(op.slug, [[op.title, op.sub_heading]])
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this line do? Is it proving a compile time check?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put it there just to make sure you were awake 😛 actually, it's just a copy/paste error. I need to remove it 😅

@jwoertink
Copy link
Member Author

Alright, here's an update that moves all of the logic to generate. Then set just looks to see if the slug has a value already and if not, calls generate to set it.

I started to add in an error and have generate raise the error if no slug could be generated, but then I saw this spec

describe "all slug candidates are blank" do
it "leaves the slug as nil" do
op = build_op(title: "")
# First string is empty. Added to make sure it is not used with
# the UUID.
slugify(op.slug, ["", op.title])
op.slug.value.should be_nil
end
end

basically this allows it to be nil. I decided that does make sense because it would be up to you to add in the validations if this field was required or not. So I turned generate in to String? which just returns nil if no slug is generated.

Co-authored-by: Matthew McGarvey <matthewmcgarvey14@gmail.com>
Comment on lines 34 to 41
# class SaveArticle < Article::SaveOperation
# before_save do
# if title.changed?
# slug.value = Avram::Slugify.generate(using: title)
# validate_uniqueness_of slug, query: ArticleQuery.new.slug
# end
# end
# end
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be updated I think

@jwoertink jwoertink merged commit bf3ad43 into main Mar 12, 2022
@jwoertink jwoertink deleted the issues/813 branch March 12, 2022 21:11
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

Successfully merging this pull request may close these issues.

Slugify doesn't allow you to update the slug
3 participants