First thought: What is your Django version? The `disabled` attribute was 
added in Django 1.9.
However by looking at your code (w/o testing anything) after 
`form.is_valid()` you should only call `form.save()`, no need to do 
`commit=False` and manually assigning user, you have already done that in 
the form constructor (by settings initial value + disabled attr).

On Sunday, September 23, 2018 at 9:25:41 PM UTC+3, Protik wrote:
>
> Hi, Todor
>
> I have tested this solution and It looks like it works only when you don't 
> disable the field (i.e the last line in the BookForm's `__init__()` method. 
> My views looks like this:
>
>
> def book_add(request):
>     user = get_object_or_404(User, id=1)
>
>     if request.method == 'POST':
>
>         f = BookForm(request.POST, user=user)
>         if f.is_valid():
>             book = f.save(commit=False)
>             book.user = user
>             book.save()
>             messages.add_message(request, messages.INFO, 'book added.')
>             return redirect('book_add')
>     else:
>         f = BookForm(user=user)
>
>     return render(request, 'blog/book_add.html', {'f': f})
>
>
> def post_update(request, post_pk):
>     user = get_object_or_404(User, id=1)
>     book = get_object_or_404(Book, pk=post_pk)
>     if request.method == 'POST':
>         f = BookForm(request.POST, instance=book, user=user)
>         if f.is_valid():
>             post = f.save(commit=False)
>             post.user = user
>             post.save()
>             messages.add_message(request, messages.INFO, 'Post added.')
>             return redirect('post_update', post.pk)
>     else:
>         f = BookForm(instance=book, user=user)
>
>     return render(request, 'blog/book_update.html', {'f': f})
>
>
> The code for models and modelform is exactly same as yours.
>
>
> Am I doing something wrong?
>
>
> On Sunday, September 23, 2018 at 9:11:55 PM UTC+5:30, Todor Velichkov 
> wrote:
>>
>> You can use the `disabled 
>> <https://docs.djangoproject.com/en/1.11/ref/forms/fields/#disabled>` 
>> attribute on form fields with a combination of HiddenInput 
>> <https://docs.djangoproject.com/en/2.1/ref/forms/widgets/#hiddeninput>
>>
>> Using the Book example from the first comment it will look like this:
>>   
>> class BookForm(forms.ModelForm):
>>     class Meta:
>>         model = Book
>>         fields = ('user', 'name')
>>         
>>     def __init__(self, *args, **kwargs):
>>         user = kwargs.pop('user')
>>         super(BookForm, self).__init__(*args, **kwargs)
>>         self.fields['user'].widget = forms.HiddenInput()
>>         self.fields['user'].initial = user
>>         self.fields['user'].disabled = True
>>
>>
>> First we hide the the user field because we dont want the user to see it, 
>> and at the same time keeping it inside form fields we wont prevent the 
>> unique_together validation.
>> Second - we disable the field and programmatically set initial value to 
>> be used during form validation
>>
>> On Sunday, September 23, 2018 at 4:57:15 PM UTC+3, Protik wrote:
>>>
>>> Hi  Todor
>>>
>>> I am experiencing the same problem. Can you please post the 
>>> possible solution?
>>>
>>> On Tuesday, October 10, 2017 at 8:26:32 AM UTC+5:30, Todor Velichkov 
>>> wrote:
>>>>
>>>> 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  
>>>> <https://docs.djangoproject.com/en/1.11/ref/forms/validation/#cleaning-and-validating-fields-that-depend-on-each-other>
>>>>
>>>> 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 
>>>> <https://docs.djangoproject.com/en/1.11/ref/forms/validation/#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() 
>>>> <https://docs.djangoproject.com/en/1.11/ref/models/instances/#django.db.models.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 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/6a12bfad-0961-4220-9a59-a0b6c243fb48%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to