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

Slugs #155

Merged
merged 3 commits into from
Mar 21, 2019
Merged

Slugs #155

merged 3 commits into from
Mar 21, 2019

Conversation

MaliRobot
Copy link
Collaborator

I think this can be safely merged. Models don't need to be updated, no migration required, just save news post and slug will be generated from the post title. I implemented the same model for COC but I am not sure if it's needed. All other entities that have slug already had some logic for auto generating slugs.

def save(self, *args, **kwargs):
if not self.slug:
self.slug = slugify(self.title)
elif self.slug != slugify(self.title):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will force the slug to be a slugified version of self.title, we want to be able to define it, but only assign as a default value the slugified version.

Could something like default=lambda self, x: slugify(self.title) work ?

Probably not as the field doesn't have the scope of self.

But you get my drift.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I got it:

  1. If post had no slug and no slug is entered - make slug from the title
  2. if post had slug but new one is entered - save slug, overwriting previous value
  3. if post had slug and no new one is entered - do nothing

Actually, it's only needed to remove elif condition, right?
I changed it and if it's ok you can merge it.

@virogenesis virogenesis merged commit fb7a236 into master Mar 21, 2019
@MaliRobot MaliRobot deleted the slugs branch March 27, 2019 23:13
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.

2 participants