4
\$\begingroup\$

I looked into possible ways to raise an error to generate an error when a slug already exist.

This is the model:

# models.py
from django.db import models
class Question(models.Model):
    title = models.CharField(max_length=100)
    slug = models.SlugField(max_length=100, db_index=True, unique=True)

for which I use a generic view:

# views.py
from .models import Question
class QuestionCreate(generic.CreateView):
    model = Question
    fields = ['title', 'text']
    def form_valid(self, form):
        instance = form.save(commit=False)
        slug = slugify(form.cleaned_data['title'])
        if Question.objects.filter(slug=slug).exists():
             form.add_error("title","This Question already exist")
             return self.form_invalid(form)

The slug is generated from the question title.

Then it was brought to my attention that I don't need to save the form first:

class QuestionCreate(generic.CreateView):
    model = Question
    fields = ['title', 'text']
    def form_valid(self, form):
        slug = slugify(form.cleaned_data['title'])
        if Question.objects.filter(slug=slug).exists():
             form.add_error("title","This Question already exist")
             return self.form_invalid(form)

Is this the best practice?

Other possibilities I considered:

# models.py
from django.db import models, IntegrityError
from django.forms import ValidationError
class Question(models.Model):
    title = models.CharField(max_length=100)
    slug = models.SlugField(max_length=100, db_index=True, unique=True)
    def save(self):
        slug = slugify(self.title)
        try:
            self.slug = slug
            super().save()    
        except IntegrityError:
            raise ValidationError("Question already exist") 

but it seems not possible to display this error message to the user.

The final option I considered:

# models.py
from .forms import QuestionForm
class QuestionCreate(generic.CreateView):
    model_form = QuestionForm

# forms;py
from django.forms import ModelForm
from .models import Question

class QuestionForm(ModelForm):
    class Meta:
         model = Question
         fields = ['title', 'text']

I still have to figure out how to raise an error message in this last situation, it's somewhere in the docs.

Which is the best practice? Or is this all acceptable code?

\$\endgroup\$
2
  • \$\begingroup\$ It's not clear what snippets you want reviewed and what snippets are part of the background story. \$\endgroup\$ Feb 10, 2017 at 0:40
  • \$\begingroup\$ I wanted to know which logic was right. Kevin brown perfectly answered it, thank you for your effort as well \$\endgroup\$
    – stetim94
    Feb 10, 2017 at 10:57

2 Answers 2

9
\$\begingroup\$

The good

So, you're trying to allow users to create new questions, but those questions must have a slug which is unique. You're enforcing this on the database level with unique=True, which is a good start.

You are using a form to handle the creation of new questions, which is definitely the right way to do this. It'll also be really useful when you need to build in the ability to edit a question, since you can just reuse the same form.

The bad

But you're enforcing the uniqueness of a question outside of the form. The form ultimately has the business logic that determines whether or not the input is valid, which is why Django has built-in support for validating forms. If you find issue with isolating your form validation logic inside of a form, I would recommend breaking it out as general utilities which can be easily tested.

Right now you're enforcing these constraints either on the model level (through save, which is definitely not the right place) or on the view level (through form_valid, which is not right but also not horrible).

Django designed the generic views to be easy to implement, assuming you follow the standard Django conventions. If you don't follow the conventions, you end up with cases like your form_valid where you have to do additional validation and then decide that it's actually invalid, forcing them to form_invalid. You shouldn't have made it to form_valid, since the state is not actually valid.

The fix

You should move your uniqueness logic into the form. Since you are only validating a single field, the title field, you only need to handle the logic for that field. This means that you can isolate your logic inside a custom cleaning method.

class QuestionForm(ModelForm):

    class Meta:
         model = Question
         fields = ['title', 'text']

    def clean_title(self, title):
        from django.utils.text import slugify
        from django.core.exceptions import ValidationError

        slug = slugify(title)

        if Question.objects.filter(slug=slug).exists():
            raise ValidationError('A question with this title already exists.')

        return title

class QuestionCreate(generic.CreateView):
    form_class = QuestionForm

This means that you no longer need the form_valid override and you don't need to worry about having to replicate that validation logic in other places which use this form.

\$\endgroup\$
1
  • \$\begingroup\$ Awesome, this was exactly what i need. Perfect + perfect explanation. Thank you so much!! Cheers! I can unfortunately only give one upvote \$\endgroup\$
    – stetim94
    Feb 10, 2017 at 10:39
2
\$\begingroup\$

I would go with something like this

 def form_valid(self, form):
    slug = slugify(form.cleaned_data['title'])
    try:
        question = Question.objects.get(slug=slug)
    except Question.DoesNotExist:
        raise ValidationError("This question already exist")

    # Proceed with saving the question...

This is also the approach used in docs

\$\endgroup\$
3
  • 1
    \$\begingroup\$ This is code review, so i am also curious why i should use this approach over what i show in the question? Does this approach allow me to show a field error? (not a non field error) \$\endgroup\$
    – stetim94
    Feb 9, 2017 at 10:13
  • \$\begingroup\$ @stetim94 because integretyerrors within transactions will lead to transaction rollback procedure on DB side. While it's not an issue for that particular case, you still should try to avoid that approach. \$\endgroup\$
    – Alex
    Feb 9, 2017 at 13:25
  • \$\begingroup\$ in my question, i have code after: Then it was brought to my attention that I don't need to save the form first, why would i use intelis solution (get + validition error) vs filter, exist + calling form_invalid? \$\endgroup\$
    – stetim94
    Feb 9, 2017 at 14:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.