Re: ModelForm unique validation is not done right IMHO.

[Todor Velichkov]

>
> It does? Can you give me a link to that please?
>

Pard me, it does not explicitly say to set values programmatically, but 
that this is the place to go when fields depend on each other, and since 
clean is a multi-step process which does not include only field validation, 
but settings values too, it kind of makes sense.

1) Form and field validation  


The form subclass’s clean() method can perform validation that requires 
> access to multiple form fields. This is where you might put in checks such 
> as “if field A is supplied, field B must contain a valid email address”. 
> *This 
> method can return a completely different dictionary if it wishes, which 
> will be used as the cleaned_data*. 
>

2) Cleaning and validating fields that depend on each other 


Suppose we add another requirement to our contact form: if the cc_myself 
> field is True, the subject must contain the word "help". *We are 
> performing validation on more than one field at a time, so the form’s 
> clean() method is a good spot to do this.*
>

3) Model.clean() 


This method should be used to provide custom model validation, *and to 
> modify attributes on your model if desired*. For instance, you could use 
> it to automatically provide a value for a field, or to do validation that 
> requires access to more than a single field.
>

Please, don't get me wrong, I'm far away from the idea of deprecating 
`commit=False`. I just personally try to void it and trying to explain my 
arguments. 

The generic error message is something that I supply in some forms of mine 
> where race conditions can happen due to high concurrency. This is why I 
> guard form.save, catch database errors and then use form.add_error to add a 
> generic error message asking for a retry.
>

Yes, that's definitely the place when something outside the validation 
process can happen, but you don't need `commit=False` here ;) 

One alternative approach to validate the instance w/o `commit=False` right 
now would be to change the form like so:

class BookForm(forms.ModelForm):

class Meta:
model = Book
fields = ('name', 'user')
widgets = {'user': forms.HiddenInput()}

def __init__(self, *args, **kwargs):
super(BookForm, self).__init__(*args, **kwargs)
self.fields['user'].disabled = True

#in the view
form = BookForm(data={'name': 'Harry Potter'}, initial={'user': user})

But if we compare this with the form in the first post, we are just 
Fixing/Patching it, i.e. fighting with it in order to make it work for us.

However, there is one more little difference here, which I want to point 
out. Image we didn't had the unique constrain, and we just wanted to hide 
the user field from the form, and we do it this way (instead of the one 
with the `instance` kwarg as in the first post). Doing this and adding the 
unique_constrain later on would yield to no more core changes (except 
changing the error msg if too ambiguous). While using the `commit=False` 
approach would require further code changes and it would be multiplied by 
the number of views the form is being used, and by the number of forms 
where the field has been omitted (i.e. commit=False has been used). Its a 
slight difference, but can lead to a big wins.

About the error message, to be honest I don't know, gonna keep thinking 
about it, one thing that came to mind is to strip the missing fields, i.e. 
instead of "Book with this User and Name already exists." to become: "Book 
with this Name already exists." but it really depends on the context. The 
one thing that I know for sure is that personally I would definitely prefer 
an ambiguous message, rather than a 500 server error.



On Monday, October 9, 2017 at 10:52:50 AM UTC+3, Florian Apolloner wrote:
>
> Hi,
>
> On Monday, October 9, 2017 at 8:52:53 AM UTC+2, Todor Velichkov wrote:
>>
>> Settings values programmatically is a cumulative operation most of the 
>> time, however when its not and things depend on each other (like your 
>> example), then even the docs suggests than one can use the form.clean 
>> method. 
>>
>
> It does? Can you give me a link to that please?
>
> If there is some other dependency outside form.cleaned_data I would prefer 
>> to use dependency injection in order to get this data and do the job done. 
>> I'm sorry I just see commit=False as an anti-pattern, because the 
>> validation needs to be repeated after that (as your example in the first 
>> post).
>>
>
> Repeated is a bit overreaching, it also checks new fields…
>
> Showing an error message which the user cannot correct is a horrible UX 
>> indeed, but still its a UX, and some people may find it as 

Re: ModelForm unique validation is not done right IMHO.

[Florian Apolloner]

Hi,

On Monday, October 9, 2017 at 8:52:53 AM UTC+2, Todor Velichkov wrote:
>
> Settings values programmatically is a cumulative operation most of the 
> time, however when its not and things depend on each other (like your 
> example), then even the docs suggests than one can use the form.clean 
> method. 
>

It does? Can you give me a link to that please?

If there is some other dependency outside form.cleaned_data I would prefer 
> to use dependency injection in order to get this data and do the job done. 
> I'm sorry I just see commit=False as an anti-pattern, because the 
> validation needs to be repeated after that (as your example in the first 
> post).
>

Repeated is a bit overreaching, it also checks new fields…

Showing an error message which the user cannot correct is a horrible UX 
> indeed, but still its a UX, and some people may find it as a better 
> alternative to a `500 server error page`, where there is no UX at all. Even 
> a generic message like 'sorry we messed up' would be useful, because the 
> user data that will be preserved into the form. However, in the example 
> shown here, this is not even the case, there is something that the user can 
> do to prevent the error.
>

The generic error message is something that I supply in some forms of mine 
where race conditions can happen due to high concurrency. This is why I 
guard form.save, catch database errors and then use form.add_error to add a 
generic error message asking for a retry. In the example shown, the user 
can do something about the error, this is correct, but the default error 
message would be rather confusing since it would cover a field which is not 
part of the form.

That said, form.save(commit=False) is there and will stay, even if it is 
just for backwards compatibility. Same goes for the partial validation of 
the instance, this is just something to many people rely on and changing is 
not really an option.

One could add a new flag to the Meta object ala validate_full_model, but 
for that to work you will have to tell us a sensible UX approach first. I 
am opposed to adding something which we agree is horrible just because the 
alternative (like I've shown) requires a few additional lines of code.

Cheers,
Florian

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/250667ec-70f8-4d9b-8813-766ee65ed5ed%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: ModelForm unique validation is not done right IMHO.

[Todor Velichkov]

Settings values programmatically is a cumulative operation most of the 
time, however when its not and things depend on each other (like your 
example), then even the docs suggests than one can use the form.clean 
method. If there is some other dependency outside form.cleaned_data I would 
prefer to use dependency injection in order to get this data and do the job 
done. I'm sorry I just see commit=False as an anti-pattern, because the 
validation needs to be repeated after that (as your example in the first 
post).

Showing an error message which the user cannot correct is a horrible UX 
indeed, but still its a UX, and some people may find it as a better 
alternative to a `500 server error page`, where there is no UX at all. Even 
a generic message like 'sorry we messed up' would be useful, because the 
user data that will be preserved into the form. However, in the example 
shown here, this is not even the case, there is something that the user can 
do to prevent the error.

Finally, yes, there may never be a 100% guarantee that an instance can 
actually be saved, but this would be outside of Django's scope? In terms of 
a model.save() things are deterministic and we should do doing our best to 
ensure that the operation is valid for execution, right? In the example I'm 
showing is not something outside of Django's knowledge to make it 
unexpected/unpreventable.

Thank you,
Todor.


On Sunday, October 8, 2017 at 7:36:58 PM UTC+3, Florian Apolloner wrote:
>
>
>
> On Saturday, October 7, 2017 at 5:07:31 PM UTC+2, Todor Velichkov wrote:
>>
>> I believe this could save a lot of headache to people. If there is no 
>> guarantee that an instance cannot be saved after the form has been 
>> validated, then do not give me an option to shoot myself into the foot.
>>
>
> Even if the whole instance were validated there is __never__ and can 
> __never__ be any guarantee that the instance can actually be saved.
>
>
>> My only concern here actually is not that we didn't know what will happen 
>> (I think we know that), but how to show the user that the developer got 
>> messed up, i.e. how to render an error for a field that is not rendered to 
>> the user? Should this error be propagated to a NON-FIELD error or something 
>> else?
>>
>
> Showing an error message which the user cannot correct because the field 
> is not in the form is horrible UX.
>
> I feel its perfectly fine to ask for a complete model instance at this 
>> stage of the validation process (lots of methods got passed from the 
>> beginning - form.clean() and instance.clean()).
>>
>
> I strongly disagree, especially for values which are set programmatically 
> after validation (with the use of save(commit=False)) depending on values 
> the user chose. Ie when the user chooses a category for a bug report and 
> the code then automatically assigns another user as owner of that 
> reuqest/ticket.
>
> Cheers,
> Florian
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/61db7623-a32b-4d1d-890d-f1ce463d8c91%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.